diff --git a/CHANGELOG.md b/CHANGELOG.md index dc71faa..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. @@ -20,6 +21,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..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 @@ -387,7 +389,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 | @@ -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 80ba30e..2880380 100644 --- a/TASKS.md +++ b/TASKS.md @@ -90,7 +90,9 @@ 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) -- Next: #11 review & mergeability events +- โœ… 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. +- โœ… #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/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..7719aa9 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) } @@ -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 89792ca..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}, @@ -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) @@ -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 @@ -666,7 +668,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..134e4fd 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). @@ -189,7 +192,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 +206,29 @@ 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) + 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 + } 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. @@ -227,6 +248,10 @@ 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" { return finish(), nil } @@ -239,19 +264,29 @@ 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. +// 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, nil + } + pr, err := client.GetPullRequest(ctx, result.Owner, result.Repo, result.Number) + 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. -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) } @@ -259,6 +294,7 @@ func pollOnce(client ciClient, result *waitResult) (string, error) { result.Title = pr.Title } result.SHA = pr.Head.SHA + result.MergeState = github.MergeStatus(pr) if pr.IsClosed() { result.State = "closed" @@ -269,7 +305,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..e2db5a7 --- /dev/null +++ b/cmd/prw/wait_ctx_test.go @@ -0,0 +1,135 @@ +package main + +import ( + "bytes" + "context" + "errors" + "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) + } +} + +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 { + 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..c3da461 100644 --- a/cmd/prw/wait_test.go +++ b/cmd/prw/wait_test.go @@ -25,16 +25,46 @@ 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 + // 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). +// 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.revalidate != nil { + if err := c.current.revalidate(); err != nil { + return nil, err + } + } + 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 +74,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 +84,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/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 cceccf2..ddbf810 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -1,6 +1,7 @@ package github import ( + "context" "encoding/json" "errors" "fmt" @@ -111,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 @@ -159,10 +164,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 { @@ -188,8 +197,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. @@ -212,36 +221,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) @@ -294,12 +303,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 @@ -327,13 +336,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) } @@ -377,7 +386,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 +416,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 @@ -419,8 +457,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 } @@ -472,9 +510,19 @@ func (c *Client) getJSON(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/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..8c6ac79 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,9 +274,51 @@ 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) } 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) + } +} 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/review.go b/internal/github/review.go new file mode 100644 index 0000000..0e53ee5 --- /dev/null +++ b/internal/github/review.go @@ -0,0 +1,105 @@ +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 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. +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..5e473d4 --- /dev/null +++ b/internal/github/review_test.go @@ -0,0 +1,117 @@ +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_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") { + 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/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/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) + } +} 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 4481065..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. @@ -59,7 +60,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/"): @@ -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/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}, diff --git a/internal/watcher/autowatch.go b/internal/watcher/autowatch.go index 297fdd5..bf53781 100644 --- a/internal/watcher/autowatch.go +++ b/internal/watcher/autowatch.go @@ -1,7 +1,10 @@ package watcher import ( + "context" + "errors" "fmt" + "net/http" "strings" "time" @@ -26,9 +29,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) + 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. @@ -42,12 +47,13 @@ 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) { +// 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{} var lastErr error @@ -58,7 +64,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)) @@ -73,6 +79,10 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { if len(found) == 0 && lastErr != nil { return result, lastErr } + 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 @@ -97,7 +107,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] && withdrawn[key] { result.Removed = append(result.Removed, pr) changed = true continue @@ -117,12 +128,50 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { return result, nil } +// 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 + } + 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 + } + 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 + } + 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 withdrawn +} + func prKey(owner, repo string, number int) string { return strings.ToLower(owner) + "/" + strings.ToLower(repo) + "#" + fmt.Sprint(number) } // 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 @@ -136,7 +185,10 @@ func (w *Watcher) syncAutoWatch() { if !ok { return } - res, err := SyncAutoWatch(searcher, sources) + 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 08fbe63..d08bf4e 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) } @@ -115,8 +115,9 @@ 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(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 +139,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 +162,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 +180,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 +190,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 +202,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) } @@ -301,3 +302,113 @@ 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) + } +} + +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) +} 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 8dc8ed8..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,8 +60,45 @@ 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) } } + +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(context.Background(), &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(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/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/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 d51eac0..70d572f 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -12,9 +12,10 @@ 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) + GetReviews(ctx context.Context, owner, repo string, number int) ([]github.Review, error) } // ConfigStore defines the interface for config persistence. @@ -70,10 +71,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 +87,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 +115,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 +182,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 +221,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) } @@ -223,11 +229,18 @@ 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). + 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, @@ -250,9 +263,91 @@ func (w *Watcher) checkPR(pr *config.WatchedPR) (closed bool, err error) { 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_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..c03a2e0 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -17,17 +17,25 @@ 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) GetCheckRuns(owner, repo, ref string) ([]github.CheckRun, 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 } 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 +47,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 +103,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 +141,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 +183,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 +241,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 +271,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 +394,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 +433,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 +467,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 +510,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 +568,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 +608,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) }