From 1cd47b94e098820a1dbf73ca88ea3e805bec5150 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:23:37 -0300 Subject: [PATCH 01/11] fix: ignore check runs superseded by a newer run of the same check When a workflow runs more than once for the same commit (e.g. an older run cancelled by `concurrency: cancel-in-progress`), the check-runs API returns one run per check suite, so the cancelled one kept the PR "failed" forever even after the newer run passed. Only the newest run per app and check name counts now, like GitHub's merge box and required checks. Verified live on zephyrproject-rtos/zephyr#105183: previously reported as failure ("Prevent Merging" cancelled), now success. Addresses Codex review on #6. Co-Authored-By: Claude Opus 5.5 --- internal/github/client.go | 35 +++++++++++++++++++- internal/github/superseded_test.go | 52 ++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 internal/github/superseded_test.go diff --git a/internal/github/client.go b/internal/github/client.go index cceccf2..47727b4 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -159,10 +159,14 @@ type CommitStatus struct { // CheckRun represents a single check run (e.g. a GitHub Actions job). type CheckRun struct { + ID int64 `json:"id"` Name string `json:"name"` Status string `json:"status"` // queued, in_progress, completed, ... Conclusion string `json:"conclusion"` // success, failure, neutral, cancelled, ... HTMLURL string `json:"html_url"` + App struct { + ID int64 `json:"id"` + } `json:"app"` } type checkRunsResponse struct { @@ -377,7 +381,7 @@ func SummarizeCI(status *CombinedStatus, runs []CheckRun) *CIStatus { } } - for _, run := range runs { + for _, run := range latestCheckRuns(runs) { total++ if NormalizeState(run.Status) != "completed" { hasPending = true @@ -407,6 +411,35 @@ func SummarizeCI(status *CombinedStatus, runs []CheckRun) *CIStatus { return &CIStatus{State: state, FailedChecks: failed, Total: total, Pending: pending} } +// latestCheckRuns drops check runs superseded by a newer run of the same check (same +// app and name). The API returns one run per check suite, so a workflow that ran more +// than once for the same commit (e.g. an older run cancelled by `concurrency: +// cancel-in-progress`) shows up twice; like GitHub's merge box and required checks, +// only the newest one counts. Run IDs grow with creation time. +func latestCheckRuns(runs []CheckRun) []CheckRun { + type checkKey struct { + app int64 + name string + } + newest := make(map[checkKey]int64, len(runs)) + for _, run := range runs { + key := checkKey{run.App.ID, run.Name} + if id, seen := newest[key]; !seen || run.ID > id { + newest[key] = run.ID + } + } + if len(newest) == len(runs) { + return runs + } + latest := make([]CheckRun, 0, len(newest)) + for _, run := range runs { + if newest[checkKey{run.App.ID, run.Name}] == run.ID { + latest = append(latest, run) + } + } + return latest +} + func hasCISignal(status *CombinedStatus, runs []CheckRun) bool { if len(runs) > 0 { return true diff --git a/internal/github/superseded_test.go b/internal/github/superseded_test.go new file mode 100644 index 0000000..0de87b4 --- /dev/null +++ b/internal/github/superseded_test.go @@ -0,0 +1,52 @@ +package github + +import "testing" + +func actionsRun(id int64, name, status, conclusion string) CheckRun { + run := CheckRun{ID: id, Name: name, Status: status, Conclusion: conclusion} + run.App.ID = 15368 // github-actions + return run +} + +func TestSummarizeCI_IgnoresRunSupersededByNewerRun(t *testing.T) { + // Shape of zephyrproject-rtos/zephyr#105183: a workflow with cancel-in-progress ran + // twice for the same commit, in two check suites; the API returns both runs. + runs := []CheckRun{ + actionsRun(66407602784, "Prevent Merging", "completed", "success"), + actionsRun(66407483043, "Prevent Merging", "completed", "cancelled"), + actionsRun(66407400000, "Build", "completed", "success"), + } + ci := SummarizeCI(&CombinedStatus{State: "pending"}, runs) + if ci.State != "success" || len(ci.FailedChecks) != 0 || ci.Total != 2 { + t.Errorf("got state=%q failed=%v total=%d, want success with 2 checks", ci.State, ci.FailedChecks, ci.Total) + } +} + +func TestSummarizeCI_NewerFailureStillCounts(t *testing.T) { + runs := []CheckRun{ + actionsRun(1, "Test", "completed", "success"), + actionsRun(2, "Test", "completed", "failure"), + } + if ci := SummarizeCI(nil, runs); ci.State != "failure" || len(ci.FailedChecks) != 1 { + t.Errorf("got state=%q failed=%v, want the newer failure", ci.State, ci.FailedChecks) + } +} + +func TestSummarizeCI_NewerRunStillInProgress(t *testing.T) { + runs := []CheckRun{ + actionsRun(1, "Test", "completed", "failure"), + actionsRun(2, "Test", "in_progress", ""), + } + if ci := SummarizeCI(nil, runs); ci.State != "pending" || ci.Pending != 1 || len(ci.FailedChecks) != 0 { + t.Errorf("got state=%q pending=%d failed=%v, want pending on the re-run", ci.State, ci.Pending, ci.FailedChecks) + } +} + +func TestSummarizeCI_SameNameFromDifferentAppsIsKept(t *testing.T) { + other := CheckRun{ID: 1, Name: "Test", Status: "completed", Conclusion: "failure"} + other.App.ID = 999 // e.g. an external CI that also names its check "Test" + runs := []CheckRun{other, actionsRun(2, "Test", "completed", "success")} + if ci := SummarizeCI(nil, runs); ci.State != "failure" { + t.Errorf("state = %q; a failure from another app must not be hidden", ci.State) + } +} From 702becdd0fc0ca014630eb23c81d84cb408027f6 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:24:16 -0300 Subject: [PATCH 02/11] fix: detect GovSlack webhook URLs (hooks.slack-gov.com) as Slack GovSlack serves incoming webhooks from its own slack-gov.com domain, so auto mode sent them the generic payload, which Slack rejects. Addresses Codex review on #7. Co-Authored-By: Claude Opus 5.5 --- internal/notify/webhook_format.go | 2 +- internal/notify/webhook_format_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/notify/webhook_format.go b/internal/notify/webhook_format.go index 4481065..7f7f29a 100644 --- a/internal/notify/webhook_format.go +++ b/internal/notify/webhook_format.go @@ -59,7 +59,7 @@ func DetectWebhookFormat(rawURL string) string { } host := strings.ToLower(u.Hostname()) switch { - case host == "hooks.slack.com": + case host == "hooks.slack.com" || host == "hooks.slack-gov.com": // GovSlack uses its own domain return WebhookFormatSlack case (host == "discord.com" || host == "discordapp.com" || strings.HasSuffix(host, ".discord.com") || strings.HasSuffix(host, ".discordapp.com")) && strings.HasPrefix(u.Path, "/api/webhooks/"): diff --git a/internal/notify/webhook_format_test.go b/internal/notify/webhook_format_test.go index ce37e3b..fcd175e 100644 --- a/internal/notify/webhook_format_test.go +++ b/internal/notify/webhook_format_test.go @@ -35,6 +35,7 @@ func TestDetectWebhookFormat(t *testing.T) { want string }{ {"https://hooks.slack.com/services/T000/B000/XXXX", WebhookFormatSlack}, + {"https://hooks.slack-gov.com/services/T000/B000/XXXX", WebhookFormatSlack}, {"https://HOOKS.SLACK.COM/services/T000/B000/XXXX", WebhookFormatSlack}, {"https://discord.com/api/webhooks/123/token", WebhookFormatDiscord}, {"https://discordapp.com/api/webhooks/123/token", WebhookFormatDiscord}, From 423a135d00c4281731c38d42e2951a6eefe5b035 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:26:30 -0300 Subject: [PATCH 03/11] fix: keep the merged/closed notification for review-request PRs A PR auto-watched with --review-requested that was merged or closed while prw was stopped no longer matched the is:open search, so the startup sync removed it as a withdrawn request before it was checked, and its final merged/closed notification was lost. Before removing review-request PRs that stopped matching, the sync now looks them up and keeps the closed ones; the regular check then notifies and drops them. If the lookup fails, the PR is removed as before. Addresses Codex review on #16. Co-Authored-By: Claude Opus 5.5 --- internal/watcher/autowatch.go | 44 ++++++++++++++++++++++++++---- internal/watcher/autowatch_test.go | 28 +++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/internal/watcher/autowatch.go b/internal/watcher/autowatch.go index 297fdd5..3eeec9b 100644 --- a/internal/watcher/autowatch.go +++ b/internal/watcher/autowatch.go @@ -26,9 +26,11 @@ var AutoWatchDescription = map[string]string{ config.AutoWatchReviewRequested: "PRs where your review is requested", } -// Searcher runs GitHub pull request searches. +// Searcher runs GitHub pull request searches. GetPullRequest tells a review request +// that was withdrawn apart from a PR that was merged or closed. type Searcher interface { SearchPullRequests(query string) (*github.SearchResult, error) + GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) } // SyncResult reports what SyncAutoWatch changed. @@ -42,9 +44,9 @@ type SyncResult struct { // SyncAutoWatch runs the search of every source in sources and updates the watch list: // matching PRs that aren't watched yet are added (tagged with their source), and PRs // added for "review-requested" that no longer match are removed, since the request -// was withdrawn or answered. PRs added for "mine" are not removed when they stop -// matching: that means they were merged or closed, and `prw run` should still notify -// about it before dropping them. PRs watched by hand are never touched. +// was withdrawn or answered. PRs that stopped matching because they were merged or +// closed are kept instead (for "mine" that's always the reason), so that `prw run` +// still notifies about it before dropping them. PRs watched by hand are never touched. // // It returns an error only if every search failed; other problems are Warnings. func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { @@ -73,6 +75,7 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { if len(found) == 0 && lastErr != nil { return result, lastErr } + closed := closedReviewRequests(client, found) err := config.Update(func(c *config.Config) error { changed := false @@ -97,7 +100,8 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { } kept := c.WatchedPRs[:0] for _, pr := range c.WatchedPRs { - if pr.Source == source && !matches[prKey(pr.Owner, pr.Repo, pr.Number)] { + key := prKey(pr.Owner, pr.Repo, pr.Number) + if pr.Source == source && !matches[key] && !closed[key] { result.Removed = append(result.Removed, pr) changed = true continue @@ -117,6 +121,36 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { return result, nil } +// closedReviewRequests finds the review-request PRs that no longer match their search +// because they were merged or closed: the search only returns open PRs, so they look +// just like withdrawn requests. It runs before the config is locked, as it makes API +// calls. A PR whose lookup fails is not reported, i.e. it's removed as before. +func closedReviewRequests(client Searcher, found map[string]*github.SearchResult) map[string]bool { + res, ok := found[config.AutoWatchReviewRequested] + if !ok || res.Incomplete { + return nil // nothing will be removed + } + cfg, err := config.Load() + if err != nil { + return nil + } + matches := make(map[string]bool, len(res.PRs)) + for _, pr := range res.PRs { + matches[prKey(pr.Owner, pr.Repo, pr.Number)] = true + } + closed := map[string]bool{} + for _, pr := range cfg.WatchedPRs { + key := prKey(pr.Owner, pr.Repo, pr.Number) + if pr.Source != config.AutoWatchReviewRequested || matches[key] { + continue + } + if ghPR, err := client.GetPullRequest(pr.Owner, pr.Repo, pr.Number); err == nil && ghPR.IsClosed() { + closed[key] = true + } + } + return closed +} + func prKey(owner, repo string, number int) string { return strings.ToLower(owner) + "/" + strings.ToLower(repo) + "#" + fmt.Sprint(number) } diff --git a/internal/watcher/autowatch_test.go b/internal/watcher/autowatch_test.go index 08fbe63..889a776 100644 --- a/internal/watcher/autowatch_test.go +++ b/internal/watcher/autowatch_test.go @@ -301,3 +301,31 @@ func TestRun_PicksUpAutoWatchEnabledWhileRunning(t *testing.T) { t.Errorf("watch list = %s", got) } } + +func TestRunOnce_NotifiesReviewRequestMergedWhileStopped(t *testing.T) { + // The PR was merged while prw wasn't running: the is:open search no longer returns + // it, which must not be mistaken for a withdrawn review request. + useTempConfig(t) + cfg := &config.Config{ + AutoWatch: []string{config.AutoWatchReviewRequested}, + WatchedPRs: []config.WatchedPR{ + {Owner: "owner", Repo: "repo", Number: 20, Source: config.AutoWatchReviewRequested, LastKnownState: "success"}, + }, + } + saveConfig(t, cfg) + merged := newPR(20, "sha20") + merged.State, merged.Merged = "closed", true + s := &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: found()}} + s.prs = map[string]*github.PullRequest{"owner/repo/20": merged} + notifier := &mockNotifier{} + + if err := New(s, cfg, notifier).RunOnce(context.Background()); err != nil { + t.Fatal(err) + } + if len(notifier.events) != 1 || notifier.events[0].CurrentState != "merged" { + t.Errorf("events = %+v, want one merged notification", notifier.events) + } + if got := watchList(t); got != "" { + t.Errorf("watch list = %q, want the merged PR dropped after notifying", got) + } +} From 35784403cd2a7e9f5175af5759036bf1ff3c16e2 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:27:08 -0300 Subject: [PATCH 04/11] fix: don't flap a `no_ci` PR to `pending` on comments and label changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Any comment or label change bumps a PR's updated_at, which restarted the no-CI grace period: `prw run` stored `pending` and, with the default `change` filter, notified "no_ci → pending" although no CI had appeared, then silently went back to no_ci three minutes later. A PR that was no_ci now stays no_ci until an actual check or status shows up. Addresses Codex review on #17. Co-Authored-By: Claude Opus 5.5 --- internal/watcher/noci_test.go | 37 +++++++++++++++++++++++++++++++++++ internal/watcher/watcher.go | 5 +++++ 2 files changed, 42 insertions(+) diff --git a/internal/watcher/noci_test.go b/internal/watcher/noci_test.go index 8dc8ed8..57b5ff0 100644 --- a/internal/watcher/noci_test.go +++ b/internal/watcher/noci_test.go @@ -64,3 +64,40 @@ func TestCheckPR_NotifiesWhenCIShowsUpLater(t *testing.T) { t.Errorf("events = %+v, want one no_ci → success", notifier.events) } } + +func TestCheckPR_StaysNoCIWhenOnlyMetadataChanges(t *testing.T) { + // A comment or label bumps updated_at, but no check or status appeared. + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha1", 10*time.Second, now)}} + notifier := &mockNotifier{} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, github.StateNoCI)}} + w := New(client, cfg, notifier) + w.now = func() time.Time { return now } + + if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + t.Fatal(err) + } + if got := cfg.WatchedPRs[0].LastKnownState; got != github.StateNoCI { + t.Errorf("state = %q, want no_ci", got) + } + if len(notifier.events) != 0 { + t.Errorf("a comment must not notify no_ci → pending, got %+v", notifier.events[0]) + } +} + +func TestCheckPR_NoCIToPendingWhenCIStartsAfterAPush(t *testing.T) { + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{ + prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha2", 10*time.Second, now)}, + checkRuns: map[string][]github.CheckRun{"sha2": {{Name: "Test", Status: "queued"}}}, + } + notifier := &mockNotifier{} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, github.StateNoCI)}} + w := New(client, cfg, notifier) + w.now = func() time.Time { return now } + + w.checkPR(&cfg.WatchedPRs[0]) + if got := cfg.WatchedPRs[0].LastKnownState; got != "pending" || len(notifier.events) != 1 { + t.Errorf("state = %q events = %d, want pending with one notification once a check exists", got, len(notifier.events)) + } +} diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index d51eac0..31085a9 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -223,6 +223,11 @@ func (w *Watcher) checkPR(pr *config.WatchedPR) (closed bool, err error) { currentState := ci.State previousState := github.NormalizeState(pr.LastKnownState) + if previousState == github.StateNoCI && currentState == "pending" && ci.Total == 0 { + // Comments and label changes also bump updated_at, which restarts the grace + // period; until an actual check or status shows up, the PR still has no CI. + currentState = github.StateNoCI + } // Check if status changed. Finding out that a PR has no CI is not a CI result, so // it isn't announced (it would otherwise ping once for every such PR). From 94e876efac9e04a41d02af07a3978c7fc35f92ed Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:35:53 -0300 Subject: [PATCH 05/11] fix: cancel in-flight GitHub requests on timeout/Ctrl-C; confirm head before returning Context: the GitHub client methods now take a context.Context, passed down to each HTTP request. Previously `prw wait --timeout`, MCP `wait_for_ci` `timeout_seconds` and client cancellation, and Ctrl-C were only noticed between polls: a stalled network could keep a poll (PR + status + check-run pages, each with a 15s HTTP timeout) running well past the deadline, and Ctrl-C did nothing until it finished. A poll cancelled this way now ends as timed out (exit 2) or interrupted (exit 130) instead of being retried as a transient error. `prw run` also stops immediately on Ctrl-C, leaving the PRs it hadn't checked unchanged. Head re-check: `prw wait` / `wait_for_ci` look up the PR and then its CI, so a commit pushed in between made them return the final result of the previous commit (e.g. `prw wait && deploy` deploying unchecked code). Before returning a final result, the PR is read again (a free 304 when unchanged); if the head moved, waiting continues on the new commit. Addresses Codex reviews on #13 and #15. Co-Authored-By: Claude Opus 5.5 --- cmd/prw/broadcast.go | 5 +- cmd/prw/main.go | 4 +- cmd/prw/mcp.go | 4 +- cmd/prw/wait.go | 45 ++++++++--- cmd/prw/wait_ctx_test.go | 97 ++++++++++++++++++++++++ cmd/prw/wait_test.go | 38 ++++++++-- internal/github/client.go | 31 ++++---- internal/github/client_test.go | 29 +++---- internal/github/efficiency_test.go | 25 +++--- internal/github/integration_test.go | 15 ++-- internal/github/search_test.go | 11 +-- internal/watcher/autowatch.go | 19 ++--- internal/watcher/autowatch_test.go | 16 ++-- internal/watcher/closed_pr_test.go | 13 ++-- internal/watcher/noci_test.go | 11 +-- internal/watcher/ratelimit_test.go | 8 +- internal/watcher/watcher.go | 33 ++++---- internal/watcher/watcher_persist_test.go | 32 +++++++- internal/watcher/watcher_test.go | 28 +++---- 19 files changed, 323 insertions(+), 141 deletions(-) create mode 100644 cmd/prw/wait_ctx_test.go diff --git a/cmd/prw/broadcast.go b/cmd/prw/broadcast.go index 856ba5c..312a463 100644 --- a/cmd/prw/broadcast.go +++ b/cmd/prw/broadcast.go @@ -1,6 +1,7 @@ package main import ( + "context" "fmt" "strings" "time" @@ -70,13 +71,13 @@ var broadcastCmd = &cobra.Command{ for i := range cfg.WatchedPRs { pr := &cfg.WatchedPRs[i] - ghPR, err := client.GetPullRequest(pr.Owner, pr.Repo, pr.Number) + ghPR, err := client.GetPullRequest(context.Background(), pr.Owner, pr.Repo, pr.Number) if err != nil { fmt.Printf("Error fetching PR %s/%s#%d: %v\n", pr.Owner, pr.Repo, pr.Number, err) continue } - ci, err := github.FetchCIStatus(client, pr.Owner, pr.Repo, ghPR.Head.SHA) + ci, err := github.FetchCIStatus(context.Background(), client, pr.Owner, pr.Repo, ghPR.Head.SHA) if err != nil { fmt.Printf("Error fetching status for %s/%s#%d: %v\n", pr.Owner, pr.Repo, pr.Number, err) continue diff --git a/cmd/prw/main.go b/cmd/prw/main.go index a7a7665..bb10c5c 100644 --- a/cmd/prw/main.go +++ b/cmd/prw/main.go @@ -157,7 +157,7 @@ func watchSinglePR(prURL string) error { // Try to fetch the PR to validate it exists and get title client := newGitHubClient(token) - pr, err := client.GetPullRequest(owner, repo, number) + pr, err := client.GetPullRequest(context.Background(), owner, repo, number) if err != nil { return fmt.Errorf("failed to fetch PR: %w", err) } @@ -229,7 +229,7 @@ func enableAutoWatch(sources []string) error { } // Run the searches now so the user sees the result immediately. - res, err := watcher.SyncAutoWatch(newGitHubClient(token), sources) + res, err := watcher.SyncAutoWatch(context.Background(), newGitHubClient(token), sources) for _, warning := range res.Warnings { fmt.Printf("Warning: %s\n", warning) } diff --git a/cmd/prw/mcp.go b/cmd/prw/mcp.go index 89792ca..2a434a5 100644 --- a/cmd/prw/mcp.go +++ b/cmd/prw/mcp.go @@ -569,7 +569,7 @@ func toolGetCIStatus(ctx context.Context, s *mcpServer, args json.RawMessage, _ return toolError("%v", err) } result := &waitResult{Owner: owner, Repo: repo, Number: number, URL: github.FormatPRURL(owner, repo, number)} - if _, err := pollOnce(client, result); err != nil { + if _, err := pollOnce(ctx, client, result); err != nil { return toolError("failed to get CI status of %s/%s#%d: %v", owner, repo, number, err) } return toolOK(ciSummary(result), result) @@ -666,7 +666,7 @@ func toolWatchPR(ctx context.Context, s *mcpServer, args json.RawMessage, _ io.W if err != nil { return toolError("%v", err) } - pr, err := client.GetPullRequest(owner, repo, number) + pr, err := client.GetPullRequest(ctx, owner, repo, number) if err != nil { return toolError("failed to fetch %s/%s#%d: %v", owner, repo, number, err) } diff --git a/cmd/prw/wait.go b/cmd/prw/wait.go index 8f44af2..77edee6 100644 --- a/cmd/prw/wait.go +++ b/cmd/prw/wait.go @@ -189,7 +189,7 @@ func (r *waitResult) exitErr() error { // ciClient is the subset of the GitHub client used by waitForCI. type ciClient interface { - GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) + GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) github.CIFetcher } @@ -203,11 +203,23 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number return result } + stopped := func() *waitResult { + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + result.TimedOut = true + } else { + result.Interrupted = true + } + return finish() + } + var lastLine string consecutiveErrors := 0 for { delay := interval - state, err := pollOnce(client, result) + state, err := pollOnce(ctx, client, result) + if err != nil && ctx.Err() != nil { + return stopped(), nil // the timeout or Ctrl-C cancelled the poll's requests + } if limited, resetAt := github.IsRateLimited(err); limited { // Not a failure of the PR or the network: wait for the quota to reset // (still bounded by --timeout) instead of burning the retry budget. @@ -228,6 +240,10 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number } else { consecutiveErrors = 0 if state != "pending" { + if headMoved(ctx, client, result) { + fmt.Fprintf(progress, "↻ %s/%s#%d: new commit pushed; waiting for its CI\n", owner, repo, number) + continue + } return finish(), nil } line := fmt.Sprintf("⏳ %s/%s#%d: %s", owner, repo, number, pendingSummary(result)) @@ -239,19 +255,26 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number sleepFunc(ctx, delay) if ctx.Err() != nil { - if errors.Is(ctx.Err(), context.DeadlineExceeded) { - result.TimedOut = true - } else { - result.Interrupted = true - } - return finish(), nil + return stopped(), nil } } } +// headMoved reports whether a new commit was pushed to the PR since result was +// polled. The CI lookup follows the PR lookup, so a push in between would otherwise +// return the result of the previous commit. Merged or closed PRs don't move; if the +// check fails, the result stands. Unchanged PRs answer 304, so this costs no quota. +func headMoved(ctx context.Context, client ciClient, result *waitResult) bool { + if result.State == "merged" || result.State == "closed" { + return false + } + pr, err := client.GetPullRequest(ctx, result.Owner, result.Repo, result.Number) + return err == nil && pr.Head.SHA != result.SHA +} + // pollOnce refreshes result from GitHub and returns the current state. -func pollOnce(client ciClient, result *waitResult) (string, error) { - pr, err := client.GetPullRequest(result.Owner, result.Repo, result.Number) +func pollOnce(ctx context.Context, client ciClient, result *waitResult) (string, error) { + pr, err := client.GetPullRequest(ctx, result.Owner, result.Repo, result.Number) if err != nil { return "", fmt.Errorf("failed to fetch PR: %w", err) } @@ -269,7 +292,7 @@ func pollOnce(client ciClient, result *waitResult) (string, error) { return result.State, nil } - ci, err := github.FetchCIStatus(client, result.Owner, result.Repo, pr.Head.SHA) + ci, err := github.FetchCIStatus(ctx, client, result.Owner, result.Repo, pr.Head.SHA) if err != nil { return "", err } diff --git a/cmd/prw/wait_ctx_test.go b/cmd/prw/wait_ctx_test.go new file mode 100644 index 0000000..fe67c12 --- /dev/null +++ b/cmd/prw/wait_ctx_test.go @@ -0,0 +1,97 @@ +package main + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/devblac/prw/internal/github" +) + +func TestWaitForCI_FollowsPushThatLandsDuringThePoll(t *testing.T) { + // The PR is read at "old", then "new" is pushed before the CI lookup of "old" + // returns success: that result must not be reported for the PR. + noSleep(t) + client := &scriptedClient{steps: []pollStep{ + {pr: openPR("old"), runs: []github.CheckRun{run("Test", "completed", "success")}, movedTo: openPR("new")}, + {pr: openPR("new"), runs: []github.CheckRun{run("Test", "in_progress", "")}}, + {pr: openPR("new"), runs: []github.CheckRun{run("Test", "completed", "failure")}}, + }} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.SHA != "new" || result.State != "failure" { + t.Errorf("got %s on %s, want the failure of the new head", result.State, result.SHA) + } +} + +func TestWaitForCI_ConfirmsHeadBeforeReturning(t *testing.T) { + noSleep(t) + client := &scriptedClient{steps: []pollStep{{pr: openPR("sha"), runs: []github.CheckRun{run("Test", "completed", "success")}}}} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.State != "success" || client.polls != 1 || client.revalidations != 1 { + t.Errorf("state=%q polls=%d revalidations=%d, want success after one poll and one head check", + result.State, client.polls, client.revalidations) + } +} + +// hangingClient is a real client against a server that never answers until the +// request is cancelled, like a stalled network. +func hangingClient(t *testing.T) *github.Client { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): + case <-time.After(10 * time.Second): + } + })) + t.Cleanup(srv.Close) + client := github.NewClient("token") + client.BaseURL = srv.URL + return client +} + +func TestWaitForCI_TimeoutCancelsTheRequestInFlight(t *testing.T) { + noSleep(t) + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + start := time.Now() + result, err := waitForCI(ctx, hangingClient(t), "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Errorf("returned after %s; the timeout must cancel the hanging request", elapsed) + } + if !result.TimedOut || exitCode(result.exitErr()) != exitWaitTimeout { + t.Errorf("result = %+v, want timed out (exit 2)", result) + } +} + +func TestWaitForCI_InterruptCancelsTheRequestInFlight(t *testing.T) { + noSleep(t) + ctx, cancel := context.WithCancel(context.Background()) + time.AfterFunc(100*time.Millisecond, cancel) // Ctrl-C + + start := time.Now() + result, err := waitForCI(ctx, hangingClient(t), "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Errorf("returned after %s; Ctrl-C must cancel the hanging request", elapsed) + } + if !result.Interrupted || exitCode(result.exitErr()) != exitWaitInterrupted { + t.Errorf("result = %+v, want interrupted (exit 130)", result) + } +} diff --git a/cmd/prw/wait_test.go b/cmd/prw/wait_test.go index 295b300..845e346 100644 --- a/cmd/prw/wait_test.go +++ b/cmd/prw/wait_test.go @@ -25,16 +25,39 @@ type pollStep struct { status *github.CombinedStatus runs []github.CheckRun ciErr error + // movedTo, if set, is what the PR looks like when waitForCI re-reads it to + // confirm the head hasn't moved, i.e. a push that landed during this poll. + movedTo *github.PullRequest } -// scriptedClient returns steps[i] on the i-th poll (repeating the last one). +// scriptedClient returns steps[i] on the i-th poll (repeating the last one). A PR +// lookup right after a poll that found a final CI result is the head re-check, not a +// new poll. type scriptedClient struct { - steps []pollStep - polls int - current pollStep + steps []pollStep + polls int + revalidations int + current pollStep + afterCI bool } -func (c *scriptedClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { +func (c *scriptedClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { + if c.afterCI { + c.afterCI = false + status := c.current.status + if status == nil { + status = &github.CombinedStatus{State: "pending"} + } + ci := github.SummarizeCI(status, c.current.runs) + github.ApplyNoCI(ci, c.current.pr.UpdatedAt, time.Now()) + if ci.State != "pending" { + c.revalidations++ + if c.current.movedTo != nil { + return c.current.movedTo, nil + } + return c.current.pr, nil + } + } i := c.polls if i >= len(c.steps) { i = len(c.steps) - 1 @@ -44,7 +67,7 @@ func (c *scriptedClient) GetPullRequest(owner, repo string, number int) (*github return c.current.pr, c.current.prErr } -func (c *scriptedClient) GetCombinedStatus(owner, repo, ref string) (*github.CombinedStatus, error) { +func (c *scriptedClient) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) { if c.current.ciErr != nil { return nil, c.current.ciErr } @@ -54,7 +77,8 @@ func (c *scriptedClient) GetCombinedStatus(owner, repo, ref string) (*github.Com return &github.CombinedStatus{State: "pending"}, nil } -func (c *scriptedClient) GetCheckRuns(owner, repo, ref string) ([]github.CheckRun, error) { +func (c *scriptedClient) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) { + c.afterCI = true return c.current.runs, nil } diff --git a/internal/github/client.go b/internal/github/client.go index 47727b4..2b6adf3 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -1,6 +1,7 @@ package github import ( + "context" "encoding/json" "errors" "fmt" @@ -192,8 +193,8 @@ type CIStatus struct { // CIFetcher is the subset of GitHub operations needed to compute CI status. type CIFetcher interface { - GetCombinedStatus(owner, repo, ref string) (*CombinedStatus, error) - GetCheckRuns(owner, repo, ref string) ([]CheckRun, error) + GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*CombinedStatus, error) + GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]CheckRun, error) } // ParsePRURL extracts owner, repo, and PR number from a GitHub PR URL. @@ -216,36 +217,36 @@ func ParsePRURL(prURL string) (owner, repo string, number int, err error) { } // GetPullRequest fetches a pull request by owner, repo, and PR number. -func (c *Client) GetPullRequest(owner, repo string, number int) (*PullRequest, error) { +func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) { path := fmt.Sprintf("/repos/%s/%s/pulls/%d", owner, repo, number) var pr PullRequest - if err := c.getJSON(path, &pr); err != nil { + if err := c.getJSON(ctx, path, &pr); err != nil { return nil, err } return &pr, nil } // GetCombinedStatus fetches the combined legacy commit status for a commit. -func (c *Client) GetCombinedStatus(owner, repo, ref string) (*CombinedStatus, error) { +func (c *Client) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*CombinedStatus, error) { path := fmt.Sprintf("/repos/%s/%s/commits/%s/status", owner, repo, url.PathEscape(ref)) var status CombinedStatus - if err := c.getJSON(path, &status); err != nil { + if err := c.getJSON(ctx, path, &status); err != nil { return nil, err } return &status, nil } // GetCheckRuns fetches the latest check runs (e.g. GitHub Actions jobs) for a commit. -func (c *Client) GetCheckRuns(owner, repo, ref string) ([]CheckRun, error) { +func (c *Client) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]CheckRun, error) { var runs []CheckRun for page := 1; page <= maxCheckRunPages; page++ { path := fmt.Sprintf("/repos/%s/%s/commits/%s/check-runs?per_page=%d&page=%d", owner, repo, url.PathEscape(ref), checkRunsPerPage, page) var resp checkRunsResponse - if err := c.getJSON(path, &resp); err != nil { + if err := c.getJSON(ctx, path, &resp); err != nil { var apiErr *APIError if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusForbidden && !apiErr.RateLimited { return nil, fmt.Errorf("%w (fine-grained tokens need the \"Checks: read\" permission)", err) @@ -298,12 +299,12 @@ type searchIssuesResponse struct { // SearchPullRequests runs a GitHub issue search (e.g. "is:pr is:open author:@me") and // returns the pull requests it finds, up to 300. Qualifiers like @me need a token. -func (c *Client) SearchPullRequests(query string) (*SearchResult, error) { +func (c *Client) SearchPullRequests(ctx context.Context, query string) (*SearchResult, error) { result := &SearchResult{} for page := 1; page <= maxSearchPages; page++ { path := fmt.Sprintf("/search/issues?q=%s&per_page=%d&page=%d", url.QueryEscape(query), searchPerPage, page) var resp searchIssuesResponse - if err := c.getJSON(path, &resp); err != nil { + if err := c.getJSON(ctx, path, &resp); err != nil { return nil, err } result.Total = resp.TotalCount @@ -331,13 +332,13 @@ func (c *Client) SearchPullRequests(query string) (*SearchResult, error) { } // FetchCIStatus fetches commit statuses and check runs for a commit and aggregates them. -func FetchCIStatus(c CIFetcher, owner, repo, ref string) (*CIStatus, error) { - status, err := c.GetCombinedStatus(owner, repo, ref) +func FetchCIStatus(ctx context.Context, c CIFetcher, owner, repo, ref string) (*CIStatus, error) { + status, err := c.GetCombinedStatus(ctx, owner, repo, ref) if err != nil { return nil, fmt.Errorf("failed to fetch commit status: %w", err) } - runs, err := c.GetCheckRuns(owner, repo, ref) + runs, err := c.GetCheckRuns(ctx, owner, repo, ref) if err != nil { return nil, fmt.Errorf("failed to fetch check runs: %w", err) } @@ -452,8 +453,8 @@ func hasCISignal(status *CombinedStatus, runs []CheckRun) bool { } // getJSON performs an authenticated GET request and decodes the JSON response into v. -func (c *Client) getJSON(path string, v interface{}) error { - req, err := http.NewRequest("GET", c.BaseURL+path, nil) +func (c *Client) getJSON(ctx context.Context, path string, v interface{}) error { + req, err := http.NewRequestWithContext(ctx, "GET", c.BaseURL+path, nil) if err != nil { return err } diff --git a/internal/github/client_test.go b/internal/github/client_test.go index 25b688f..0f7168f 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -1,6 +1,7 @@ package github import ( + "context" "fmt" "io" "net/http" @@ -144,7 +145,7 @@ func TestGetPullRequest(t *testing.T) { }, } - pr, err := client.GetPullRequest("owner", "repo", 123) + pr, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if tt.wantErr { if err == nil { t.Error("expected error, got nil") @@ -228,7 +229,7 @@ func TestGetCombinedStatus(t *testing.T) { }, } - status, err := client.GetCombinedStatus("owner", "repo", "abc123") + status, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if tt.wantErr { if err == nil { t.Error("expected error, got nil") @@ -333,7 +334,7 @@ func TestGetPullRequest_NetworkError(t *testing.T) { }, } - _, err := client.GetPullRequest("owner", "repo", 123) + _, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if err == nil { t.Error("expected error for network failure, got nil") } @@ -355,7 +356,7 @@ func TestGetCombinedStatus_NetworkError(t *testing.T) { }, } - _, err := client.GetCombinedStatus("owner", "repo", "abc123") + _, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err == nil { t.Error("expected error for network failure, got nil") } @@ -381,7 +382,7 @@ func TestGetPullRequest_InvalidJSON(t *testing.T) { }, } - _, err := client.GetPullRequest("owner", "repo", 123) + _, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if err == nil { t.Error("expected error for invalid JSON, got nil") } @@ -407,7 +408,7 @@ func TestGetCombinedStatus_InvalidJSON(t *testing.T) { }, } - _, err := client.GetCombinedStatus("owner", "repo", "abc123") + _, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err == nil { t.Error("expected error for invalid JSON, got nil") } @@ -433,7 +434,7 @@ func TestGetPullRequest_ErrorResponse(t *testing.T) { }, } - _, err := client.GetPullRequest("owner", "repo", 123) + _, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if err == nil { t.Error("expected error for 403 response, got nil") } @@ -459,7 +460,7 @@ func TestGetCombinedStatus_ErrorResponse(t *testing.T) { }, } - _, err := client.GetCombinedStatus("owner", "repo", "abc123") + _, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err == nil { t.Error("expected error for 404 response, got nil") } @@ -539,7 +540,7 @@ func TestGetCombinedStatus_EmptyState(t *testing.T) { }, } - status, err := client.GetCombinedStatus("owner", "repo", "abc123") + status, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -570,7 +571,7 @@ func TestGetJSON_OmitsAuthorizationWithoutToken(t *testing.T) { }, } - if _, err := client.GetPullRequest("owner", "repo", 1); err != nil { + if _, err := client.GetPullRequest(context.Background(), "owner", "repo", 1); err != nil { t.Fatalf("unexpected error: %v", err) } if len(gotAuth) != 0 { @@ -607,7 +608,7 @@ func TestGetCheckRuns_Paginates(t *testing.T) { }, } - runs, err := client.GetCheckRuns("owner", "repo", "abc123") + runs, err := client.GetCheckRuns(context.Background(), "owner", "repo", "abc123") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -632,7 +633,7 @@ func TestGetCheckRuns_ForbiddenHint(t *testing.T) { }, } - _, err := client.GetCheckRuns("owner", "repo", "abc123") + _, err := client.GetCheckRuns(context.Background(), "owner", "repo", "abc123") if err == nil { t.Fatal("expected error for 403 response, got nil") } @@ -662,7 +663,7 @@ func TestFetchCIStatus_ActionsOnlyRepo(t *testing.T) { }, } - ci, err := FetchCIStatus(client, "owner", "repo", "abc123") + ci, err := FetchCIStatus(context.Background(), client, "owner", "repo", "abc123") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -690,7 +691,7 @@ func TestFetchCIStatus_PropagatesErrors(t *testing.T) { }, } - _, err := FetchCIStatus(client, "owner", "repo", "abc123") + _, err := FetchCIStatus(context.Background(), client, "owner", "repo", "abc123") if err == nil || !strings.Contains(err.Error(), "check runs") { t.Errorf("expected check runs error, got %v", err) } diff --git a/internal/github/efficiency_test.go b/internal/github/efficiency_test.go index 8e79432..1fc6866 100644 --- a/internal/github/efficiency_test.go +++ b/internal/github/efficiency_test.go @@ -1,6 +1,7 @@ package github import ( + "context" "errors" "fmt" "net/http" @@ -37,7 +38,7 @@ func TestGetJSON_UsesETagAndServesCachedBodyOn304(t *testing.T) { }) for i := 0; i < 3; i++ { - pr, err := c.GetPullRequest("o", "r", 7) + pr, err := c.GetPullRequest(context.Background(), "o", "r", 7) if err != nil { t.Fatalf("request %d: %v", i, err) } @@ -63,13 +64,13 @@ func TestGetJSON_RefreshesCacheWhenResourceChanges(t *testing.T) { fmt.Fprintf(w, `{"state":"pending","total_count":%d}`, v) }) - s, _ := c.GetCombinedStatus("o", "r", "sha") + s, _ := c.GetCombinedStatus(context.Background(), "o", "r", "sha") atomic.StoreInt32(&version, 2) - s2, err := c.GetCombinedStatus("o", "r", "sha") + s2, err := c.GetCombinedStatus(context.Background(), "o", "r", "sha") if err != nil { t.Fatal(err) } - s3, _ := c.GetCombinedStatus("o", "r", "sha") + s3, _ := c.GetCombinedStatus(context.Background(), "o", "r", "sha") if s.TotalCount != 1 || s2.TotalCount != 2 || s3.TotalCount != 2 { t.Errorf("got %d, %d, %d; want 1, 2, 2", s.TotalCount, s2.TotalCount, s3.TotalCount) } @@ -85,8 +86,8 @@ func TestGetJSON_CachesPerPath(t *testing.T) { fmt.Fprintf(w, `{"number":%s}`, n) }) - a, _ := c.GetPullRequest("o", "r", 1) - b, _ := c.GetPullRequest("o", "r", 2) + a, _ := c.GetPullRequest(context.Background(), "o", "r", 1) + b, _ := c.GetPullRequest(context.Background(), "o", "r", 2) if a.Number != 1 || b.Number != 2 { t.Errorf("got #%d and #%d", a.Number, b.Number) } @@ -99,15 +100,15 @@ func TestGetJSON_NoETagMeansNoConditionalRequest(t *testing.T) { } fmt.Fprint(w, `{"number":1}`) }) - c.GetPullRequest("o", "r", 1) - c.GetPullRequest("o", "r", 1) + c.GetPullRequest(context.Background(), "o", "r", 1) + c.GetPullRequest(context.Background(), "o", "r", 1) } func TestGetJSON_Unexpected304IsAnError(t *testing.T) { c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotModified) }) - if _, err := c.GetPullRequest("o", "r", 1); err == nil { + if _, err := c.GetPullRequest(context.Background(), "o", "r", 1); err == nil { t.Error("a 304 without a cached body must not decode as an empty PR") } } @@ -134,7 +135,7 @@ func TestClient_RecordsRateLimitHeaders(t *testing.T) { if c.RateLimit().Known { t.Error("rate limit should be unknown before any request") } - c.GetPullRequest("o", "r", 1) + c.GetPullRequest(context.Background(), "o", "r", 1) rl := c.RateLimit() if !rl.Known || rl.Limit != 5000 || rl.Remaining != 4321 || rl.Reset.Unix() != reset { t.Errorf("RateLimit() = %+v", rl) @@ -251,7 +252,7 @@ func TestGetCheckRuns_RateLimitHasNoPermissionHint(t *testing.T) { w.WriteHeader(http.StatusForbidden) fmt.Fprint(w, `{"message":"API rate limit exceeded"}`) }) - _, err := c.GetCheckRuns("o", "r", "sha") + _, err := c.GetCheckRuns(context.Background(), "o", "r", "sha") if limited, _ := IsRateLimited(err); !limited { t.Fatalf("expected a rate limit error, got %v", err) } @@ -273,7 +274,7 @@ func TestClient_ConcurrentRequestsAreSafe(t *testing.T) { wg.Add(1) go func(n int) { defer wg.Done() - c.GetPullRequest("o", "r", n%3) + c.GetPullRequest(context.Background(), "o", "r", n%3) c.RateLimit() }(i) } diff --git a/internal/github/integration_test.go b/internal/github/integration_test.go index 0c23377..26f4817 100644 --- a/internal/github/integration_test.go +++ b/internal/github/integration_test.go @@ -14,6 +14,7 @@ package github import ( + "context" "errors" "net/http" "os" @@ -40,7 +41,7 @@ func integrationClient(t *testing.T) *Client { func TestIntegration_ActionsOnlyCommitResolvesToSuccess(t *testing.T) { c := integrationClient(t) - status, err := c.GetCombinedStatus(fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) + status, err := c.GetCombinedStatus(context.Background(), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) if err != nil { t.Fatalf("GetCombinedStatus: %v", err) } @@ -49,7 +50,7 @@ func TestIntegration_ActionsOnlyCommitResolvesToSuccess(t *testing.T) { t.Logf("note: legacy status changed to state=%q total_count=%d", status.State, status.TotalCount) } - runs, err := c.GetCheckRuns(fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) + runs, err := c.GetCheckRuns(context.Background(), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) if err != nil { t.Fatalf("GetCheckRuns: %v", err) } @@ -65,7 +66,7 @@ func TestIntegration_ActionsOnlyCommitResolvesToSuccess(t *testing.T) { } func TestIntegration_FetchCIStatus(t *testing.T) { - ci, err := FetchCIStatus(integrationClient(t), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) + ci, err := FetchCIStatus(context.Background(), integrationClient(t), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) if err != nil { t.Fatalf("FetchCIStatus: %v", err) } @@ -78,7 +79,7 @@ func TestIntegration_FetchCIStatus(t *testing.T) { } func TestIntegration_MergedPullRequest(t *testing.T) { - pr, err := integrationClient(t).GetPullRequest(fixtureOwner, fixtureRepo, fixtureMergedPR) + pr, err := integrationClient(t).GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, fixtureMergedPR) if err != nil { t.Fatalf("GetPullRequest: %v", err) } @@ -116,13 +117,13 @@ func TestIntegration_ConditionalRequestsAreFree(t *testing.T) { rec := &statusRecorder{} c.HTTPClient.Transport = rec - first, err := c.GetPullRequest(fixtureOwner, fixtureRepo, fixtureMergedPR) + first, err := c.GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, fixtureMergedPR) if err != nil { t.Fatalf("first request: %v", err) } before := c.RateLimit() - second, err := c.GetPullRequest(fixtureOwner, fixtureRepo, fixtureMergedPR) + second, err := c.GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, fixtureMergedPR) if err != nil { t.Fatalf("second request: %v", err) } @@ -146,7 +147,7 @@ func TestIntegration_ConditionalRequestsAreFree(t *testing.T) { } func TestIntegration_MissingPullRequestIs404(t *testing.T) { - _, err := integrationClient(t).GetPullRequest(fixtureOwner, fixtureRepo, 999999) + _, err := integrationClient(t).GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, 999999) var apiErr *APIError if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusNotFound { t.Errorf("expected a 404 APIError, got %v", err) diff --git a/internal/github/search_test.go b/internal/github/search_test.go index 3fcc3aa..3416580 100644 --- a/internal/github/search_test.go +++ b/internal/github/search_test.go @@ -1,6 +1,7 @@ package github import ( + "context" "fmt" "net/http" "strconv" @@ -27,7 +28,7 @@ func TestSearchPullRequests_ParsesResultsAndSkipsIssues(t *testing.T) { searchItem("Alice", "api", 1, true), searchItem("bob", "web", 2, false), searchItem("bob", "web", 3, true)) }) - res, err := c.SearchPullRequests("is:pr is:open author:@me") + res, err := c.SearchPullRequests(context.Background(), "is:pr is:open author:@me") if err != nil { t.Fatal(err) } @@ -62,7 +63,7 @@ func TestSearchPullRequests_Paginates(t *testing.T) { fmt.Fprintf(w, `{"total_count":%d,"items":[%s]}`, searchPerPage+5, strings.Join(items, ",")) }) - res, err := c.SearchPullRequests("q") + res, err := c.SearchPullRequests(context.Background(), "q") if err != nil { t.Fatal(err) } @@ -82,7 +83,7 @@ func TestSearchPullRequests_CapsAndMarksIncomplete(t *testing.T) { fmt.Fprintf(w, `{"total_count":5000,"items":[%s]}`, strings.Join(items, ",")) }) - res, err := c.SearchPullRequests("q") + res, err := c.SearchPullRequests(context.Background(), "q") if err != nil { t.Fatal(err) } @@ -98,7 +99,7 @@ func TestSearchPullRequests_IncompleteFromGitHub(t *testing.T) { c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, `{"total_count":1,"incomplete_results":true,"items":[%s]}`, searchItem("o", "r", 1, true)) }) - res, err := c.SearchPullRequests("q") + res, err := c.SearchPullRequests(context.Background(), "q") if err != nil { t.Fatal(err) } @@ -111,7 +112,7 @@ func TestSearchPullRequests_Error(t *testing.T) { c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"message":"Validation Failed"}`, http.StatusUnprocessableEntity) }) - if _, err := c.SearchPullRequests("q"); err == nil || !strings.Contains(err.Error(), "422") { + if _, err := c.SearchPullRequests(context.Background(), "q"); err == nil || !strings.Contains(err.Error(), "422") { t.Errorf("expected a 422 error, got %v", err) } } diff --git a/internal/watcher/autowatch.go b/internal/watcher/autowatch.go index 3eeec9b..ed9955b 100644 --- a/internal/watcher/autowatch.go +++ b/internal/watcher/autowatch.go @@ -1,6 +1,7 @@ package watcher import ( + "context" "fmt" "strings" "time" @@ -29,8 +30,8 @@ var AutoWatchDescription = map[string]string{ // Searcher runs GitHub pull request searches. GetPullRequest tells a review request // that was withdrawn apart from a PR that was merged or closed. type Searcher interface { - SearchPullRequests(query string) (*github.SearchResult, error) - GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) + SearchPullRequests(ctx context.Context, query string) (*github.SearchResult, error) + GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) } // SyncResult reports what SyncAutoWatch changed. @@ -49,7 +50,7 @@ type SyncResult struct { // still notifies about it before dropping them. PRs watched by hand are never touched. // // It returns an error only if every search failed; other problems are Warnings. -func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { +func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*SyncResult, error) { result := &SyncResult{} found := map[string]*github.SearchResult{} var lastErr error @@ -60,7 +61,7 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { result.Warnings = append(result.Warnings, fmt.Sprintf("unknown auto-watch source %q ignored", source)) continue } - res, err := client.SearchPullRequests(query) + res, err := client.SearchPullRequests(ctx, query) if err != nil { lastErr = err result.Warnings = append(result.Warnings, fmt.Sprintf("search for %s failed: %v", AutoWatchDescription[source], err)) @@ -75,7 +76,7 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { if len(found) == 0 && lastErr != nil { return result, lastErr } - closed := closedReviewRequests(client, found) + closed := closedReviewRequests(ctx, client, found) err := config.Update(func(c *config.Config) error { changed := false @@ -125,7 +126,7 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { // because they were merged or closed: the search only returns open PRs, so they look // just like withdrawn requests. It runs before the config is locked, as it makes API // calls. A PR whose lookup fails is not reported, i.e. it's removed as before. -func closedReviewRequests(client Searcher, found map[string]*github.SearchResult) map[string]bool { +func closedReviewRequests(ctx context.Context, client Searcher, found map[string]*github.SearchResult) map[string]bool { res, ok := found[config.AutoWatchReviewRequested] if !ok || res.Incomplete { return nil // nothing will be removed @@ -144,7 +145,7 @@ func closedReviewRequests(client Searcher, found map[string]*github.SearchResult if pr.Source != config.AutoWatchReviewRequested || matches[key] { continue } - if ghPR, err := client.GetPullRequest(pr.Owner, pr.Repo, pr.Number); err == nil && ghPR.IsClosed() { + if ghPR, err := client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number); err == nil && ghPR.IsClosed() { closed[key] = true } } @@ -156,7 +157,7 @@ func prKey(owner, repo string, number int) string { } // syncAutoWatch refreshes the auto-watch searches during `prw run` and reports changes. -func (w *Watcher) syncAutoWatch() { +func (w *Watcher) syncAutoWatch(ctx context.Context) { sources := w.config.AutoWatch if fresh, err := config.Load(); err == nil { sources = fresh.AutoWatch // `prw watch --mine` may have run since startup @@ -170,7 +171,7 @@ func (w *Watcher) syncAutoWatch() { if !ok { return } - res, err := SyncAutoWatch(searcher, sources) + res, err := SyncAutoWatch(ctx, searcher, sources) for _, warning := range res.Warnings { fmt.Printf("Warning: auto-watch: %s\n", warning) } diff --git a/internal/watcher/autowatch_test.go b/internal/watcher/autowatch_test.go index 889a776..a2b23c1 100644 --- a/internal/watcher/autowatch_test.go +++ b/internal/watcher/autowatch_test.go @@ -22,7 +22,7 @@ type fakeSearcher struct { searches int } -func (f *fakeSearcher) SearchPullRequests(query string) (*github.SearchResult, error) { +func (f *fakeSearcher) SearchPullRequests(ctx context.Context, query string) (*github.SearchResult, error) { f.searches++ for source, q := range autoWatchQueries { if q == query { @@ -82,7 +82,7 @@ func TestSyncAutoWatch_AddsMatchesTaggedWithSource(t *testing.T) { config.AutoWatchReviewRequested: found(3), }} - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { t.Fatal(err) } @@ -116,7 +116,7 @@ func TestSyncAutoWatch_DropsWithdrawnReviewRequestsButKeepsMine(t *testing.T) { config.AutoWatchReviewRequested: found(21), }} - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { t.Fatal(err) } @@ -138,7 +138,7 @@ func TestSyncAutoWatch_IncompleteResultsNeverRemove(t *testing.T) { partial.Incomplete, partial.Total = true, 2 s := &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: partial}} - res, err := SyncAutoWatch(s, []string{config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchReviewRequested}) if err != nil { t.Fatal(err) } @@ -161,7 +161,7 @@ func TestSyncAutoWatch_OneFailedSearchDoesNotBlockTheOther(t *testing.T) { errs: map[string]error{config.AutoWatchReviewRequested: errors.New("boom")}, } - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { t.Fatalf("a partial failure is a warning, got %v", err) } @@ -179,7 +179,7 @@ func TestSyncAutoWatch_AllSearchesFailing(t *testing.T) { boom := errors.New("network down") s := &fakeSearcher{errs: map[string]error{config.AutoWatchMine: boom}} - if _, err := SyncAutoWatch(s, []string{config.AutoWatchMine}); !errors.Is(err, boom) { + if _, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine}); !errors.Is(err, boom) { t.Errorf("err = %v, want the search error", err) } } @@ -189,7 +189,7 @@ func TestSyncAutoWatch_SkipsSourceDisabledMeanwhile(t *testing.T) { saveConfig(t, &config.Config{}) // `prw unwatch --mine` ran after the caller read the config s := &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchMine: found(1)}} - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine}) if err != nil { t.Fatal(err) } @@ -201,7 +201,7 @@ func TestSyncAutoWatch_SkipsSourceDisabledMeanwhile(t *testing.T) { func TestSyncAutoWatch_UnknownSource(t *testing.T) { useTempConfig(t) saveConfig(t, &config.Config{AutoWatch: []string{"bogus"}}) - res, err := SyncAutoWatch(&fakeSearcher{}, []string{"bogus"}) + res, err := SyncAutoWatch(context.Background(), &fakeSearcher{}, []string{"bogus"}) if err != nil { t.Fatal(err) } diff --git a/internal/watcher/closed_pr_test.go b/internal/watcher/closed_pr_test.go index 6127ddc..f5e69bd 100644 --- a/internal/watcher/closed_pr_test.go +++ b/internal/watcher/closed_pr_test.go @@ -1,6 +1,7 @@ package watcher import ( + "context" "testing" "github.com/devblac/prw/internal/config" @@ -19,7 +20,7 @@ func TestCheckPR_MergedPRIsRemoved(t *testing.T) { cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "success")}} w := New(client, cfg, notifier) - closed, err := w.checkPR(&cfg.WatchedPRs[0]) + closed, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -44,7 +45,7 @@ func TestCheckPR_ClosedWithoutMergeIsRemoved(t *testing.T) { cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "failure")}} w := New(client, cfg, notifier) - closed, err := w.checkPR(&cfg.WatchedPRs[0]) + closed, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -62,7 +63,7 @@ func TestCheckPR_MergedNotifiesEvenWithFailFilter(t *testing.T) { cfg := &config.Config{NotificationFilter: config.NotificationFilterFail, WatchedPRs: []config.WatchedPR{watched(1, "success")}} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("unexpected error: %v", err) } if len(notifier.events) != 1 { @@ -80,7 +81,7 @@ func TestCheckPR_OpenPRIsNotRemoved(t *testing.T) { cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "pending")}} w := New(client, cfg, &mockNotifier{}) - closed, err := w.checkPR(&cfg.WatchedPRs[0]) + closed, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -107,7 +108,7 @@ func TestCheckAllPRs_RemovesOnlyClosedPRs(t *testing.T) { inMemory := &config.Config{PollIntervalSeconds: 20, WatchedPRs: []config.WatchedPR{watched(1, "pending"), watched(2, "success")}} w := New(client, inMemory, &mockNotifier{}) - w.checkAllPRs() + w.checkAllPRs(context.Background()) if len(w.config.WatchedPRs) != 1 || w.config.WatchedPRs[0].Number != 1 { t.Fatalf("in-memory watch list after checkAllPRs = %+v, want only #1", w.config.WatchedPRs) @@ -144,7 +145,7 @@ func TestCheckAllPRs_RemovalSurvivesConcurrentChange(t *testing.T) { t.Fatal(err) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, _ := config.Load() if len(loaded.WatchedPRs) != 1 || loaded.WatchedPRs[0].Number != 2 { diff --git a/internal/watcher/noci_test.go b/internal/watcher/noci_test.go index 57b5ff0..d5b32e9 100644 --- a/internal/watcher/noci_test.go +++ b/internal/watcher/noci_test.go @@ -1,6 +1,7 @@ package watcher import ( + "context" "testing" "time" @@ -23,7 +24,7 @@ func TestCheckPR_ReportsNoCIWithoutNotifying(t *testing.T) { w := New(client, cfg, notifier) w.now = func() time.Time { return now } - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatal(err) } if got := cfg.WatchedPRs[0].LastKnownState; got != github.StateNoCI { @@ -41,7 +42,7 @@ func TestCheckPR_StaysPendingDuringGracePeriod(t *testing.T) { w := New(client, cfg, &mockNotifier{}) w.now = func() time.Time { return now } - w.checkPR(&cfg.WatchedPRs[0]) + w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if got := cfg.WatchedPRs[0].LastKnownState; got != "pending" { t.Errorf("a PR pushed 20s ago must stay pending while CI registers, got %q", got) } @@ -59,7 +60,7 @@ func TestCheckPR_NotifiesWhenCIShowsUpLater(t *testing.T) { w := New(client, cfg, notifier) w.now = func() time.Time { return now } - w.checkPR(&cfg.WatchedPRs[0]) + w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if len(notifier.events) != 1 || notifier.events[0].PreviousState != github.StateNoCI || notifier.events[0].CurrentState != "success" { t.Errorf("events = %+v, want one no_ci → success", notifier.events) } @@ -74,7 +75,7 @@ func TestCheckPR_StaysNoCIWhenOnlyMetadataChanges(t *testing.T) { w := New(client, cfg, notifier) w.now = func() time.Time { return now } - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatal(err) } if got := cfg.WatchedPRs[0].LastKnownState; got != github.StateNoCI { @@ -96,7 +97,7 @@ func TestCheckPR_NoCIToPendingWhenCIStartsAfterAPush(t *testing.T) { w := New(client, cfg, notifier) w.now = func() time.Time { return now } - w.checkPR(&cfg.WatchedPRs[0]) + w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if got := cfg.WatchedPRs[0].LastKnownState; got != "pending" || len(notifier.events) != 1 { t.Errorf("state = %q events = %d, want pending with one notification once a check exists", got, len(notifier.events)) } diff --git a/internal/watcher/ratelimit_test.go b/internal/watcher/ratelimit_test.go index 146883f..d15e962 100644 --- a/internal/watcher/ratelimit_test.go +++ b/internal/watcher/ratelimit_test.go @@ -16,12 +16,12 @@ type countingClient struct { prErr error } -func (c *countingClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { +func (c *countingClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { c.calls++ if c.prErr != nil { return nil, c.prErr } - return c.mockGitHubClient.GetPullRequest(owner, repo, number) + return c.mockGitHubClient.GetPullRequest(context.Background(), owner, repo, number) } func TestCheckAllPRs_StopsAndPausesOnRateLimit(t *testing.T) { @@ -37,7 +37,7 @@ func TestCheckAllPRs_StopsAndPausesOnRateLimit(t *testing.T) { w := New(client, cfg, &mockNotifier{}) w.now = func() time.Time { return reset.Add(-10 * time.Minute) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) if client.calls != 1 { t.Errorf("must stop after the first rate-limited request, made %d", client.calls) @@ -95,7 +95,7 @@ func TestCheckAllPRs_OtherErrorsDoNotPause(t *testing.T) { } w := New(client, cfg, &mockNotifier{}) - w.checkAllPRs() + w.checkAllPRs(context.Background()) if client.calls != 2 { t.Errorf("a non-rate-limit error must not stop the round, made %d calls", client.calls) diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 31085a9..53aa364 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -12,9 +12,9 @@ import ( // GitHubClient defines the interface for GitHub operations. type GitHubClient interface { - GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) - GetCombinedStatus(owner, repo, ref string) (*github.CombinedStatus, error) - GetCheckRuns(owner, repo, ref string) ([]github.CheckRun, error) + GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) + GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) + GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) } // ConfigStore defines the interface for config persistence. @@ -70,10 +70,10 @@ func (w *Watcher) Run(ctx context.Context) error { // Check immediately on startup if len(w.config.AutoWatch) > 0 { - w.syncAutoWatch() + w.syncAutoWatch(ctx) w.reloadWatchedPRs() } - w.checkAllPRs() + w.checkAllPRs(ctx) for { select { @@ -86,10 +86,10 @@ func (w *Watcher) Run(ctx context.Context) error { } w.reloadWatchedPRs() if len(w.config.AutoWatch) > 0 && w.now().Sub(w.lastAutoWatch) >= AutoWatchInterval { - w.syncAutoWatch() + w.syncAutoWatch(ctx) w.reloadWatchedPRs() } - w.checkAllPRs() + w.checkAllPRs(ctx) } } } @@ -114,23 +114,28 @@ func (w *Watcher) reloadWatchedPRs() { func (w *Watcher) RunOnce(ctx context.Context) error { fmt.Printf("Running one-time check with %d second poll interval...\n", w.config.PollIntervalSeconds) if len(w.config.AutoWatch) > 0 { - w.syncAutoWatch() + w.syncAutoWatch(ctx) w.reloadWatchedPRs() } if len(w.config.WatchedPRs) == 0 { fmt.Println(noPRsMessage) return nil } - w.checkAllPRs() + w.checkAllPRs(ctx) return nil } -func (w *Watcher) checkAllPRs() { +func (w *Watcher) checkAllPRs(ctx context.Context) { var stillWatched, done []config.WatchedPR for i := range w.config.WatchedPRs { pr := w.config.WatchedPRs[i] - closed, err := w.checkPR(&pr) + closed, err := w.checkPR(ctx, &pr) + if err != nil && ctx.Err() != nil { + // Stopped mid-round (Ctrl-C): keep this and the remaining PRs unchanged. + stillWatched = append(stillWatched, w.config.WatchedPRs[i:]...) + break + } if err != nil { stillWatched = append(stillWatched, pr) // keep watching; preserve any partial update (e.g. title) if limited, resetAt := github.IsRateLimited(err); limited { @@ -176,9 +181,9 @@ func (w *Watcher) pauseForRateLimit(resetAt time.Time) { // checkPR checks one PR's CI status and reports whether it is now closed (merged or // not), in which case the caller should stop watching it. -func (w *Watcher) checkPR(pr *config.WatchedPR) (closed bool, err error) { +func (w *Watcher) checkPR(ctx context.Context, pr *config.WatchedPR) (closed bool, err error) { // Fetch the PR to get its state and current head SHA - ghPR, err := w.client.GetPullRequest(pr.Owner, pr.Repo, pr.Number) + ghPR, err := w.client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number) if err != nil { return false, fmt.Errorf("failed to fetch PR: %w", err) } @@ -215,7 +220,7 @@ func (w *Watcher) checkPR(pr *config.WatchedPR) (closed bool, err error) { currentSHA := ghPR.Head.SHA // Aggregate commit statuses and check runs (GitHub Actions) for the head commit - ci, err := github.FetchCIStatus(w.client, pr.Owner, pr.Repo, currentSHA) + ci, err := github.FetchCIStatus(ctx, w.client, pr.Owner, pr.Repo, currentSHA) if err != nil { return false, fmt.Errorf("failed to fetch status: %w", err) } diff --git a/internal/watcher/watcher_persist_test.go b/internal/watcher/watcher_persist_test.go index 2bdd92d..80b057e 100644 --- a/internal/watcher/watcher_persist_test.go +++ b/internal/watcher/watcher_persist_test.go @@ -1,6 +1,7 @@ package watcher import ( + "context" "os" "path/filepath" "testing" @@ -63,7 +64,7 @@ func TestWatcherReloadsWatchListEachTick(t *testing.T) { t.Fatalf("watcher did not pick up the new watch list: %+v", w.config.WatchedPRs) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, err := config.Load() if err != nil { @@ -106,7 +107,7 @@ func TestWatcherDoesNotResurrectUnwatchedPR(t *testing.T) { t.Fatal(err) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, _ := config.Load() if len(loaded.WatchedPRs) != 1 || loaded.WatchedPRs[0].Number != 1 { @@ -141,7 +142,7 @@ func TestWatcherKeepsPRAddedDuringPoll(t *testing.T) { t.Fatal(err) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, _ := config.Load() if len(loaded.WatchedPRs) != 2 { @@ -171,7 +172,7 @@ func TestWatcherDoesNotPersistRunOnlyOverrides(t *testing.T) { prs: map[string]*github.PullRequest{"owner/repo/1": newPR(1, "sha1")}, statuses: map[string]*github.CombinedStatus{"sha1": {State: "success", SHA: "sha1"}}, } - New(client, inMemory, &mockNotifier{}).checkAllPRs() + New(client, inMemory, &mockNotifier{}).checkAllPRs(context.Background()) loaded, _ := config.Load() if loaded.NotificationFilter != config.NotificationFilterChange { @@ -200,3 +201,26 @@ func TestWatcherReloadKeepsListOnLoadError(t *testing.T) { t.Errorf("a config read error must not empty the watch list: %+v", w.config.WatchedPRs) } } + +func TestCheckAllPRs_StoppedMidRoundKeepsEveryPR(t *testing.T) { + useTempConfig(t) + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "success"), watched(2, "failure")}} + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() // Ctrl-C: every request now fails with context.Canceled + client := &mockGitHubClient{err: ctx.Err()} + notifier := &mockNotifier{} + w := New(client, cfg, notifier) + + w.checkAllPRs(ctx) + + loaded, _ := config.Load() + if len(loaded.WatchedPRs) != 2 || loaded.WatchedPRs[0].LastKnownState != "success" || loaded.WatchedPRs[1].LastKnownState != "failure" { + t.Errorf("watch list = %+v, want both PRs unchanged", loaded.WatchedPRs) + } + if len(notifier.events) != 0 { + t.Errorf("events = %+v", notifier.events) + } +} diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 4cecfcd..53b45d8 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -20,14 +20,14 @@ type mockGitHubClient struct { err error } -func (m *mockGitHubClient) GetCheckRuns(owner, repo, ref string) ([]github.CheckRun, error) { +func (m *mockGitHubClient) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) { if m.err != nil { return nil, m.err } return m.checkRuns[ref], nil } -func (m *mockGitHubClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { +func (m *mockGitHubClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { if m.err != nil { return nil, m.err } @@ -39,7 +39,7 @@ func (m *mockGitHubClient) GetPullRequest(owner, repo string, number int) (*gith return pr, nil } -func (m *mockGitHubClient) GetCombinedStatus(owner, repo, ref string) (*github.CombinedStatus, error) { +func (m *mockGitHubClient) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) { if m.err != nil { return nil, m.err } @@ -95,7 +95,7 @@ func TestWatcherNotificationError(t *testing.T) { w := New(client, cfg, notifier) // Should not return error, just print warning - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Errorf("checkPR should not fail when notification fails: %v", err) } } @@ -133,7 +133,7 @@ func TestWatcherNoStatusChange(t *testing.T) { w := New(client, cfg, notifier) // Check the PR once - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -175,7 +175,7 @@ func TestWatcherStatusChange(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -233,7 +233,7 @@ func TestWatcherFirstCheck(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -263,7 +263,7 @@ func TestWatcherGitHubError(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - _, err := w.checkPR(&cfg.WatchedPRs[0]) + _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err == nil { t.Error("expected error when GitHub API fails") } @@ -386,7 +386,7 @@ func TestWatcherUpdateTitle(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -425,7 +425,7 @@ func TestWatcherStatusError(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - _, err := w.checkPR(&cfg.WatchedPRs[0]) + _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err == nil { t.Error("expected error when status fetch fails") } @@ -459,7 +459,7 @@ func TestWatcherCheckAllPRs(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - w.checkAllPRs() + w.checkAllPRs(context.Background()) // Both PRs should be notified if len(notifier.events) != 2 { @@ -502,7 +502,7 @@ func TestWatcherCheckAllPRsWithError(t *testing.T) { w := New(client, cfg, notifier) // This should handle errors gracefully and print warnings - w.checkAllPRs() + w.checkAllPRs(context.Background()) // Only one PR should be successfully checked if len(notifier.events) != 1 { @@ -560,7 +560,7 @@ func TestWatcherNotificationFilterFailOnly(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -600,7 +600,7 @@ func TestWatcherNotificationFilterSuccessOnly(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } From 20e5b1b3dfefd95d0eef189f0e7c73b29f806075 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:37:36 -0300 Subject: [PATCH 06/11] fix: evict one ETag cache entry instead of clearing the whole cache When the 512-entry cache was full, every new 200 response wiped it. With more paths polled in rotation than fit (~170 PRs, fewer with paginated check runs) it was cleared over and over and nearly every request became unconditional, losing the free 304s. A new path now evicts a single random entry (refreshing a cached path evicts nothing). Random rather than LRU on purpose: cyclic polling is LRU's worst case. Simulated with 640 paths: 64% hits, was 0%. Addresses Codex review on #14. Co-Authored-By: Claude Opus 5.5 --- internal/github/client.go | 12 ++++++++- internal/github/efficiency_test.go | 42 ++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/internal/github/client.go b/internal/github/client.go index 2b6adf3..a5b9069 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -506,9 +506,19 @@ func (c *Client) getJSON(ctx context.Context, path string, v interface{}) error func (c *Client) storeCache(path string, entry cachedResponse) { c.mu.Lock() defer c.mu.Unlock() - if c.cache == nil || len(c.cache) >= maxCacheEntries { + if c.cache == nil { c.cache = make(map[string]cachedResponse) } + if _, exists := c.cache[path]; !exists && len(c.cache) >= maxCacheEntries { + // Make room by evicting one entry at random (map iteration order is random). + // Clearing the whole cache, or LRU, would make every request unconditional + // once more paths are polled in rotation than fit: polling cycles through + // the same paths, which is LRU's worst case. Random eviction keeps most hits. + for victim := range c.cache { + delete(c.cache, victim) + break + } + } c.cache[path] = entry } diff --git a/internal/github/efficiency_test.go b/internal/github/efficiency_test.go index 1fc6866..8c6ac79 100644 --- a/internal/github/efficiency_test.go +++ b/internal/github/efficiency_test.go @@ -280,3 +280,45 @@ func TestClient_ConcurrentRequestsAreSafe(t *testing.T) { } wg.Wait() } + +func TestStoreCache_FullCacheKeepsOtherEntries(t *testing.T) { + c := NewClient("") + for i := 0; i < maxCacheEntries; i++ { + c.storeCache(fmt.Sprintf("/p/%d", i), cachedResponse{etag: "e"}) + } + c.storeCache("/p/0", cachedResponse{etag: "refreshed"}) // a changed resource + if n := len(c.cache); n != maxCacheEntries || c.cache["/p/0"].etag != "refreshed" { + t.Errorf("refreshing a cached path must not evict anything: %d entries", n) + } + c.storeCache("/new", cachedResponse{etag: "e"}) + if n := len(c.cache); n != maxCacheEntries { + t.Errorf("a new path must evict exactly one entry, cache has %d", n) + } +} + +func TestStoreCache_PollingMorePathsThanFitStillHits(t *testing.T) { + // e.g. ~200 watched PRs, 3 paths each, polled in rotation. + c := NewClient("") + paths := maxCacheEntries + maxCacheEntries/4 + hits, lookups := 0, 0 + for round := 0; round < 20; round++ { + for i := 0; i < paths; i++ { + path := fmt.Sprintf("/p/%d", i) + if _, ok := c.cache[path]; ok { + if round > 0 { + hits++ + } + } else { + c.storeCache(path, cachedResponse{etag: "e"}) + } + if round > 0 { + lookups++ + } + } + } + rate := float64(hits) / float64(lookups) + t.Logf("hit rate %.0f%% polling %d paths through a %d-entry cache", rate*100, paths, maxCacheEntries) + if rate < 0.5 { + t.Errorf("hit rate %.0f%%; overflowing the cache must not make every request unconditional", rate*100) + } +} From 2614d38b1723496e2c29ac92ada246e9642ab8eb Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:38:01 -0300 Subject: [PATCH 07/11] docs: changelog, README and tasks for the Codex review fixes Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 +++++++ README.md | 2 +- TASKS.md | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc71faa..4a90e9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Integration tests against the real GitHub API (`make integration`, build tag `integration`), run nightly by a new `Integration` workflow. They fail against the old status-only logic. ### Fixed +- A check whose workflow ran more than once for the same commit (e.g. an older run cancelled by `concurrency: cancel-in-progress`) kept the PR failed even after the newer run passed. Only the newest run of each check counts now, like on GitHub. +- `prw wait` and the MCP `wait_for_ci` tool could return the final result of the previous commit when a new one was pushed during a poll. The PR head is now re-checked before returning, and waiting continues on the new commit. +- `prw wait --timeout`, the MCP `timeout_seconds` / cancellation and Ctrl-C now cancel the GitHub requests in flight; on a stalled network they used to be noticed only after the whole poll finished (up to 15 seconds per request). `prw run` also stops at once on Ctrl-C. +- PRs auto-watched with `--review-requested` that were merged or closed while `prw` wasn't running are no longer dropped as withdrawn review requests: they get their final `merged`/`closed` notification first. +- A `no_ci` PR no longer flaps to `pending` (with a "no_ci → pending" notification) when someone comments or changes a label; it stays `no_ci` until an actual check or status appears. +- The ETag cache no longer clears itself entirely when full, which made almost every request unconditional when watching many PRs; it evicts one entry instead. +- GovSlack webhook URLs (`hooks.slack-gov.com`) are detected as Slack. - PRs without any CI are reported as `no_ci` instead of `pending` forever. Previously `prw wait` and the MCP `wait_for_ci` tool blocked until their timeout on such PRs (repositories without CI, or stacked PRs whose base branch runs none). A PR is `no_ci` once it has no checks or statuses at all and hasn't been updated for 3 minutes, so CI that is still registering after a push isn't mistaken for missing CI. `prw wait` exits `4` for it; `prw run` doesn't notify about it. - PR owner and repository names are compared case-insensitively (as on GitHub), so `DevBlac/PRW#5` and `devblac/prw#5` are no longer two separate watch-list entries. - Requests without a token no longer send an empty `Authorization: Bearer ` header, which GitHub rejects with 401 even for public repositories. diff --git a/README.md b/README.md index f54da96..739db12 100644 --- a/README.md +++ b/README.md @@ -387,7 +387,7 @@ If you configure a `webhook_url`, `prw` will POST to it on every status change. | URL | Format | What you get | |-----|--------|--------------| -| `https://hooks.slack.com/...` | `slack` | A message with the PR link, status transition and failed checks | +| `https://hooks.slack.com/...` (or GovSlack `hooks.slack-gov.com`) | `slack` | A message with the PR link, status transition and failed checks | | `https://discord.com/api/webhooks/...` | `discord` | A colored embed (green / red / yellow) with the same details | | anything else | `generic` | Raw JSON (below), for your own services | diff --git a/TASKS.md b/TASKS.md index 80ba30e..6c34e5a 100644 --- a/TASKS.md +++ b/TASKS.md @@ -90,6 +90,7 @@ Legend — Impact: 1–5 · Effort: S (≤½ day), M (1–2 days), L (3+ days) - ✅ #10 `prw watch --mine` / `--review-requested` — done (search-backed subscriptions refreshed every 5 min by `prw run`; verified live: 18 open PRs across 7 repos found, CI checked, cleanly removed by `unwatch --mine`) - 🆕 #18 **Report "no CI" instead of `pending` forever** (found while testing #10: 14 of 18 real PRs have zero checks — repos without CI, or stacked PRs whose base isn't `main`). Today `prw wait` / `wait_for_ci` would wait until the timeout on them. Needs a grace period after the last push (CI takes a moment to register checks) before concluding "no CI". Impact 4, effort S–M. - ✅ #18 `no_ci` state — done (0 checks + PR not updated for 3 min → `no_ci`; `prw wait` exits 4; no notification; verified live: 11 real PRs went from `pending` to `no_ci`, `prw wait` on one returns in 2s instead of the 30-min timeout) +- ✅ Codex review feedback on #6, #7, #13–#17 — done in `fix/codex-review`: superseded check runs (verified live on zephyrproject-rtos/zephyr#105183: was `failure`, now `success`), head re-check in `prw wait`, context propagation into every API request, closed review-request PRs keep their notification, no `no_ci` flapping, ETag cache eviction, GovSlack. - Next: #11 review & mergeability events ## Planned From 04ad98581512eb1263d74f35d6dd482ca2e7b220 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 03:11:36 -0300 Subject: [PATCH 08/11] feat: notify about reviews and merge conflicts, not just CI `prw run` now also tells you when a watched PR is approved, gets changes requested, runs into merge conflicts, or becomes ready to merge after branch protection, conflicts or an outdated branch were holding it back. - Review decision from /pulls/{n}/reviews: each reviewer's latest approval or change request counts, comments don't, dismissals withdraw it. One extra request per PR per poll, a free 304 when nothing changed. - Merge state from the PR's mergeable_state (already fetched); "unknown" while GitHub computes it keeps the last value. - Quiet by design: nothing on the first check of a PR; "ready" is not sent with the CI or approval notification that caused it, nor for repositories without branch protection (unstable -> clean is just CI passing). - Events carry a Kind; console, native, Slack/Discord ("Review:" / "Merge:") and generic webhooks ("pr_review_change" / "pr_merge_state_change") render them. --on fail keeps changes_requested and conflict, --on success approved and ready. - `prw list` gains REVIEW and MERGE columns (review/merge in --json and MCP list_watched); `prw wait --json` and the MCP tools report merge_state. - `prw config set pr_events false` turns it off. Verified live: devblac/prw#12 ready, cli/cli#14727 blocked; a simulated blocked -> ready notified once, ready -> blocked stayed silent. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + README.md | 25 ++++ TASKS.md | 3 +- cmd/prw/main.go | 30 +++- cmd/prw/mcp.go | 6 +- cmd/prw/wait.go | 30 ++-- internal/config/config.go | 9 ++ internal/github/client.go | 4 + internal/github/review.go | 102 ++++++++++++++ internal/github/review_test.go | 83 +++++++++++ internal/notify/native.go | 2 +- internal/notify/notify.go | 22 ++- internal/notify/pr_events_format_test.go | 75 ++++++++++ internal/notify/webhook_format.go | 45 +++++- internal/watcher/pr_events_test.go | 171 +++++++++++++++++++++++ internal/watcher/watcher.go | 85 +++++++++++ internal/watcher/watcher_test.go | 8 ++ 17 files changed, 671 insertions(+), 30 deletions(-) create mode 100644 internal/github/review.go create mode 100644 internal/github/review_test.go create mode 100644 internal/notify/pr_events_format_test.go create mode 100644 internal/watcher/pr_events_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a90e9c..5ebb476 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `prw run` now notifies once (`merged` or `closed`) and automatically stops watching a PR once it is merged or closed, instead of polling it forever. This notification always fires, regardless of `--on`/`notification_filter`. ### Added +- Review and merge events: `prw run` notifies when a PR is approved, gets changes requested, runs into merge conflicts, or becomes ready to merge after branch protection was holding it back. Shown in console, native, Slack/Discord and generic webhooks (`type` `pr_review_change` / `pr_merge_state_change`), filtered by `--on` (`fail`: changes requested and conflicts; `success`: approved and ready). `prw list` gains `REVIEW` and `MERGE` columns (`review` / `merge` in `--json` and MCP `list_watched`); `prw wait --json` and the MCP tools report `merge_state`. Disable with `prw config set pr_events false`. - Auto-watch: `prw watch --mine` follows all your open PRs and `prw watch --review-requested` the PRs where your review is requested, across every repository. Matches are added immediately; `prw run` refreshes the searches every 5 minutes. Review-request PRs are dropped when the request goes away; PRs watched by hand are never touched by it. `prw unwatch --mine` / `--review-requested` turns it off and removes the PRs it added. `prw list` gains an `ADDED BY` column (and `source` in `--json`); `prw config show` shows `auto_watch`. - `prw mcp`: a Model Context Protocol server over stdio for AI coding agents, with tools `get_ci_status`, `wait_for_ci` (progress notifications, cancellation, fail-fast), `list_watched`, `watch_pr` and `unwatch_pr`. Implemented without new dependencies (the official Go SDK needs Go 1.25). - Conditional requests (ETags): repeated polls of unchanged resources are answered with `304 Not Modified`, which doesn't count against the authenticated rate limit. Measured against the real API: after the first poll, 5 further polls of a PR (15 requests) consumed 0 quota. Previously each watched PR cost 3 requests per poll (~540/hour at the default 20s interval), so about 9 PRs exhausted the 5,000/hour limit. diff --git a/README.md b/README.md index 739db12..76f651b 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,7 @@ The tool is deliberately minimal. It does one thing well: watch PRs and tell you - **Auto-cleanup**: merged/closed PRs are notified once and dropped from the watch list by `prw run` - **Watch multiple PRs** from different repositories simultaneously - **Instant notifications** when CI status changes (pending, success, failure, error) +- **Review & merge events**: approved, changes requested, merge conflicts, ready to merge - **Chat-ops broadcast**: one command to push current PR status to Slack/Discord (`prw broadcast`) - **Webhook support** for Slack, Discord, or custom integrations - **Native OS notifications** via system toasts (macOS/Linux/Windows) @@ -345,6 +346,7 @@ prw config set webhook_url https://hooks.slack.com/services/YOUR/WEBHOOK/URL - **`webhook_url`**: Optional HTTP endpoint for notifications - **`webhook_format`**: Payload format: `auto` (default, detected from the URL), `slack`, `discord`, or `generic` - **`notification_native`**: Enable native OS notifications (true/false, default: false) +- **`pr_events`**: Also notify about reviews and merge conflicts / readiness (true/false, default: true) - **`github_token`**: GitHub Personal Access Token (prefer env var `GITHUB_TOKEN`) ## Notifications @@ -432,6 +434,29 @@ prw run --on change The same setting can be persisted via `prw config set notification_filter `. +### Reviews and merge conflicts + +Besides CI, `prw run` notifies when a PR's review decision or merge state changes: + +| Event | When | +|-------|------| +| `approved` | A reviewer approved and nobody's latest review requests changes | +| `changes_requested` | A reviewer's latest review requests changes | +| `conflict` | The PR has merge conflicts with its base branch | +| `ready` | GitHub says the PR can be merged, after branch protection, conflicts or an outdated branch were holding it back | + +``` +🔔 Review Update! + PR: kubernetes/kubernetes#12345 + Review: none → approved +``` + +Webhooks get the same shapes as CI changes, with `"type": "pr_review_change"` or `"pr_merge_state_change"` in the generic payload (and `Review:` / `Merge:` instead of `Status:` in Slack and Discord messages). With `--on fail` you only get `changes_requested` and `conflict`; with `--on success`, `approved` and `ready`. + +A few rules keep this quiet: nothing is announced the first time a PR is checked; `ready` isn't sent together with the CI or approval notification that caused it, and not at all for repositories without branch protection (there, "CI passed" already means it); dismissed reviews and new review rounds don't notify until someone approves or requests changes. `prw list` shows the last review and merge state in the `REVIEW` and `MERGE` columns, `prw wait --json` and the MCP tools include `merge_state`. + +Turn these off with `prw config set pr_events false`. They cost one extra request per PR per poll (the reviews), answered with a free `304` when nothing changed. + ### Merged and closed PRs Once `prw run` sees that a watched PR was merged or closed, it sends one final notification (`CurrentState` is `merged` or `closed`, so you'll see e.g. `success → merged`) and removes the PR from the watch list — regardless of `--on`/`notification_filter`, since a merge is a lifecycle event, not a CI result. This keeps the watch list from growing forever. `prw list` and `prw broadcast` are unaffected: a PR only drops out once `prw run` has actually polled it. diff --git a/TASKS.md b/TASKS.md index 6c34e5a..2880380 100644 --- a/TASKS.md +++ b/TASKS.md @@ -91,7 +91,8 @@ Legend — Impact: 1–5 · Effort: S (≤½ day), M (1–2 days), L (3+ days) - 🆕 #18 **Report "no CI" instead of `pending` forever** (found while testing #10: 14 of 18 real PRs have zero checks — repos without CI, or stacked PRs whose base isn't `main`). Today `prw wait` / `wait_for_ci` would wait until the timeout on them. Needs a grace period after the last push (CI takes a moment to register checks) before concluding "no CI". Impact 4, effort S–M. - ✅ #18 `no_ci` state — done (0 checks + PR not updated for 3 min → `no_ci`; `prw wait` exits 4; no notification; verified live: 11 real PRs went from `pending` to `no_ci`, `prw wait` on one returns in 2s instead of the 30-min timeout) - ✅ Codex review feedback on #6, #7, #13–#17 — done in `fix/codex-review`: superseded check runs (verified live on zephyrproject-rtos/zephyr#105183: was `failure`, now `success`), head re-check in `prw wait`, context propagation into every API request, closed review-request PRs keep their notification, no `no_ci` flapping, ETag cache eviction, GovSlack. -- Next: #11 review & mergeability events +- ✅ #11 Review & mergeability events — done (approved / changes_requested / conflict / ready; verified live: devblac/prw#12 `ready`, cli/cli#14727 `blocked`, a simulated blocked → ready notified once and ready → blocked stayed silent) +- Next: #12 `prw checks ` ## Planned diff --git a/cmd/prw/main.go b/cmd/prw/main.go index bb10c5c..7719aa9 100644 --- a/cmd/prw/main.go +++ b/cmd/prw/main.go @@ -283,8 +283,8 @@ var listCmd = &cobra.Command{ } w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0) - fmt.Fprintln(w, "REPO\tPR\tSTATUS\tLAST CHECKED\tADDED BY\tTITLE") - fmt.Fprintln(w, "----\t--\t------\t------------\t--------\t-----") + fmt.Fprintln(w, "REPO\tPR\tSTATUS\tREVIEW\tMERGE\tLAST CHECKED\tADDED BY\tTITLE") + fmt.Fprintln(w, "----\t--\t------\t------\t-----\t------------\t--------\t-----") for _, pr := range cfg.WatchedPRs { repo := fmt.Sprintf("%s/%s", pr.Owner, pr.Repo) @@ -305,7 +305,7 @@ var listCmd = &cobra.Command{ if pr.Source != "" { addedBy = "--" + pr.Source } - fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\n", repo, prNum, status, lastChecked, addedBy, title) + fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n", repo, prNum, status, orDash(pr.LastReviewState), orDash(pr.LastMergeState), lastChecked, addedBy, title) } w.Flush() @@ -484,6 +484,7 @@ var configShowCmd = &cobra.Command{ fmt.Printf("webhook_format: %s\n", notify.NormalizeWebhookFormat(cfg.WebhookFormat)) fmt.Printf("notification_filter: %s\n", cfg.NotificationFilter) fmt.Printf("notification_native: %v\n", cfg.NotificationNative) + fmt.Printf("pr_events: %v\n", !cfg.DisablePREvents) _, tokenSource := cfg.ResolveToken() fmt.Printf("github_token: %s\n", tokenSource) @@ -510,7 +511,8 @@ Supported keys: - webhook_format: auto, slack, discord, or generic (default: auto, detected from the URL) - github_token: GitHub personal access token - notification_filter: change, fail, or success - - notification_native: enable native OS notifications (true/false)`, + - notification_native: enable native OS notifications (true/false) + - pr_events: notify about reviews and merge conflicts / readiness, not just CI (true/false, default: true)`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { key := args[0] @@ -557,6 +559,12 @@ func applyConfigSet(cfg *config.Config, key, value string) error { return fmt.Errorf("notification_native must be true or false") } cfg.NotificationNative = enabled + case "pr_events": + enabled, err := strconv.ParseBool(value) + if err != nil { + return fmt.Errorf("pr_events must be true or false") + } + cfg.DisablePREvents = !enabled default: return fmt.Errorf("unknown config key: %s", key) } @@ -595,6 +603,8 @@ func applyConfigUnset(cfg *config.Config, key string) error { cfg.NotificationFilter = config.NotificationFilterChange case "notification_native": cfg.NotificationNative = false + case "pr_events": + cfg.DisablePREvents = false default: return fmt.Errorf("unknown config key: %s", key) } @@ -623,6 +633,16 @@ type listPROutput struct { LastChecked *time.Time `json:"last_checked,omitempty"` Title string `json:"title,omitempty"` Source string `json:"source,omitempty"` // auto-watch source that added it; empty if added by hand + Review string `json:"review,omitempty"` // approved, changes_requested or none; empty until checked + Merge string `json:"merge,omitempty"` // ready, conflict, blocked, behind, unstable or draft +} + +// orDash shows a state that hasn't been observed yet as "-". +func orDash(s string) string { + if s == "" { + return "-" + } + return s } func outputJSONList(prs []config.WatchedPR) error { @@ -647,6 +667,8 @@ func outputJSONList(prs []config.WatchedPR) error { LastChecked: lastChecked, Title: pr.Title, Source: pr.Source, + Review: pr.LastReviewState, + Merge: pr.LastMergeState, }) } diff --git a/cmd/prw/mcp.go b/cmd/prw/mcp.go index 2a434a5..c126960 100644 --- a/cmd/prw/mcp.go +++ b/cmd/prw/mcp.go @@ -428,7 +428,7 @@ var mcpTools = []map[string]interface{}{ { "name": "get_ci_status", "title": "Get PR CI status", - "description": "Get the current CI status of a GitHub pull request: overall state (success, failure, error, pending, merged, closed, or no_ci when the PR has no checks at all), check counts, and failed checks with links. Aggregates GitHub Actions check runs and legacy commit statuses. Returns immediately.", + "description": "Get the current CI status of a GitHub pull request: overall state (success, failure, error, pending, merged, closed, or no_ci when the PR has no checks at all), check counts, failed checks with links, and merge_state (ready, conflict, blocked, behind, unstable, draft) telling whether the PR can be merged. Aggregates GitHub Actions check runs and legacy commit statuses. Returns immediately.", "inputSchema": map[string]interface{}{ "type": "object", "properties": map[string]interface{}{"pr_url": prURLSchema}, @@ -626,6 +626,8 @@ type watchedPROutput struct { Number int `json:"number"` Title string `json:"title,omitempty"` LastState string `json:"last_known_state"` + Review string `json:"review,omitempty"` // approved, changes_requested, none + Merge string `json:"merge,omitempty"` // ready, conflict, blocked, behind, unstable, draft LastChecked *time.Time `json:"last_checked,omitempty"` } @@ -642,7 +644,7 @@ func toolListWatched(ctx context.Context, s *mcpServer, _ json.RawMessage, _ io. } out := watchedPROutput{ URL: github.FormatPRURL(pr.Owner, pr.Repo, pr.Number), Owner: pr.Owner, Repo: pr.Repo, - Number: pr.Number, Title: pr.Title, LastState: state, + Number: pr.Number, Title: pr.Title, LastState: state, Review: pr.LastReviewState, Merge: pr.LastMergeState, } if !pr.LastChecked.IsZero() { t := pr.LastChecked diff --git a/cmd/prw/wait.go b/cmd/prw/wait.go index 77edee6..cc3e421 100644 --- a/cmd/prw/wait.go +++ b/cmd/prw/wait.go @@ -149,19 +149,22 @@ Progress goes to stderr; the result goes to stdout (as JSON with --json).`, // waitResult is the outcome of waitForCI; it is also the --json output. type waitResult struct { - Owner string `json:"owner"` - Repo string `json:"repo"` - Number int `json:"number"` - Title string `json:"title,omitempty"` - URL string `json:"url"` - SHA string `json:"sha,omitempty"` - State string `json:"state"` // success, failure, error, pending, merged, closed - FailedChecks []github.FailedCheck `json:"failed_checks,omitempty"` - TotalChecks int `json:"total_checks"` - PendingChecks int `json:"pending_checks"` - ElapsedSeconds float64 `json:"elapsed_seconds"` - TimedOut bool `json:"timed_out,omitempty"` - Interrupted bool `json:"interrupted,omitempty"` + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + Title string `json:"title,omitempty"` + URL string `json:"url"` + SHA string `json:"sha,omitempty"` + State string `json:"state"` // success, failure, error, pending, merged, closed + FailedChecks []github.FailedCheck `json:"failed_checks,omitempty"` + TotalChecks int `json:"total_checks"` + PendingChecks int `json:"pending_checks"` + // MergeState is GitHub's mergeability (ready, conflict, blocked, behind, unstable, + // draft); empty while GitHub is still computing it. + MergeState string `json:"merge_state,omitempty"` + ElapsedSeconds float64 `json:"elapsed_seconds"` + TimedOut bool `json:"timed_out,omitempty"` + Interrupted bool `json:"interrupted,omitempty"` } // exitErr maps the result to the command's exit code (nil means exit 0). @@ -282,6 +285,7 @@ func pollOnce(ctx context.Context, client ciClient, result *waitResult) (string, result.Title = pr.Title } result.SHA = pr.Head.SHA + result.MergeState = github.MergeStatus(pr) if pr.IsClosed() { result.State = "closed" diff --git a/internal/config/config.go b/internal/config/config.go index fb78d24..b722054 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,6 +27,9 @@ type Config struct { GitHubToken string `json:"github_token,omitempty"` NotificationFilter string `json:"notification_filter,omitempty"` NotificationNative bool `json:"notification_native,omitempty"` + // DisablePREvents turns off review (approved / changes requested) and merge + // (conflict / ready to merge) notifications; `pr_events` in `prw config`. + DisablePREvents bool `json:"disable_pr_events,omitempty"` // AutoWatch lists the searches (AutoWatchMine, AutoWatchReviewRequested) whose // matching PRs are added to the watch list automatically. @@ -101,6 +104,10 @@ type WatchedPR struct { LastKnownState string `json:"last_known_state,omitempty"` LastChecked time.Time `json:"last_checked,omitempty"` Title string `json:"title,omitempty"` + // LastReviewState and LastMergeState are the review decision and merge state + // (github.Review* / github.Merge*) seen on the last check; empty until observed. + LastReviewState string `json:"last_review_state,omitempty"` + LastMergeState string `json:"last_merge_state,omitempty"` // Source is the auto-watch source that added this PR, or empty if it was added by // hand with `prw watch `. Source string `json:"source,omitempty"` @@ -280,6 +287,8 @@ func (c *Config) ApplyPRState(pr WatchedPR) bool { w.LastKnownSHA = pr.LastKnownSHA w.LastKnownState = pr.LastKnownState w.LastChecked = pr.LastChecked + w.LastReviewState = pr.LastReviewState + w.LastMergeState = pr.LastMergeState if pr.Title != "" { w.Title = pr.Title } diff --git a/internal/github/client.go b/internal/github/client.go index a5b9069..ddbf810 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -112,6 +112,10 @@ type PullRequest struct { // UpdatedAt changes on every push (and on comments, labels, ...), so it is an // upper bound for how recently the head commit was pushed. UpdatedAt time.Time `json:"updated_at"` + // MergeableState is GitHub's verdict on whether the PR can be merged (clean, + // dirty, blocked, behind, unstable, draft, ...); "unknown" while it's computed. + // See MergeStatus. + MergeableState string `json:"mergeable_state"` } // StateNoCI is reported instead of "pending" for a PR that has no checks or diff --git a/internal/github/review.go b/internal/github/review.go new file mode 100644 index 0000000..a3de91e --- /dev/null +++ b/internal/github/review.go @@ -0,0 +1,102 @@ +package github + +import ( + "context" + "fmt" + "strings" +) + +// Review decisions, derived from the reviews of a PR. +const ( + ReviewApproved = "approved" + ReviewChangesRequested = "changes_requested" + ReviewNone = "none" +) + +// Merge states, derived from a PR's mergeable_state. +const ( + MergeReady = "ready" // clean: every requirement is met + MergeConflict = "conflict" // dirty: merge conflicts with the base branch + MergeBlocked = "blocked" // a branch protection requirement isn't met yet + MergeBehind = "behind" // the branch must be updated with the base first + MergeUnstable = "unstable" // mergeable, but non-required checks didn't pass + MergeDraft = "draft" +) + +const ( + reviewsPerPage = 100 + maxReviewsPages = 3 +) + +// Review is a single submitted pull request review. +type Review struct { + ID int64 `json:"id"` + User struct { + Login string `json:"login"` + } `json:"user"` + State string `json:"state"` // APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED, PENDING +} + +// GetReviews fetches the reviews of a pull request, oldest first. +func (c *Client) GetReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) { + var reviews []Review + for page := 1; page <= maxReviewsPages; page++ { + path := fmt.Sprintf("/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d", owner, repo, number, reviewsPerPage, page) + var batch []Review + if err := c.getJSON(ctx, path, &batch); err != nil { + return nil, err + } + reviews = append(reviews, batch...) + if len(batch) < reviewsPerPage { + break + } + } + return reviews, nil +} + +// ReviewDecision summarizes reviews like GitHub's review decision: changes requested +// if any reviewer's latest review requests changes, otherwise approved if any +// reviewer's latest review approves, otherwise none. Comments don't change a +// reviewer's verdict; a dismissed review withdraws it. Unlike GitHub's own decision, +// it doesn't know whether the approvals satisfy branch protection (MergeStatus does). +func ReviewDecision(reviews []Review) string { + verdict := map[string]string{} // reviewer → latest APPROVED / CHANGES_REQUESTED + for _, r := range reviews { + login := strings.ToLower(r.User.Login) + switch strings.ToUpper(r.State) { + case "APPROVED", "CHANGES_REQUESTED": + verdict[login] = strings.ToUpper(r.State) + case "DISMISSED": + delete(verdict, login) + } + } + decision := ReviewNone + for _, v := range verdict { + if v == "CHANGES_REQUESTED" { + return ReviewChangesRequested + } + decision = ReviewApproved + } + return decision +} + +// MergeStatus maps the PR's mergeable_state to one of the Merge* constants. It +// returns "" while GitHub is still computing it ("unknown"), when the previous value +// should be kept. +func MergeStatus(pr *PullRequest) string { + switch strings.ToLower(pr.MergeableState) { + case "clean", "has_hooks": + return MergeReady + case "dirty": + return MergeConflict + case "blocked": + return MergeBlocked + case "behind": + return MergeBehind + case "unstable": + return MergeUnstable + case "draft": + return MergeDraft + } + return "" +} diff --git a/internal/github/review_test.go b/internal/github/review_test.go new file mode 100644 index 0000000..20eb44a --- /dev/null +++ b/internal/github/review_test.go @@ -0,0 +1,83 @@ +package github + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func rev(login, state string) Review { + var r Review + r.User.Login = login + r.State = state + return r +} + +func TestReviewDecision(t *testing.T) { + tests := []struct { + name string + reviews []Review + want string + }{ + {"no reviews", nil, ReviewNone}, + {"only comments", []Review{rev("a", "COMMENTED")}, ReviewNone}, + {"approved", []Review{rev("a", "APPROVED")}, ReviewApproved}, + {"a comment after approving keeps the approval", []Review{rev("a", "APPROVED"), rev("a", "COMMENTED")}, ReviewApproved}, + {"changes requested wins over another approval", []Review{rev("a", "APPROVED"), rev("b", "CHANGES_REQUESTED")}, ReviewChangesRequested}, + {"approving after requesting changes", []Review{rev("b", "CHANGES_REQUESTED"), rev("b", "APPROVED")}, ReviewApproved}, + {"dismissed", []Review{rev("b", "CHANGES_REQUESTED"), rev("b", "DISMISSED")}, ReviewNone}, + {"logins are case-insensitive", []Review{rev("Bob", "CHANGES_REQUESTED"), rev("bob", "APPROVED")}, ReviewApproved}, + {"pending reviews don't count", []Review{rev("a", "PENDING")}, ReviewNone}, + } + for _, tt := range tests { + if got := ReviewDecision(tt.reviews); got != tt.want { + t.Errorf("%s: got %q, want %q", tt.name, got, tt.want) + } + } +} + +func TestMergeStatus(t *testing.T) { + for state, want := range map[string]string{ + "clean": MergeReady, "has_hooks": MergeReady, "dirty": MergeConflict, "blocked": MergeBlocked, + "behind": MergeBehind, "unstable": MergeUnstable, "draft": MergeDraft, "unknown": "", "": "", + } { + if got := MergeStatus(&PullRequest{MergeableState: state}); got != want { + t.Errorf("MergeStatus(%q) = %q, want %q", state, got, want) + } + } +} + +func TestGetReviews_Paginates(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.URL.Path, "/repos/o/r/pulls/7/reviews") { + http.NotFound(w, r) + return + } + count := reviewsPerPage + if r.URL.Query().Get("page") == "2" { + count = 1 + } + parts := make([]string, count) + for i := range parts { + parts[i] = fmt.Sprintf(`{"id":%d,"user":{"login":"u"},"state":"COMMENTED"}`, i) + } + if r.URL.Query().Get("page") == "2" { + parts[0] = `{"id":999,"user":{"login":"u"},"state":"APPROVED"}` + } + fmt.Fprint(w, "["+strings.Join(parts, ",")+"]") + })) + defer srv.Close() + c := NewClient("token") + c.BaseURL = srv.URL + + reviews, err := c.GetReviews(context.Background(), "o", "r", 7) + if err != nil { + t.Fatal(err) + } + if len(reviews) != reviewsPerPage+1 || ReviewDecision(reviews) != ReviewApproved { + t.Errorf("got %d reviews, decision %q; want the approval on page 2 to count", len(reviews), ReviewDecision(reviews)) + } +} diff --git a/internal/notify/native.go b/internal/notify/native.go index d2ad112..86f6f9b 100644 --- a/internal/notify/native.go +++ b/internal/notify/native.go @@ -55,7 +55,7 @@ func (n *NativeNotifier) Notify(event *StatusChangeEvent) error { return nil } - title := fmt.Sprintf("PR Status Change: %s/%s#%d", event.Owner, event.Repo, event.Number) + title := fmt.Sprintf("PR %s Change: %s/%s#%d", stateLabel(event.Kind), event.Owner, event.Repo, event.Number) message := fmt.Sprintf("%s → %s", event.PreviousState, event.CurrentState) if event.Title != "" { message = fmt.Sprintf("%s\n%s", event.Title, message) diff --git a/internal/notify/notify.go b/internal/notify/notify.go index ae8bf20..4196a9f 100644 --- a/internal/notify/notify.go +++ b/internal/notify/notify.go @@ -12,8 +12,17 @@ import ( "github.com/devblac/prw/internal/github" ) -// StatusChangeEvent represents a CI status change for a PR. +// Event kinds. CI status changes (and merged/closed) have an empty Kind, as before. +const ( + EventKindCI = "" + EventKindReview = "review" // states are github.Review* + EventKindMerge = "merge" // states are github.Merge* +) + +// StatusChangeEvent represents a change of a PR: its CI status by default, or its +// review decision or merge state, depending on Kind. type StatusChangeEvent struct { + Kind string Owner string Repo string Number int @@ -65,12 +74,19 @@ func NewConsoleNotifier() *ConsoleNotifier { func (c *ConsoleNotifier) Notify(event *StatusChangeEvent) error { prURL := github.FormatPRURL(event.Owner, event.Repo, event.Number) - fmt.Printf("\n🔔 Status Change Detected!\n") + switch event.Kind { + case EventKindReview: + fmt.Printf("\n🔔 Review Update!\n") + case EventKindMerge: + fmt.Printf("\n🔔 Merge Status Change!\n") + default: + fmt.Printf("\n🔔 Status Change Detected!\n") + } fmt.Printf(" PR: %s/%s#%d\n", event.Owner, event.Repo, event.Number) if event.Title != "" { fmt.Printf(" Title: %s\n", event.Title) } - fmt.Printf(" Status: %s → %s\n", event.PreviousState, event.CurrentState) + fmt.Printf(" %s: %s → %s\n", stateLabel(event.Kind), event.PreviousState, event.CurrentState) if len(event.FailedChecks) > 0 { fmt.Printf(" Failed checks:\n") for _, check := range event.FailedChecks { diff --git a/internal/notify/pr_events_format_test.go b/internal/notify/pr_events_format_test.go new file mode 100644 index 0000000..21ca17e --- /dev/null +++ b/internal/notify/pr_events_format_test.go @@ -0,0 +1,75 @@ +package notify + +import ( + "encoding/json" + "os/exec" + "strings" + "testing" + + "github.com/devblac/prw/internal/github" +) + +func prEvent(kind, prev, current string) *StatusChangeEvent { + return &StatusChangeEvent{Kind: kind, Owner: "o", Repo: "r", Number: 1, Title: "Add feature", PreviousState: prev, CurrentState: current} +} + +func TestPayloads_ReviewEvent(t *testing.T) { + event := prEvent(EventKindReview, github.ReviewNone, github.ReviewApproved) + + if text := slackPayload(event)["text"]; !strings.HasPrefix(text, "✅ ") || !strings.Contains(text, "Review: `none → approved`") { + t.Errorf("slack text = %q", text) + } + embed := discordPayload(event)["embeds"].([]discordEmbed)[0] + if !strings.Contains(embed.Description, "Review: `none → approved`") || embed.Color != colorSuccess { + t.Errorf("discord embed = %+v", embed) + } + data, _ := json.Marshal(genericPayload(event)) + if !strings.Contains(string(data), `"type":"pr_review_change"`) || !strings.Contains(string(data), `"current_state":"approved"`) { + t.Errorf("generic payload = %s", data) + } +} + +func TestPayloads_MergeEvents(t *testing.T) { + conflict := prEvent(EventKindMerge, github.MergeReady, github.MergeConflict) + if text := slackPayload(conflict)["text"]; !strings.HasPrefix(text, "⚠️ ") || !strings.Contains(text, "Merge: `ready → conflict`") { + t.Errorf("slack text = %q", text) + } + if c := discordPayload(conflict)["embeds"].([]discordEmbed)[0].Color; c != colorFailure { + t.Errorf("conflict color = %#x", c) + } + ready := prEvent(EventKindMerge, github.MergeBlocked, github.MergeReady) + if text := slackPayload(ready)["text"]; !strings.HasPrefix(text, "🚀 ") { + t.Errorf("slack text = %q", text) + } + if data, _ := json.Marshal(genericPayload(ready)); !strings.Contains(string(data), `"type":"pr_merge_state_change"`) { + t.Errorf("generic payload = %s", data) + } +} + +func TestPayloads_CIEventsKeepTheirType(t *testing.T) { + event := prEvent(EventKindCI, "pending", "success") + if data, _ := json.Marshal(genericPayload(event)); !strings.Contains(string(data), `"type":"pr_status_change"`) { + t.Errorf("generic payload = %s", data) + } + if text := slackPayload(event)["text"]; !strings.Contains(text, "Status: `pending → success`") { + t.Errorf("slack text = %q", text) + } +} + +func TestNativeTitle_NamesTheKind(t *testing.T) { + var seen string + old := runNativeCommand + runNativeCommand = func(cmd *exec.Cmd) error { + seen = strings.Join(append(cmd.Args, cmd.Env...), "\n") // title is an arg or env var + return nil + } + t.Cleanup(func() { runNativeCommand = old }) + + n := &NativeNotifier{enabled: true} + if err := n.Notify(prEvent(EventKindReview, github.ReviewNone, github.ReviewChangesRequested)); err != nil { + t.Fatal(err) + } + if seen != "" && !strings.Contains(seen, "PR Review Change: o/r#1") { + t.Errorf("native notification doesn't name the review:\n%s", seen) + } +} diff --git a/internal/notify/webhook_format.go b/internal/notify/webhook_format.go index 7f7f29a..731d6e9 100644 --- a/internal/notify/webhook_format.go +++ b/internal/notify/webhook_format.go @@ -30,6 +30,7 @@ const ( colorFailure = 0xD1242F colorPending = 0xDBAB09 colorNoCI = 0x8C959F + colorChanges = 0xBC4C00 ) // IsValidWebhookFormat reports whether value is a supported webhook format. @@ -91,7 +92,7 @@ func buildWebhookBody(format string, event *StatusChangeEvent) ([]byte, error) { func genericPayload(event *StatusChangeEvent) WebhookPayload { return WebhookPayload{ - Type: "pr_status_change", + Type: payloadType(event.Kind), Owner: event.Owner, Repo: event.Repo, PRNumber: event.Number, @@ -105,14 +106,44 @@ func genericPayload(event *StatusChangeEvent) WebhookPayload { } } +// payloadType is the generic payload's "type" for each event kind. +func payloadType(kind string) string { + switch kind { + case EventKindReview: + return "pr_review_change" + case EventKindMerge: + return "pr_merge_state_change" + default: + return "pr_status_change" + } +} + +// stateLabel names what changed, e.g. "Status: pending → success". +func stateLabel(kind string) string { + switch kind { + case EventKindReview: + return "Review" + case EventKindMerge: + return "Merge" + default: + return "Status" + } +} + func stateEmoji(state string) string { switch state { - case "success": + case "success", github.ReviewApproved: return "✅" case "failure", "error": return "❌" case github.StateNoCI: return "⚪" + case github.ReviewChangesRequested: + return "📝" + case github.MergeConflict: + return "⚠️" + case github.MergeReady: + return "🚀" default: return "⏳" } @@ -120,10 +151,12 @@ func stateEmoji(state string) string { func stateColor(state string) int { switch state { - case "success": + case "success", github.ReviewApproved, github.MergeReady: return colorSuccess - case "failure", "error": + case "failure", "error", github.MergeConflict: return colorFailure + case github.ReviewChangesRequested: + return colorChanges case github.StateNoCI: return colorNoCI default: @@ -156,7 +189,7 @@ func slackPayload(event *StatusChangeEvent) map[string]string { if event.Title != "" { fmt.Fprintf(&b, " — %s", slackEscape(event.Title)) } - fmt.Fprintf(&b, "\nStatus: `%s`", transition(event)) + fmt.Fprintf(&b, "\n%s: `%s`", stateLabel(event.Kind), transition(event)) for i, check := range event.FailedChecks { if i == maxChecksListed { @@ -188,7 +221,7 @@ func discordPayload(event *StatusChangeEvent) map[string]interface{} { } var desc strings.Builder - fmt.Fprintf(&desc, "Status: `%s`", transition(event)) + fmt.Fprintf(&desc, "%s: `%s`", stateLabel(event.Kind), transition(event)) for i, check := range event.FailedChecks { if i == maxChecksListed { fmt.Fprintf(&desc, "\n… and %d more", len(event.FailedChecks)-maxChecksListed) diff --git a/internal/watcher/pr_events_test.go b/internal/watcher/pr_events_test.go new file mode 100644 index 0000000..d02b020 --- /dev/null +++ b/internal/watcher/pr_events_test.go @@ -0,0 +1,171 @@ +package watcher + +import ( + "context" + "testing" + + "github.com/devblac/prw/internal/config" + "github.com/devblac/prw/internal/github" + "github.com/devblac/prw/internal/notify" +) + +func review(login, state string) github.Review { + var r github.Review + r.User.Login = login + r.State = state + return r +} + +// prEventsCase is one check of PR #1 whose CI stays green, starting from the given +// stored review and merge states. +type prEventsCase struct { + lastReview, lastMerge string + reviews []github.Review + mergeableState string + filter string + disabled bool +} + +func runPREventsCheck(t *testing.T, c prEventsCase) (*config.WatchedPR, []*notify.StatusChangeEvent) { + t.Helper() + pr := newPR(1, "sha1") + pr.State = "open" + pr.MergeableState = c.mergeableState + client := &mockGitHubClient{ + prs: map[string]*github.PullRequest{"owner/repo/1": pr}, + statuses: map[string]*github.CombinedStatus{"sha1": {State: "success", TotalCount: 1}}, + reviews: map[string][]github.Review{"owner/repo/1": c.reviews}, + } + watchedPR := watched(1, "success") + watchedPR.LastReviewState, watchedPR.LastMergeState = c.lastReview, c.lastMerge + cfg := &config.Config{NotificationFilter: c.filter, DisablePREvents: c.disabled, WatchedPRs: []config.WatchedPR{watchedPR}} + notifier := &mockNotifier{} + + if _, err := New(client, cfg, notifier).checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { + t.Fatal(err) + } + return &cfg.WatchedPRs[0], notifier.events +} + +func TestCheckPR_NotifiesApproval(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewNone, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("alice", "COMMENTED"), review("alice", "APPROVED")}, + mergeableState: "blocked", // e.g. a second approval is required + }) + if len(events) != 1 || events[0].Kind != notify.EventKindReview || events[0].PreviousState != "none" || events[0].CurrentState != "approved" { + t.Fatalf("events = %+v, want one none → approved review event", events) + } + if pr.LastReviewState != github.ReviewApproved || pr.LastMergeState != github.MergeBlocked { + t.Errorf("stored review=%q merge=%q", pr.LastReviewState, pr.LastMergeState) + } +} + +func TestCheckPR_NotifiesChangesRequested(t *testing.T) { + _, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewApproved, lastMerge: github.MergeReady, + reviews: []github.Review{review("alice", "APPROVED"), review("bob", "CHANGES_REQUESTED")}, + mergeableState: "clean", + }) + if len(events) != 1 || events[0].CurrentState != github.ReviewChangesRequested { + t.Fatalf("events = %+v, want approved → changes_requested", events) + } +} + +func TestCheckPR_FirstObservationDoesNotNotify(t *testing.T) { + // Existing watch lists (and newly added PRs) have no stored review/merge state yet. + pr, events := runPREventsCheck(t, prEventsCase{ + reviews: []github.Review{review("alice", "APPROVED")}, + mergeableState: "dirty", + }) + if len(events) != 0 { + t.Errorf("events = %+v, want none on the first check", events) + } + if pr.LastReviewState != github.ReviewApproved || pr.LastMergeState != github.MergeConflict { + t.Errorf("stored review=%q merge=%q, want them recorded", pr.LastReviewState, pr.LastMergeState) + } +} + +func TestCheckPR_NotifiesNewConflict(t *testing.T) { + _, events := runPREventsCheck(t, prEventsCase{lastReview: github.ReviewNone, lastMerge: github.MergeReady, mergeableState: "dirty"}) + if len(events) != 1 || events[0].Kind != notify.EventKindMerge || events[0].CurrentState != github.MergeConflict { + t.Fatalf("events = %+v, want ready → conflict", events) + } +} + +func TestCheckPR_ReadyToMergeAfterBranchProtectionIsSatisfied(t *testing.T) { + // The required approval came from someone whose review was already counted, + // or a required reviewer was satisfied by CODEOWNERS: only the merge state changes. + _, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewApproved, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("alice", "APPROVED")}, + mergeableState: "clean", + }) + if len(events) != 1 || events[0].CurrentState != github.MergeReady || events[0].PreviousState != github.MergeBlocked { + t.Fatalf("events = %+v, want blocked → ready", events) + } +} + +func TestCheckPR_ReadyIsNotSentWithTheApprovalThatCausedIt(t *testing.T) { + _, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewNone, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("alice", "APPROVED")}, + mergeableState: "clean", + }) + if len(events) != 1 || events[0].Kind != notify.EventKindReview { + t.Fatalf("events = %+v, want only the approval", events) + } +} + +func TestCheckPR_UnstableToReadyIsLeftToTheCINotification(t *testing.T) { + // No branch protection: the PR is "unstable" while checks run and "clean" once they + // pass, which the CI success notification already announces. + _, events := runPREventsCheck(t, prEventsCase{lastReview: github.ReviewNone, lastMerge: github.MergeUnstable, mergeableState: "clean"}) + if len(events) != 0 { + t.Errorf("events = %+v, want none", events) + } +} + +func TestCheckPR_UnknownMergeStateKeepsTheLastOne(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{lastReview: github.ReviewNone, lastMerge: github.MergeConflict, mergeableState: "unknown"}) + if len(events) != 0 || pr.LastMergeState != github.MergeConflict { + t.Errorf("events=%+v merge=%q; while GitHub computes mergeability nothing changes", events, pr.LastMergeState) + } +} + +func TestCheckPR_DismissedReviewWithdrawsTheVerdict(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewChangesRequested, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("bob", "CHANGES_REQUESTED"), review("bob", "DISMISSED")}, + mergeableState: "blocked", + }) + if len(events) != 0 || pr.LastReviewState != github.ReviewNone { + t.Errorf("events=%+v review=%q, want silently back to none", events, pr.LastReviewState) + } +} + +func TestCheckPR_PREventFilters(t *testing.T) { + tests := []struct { + filter string + c prEventsCase + want int + }{ + {config.NotificationFilterFail, prEventsCase{lastReview: "none", lastMerge: "blocked", reviews: []github.Review{review("a", "APPROVED")}, mergeableState: "blocked"}, 0}, + {config.NotificationFilterFail, prEventsCase{lastReview: "none", lastMerge: "ready", mergeableState: "dirty"}, 1}, + {config.NotificationFilterSuccess, prEventsCase{lastReview: "none", lastMerge: "ready", mergeableState: "dirty"}, 0}, + {config.NotificationFilterSuccess, prEventsCase{lastReview: "approved", lastMerge: "blocked", reviews: []github.Review{review("a", "APPROVED")}, mergeableState: "clean"}, 1}, + } + for _, tt := range tests { + tt.c.filter = tt.filter + if _, events := runPREventsCheck(t, tt.c); len(events) != tt.want { + t.Errorf("filter %s, %+v: %d events, want %d", tt.filter, tt.c, len(events), tt.want) + } + } +} + +func TestCheckPR_PREventsCanBeDisabled(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{lastReview: "none", lastMerge: "ready", mergeableState: "dirty", disabled: true}) + if len(events) != 0 || pr.LastMergeState != "ready" { + t.Errorf("events=%+v merge=%q, want pr_events=false to skip review/merge checks", events, pr.LastMergeState) + } +} diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 53aa364..70d572f 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -15,6 +15,7 @@ type GitHubClient interface { GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) + GetReviews(ctx context.Context, owner, repo string, number int) ([]github.Review, error) } // ConfigStore defines the interface for config persistence. @@ -236,8 +237,10 @@ func (w *Watcher) checkPR(ctx context.Context, pr *config.WatchedPR) (closed boo // Check if status changed. Finding out that a PR has no CI is not a CI result, so // it isn't announced (it would otherwise ping once for every such PR). + notifiedCI := false if previousState != "" && previousState != currentState && currentState != github.StateNoCI && shouldNotify(w.config.NotificationFilter, currentState) { + notifiedCI = true event := ¬ify.StatusChangeEvent{ Owner: pr.Owner, Repo: pr.Repo, @@ -260,9 +263,91 @@ func (w *Watcher) checkPR(ctx context.Context, pr *config.WatchedPR) (closed boo pr.LastKnownState = currentState pr.LastChecked = time.Now() + if !w.config.DisablePREvents { + if err := w.checkPREvents(ctx, pr, ghPR, notifiedCI); err != nil { + return false, err + } + } return false, nil } +// checkPREvents notifies about changes of the PR's review decision and merge state. +// notifiedCI tells whether a CI notification was just sent for this PR. Nothing is +// announced the first time a PR is seen (the previous state is empty). +func (w *Watcher) checkPREvents(ctx context.Context, pr *config.WatchedPR, ghPR *github.PullRequest, notifiedCI bool) error { + reviews, err := w.client.GetReviews(ctx, pr.Owner, pr.Repo, pr.Number) + if err != nil { + return fmt.Errorf("failed to fetch reviews: %w", err) + } + notified := notifiedCI + + review := github.ReviewDecision(reviews) + if prev := pr.LastReviewState; prev != "" && prev != review && review != github.ReviewNone && + shouldNotifyPREvent(w.config.NotificationFilter, review) { + w.notifyPREvent(pr, ghPR, notify.EventKindReview, prev, review) + notified = true + } + pr.LastReviewState = review + + merge := github.MergeStatus(ghPR) + if merge == "" { + return nil // GitHub is still computing it; keep the last known state + } + if prev := pr.LastMergeState; prev != "" && prev != merge && mergeWorthNotifying(prev, merge, notified) && + shouldNotifyPREvent(w.config.NotificationFilter, merge) { + w.notifyPREvent(pr, ghPR, notify.EventKindMerge, prev, merge) + } + pr.LastMergeState = merge + return nil +} + +// mergeWorthNotifying decides which merge state changes are announced: new conflicts, +// and a PR becoming ready to merge after something other than its CI was holding it +// back (branch protection, conflicts, an outdated branch). Without branch protection a +// PR goes from unstable to ready as soon as CI passes, which the CI notification +// already says; the same goes for an approval that unblocks it, so "ready" isn't sent +// together with another notification. +func mergeWorthNotifying(prev, merge string, notified bool) bool { + switch merge { + case github.MergeConflict: + return true + case github.MergeReady: + return !notified && (prev == github.MergeBlocked || prev == github.MergeConflict || prev == github.MergeBehind) + } + return false +} + +func (w *Watcher) notifyPREvent(pr *config.WatchedPR, ghPR *github.PullRequest, kind, prev, current string) { + event := ¬ify.StatusChangeEvent{ + Kind: kind, + Owner: pr.Owner, + Repo: pr.Repo, + Number: pr.Number, + Title: pr.Title, + PreviousState: prev, + CurrentState: current, + SHA: ghPR.Head.SHA, + Timestamp: time.Now(), + } + if err := w.notifier.Notify(event); err != nil { + fmt.Printf("Warning: notification failed: %v\n", err) + } +} + +// shouldNotifyPREvent applies the notification filter to review and merge events: +// "fail" keeps the ones that need action (changes requested, conflicts), "success" +// the good news (approved, ready to merge). +func shouldNotifyPREvent(filter, state string) bool { + switch config.NormalizeNotificationFilter(filter) { + case config.NotificationFilterFail: + return state == github.ReviewChangesRequested || state == github.MergeConflict + case config.NotificationFilterSuccess: + return state == github.ReviewApproved || state == github.MergeReady + default: + return true + } +} + func shouldNotify(filter, currentState string) bool { if !config.IsValidNotificationFilter(filter) { filter = config.NotificationFilterChange diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 53b45d8..c03a2e0 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -17,9 +17,17 @@ type mockGitHubClient struct { prs map[string]*github.PullRequest statuses map[string]*github.CombinedStatus checkRuns map[string][]github.CheckRun + reviews map[string][]github.Review // keyed like prs err error } +func (m *mockGitHubClient) GetReviews(ctx context.Context, owner, repo string, number int) ([]github.Review, error) { + if m.err != nil { + return nil, m.err + } + return m.reviews[fmt.Sprintf("%s/%s/%d", owner, repo, number)], nil +} + func (m *mockGitHubClient) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) { if m.err != nil { return nil, m.err From 8f217bd24153e1e89a25c2f9fd6bd6a8cd430454 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sat, 24 Oct 2026 15:34:23 -0300 Subject: [PATCH 09/11] fix: don't take a failed head re-check as "head unchanged" If the request confirming the PR head failed, `prw wait` returned the final result of the commit it had polled, even if a push had just landed, and a timeout or Ctrl-C during that request ended as success/failure instead of timed out / interrupted. Its errors now go through the same path as poll errors: retried when transient, reported as timeout or interruption when the context is done. Addresses Codex review on #18. Co-Authored-By: Claude Opus 5.5 --- cmd/prw/wait.go | 27 ++++++++++++++++++--------- cmd/prw/wait_ctx_test.go | 38 ++++++++++++++++++++++++++++++++++++++ cmd/prw/wait_test.go | 7 +++++++ 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/cmd/prw/wait.go b/cmd/prw/wait.go index 77edee6..32895a0 100644 --- a/cmd/prw/wait.go +++ b/cmd/prw/wait.go @@ -217,6 +217,12 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number for { delay := interval state, err := pollOnce(ctx, client, result) + moved := false + if err == nil && state != "pending" { + // Errors confirming the head are handled like poll errors: retried, or + // reported as a timeout / interruption, never taken as "head unchanged". + moved, err = headMoved(ctx, client, result) + } if err != nil && ctx.Err() != nil { return stopped(), nil // the timeout or Ctrl-C cancelled the poll's requests } @@ -239,11 +245,11 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number fmt.Fprintf(progress, "⚠️ %v (retrying)\n", err) } else { consecutiveErrors = 0 + if moved { + fmt.Fprintf(progress, "↻ %s/%s#%d: new commit pushed; waiting for its CI\n", owner, repo, number) + continue + } if state != "pending" { - if headMoved(ctx, client, result) { - fmt.Fprintf(progress, "↻ %s/%s#%d: new commit pushed; waiting for its CI\n", owner, repo, number) - continue - } return finish(), nil } line := fmt.Sprintf("⏳ %s/%s#%d: %s", owner, repo, number, pendingSummary(result)) @@ -262,14 +268,17 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number // headMoved reports whether a new commit was pushed to the PR since result was // polled. The CI lookup follows the PR lookup, so a push in between would otherwise -// return the result of the previous commit. Merged or closed PRs don't move; if the -// check fails, the result stands. Unchanged PRs answer 304, so this costs no quota. -func headMoved(ctx context.Context, client ciClient, result *waitResult) bool { +// return the result of the previous commit. Merged or closed PRs don't move. +// Unchanged PRs answer 304, so this costs no quota. +func headMoved(ctx context.Context, client ciClient, result *waitResult) (bool, error) { if result.State == "merged" || result.State == "closed" { - return false + return false, nil } pr, err := client.GetPullRequest(ctx, result.Owner, result.Repo, result.Number) - return err == nil && pr.Head.SHA != result.SHA + if err != nil { + return false, fmt.Errorf("failed to confirm the PR head: %w", err) + } + return pr.Head.SHA != result.SHA, nil } // pollOnce refreshes result from GitHub and returns the current state. diff --git a/cmd/prw/wait_ctx_test.go b/cmd/prw/wait_ctx_test.go index fe67c12..e2db5a7 100644 --- a/cmd/prw/wait_ctx_test.go +++ b/cmd/prw/wait_ctx_test.go @@ -3,6 +3,7 @@ package main import ( "bytes" "context" + "errors" "net/http" "net/http/httptest" "testing" @@ -44,6 +45,43 @@ func TestWaitForCI_ConfirmsHeadBeforeReturning(t *testing.T) { } } +func TestWaitForCI_RetriesWhenTheHeadCheckFails(t *testing.T) { + // A push landed during the poll, and the request that would notice it failed: the + // old commit's result must not be returned. + noSleep(t) + client := &scriptedClient{steps: []pollStep{ + {pr: openPR("old"), runs: []github.CheckRun{run("Test", "completed", "success")}, + revalidate: func() error { return errors.New("connection reset") }}, + {pr: openPR("new"), runs: []github.CheckRun{run("Test", "completed", "failure")}}, + }} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.SHA != "new" || result.State != "failure" { + t.Errorf("got %s on %s, want the failure of the new head", result.State, result.SHA) + } +} + +func TestWaitForCI_InterruptDuringTheHeadCheck(t *testing.T) { + noSleep(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + client := &scriptedClient{steps: []pollStep{{ + pr: openPR("sha"), runs: []github.CheckRun{run("Test", "completed", "success")}, + revalidate: func() error { cancel(); return ctx.Err() }, // Ctrl-C while it's in flight + }}} + + result, err := waitForCI(ctx, client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if !result.Interrupted || exitCode(result.exitErr()) != exitWaitInterrupted { + t.Errorf("result = %+v, want interrupted (exit 130), not success", result) + } +} + // hangingClient is a real client against a server that never answers until the // request is cancelled, like a stalled network. func hangingClient(t *testing.T) *github.Client { diff --git a/cmd/prw/wait_test.go b/cmd/prw/wait_test.go index 845e346..c3da461 100644 --- a/cmd/prw/wait_test.go +++ b/cmd/prw/wait_test.go @@ -28,6 +28,8 @@ type pollStep struct { // movedTo, if set, is what the PR looks like when waitForCI re-reads it to // confirm the head hasn't moved, i.e. a push that landed during this poll. movedTo *github.PullRequest + // revalidate, if set, runs on that re-read and its error is returned. + revalidate func() error } // scriptedClient returns steps[i] on the i-th poll (repeating the last one). A PR @@ -52,6 +54,11 @@ func (c *scriptedClient) GetPullRequest(ctx context.Context, owner, repo string, github.ApplyNoCI(ci, c.current.pr.UpdatedAt, time.Now()) if ci.State != "pending" { c.revalidations++ + if c.current.revalidate != nil { + if err := c.current.revalidate(); err != nil { + return nil, err + } + } if c.current.movedTo != nil { return c.current.movedTo, nil } From 3ab10ed5b67e66dbb0f253a100e5fc2f1e675ad6 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sat, 24 Oct 2026 15:36:24 -0300 Subject: [PATCH 10/11] fix: only drop review-request PRs once they're proven withdrawn A review-request PR that stopped matching its search was dropped whenever the lookup meant to tell "merged/closed" apart from "request withdrawn" failed. Interrupting `prw run` (Ctrl-C) during that lookup, a network error or a rate limit therefore silently removed PRs together with their final merged/closed notification. Now a PR is only dropped when it's confirmed open (the request was withdrawn or answered) or GitHub answers 404; otherwise it's kept until the next sync. A cancelled sync returns before touching the config. Addresses Codex review on #18. Co-Authored-By: Claude Opus 5.5 --- internal/watcher/autowatch.go | 41 ++++++++++----- internal/watcher/autowatch_test.go | 83 ++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 12 deletions(-) diff --git a/internal/watcher/autowatch.go b/internal/watcher/autowatch.go index ed9955b..bf53781 100644 --- a/internal/watcher/autowatch.go +++ b/internal/watcher/autowatch.go @@ -2,7 +2,9 @@ package watcher import ( "context" + "errors" "fmt" + "net/http" "strings" "time" @@ -49,7 +51,8 @@ type SyncResult struct { // closed are kept instead (for "mine" that's always the reason), so that `prw run` // still notifies about it before dropping them. PRs watched by hand are never touched. // -// It returns an error only if every search failed; other problems are Warnings. +// It returns an error if every search failed or ctx was cancelled (then nothing is +// changed); other problems are Warnings. func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*SyncResult, error) { result := &SyncResult{} found := map[string]*github.SearchResult{} @@ -76,7 +79,10 @@ func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*Syn if len(found) == 0 && lastErr != nil { return result, lastErr } - closed := closedReviewRequests(ctx, client, found) + withdrawn := withdrawnReviewRequests(ctx, client, found) + if err := ctx.Err(); err != nil { + return result, err // interrupted: lookups that didn't finish prove nothing + } err := config.Update(func(c *config.Config) error { changed := false @@ -102,7 +108,7 @@ func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*Syn kept := c.WatchedPRs[:0] for _, pr := range c.WatchedPRs { key := prKey(pr.Owner, pr.Repo, pr.Number) - if pr.Source == source && !matches[key] && !closed[key] { + if pr.Source == source && !matches[key] && withdrawn[key] { result.Removed = append(result.Removed, pr) changed = true continue @@ -122,11 +128,14 @@ func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*Syn return result, nil } -// closedReviewRequests finds the review-request PRs that no longer match their search -// because they were merged or closed: the search only returns open PRs, so they look -// just like withdrawn requests. It runs before the config is locked, as it makes API -// calls. A PR whose lookup fails is not reported, i.e. it's removed as before. -func closedReviewRequests(ctx context.Context, client Searcher, found map[string]*github.SearchResult) map[string]bool { +// withdrawnReviewRequests finds the review-request PRs that can be dropped: they no +// longer match their search and are still open (so the request was withdrawn or +// answered), or are gone (404). The search only returns open PRs, so a merged or +// closed PR looks the same until it's looked up; those are kept for the regular check +// to notify about. A failed lookup (network, rate limit, cancellation) proves nothing, +// so that PR is kept until the next sync. It runs before the config is locked, as it +// makes API calls. +func withdrawnReviewRequests(ctx context.Context, client Searcher, found map[string]*github.SearchResult) map[string]bool { res, ok := found[config.AutoWatchReviewRequested] if !ok || res.Incomplete { return nil // nothing will be removed @@ -139,17 +148,22 @@ func closedReviewRequests(ctx context.Context, client Searcher, found map[string for _, pr := range res.PRs { matches[prKey(pr.Owner, pr.Repo, pr.Number)] = true } - closed := map[string]bool{} + withdrawn := map[string]bool{} for _, pr := range cfg.WatchedPRs { key := prKey(pr.Owner, pr.Repo, pr.Number) if pr.Source != config.AutoWatchReviewRequested || matches[key] { continue } - if ghPR, err := client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number); err == nil && ghPR.IsClosed() { - closed[key] = true + ghPR, err := client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number) + var apiErr *github.APIError + switch { + case err == nil: + withdrawn[key] = !ghPR.IsClosed() + case errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound: + withdrawn[key] = true // deleted, or no longer visible to us } } - return closed + return withdrawn } func prKey(owner, repo string, number int) string { @@ -172,6 +186,9 @@ func (w *Watcher) syncAutoWatch(ctx context.Context) { return } res, err := SyncAutoWatch(ctx, searcher, sources) + if ctx.Err() != nil { + return // stopping; the failed searches and lookups aren't worth reporting + } for _, warning := range res.Warnings { fmt.Printf("Warning: auto-watch: %s\n", warning) } diff --git a/internal/watcher/autowatch_test.go b/internal/watcher/autowatch_test.go index a2b23c1..d08bf4e 100644 --- a/internal/watcher/autowatch_test.go +++ b/internal/watcher/autowatch_test.go @@ -115,6 +115,7 @@ func TestSyncAutoWatch_DropsWithdrawnReviewRequestsButKeepsMine(t *testing.T) { config.AutoWatchMine: found(), config.AutoWatchReviewRequested: found(21), }} + s.prs = map[string]*github.PullRequest{"owner/repo/20": openReviewPR(20)} res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { @@ -329,3 +330,85 @@ func TestRunOnce_NotifiesReviewRequestMergedWhileStopped(t *testing.T) { t.Errorf("watch list = %q, want the merged PR dropped after notifying", got) } } + +func openReviewPR(number int) *github.PullRequest { + pr := newPR(number, fmt.Sprintf("sha%d", number)) + pr.State = "open" + return pr +} + +// failingLookups answers GetPullRequest with errs (keyed like prs) for the given PRs. +type failingLookups struct { + *fakeSearcher + errs map[string]error +} + +func (f *failingLookups) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { + if err := f.errs[fmt.Sprintf("%s/%s/%d", owner, repo, number)]; err != nil { + return nil, err + } + return f.fakeSearcher.GetPullRequest(ctx, owner, repo, number) +} + +func TestSyncAutoWatch_RemovesReviewRequestsOnlyWhenProvenWithdrawn(t *testing.T) { + useTempConfig(t) + reviewPR := func(n int) config.WatchedPR { + return config.WatchedPR{Owner: "owner", Repo: "repo", Number: n, Source: config.AutoWatchReviewRequested} + } + saveConfig(t, &config.Config{ + AutoWatch: []string{config.AutoWatchReviewRequested}, + WatchedPRs: []config.WatchedPR{reviewPR(1), reviewPR(2), reviewPR(3)}, + }) + s := &failingLookups{ + fakeSearcher: &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: found()}}, + errs: map[string]error{ + "owner/repo/2": &github.APIError{StatusCode: 404}, // deleted or no access + "owner/repo/3": errors.New("connection reset"), + }, + } + s.prs = map[string]*github.PullRequest{"owner/repo/1": openReviewPR(1)} + + if _, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchReviewRequested}); err != nil { + t.Fatal(err) + } + if got := watchList(t); got != "3:review-requested" { + t.Errorf("watch list = %q, want #1 (open) and #2 (404) dropped, #3 (lookup failed) kept", got) + } +} + +func TestSyncAutoWatch_InterruptedDuringLookupsChangesNothing(t *testing.T) { + useTempConfig(t) + saveConfig(t, &config.Config{ + AutoWatch: []string{config.AutoWatchReviewRequested}, + WatchedPRs: []config.WatchedPR{ + {Owner: "owner", Repo: "repo", Number: 1, Source: config.AutoWatchReviewRequested}, + {Owner: "owner", Repo: "repo", Number: 2, Source: config.AutoWatchReviewRequested}, + }, + }) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + s := &failingLookups{ + fakeSearcher: &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: found(9)}}, + } + s.prs = map[string]*github.PullRequest{"owner/repo/1": openReviewPR(1)} + s.errs = map[string]error{"owner/repo/2": context.Canceled} + cancelOnLookup := &cancellingSearcher{failingLookups: s, cancel: cancel} + + if _, err := SyncAutoWatch(ctx, cancelOnLookup, []string{config.AutoWatchReviewRequested}); !errors.Is(err, context.Canceled) { + t.Fatalf("err = %v, want context.Canceled", err) + } + if got := watchList(t); got != "1:review-requested 2:review-requested" { + t.Errorf("watch list = %q, want it untouched (no removals, no additions)", got) + } +} + +// cancellingSearcher cancels the context (Ctrl-C) when the first PR lookup starts. +type cancellingSearcher struct { + *failingLookups + cancel context.CancelFunc +} + +func (c *cancellingSearcher) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { + c.cancel() + return c.failingLookups.GetPullRequest(ctx, owner, repo, number) +} From f9b6f410f305e69f177635ffd1370ee33fe86aee Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sat, 24 Oct 2026 15:37:26 -0300 Subject: [PATCH 11/11] fix: read every page of reviews, not just the first 300 Reviews come oldest first, so the 3-page cap dropped the newest approvals, change requests and dismissals on PRs with more than 300 reviews, and the review state stayed stale. Pagination now runs to the last page, with a 100-page cap only as a runaway guard. Addresses Codex review on #19. Co-Authored-By: Claude Opus 5.5 --- internal/github/review.go | 7 +++++-- internal/github/review_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/internal/github/review.go b/internal/github/review.go index a3de91e..0e53ee5 100644 --- a/internal/github/review.go +++ b/internal/github/review.go @@ -24,8 +24,11 @@ const ( ) const ( - reviewsPerPage = 100 - maxReviewsPages = 3 + reviewsPerPage = 100 + // maxReviewsPages only guards against a runaway loop: reviews come oldest first, + // so stopping early would drop the newest verdicts. Every page is fetched (with a + // free 304 when unchanged), so 10,000 reviews is far beyond any real PR. + maxReviewsPages = 100 ) // Review is a single submitted pull request review. diff --git a/internal/github/review_test.go b/internal/github/review_test.go index 20eb44a..5e473d4 100644 --- a/internal/github/review_test.go +++ b/internal/github/review_test.go @@ -50,6 +50,40 @@ func TestMergeStatus(t *testing.T) { } } +func TestGetReviews_ReadsPastThreeHundredReviews(t *testing.T) { + // 350 comments, then a change request on page 4: the newest verdict must count. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var page int + fmt.Sscan(r.URL.Query().Get("page"), &page) + count := reviewsPerPage + if page == 4 { + count = 51 + } + parts := make([]string, count) + for i := range parts { + parts[i] = `{"user":{"login":"u"},"state":"COMMENTED"}` + } + if page == 1 { + parts[0] = `{"user":{"login":"a"},"state":"APPROVED"}` + } + if page == 4 { + parts[50] = `{"user":{"login":"b"},"state":"CHANGES_REQUESTED"}` + } + fmt.Fprint(w, "["+strings.Join(parts, ",")+"]") + })) + defer srv.Close() + c := NewClient("token") + c.BaseURL = srv.URL + + reviews, err := c.GetReviews(context.Background(), "o", "r", 7) + if err != nil { + t.Fatal(err) + } + if len(reviews) != 351 || ReviewDecision(reviews) != ReviewChangesRequested { + t.Errorf("got %d reviews, decision %q; want 351 and the change request on page 4", len(reviews), ReviewDecision(reviews)) + } +} + func TestGetReviews_Paginates(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !strings.HasSuffix(r.URL.Path, "/repos/o/r/pulls/7/reviews") {