diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a90e9c..96b8283 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ 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 +- `prw checks `: every check run and commit status of a PR's head commit with its result, duration and link, failures first; `--json` for scripts. Superseded runs are left out. Exits `0` passed, `1` failed, `3` error, `4` no CI, `8` still running. +- Review and merge events: `prw run` notifies when a PR is approved, gets changes requested, runs into merge conflicts, or becomes ready to merge after branch protection was holding it back. Shown in console, native, Slack/Discord and generic webhooks (`type` `pr_review_change` / `pr_merge_state_change`), filtered by `--on` (`fail`: changes requested and conflicts; `success`: approved and ready). `prw list` gains `REVIEW` and `MERGE` columns (`review` / `merge` in `--json` and MCP `list_watched`); `prw wait --json` and the MCP tools report `merge_state`. Disable with `prw config set pr_events false`. - Auto-watch: `prw watch --mine` follows all your open PRs and `prw watch --review-requested` the PRs where your review is requested, across every repository. Matches are added immediately; `prw run` refreshes the searches every 5 minutes. Review-request PRs are dropped when the request goes away; PRs watched by hand are never touched by it. `prw unwatch --mine` / `--review-requested` turns it off and removes the PRs it added. `prw list` gains an `ADDED BY` column (and `source` in `--json`); `prw config show` shows `auto_watch`. - `prw mcp`: a Model Context Protocol server over stdio for AI coding agents, with tools `get_ci_status`, `wait_for_ci` (progress notifications, cancellation, fail-fast), `list_watched`, `watch_pr` and `unwatch_pr`. Implemented without new dependencies (the official Go SDK needs Go 1.25). - Conditional requests (ETags): repeated polls of unchanged resources are answered with `304 Not Modified`, which doesn't count against the authenticated rate limit. Measured against the real API: after the first poll, 5 further polls of a PR (15 requests) consumed 0 quota. Previously each watched PR cost 3 requests per poll (~540/hour at the default 20s interval), so about 9 PRs exhausted the 5,000/hour limit. diff --git a/README.md b/README.md index 739db12..211264e 100644 --- a/README.md +++ b/README.md @@ -31,12 +31,14 @@ The tool is deliberately minimal. It does one thing well: watch PRs and tell you ## Features +- **`prw checks`**: every check of a PR with its result, duration and link - **`prw wait`**: block until a PR's CI finishes and exit 0/1/2 — for scripts, pipelines and AI coding agents - **MCP server** (`prw mcp`): lets Claude Code, Claude Desktop, Cursor and other agents check and wait for PR CI as tools - **Auto-watch**: `prw watch --mine` / `--review-requested` follow your PRs and review requests across all repositories, no URLs needed - **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) @@ -265,6 +267,24 @@ With `--json`, stdout is a single object: **PRs without CI.** Some PRs never get a single check — the repository has no CI, or none runs for the PR's base branch (for example a stacked PR whose base isn't `main`). Right after a push, though, every PR briefly has zero checks while CI registers them. So prw only concludes `no_ci` once the PR has had no checks or statuses at all **and** hasn't been updated for 3 minutes; until then it reports `pending`. `no_ci` exits with `4`, not `0`, so `prw wait && deploy` doesn't deploy a PR that no CI looked at. `prw run` shows `no_ci` in `prw list` but doesn't send a notification for it, and notifies normally if CI shows up later (e.g. after the PR is retargeted). +## Seeing every check: `prw checks` + +`prw checks` lists each check of a PR's head commit (GitHub Actions jobs, other check runs and legacy commit statuses) with its result, how long it took and a link to it — failures first, then running ones. The PR does not need to be on the watch list. + +``` +$ prw checks https://github.com/owner/repo/pull/123 +owner/repo#123 — Add feature +abc1234 · failure · 1 fail, 1 pending, 3 pass · merge: blocked + +❌ fail Test (ubuntu-latest) 4m02s https://github.com/owner/repo/actions/runs/…/job/… +⏳ pending E2E 1m10s… https://github.com/owner/repo/actions/runs/…/job/… +✅ pass Build 45s https://github.com/owner/repo/actions/runs/…/job/… +✅ pass Lint 21s https://github.com/owner/repo/actions/runs/…/job/… +✅ pass ci/jenkins https://jenkins.example.com/job/… +``` + +Runs superseded by a newer run of the same check (e.g. cancelled by `concurrency: cancel-in-progress`) are left out, as they don't count. `--json` prints the same as an object with a `checks` array (`name`, `bucket`, `state`, `kind`, `url`, `started_at`, `completed_at`). Exit codes match `prw wait` — `0` passed (or merged), `1` failed, `3` error or closed without merging, `4` no CI — plus `8` while checks are still running. + ## Using prw from AI agents: `prw mcp` `prw mcp` runs a [Model Context Protocol](https://modelcontextprotocol.io) server over stdio, so coding agents can check a PR's CI and wait for it after pushing — and read which checks failed — without screen-scraping `gh` output. @@ -345,6 +365,7 @@ prw config set webhook_url https://hooks.slack.com/services/YOUR/WEBHOOK/URL - **`webhook_url`**: Optional HTTP endpoint for notifications - **`webhook_format`**: Payload format: `auto` (default, detected from the URL), `slack`, `discord`, or `generic` - **`notification_native`**: Enable native OS notifications (true/false, default: false) +- **`pr_events`**: Also notify about reviews and merge conflicts / readiness (true/false, default: true) - **`github_token`**: GitHub Personal Access Token (prefer env var `GITHUB_TOKEN`) ## Notifications @@ -432,6 +453,29 @@ prw run --on change The same setting can be persisted via `prw config set notification_filter `. +### Reviews and merge conflicts + +Besides CI, `prw run` notifies when a PR's review decision or merge state changes: + +| Event | When | +|-------|------| +| `approved` | A reviewer approved and nobody's latest review requests changes | +| `changes_requested` | A reviewer's latest review requests changes | +| `conflict` | The PR has merge conflicts with its base branch | +| `ready` | GitHub says the PR can be merged, after branch protection, conflicts or an outdated branch were holding it back | + +``` +🔔 Review Update! + PR: kubernetes/kubernetes#12345 + Review: none → approved +``` + +Webhooks get the same shapes as CI changes, with `"type": "pr_review_change"` or `"pr_merge_state_change"` in the generic payload (and `Review:` / `Merge:` instead of `Status:` in Slack and Discord messages). With `--on fail` you only get `changes_requested` and `conflict`; with `--on success`, `approved` and `ready`. + +A few rules keep this quiet: nothing is announced the first time a PR is checked; `ready` isn't sent together with the CI or approval notification that caused it, and not at all for repositories without branch protection (there, "CI passed" already means it); dismissed reviews and new review rounds don't notify until someone approves or requests changes. `prw list` shows the last review and merge state in the `REVIEW` and `MERGE` columns, `prw wait --json` and the MCP tools include `merge_state`. + +Turn these off with `prw config set pr_events false`. They cost one extra request per PR per poll (the reviews), answered with a free `304` when nothing changed. + ### Merged and closed PRs Once `prw run` sees that a watched PR was merged or closed, it sends one final notification (`CurrentState` is `merged` or `closed`, so you'll see e.g. `success → merged`) and removes the PR from the watch list — regardless of `--on`/`notification_filter`, since a merge is a lifecycle event, not a CI result. This keeps the watch list from growing forever. `prw list` and `prw broadcast` are unaffected: a PR only drops out once `prw run` has actually polled it. diff --git a/TASKS.md b/TASKS.md index 6c34e5a..e8673ed 100644 --- a/TASKS.md +++ b/TASKS.md @@ -91,7 +91,10 @@ Legend — Impact: 1–5 · Effort: S (≤½ day), M (1–2 days), L (3+ days) - 🆕 #18 **Report "no CI" instead of `pending` forever** (found while testing #10: 14 of 18 real PRs have zero checks — repos without CI, or stacked PRs whose base isn't `main`). Today `prw wait` / `wait_for_ci` would wait until the timeout on them. Needs a grace period after the last push (CI takes a moment to register checks) before concluding "no CI". Impact 4, effort S–M. - ✅ #18 `no_ci` state — done (0 checks + PR not updated for 3 min → `no_ci`; `prw wait` exits 4; no notification; verified live: 11 real PRs went from `pending` to `no_ci`, `prw wait` on one returns in 2s instead of the 30-min timeout) - ✅ Codex review feedback on #6, #7, #13–#17 — done in `fix/codex-review`: superseded check runs (verified live on zephyrproject-rtos/zephyr#105183: was `failure`, now `success`), head re-check in `prw wait`, context propagation into every API request, closed review-request PRs keep their notification, no `no_ci` flapping, ETag cache eviction, GovSlack. -- Next: #11 review & mergeability events +- ✅ #11 Review & mergeability events — done (approved / changes_requested / conflict / ready; verified live: devblac/prw#12 `ready`, cli/cli#14727 `blocked`, a simulated blocked → ready notified once and ready → blocked stayed silent) +- ✅ Codex follow-ups on #18/#19 — failed head re-check no longer accepted as "unchanged"; review-request PRs only dropped when proven withdrawn (open or 404), never on a cancelled or failed lookup; all review pages read. +- ✅ #12 `prw checks ` — done (buckets fail/cancel/pending/pass/neutral/skipping, durations, links, `--json`, exit 8 while running; verified live on devblac/docker-lint#5, zephyrproject-rtos/zephyr#105183 — 35 checks, superseded run hidden — and devblac/prw#13) +- Next: #15 failed job log excerpt ## Planned diff --git a/cmd/prw/checks.go b/cmd/prw/checks.go new file mode 100644 index 0000000..8266324 --- /dev/null +++ b/cmd/prw/checks.go @@ -0,0 +1,297 @@ +package main + +import ( + "context" + "encoding/json" + "fmt" + "io" + "os" + "sort" + "strings" + "text/tabwriter" + "time" + + "github.com/spf13/cobra" + + "github.com/devblac/prw/internal/config" + "github.com/devblac/prw/internal/github" +) + +// exitChecksPending is `prw checks`' exit code while CI is still running; the other +// codes are the ones of `prw wait`. 8 is what `gh pr checks` uses for pending. +const exitChecksPending = 8 + +// Buckets of individual checks, in display order. +const ( + checkFail = "fail" + checkCancel = "cancel" + checkPending = "pending" + checkPass = "pass" + checkNeutral = "neutral" + checkSkipping = "skipping" +) + +var checksJSON bool + +func init() { + rootCmd.AddCommand(checksCmd) + checksCmd.Flags().BoolVar(&checksJSON, "json", false, "print the checks as JSON") +} + +var checksCmd = &cobra.Command{ + Use: "checks ", + Short: "Show every check of a PR with its result, duration and link", + Long: `Show the individual checks (GitHub Actions jobs and other check runs, plus legacy +commit statuses) of a pull request's head commit: failures first, then running +ones, then the rest. The PR does not need to be on the watch list. + +Exit codes: + 0 all checks passed + 1 a check failed + 3 error (invalid URL, API error, PR closed without merging) + 4 the PR has no CI + 8 checks are still running`, + Example: ` prw checks https://github.com/owner/repo/pull/123 + prw checks https://github.com/owner/repo/pull/123 --json`, + SilenceUsage: true, + Args: func(cmd *cobra.Command, args []string) error { + if len(args) != 1 { + return waitErrorf(exitWaitError, "expected exactly one PR URL, got %d argument(s)", len(args)) + } + return nil + }, + RunE: func(cmd *cobra.Command, args []string) error { + owner, repo, number, err := github.ParsePRURL(args[0]) + if err != nil { + return waitErrorf(exitWaitError, "invalid PR URL: %v", err) + } + cfg, err := config.Load() + if err != nil { + return waitErrorf(exitWaitError, "failed to load config: %v", err) + } + token := cfg.GetToken() + if token == "" { + return &exitError{code: exitWaitError, err: errMissingToken} + } + + report, err := fetchChecks(context.Background(), newGitHubClient(token), owner, repo, number, time.Now()) + if err != nil { + return &exitError{code: exitWaitError, err: err} + } + if checksJSON { + enc := json.NewEncoder(os.Stdout) + enc.SetIndent("", " ") + if err := enc.Encode(report); err != nil { + return &exitError{code: exitWaitError, err: err} + } + } else { + printChecks(os.Stdout, report, time.Now()) + } + return report.exitErr() + }, +} + +// checkEntry is one check run or commit status; it is also the --json output. +type checkEntry struct { + Name string `json:"name"` + Bucket string `json:"bucket"` // fail, cancel, pending, pass, neutral, skipping + State string `json:"state"` // GitHub's conclusion, or status while not completed + Kind string `json:"kind"` // check_run or status + URL string `json:"url,omitempty"` // job / details page + StartedAt *time.Time `json:"started_at,omitempty"` // check runs only + CompletedAt *time.Time `json:"completed_at,omitempty"` +} + +// checksReport is the output of `prw checks`. +type checksReport 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"` + State string `json:"state"` // overall, as in `prw wait` + MergeState string `json:"merge_state,omitempty"` + Checks []checkEntry `json:"checks"` +} + +// fetchChecks gets the head commit's check runs and statuses. Superseded runs of the +// same check are left out, as they don't count towards the result. +func fetchChecks(ctx context.Context, client ciClient, owner, repo string, number int, now time.Time) (*checksReport, error) { + pr, err := client.GetPullRequest(ctx, owner, repo, number) + if err != nil { + return nil, fmt.Errorf("failed to fetch PR: %w", err) + } + sha := pr.Head.SHA + status, err := client.GetCombinedStatus(ctx, owner, repo, sha) + if err != nil { + return nil, fmt.Errorf("failed to fetch commit status: %w", err) + } + runs, err := client.GetCheckRuns(ctx, owner, repo, sha) + if err != nil { + return nil, fmt.Errorf("failed to fetch check runs: %w", err) + } + + ci := github.SummarizeCI(status, runs) + github.ApplyNoCI(ci, pr.UpdatedAt, now) + report := &checksReport{ + Owner: owner, Repo: repo, Number: number, Title: pr.Title, SHA: sha, + URL: github.FormatPRURL(owner, repo, number), State: ci.State, MergeState: github.MergeStatus(pr), + Checks: []checkEntry{}, + } + if pr.IsClosed() { + report.State = "closed" + if pr.Merged { + report.State = "merged" + } + } + + for _, run := range github.LatestCheckRuns(runs) { + entry := checkEntry{Name: run.Name, Kind: "check_run", URL: run.HTMLURL, State: strings.ToLower(run.Conclusion)} + if !strings.EqualFold(run.Status, "completed") { + entry.State = strings.ToLower(run.Status) + } + entry.Bucket = checkRunBucket(run) + if !run.StartedAt.IsZero() { + t := run.StartedAt + entry.StartedAt = &t + } + if !run.CompletedAt.IsZero() { + t := run.CompletedAt + entry.CompletedAt = &t + } + report.Checks = append(report.Checks, entry) + } + for _, s := range status.Statuses { + state := github.NormalizeState(s.State) + bucket := checkPending + switch state { + case "success": + bucket = checkPass + case "failure", "error": + bucket = checkFail + } + report.Checks = append(report.Checks, checkEntry{Name: s.Context, Kind: "status", URL: s.TargetURL, State: state, Bucket: bucket}) + } + + order := map[string]int{checkFail: 0, checkCancel: 1, checkPending: 2, checkPass: 3, checkNeutral: 4, checkSkipping: 5} + sort.SliceStable(report.Checks, func(i, j int) bool { + a, b := report.Checks[i], report.Checks[j] + if order[a.Bucket] != order[b.Bucket] { + return order[a.Bucket] < order[b.Bucket] + } + return strings.ToLower(a.Name) < strings.ToLower(b.Name) + }) + return report, nil +} + +// checkRunBucket classifies a check run like SummarizeCI does: everything that isn't a +// pass, neutral or skip is a failure (cancelled ones are shown apart). +func checkRunBucket(run github.CheckRun) string { + if !strings.EqualFold(run.Status, "completed") { + return checkPending + } + switch strings.ToLower(run.Conclusion) { + case "success": + return checkPass + case "neutral": + return checkNeutral + case "skipped": + return checkSkipping + case "cancelled": + return checkCancel + default: // failure, timed_out, action_required, startup_failure, stale + return checkFail + } +} + +func (r *checksReport) exitErr() error { + pr := fmt.Sprintf("%s/%s#%d", r.Owner, r.Repo, r.Number) + switch r.State { + case "success", "merged": + return nil + case "closed": + // Like `prw wait`: an abandoned PR must not pass `prw checks "$PR" && ...`. + return waitErrorf(exitWaitError, "%s was closed without merging", pr) + case "failure", "error": + return waitErrorf(exitWaitFailed, "CI of %s finished with %s", pr, r.State) + case github.StateNoCI: + return waitErrorf(exitWaitNoCI, "%s has no CI", pr) + default: + return &exitError{code: exitChecksPending, err: fmt.Errorf("CI of %s is still running", pr)} + } +} + +var bucketIcon = map[string]string{ + checkFail: "❌", checkCancel: "🚫", checkPending: "⏳", checkPass: "✅", checkNeutral: "⚪", checkSkipping: "⏭", +} + +func printChecks(w io.Writer, r *checksReport, now time.Time) { + fmt.Fprintf(w, "%s/%s#%d", r.Owner, r.Repo, r.Number) + if r.Title != "" { + fmt.Fprintf(w, " — %s", r.Title) + } + fmt.Fprintln(w) + + counts := map[string]int{} + for _, c := range r.Checks { + counts[c.Bucket]++ + } + var parts []string + for _, b := range []string{checkFail, checkCancel, checkPending, checkPass, checkNeutral, checkSkipping} { + if counts[b] > 0 { + parts = append(parts, fmt.Sprintf("%d %s", counts[b], b)) + } + } + summary := fmt.Sprintf("%s · %s", shortSHA(r.SHA), r.State) + if len(parts) > 0 { + summary += " · " + strings.Join(parts, ", ") + } + if r.MergeState != "" { + summary += " · merge: " + r.MergeState + } + fmt.Fprintln(w, summary) + + if len(r.Checks) == 0 { + fmt.Fprintln(w, "\nNo checks or statuses reported for this commit.") + return + } + fmt.Fprintln(w) + tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) + for _, c := range r.Checks { + fmt.Fprintf(tw, "%s %s\t%s\t%s\t%s\n", bucketIcon[c.Bucket], c.Bucket, c.Name, checkDuration(c, now), c.URL) + } + tw.Flush() +} + +// checkDuration is how long a check took, or has been running so far ("3m10s…"). +func checkDuration(c checkEntry, now time.Time) string { + if c.StartedAt == nil { + return "" + } + if c.CompletedAt == nil { + return formatCheckDuration(now.Sub(*c.StartedAt)) + "…" + } + return formatCheckDuration(c.CompletedAt.Sub(*c.StartedAt)) +} + +func formatCheckDuration(d time.Duration) string { + if d < 0 { + d = 0 + } + d = d.Round(time.Second) + if d < time.Minute { + return fmt.Sprintf("%ds", int(d.Seconds())) + } + if d < time.Hour { + return fmt.Sprintf("%dm%02ds", int(d.Minutes()), int(d.Seconds())%60) + } + return fmt.Sprintf("%dh%02dm", int(d.Hours()), int(d.Minutes())%60) +} + +func shortSHA(sha string) string { + if len(sha) > 7 { + return sha[:7] + } + return sha +} diff --git a/cmd/prw/checks_test.go b/cmd/prw/checks_test.go new file mode 100644 index 0000000..4d343b3 --- /dev/null +++ b/cmd/prw/checks_test.go @@ -0,0 +1,163 @@ +package main + +import ( + "bytes" + "context" + "fmt" + "net/http" + "strings" + "testing" + "time" + + "github.com/devblac/prw/internal/github" +) + +var checksNow = time.Date(2026, 10, 24, 12, 0, 0, 0, time.UTC) + +func timedRun(id int64, name, status, conclusion string, started, completed time.Duration) github.CheckRun { + r := github.CheckRun{ID: id, Name: name, Status: status, Conclusion: conclusion, HTMLURL: "https://ci/" + name} + r.App.ID = 15368 + if started != 0 { + r.StartedAt = checksNow.Add(-started) + } + if completed != 0 { + r.CompletedAt = checksNow.Add(-completed) + } + return r +} + +func checksFixture() *scriptedClient { + pr := openPR("abc1234def") + pr.MergeableState = "blocked" + return &scriptedClient{steps: []pollStep{{ + pr: pr, + status: &github.CombinedStatus{State: "failure", TotalCount: 1, Statuses: []github.CommitStatus{ + {Context: "ci/jenkins", State: "error", TargetURL: "https://jenkins/1"}, + }}, + runs: []github.CheckRun{ + timedRun(1, "Build", "completed", "success", 5*time.Minute, 4*time.Minute), + timedRun(2, "Test", "completed", "failure", 5*time.Minute, time.Minute), + timedRun(3, "Lint", "in_progress", "", 70*time.Second, 0), + timedRun(4, "Deploy", "completed", "skipped", 0, 0), + timedRun(5, "E2E", "completed", "cancelled", 3*time.Minute, 2*time.Minute), + timedRun(6, "Docs", "completed", "failure", 9*time.Minute, 8*time.Minute), // superseded by 7 + timedRun(7, "Docs", "completed", "success", 2*time.Minute, time.Minute), + }, + }}} +} + +func TestFetchChecks_BucketsAndOrder(t *testing.T) { + report, err := fetchChecks(context.Background(), checksFixture(), "o", "r", 1, checksNow) + if err != nil { + t.Fatal(err) + } + var got []string + for _, c := range report.Checks { + got = append(got, c.Bucket+":"+c.Name) + } + want := "fail:ci/jenkins fail:Test cancel:E2E pending:Lint pass:Build pass:Docs skipping:Deploy" + if strings.Join(got, " ") != want { + t.Errorf("checks = %s\nwant %s", strings.Join(got, " "), want) + } + if report.State != "failure" || report.MergeState != github.MergeBlocked || report.SHA != "abc1234def" { + t.Errorf("report = %+v", report) + } + if code := exitCode(report.exitErr()); code != exitWaitFailed { + t.Errorf("exit code = %d, want 1", code) + } +} + +func TestPrintChecks(t *testing.T) { + report, err := fetchChecks(context.Background(), checksFixture(), "o", "r", 1, checksNow) + if err != nil { + t.Fatal(err) + } + var buf bytes.Buffer + printChecks(&buf, report, checksNow) + out := buf.String() + for _, want := range []string{ + "o/r#1 — Add feature", + "abc1234 · failure · 2 fail, 1 cancel, 1 pending, 2 pass, 1 skipping · merge: blocked", + "❌ fail", "https://jenkins/1", + "Test", "4m00s", // 5m ago → 1m ago + "Lint", "1m10s…", // still running + } { + if !strings.Contains(out, want) { + t.Errorf("output lacks %q:\n%s", want, out) + } + } + if strings.Count(out, "https://ci/Docs") != 1 { + t.Errorf("superseded run of Docs must not be listed:\n%s", out) + } +} + +func TestChecksExitCodes(t *testing.T) { + for state, want := range map[string]int{ + "success": 0, "merged": 0, "closed": exitWaitError, "failure": exitWaitFailed, "error": exitWaitFailed, + github.StateNoCI: exitWaitNoCI, "pending": exitChecksPending, + } { + r := &checksReport{Owner: "o", Repo: "r", Number: 1, State: state} + if got := exitCode(r.exitErr()); got != want { + t.Errorf("%s: exit %d, want %d", state, got, want) + } + } +} + +func TestFormatCheckDuration(t *testing.T) { + for d, want := range map[time.Duration]string{ + 0: "0s", 42 * time.Second: "42s", 61 * time.Second: "1m01s", 3*time.Hour + 5*time.Minute: "3h05m", -time.Second: "0s", + } { + if got := formatCheckDuration(d); got != want { + t.Errorf("formatCheckDuration(%s) = %q, want %q", d, got, want) + } + } +} + +func TestChecksCmd_PendingExitCode(t *testing.T) { + setupWaitCommand(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/pulls/1"): + fmt.Fprint(w, `{"number":1,"title":"WIP","state":"open","mergeable_state":"unstable","head":{"sha":"abc1234"}}`) + case strings.HasSuffix(r.URL.Path, "/status"): + fmt.Fprint(w, `{"state":"pending","total_count":0,"statuses":[]}`) + default: + fmt.Fprint(w, `{"total_count":1,"check_runs":[{"id":1,"name":"Test","status":"in_progress","started_at":"2026-10-24T11:59:00Z"}]}`) + } + }) + checksJSON = false + + out, err := captureStdout(func() error { + return checksCmd.RunE(checksCmd, []string{"https://github.com/owner/repo/pull/1"}) + }) + if code := exitCode(err); code != exitChecksPending { + t.Errorf("exit code = %d (err=%v), want 8", code, err) + } + if !strings.Contains(out, "⏳ pending Test") || !strings.Contains(out, "merge: unstable") { + t.Errorf("stdout:\n%s", out) + } +} + +func TestChecksCmd_NoChecks(t *testing.T) { + setupWaitCommand(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/pulls/1"): + fmt.Fprint(w, `{"number":1,"state":"open","updated_at":"2026-01-01T00:00:00Z","head":{"sha":"abc1234"}}`) + case strings.HasSuffix(r.URL.Path, "/status"): + fmt.Fprint(w, `{"state":"pending","total_count":0,"statuses":[]}`) + default: + fmt.Fprint(w, `{"total_count":0,"check_runs":[]}`) + } + }) + checksJSON = true + t.Cleanup(func() { checksJSON = false }) + + out, err := captureStdout(func() error { + return checksCmd.RunE(checksCmd, []string{"https://github.com/owner/repo/pull/1"}) + }) + if code := exitCode(err); code != exitWaitNoCI { + t.Errorf("exit code = %d (err=%v), want 4", code, err) + } + if !strings.Contains(out, `"state": "no_ci"`) || !strings.Contains(out, `"checks": []`) { + t.Errorf("stdout:\n%s", out) + } +} diff --git a/cmd/prw/main.go b/cmd/prw/main.go index bb10c5c..7719aa9 100644 --- a/cmd/prw/main.go +++ b/cmd/prw/main.go @@ -283,8 +283,8 @@ var listCmd = &cobra.Command{ } w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0) - fmt.Fprintln(w, "REPO\tPR\tSTATUS\tLAST CHECKED\tADDED BY\tTITLE") - fmt.Fprintln(w, "----\t--\t------\t------------\t--------\t-----") + fmt.Fprintln(w, "REPO\tPR\tSTATUS\tREVIEW\tMERGE\tLAST CHECKED\tADDED BY\tTITLE") + fmt.Fprintln(w, "----\t--\t------\t------\t-----\t------------\t--------\t-----") for _, pr := range cfg.WatchedPRs { repo := fmt.Sprintf("%s/%s", pr.Owner, pr.Repo) @@ -305,7 +305,7 @@ var listCmd = &cobra.Command{ if pr.Source != "" { addedBy = "--" + pr.Source } - fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\n", repo, prNum, status, lastChecked, addedBy, title) + fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n", repo, prNum, status, orDash(pr.LastReviewState), orDash(pr.LastMergeState), lastChecked, addedBy, title) } w.Flush() @@ -484,6 +484,7 @@ var configShowCmd = &cobra.Command{ fmt.Printf("webhook_format: %s\n", notify.NormalizeWebhookFormat(cfg.WebhookFormat)) fmt.Printf("notification_filter: %s\n", cfg.NotificationFilter) fmt.Printf("notification_native: %v\n", cfg.NotificationNative) + fmt.Printf("pr_events: %v\n", !cfg.DisablePREvents) _, tokenSource := cfg.ResolveToken() fmt.Printf("github_token: %s\n", tokenSource) @@ -510,7 +511,8 @@ Supported keys: - webhook_format: auto, slack, discord, or generic (default: auto, detected from the URL) - github_token: GitHub personal access token - notification_filter: change, fail, or success - - notification_native: enable native OS notifications (true/false)`, + - notification_native: enable native OS notifications (true/false) + - pr_events: notify about reviews and merge conflicts / readiness, not just CI (true/false, default: true)`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { key := args[0] @@ -557,6 +559,12 @@ func applyConfigSet(cfg *config.Config, key, value string) error { return fmt.Errorf("notification_native must be true or false") } cfg.NotificationNative = enabled + case "pr_events": + enabled, err := strconv.ParseBool(value) + if err != nil { + return fmt.Errorf("pr_events must be true or false") + } + cfg.DisablePREvents = !enabled default: return fmt.Errorf("unknown config key: %s", key) } @@ -595,6 +603,8 @@ func applyConfigUnset(cfg *config.Config, key string) error { cfg.NotificationFilter = config.NotificationFilterChange case "notification_native": cfg.NotificationNative = false + case "pr_events": + cfg.DisablePREvents = false default: return fmt.Errorf("unknown config key: %s", key) } @@ -623,6 +633,16 @@ type listPROutput struct { LastChecked *time.Time `json:"last_checked,omitempty"` Title string `json:"title,omitempty"` Source string `json:"source,omitempty"` // auto-watch source that added it; empty if added by hand + Review string `json:"review,omitempty"` // approved, changes_requested or none; empty until checked + Merge string `json:"merge,omitempty"` // ready, conflict, blocked, behind, unstable or draft +} + +// orDash shows a state that hasn't been observed yet as "-". +func orDash(s string) string { + if s == "" { + return "-" + } + return s } func outputJSONList(prs []config.WatchedPR) error { @@ -647,6 +667,8 @@ func outputJSONList(prs []config.WatchedPR) error { LastChecked: lastChecked, Title: pr.Title, Source: pr.Source, + Review: pr.LastReviewState, + Merge: pr.LastMergeState, }) } diff --git a/cmd/prw/mcp.go b/cmd/prw/mcp.go index 2a434a5..c126960 100644 --- a/cmd/prw/mcp.go +++ b/cmd/prw/mcp.go @@ -428,7 +428,7 @@ var mcpTools = []map[string]interface{}{ { "name": "get_ci_status", "title": "Get PR CI status", - "description": "Get the current CI status of a GitHub pull request: overall state (success, failure, error, pending, merged, closed, or no_ci when the PR has no checks at all), check counts, and failed checks with links. Aggregates GitHub Actions check runs and legacy commit statuses. Returns immediately.", + "description": "Get the current CI status of a GitHub pull request: overall state (success, failure, error, pending, merged, closed, or no_ci when the PR has no checks at all), check counts, failed checks with links, and merge_state (ready, conflict, blocked, behind, unstable, draft) telling whether the PR can be merged. Aggregates GitHub Actions check runs and legacy commit statuses. Returns immediately.", "inputSchema": map[string]interface{}{ "type": "object", "properties": map[string]interface{}{"pr_url": prURLSchema}, @@ -626,6 +626,8 @@ type watchedPROutput struct { Number int `json:"number"` Title string `json:"title,omitempty"` LastState string `json:"last_known_state"` + Review string `json:"review,omitempty"` // approved, changes_requested, none + Merge string `json:"merge,omitempty"` // ready, conflict, blocked, behind, unstable, draft LastChecked *time.Time `json:"last_checked,omitempty"` } @@ -642,7 +644,7 @@ func toolListWatched(ctx context.Context, s *mcpServer, _ json.RawMessage, _ io. } out := watchedPROutput{ URL: github.FormatPRURL(pr.Owner, pr.Repo, pr.Number), Owner: pr.Owner, Repo: pr.Repo, - Number: pr.Number, Title: pr.Title, LastState: state, + Number: pr.Number, Title: pr.Title, LastState: state, Review: pr.LastReviewState, Merge: pr.LastMergeState, } if !pr.LastChecked.IsZero() { t := pr.LastChecked diff --git a/cmd/prw/wait.go b/cmd/prw/wait.go index 32895a0..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). @@ -291,6 +294,7 @@ func pollOnce(ctx context.Context, client ciClient, result *waitResult) (string, result.Title = pr.Title } result.SHA = pr.Head.SHA + result.MergeState = github.MergeStatus(pr) if pr.IsClosed() { result.State = "closed" diff --git a/internal/config/config.go b/internal/config/config.go index fb78d24..b722054 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,6 +27,9 @@ type Config struct { GitHubToken string `json:"github_token,omitempty"` NotificationFilter string `json:"notification_filter,omitempty"` NotificationNative bool `json:"notification_native,omitempty"` + // DisablePREvents turns off review (approved / changes requested) and merge + // (conflict / ready to merge) notifications; `pr_events` in `prw config`. + DisablePREvents bool `json:"disable_pr_events,omitempty"` // AutoWatch lists the searches (AutoWatchMine, AutoWatchReviewRequested) whose // matching PRs are added to the watch list automatically. @@ -101,6 +104,10 @@ type WatchedPR struct { LastKnownState string `json:"last_known_state,omitempty"` LastChecked time.Time `json:"last_checked,omitempty"` Title string `json:"title,omitempty"` + // LastReviewState and LastMergeState are the review decision and merge state + // (github.Review* / github.Merge*) seen on the last check; empty until observed. + LastReviewState string `json:"last_review_state,omitempty"` + LastMergeState string `json:"last_merge_state,omitempty"` // Source is the auto-watch source that added this PR, or empty if it was added by // hand with `prw watch `. Source string `json:"source,omitempty"` @@ -280,6 +287,8 @@ func (c *Config) ApplyPRState(pr WatchedPR) bool { w.LastKnownSHA = pr.LastKnownSHA w.LastKnownState = pr.LastKnownState w.LastChecked = pr.LastChecked + w.LastReviewState = pr.LastReviewState + w.LastMergeState = pr.LastMergeState if pr.Title != "" { w.Title = pr.Title } diff --git a/internal/github/client.go b/internal/github/client.go index a5b9069..87ba773 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -112,6 +112,10 @@ type PullRequest struct { // UpdatedAt changes on every push (and on comments, labels, ...), so it is an // upper bound for how recently the head commit was pushed. UpdatedAt time.Time `json:"updated_at"` + // MergeableState is GitHub's verdict on whether the PR can be merged (clean, + // dirty, blocked, behind, unstable, draft, ...); "unknown" while it's computed. + // See MergeStatus. + MergeableState string `json:"mergeable_state"` } // StateNoCI is reported instead of "pending" for a PR that has no checks or @@ -165,7 +169,10 @@ type CheckRun struct { Status string `json:"status"` // queued, in_progress, completed, ... Conclusion string `json:"conclusion"` // success, failure, neutral, cancelled, ... HTMLURL string `json:"html_url"` - App struct { + // StartedAt and CompletedAt are zero while queued / running. + StartedAt time.Time `json:"started_at"` + CompletedAt time.Time `json:"completed_at"` + App struct { ID int64 `json:"id"` } `json:"app"` } @@ -382,7 +389,7 @@ func SummarizeCI(status *CombinedStatus, runs []CheckRun) *CIStatus { } } - for _, run := range latestCheckRuns(runs) { + for _, run := range LatestCheckRuns(runs) { total++ if NormalizeState(run.Status) != "completed" { hasPending = true @@ -412,12 +419,12 @@ 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 +// 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 { +func LatestCheckRuns(runs []CheckRun) []CheckRun { type checkKey struct { app int64 name string 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/notify/native.go b/internal/notify/native.go index d2ad112..86f6f9b 100644 --- a/internal/notify/native.go +++ b/internal/notify/native.go @@ -55,7 +55,7 @@ func (n *NativeNotifier) Notify(event *StatusChangeEvent) error { return nil } - title := fmt.Sprintf("PR Status Change: %s/%s#%d", event.Owner, event.Repo, event.Number) + title := fmt.Sprintf("PR %s Change: %s/%s#%d", stateLabel(event.Kind), event.Owner, event.Repo, event.Number) message := fmt.Sprintf("%s → %s", event.PreviousState, event.CurrentState) if event.Title != "" { message = fmt.Sprintf("%s\n%s", event.Title, message) diff --git a/internal/notify/notify.go b/internal/notify/notify.go index ae8bf20..4196a9f 100644 --- a/internal/notify/notify.go +++ b/internal/notify/notify.go @@ -12,8 +12,17 @@ import ( "github.com/devblac/prw/internal/github" ) -// StatusChangeEvent represents a CI status change for a PR. +// Event kinds. CI status changes (and merged/closed) have an empty Kind, as before. +const ( + EventKindCI = "" + EventKindReview = "review" // states are github.Review* + EventKindMerge = "merge" // states are github.Merge* +) + +// StatusChangeEvent represents a change of a PR: its CI status by default, or its +// review decision or merge state, depending on Kind. type StatusChangeEvent struct { + Kind string Owner string Repo string Number int @@ -65,12 +74,19 @@ func NewConsoleNotifier() *ConsoleNotifier { func (c *ConsoleNotifier) Notify(event *StatusChangeEvent) error { prURL := github.FormatPRURL(event.Owner, event.Repo, event.Number) - fmt.Printf("\n🔔 Status Change Detected!\n") + switch event.Kind { + case EventKindReview: + fmt.Printf("\n🔔 Review Update!\n") + case EventKindMerge: + fmt.Printf("\n🔔 Merge Status Change!\n") + default: + fmt.Printf("\n🔔 Status Change Detected!\n") + } fmt.Printf(" PR: %s/%s#%d\n", event.Owner, event.Repo, event.Number) if event.Title != "" { fmt.Printf(" Title: %s\n", event.Title) } - fmt.Printf(" Status: %s → %s\n", event.PreviousState, event.CurrentState) + fmt.Printf(" %s: %s → %s\n", stateLabel(event.Kind), event.PreviousState, event.CurrentState) if len(event.FailedChecks) > 0 { fmt.Printf(" Failed checks:\n") for _, check := range event.FailedChecks { diff --git a/internal/notify/pr_events_format_test.go b/internal/notify/pr_events_format_test.go new file mode 100644 index 0000000..21ca17e --- /dev/null +++ b/internal/notify/pr_events_format_test.go @@ -0,0 +1,75 @@ +package notify + +import ( + "encoding/json" + "os/exec" + "strings" + "testing" + + "github.com/devblac/prw/internal/github" +) + +func prEvent(kind, prev, current string) *StatusChangeEvent { + return &StatusChangeEvent{Kind: kind, Owner: "o", Repo: "r", Number: 1, Title: "Add feature", PreviousState: prev, CurrentState: current} +} + +func TestPayloads_ReviewEvent(t *testing.T) { + event := prEvent(EventKindReview, github.ReviewNone, github.ReviewApproved) + + if text := slackPayload(event)["text"]; !strings.HasPrefix(text, "✅ ") || !strings.Contains(text, "Review: `none → approved`") { + t.Errorf("slack text = %q", text) + } + embed := discordPayload(event)["embeds"].([]discordEmbed)[0] + if !strings.Contains(embed.Description, "Review: `none → approved`") || embed.Color != colorSuccess { + t.Errorf("discord embed = %+v", embed) + } + data, _ := json.Marshal(genericPayload(event)) + if !strings.Contains(string(data), `"type":"pr_review_change"`) || !strings.Contains(string(data), `"current_state":"approved"`) { + t.Errorf("generic payload = %s", data) + } +} + +func TestPayloads_MergeEvents(t *testing.T) { + conflict := prEvent(EventKindMerge, github.MergeReady, github.MergeConflict) + if text := slackPayload(conflict)["text"]; !strings.HasPrefix(text, "⚠️ ") || !strings.Contains(text, "Merge: `ready → conflict`") { + t.Errorf("slack text = %q", text) + } + if c := discordPayload(conflict)["embeds"].([]discordEmbed)[0].Color; c != colorFailure { + t.Errorf("conflict color = %#x", c) + } + ready := prEvent(EventKindMerge, github.MergeBlocked, github.MergeReady) + if text := slackPayload(ready)["text"]; !strings.HasPrefix(text, "🚀 ") { + t.Errorf("slack text = %q", text) + } + if data, _ := json.Marshal(genericPayload(ready)); !strings.Contains(string(data), `"type":"pr_merge_state_change"`) { + t.Errorf("generic payload = %s", data) + } +} + +func TestPayloads_CIEventsKeepTheirType(t *testing.T) { + event := prEvent(EventKindCI, "pending", "success") + if data, _ := json.Marshal(genericPayload(event)); !strings.Contains(string(data), `"type":"pr_status_change"`) { + t.Errorf("generic payload = %s", data) + } + if text := slackPayload(event)["text"]; !strings.Contains(text, "Status: `pending → success`") { + t.Errorf("slack text = %q", text) + } +} + +func TestNativeTitle_NamesTheKind(t *testing.T) { + var seen string + old := runNativeCommand + runNativeCommand = func(cmd *exec.Cmd) error { + seen = strings.Join(append(cmd.Args, cmd.Env...), "\n") // title is an arg or env var + return nil + } + t.Cleanup(func() { runNativeCommand = old }) + + n := &NativeNotifier{enabled: true} + if err := n.Notify(prEvent(EventKindReview, github.ReviewNone, github.ReviewChangesRequested)); err != nil { + t.Fatal(err) + } + if seen != "" && !strings.Contains(seen, "PR Review Change: o/r#1") { + t.Errorf("native notification doesn't name the review:\n%s", seen) + } +} diff --git a/internal/notify/webhook_format.go b/internal/notify/webhook_format.go index 7f7f29a..731d6e9 100644 --- a/internal/notify/webhook_format.go +++ b/internal/notify/webhook_format.go @@ -30,6 +30,7 @@ const ( colorFailure = 0xD1242F colorPending = 0xDBAB09 colorNoCI = 0x8C959F + colorChanges = 0xBC4C00 ) // IsValidWebhookFormat reports whether value is a supported webhook format. @@ -91,7 +92,7 @@ func buildWebhookBody(format string, event *StatusChangeEvent) ([]byte, error) { func genericPayload(event *StatusChangeEvent) WebhookPayload { return WebhookPayload{ - Type: "pr_status_change", + Type: payloadType(event.Kind), Owner: event.Owner, Repo: event.Repo, PRNumber: event.Number, @@ -105,14 +106,44 @@ func genericPayload(event *StatusChangeEvent) WebhookPayload { } } +// payloadType is the generic payload's "type" for each event kind. +func payloadType(kind string) string { + switch kind { + case EventKindReview: + return "pr_review_change" + case EventKindMerge: + return "pr_merge_state_change" + default: + return "pr_status_change" + } +} + +// stateLabel names what changed, e.g. "Status: pending → success". +func stateLabel(kind string) string { + switch kind { + case EventKindReview: + return "Review" + case EventKindMerge: + return "Merge" + default: + return "Status" + } +} + func stateEmoji(state string) string { switch state { - case "success": + case "success", github.ReviewApproved: return "✅" case "failure", "error": return "❌" case github.StateNoCI: return "⚪" + case github.ReviewChangesRequested: + return "📝" + case github.MergeConflict: + return "⚠️" + case github.MergeReady: + return "🚀" default: return "⏳" } @@ -120,10 +151,12 @@ func stateEmoji(state string) string { func stateColor(state string) int { switch state { - case "success": + case "success", github.ReviewApproved, github.MergeReady: return colorSuccess - case "failure", "error": + case "failure", "error", github.MergeConflict: return colorFailure + case github.ReviewChangesRequested: + return colorChanges case github.StateNoCI: return colorNoCI default: @@ -156,7 +189,7 @@ func slackPayload(event *StatusChangeEvent) map[string]string { if event.Title != "" { fmt.Fprintf(&b, " — %s", slackEscape(event.Title)) } - fmt.Fprintf(&b, "\nStatus: `%s`", transition(event)) + fmt.Fprintf(&b, "\n%s: `%s`", stateLabel(event.Kind), transition(event)) for i, check := range event.FailedChecks { if i == maxChecksListed { @@ -188,7 +221,7 @@ func discordPayload(event *StatusChangeEvent) map[string]interface{} { } var desc strings.Builder - fmt.Fprintf(&desc, "Status: `%s`", transition(event)) + fmt.Fprintf(&desc, "%s: `%s`", stateLabel(event.Kind), transition(event)) for i, check := range event.FailedChecks { if i == maxChecksListed { fmt.Fprintf(&desc, "\n… and %d more", len(event.FailedChecks)-maxChecksListed) diff --git a/internal/watcher/pr_events_test.go b/internal/watcher/pr_events_test.go new file mode 100644 index 0000000..d02b020 --- /dev/null +++ b/internal/watcher/pr_events_test.go @@ -0,0 +1,171 @@ +package watcher + +import ( + "context" + "testing" + + "github.com/devblac/prw/internal/config" + "github.com/devblac/prw/internal/github" + "github.com/devblac/prw/internal/notify" +) + +func review(login, state string) github.Review { + var r github.Review + r.User.Login = login + r.State = state + return r +} + +// prEventsCase is one check of PR #1 whose CI stays green, starting from the given +// stored review and merge states. +type prEventsCase struct { + lastReview, lastMerge string + reviews []github.Review + mergeableState string + filter string + disabled bool +} + +func runPREventsCheck(t *testing.T, c prEventsCase) (*config.WatchedPR, []*notify.StatusChangeEvent) { + t.Helper() + pr := newPR(1, "sha1") + pr.State = "open" + pr.MergeableState = c.mergeableState + client := &mockGitHubClient{ + prs: map[string]*github.PullRequest{"owner/repo/1": pr}, + statuses: map[string]*github.CombinedStatus{"sha1": {State: "success", TotalCount: 1}}, + reviews: map[string][]github.Review{"owner/repo/1": c.reviews}, + } + watchedPR := watched(1, "success") + watchedPR.LastReviewState, watchedPR.LastMergeState = c.lastReview, c.lastMerge + cfg := &config.Config{NotificationFilter: c.filter, DisablePREvents: c.disabled, WatchedPRs: []config.WatchedPR{watchedPR}} + notifier := &mockNotifier{} + + if _, err := New(client, cfg, notifier).checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { + t.Fatal(err) + } + return &cfg.WatchedPRs[0], notifier.events +} + +func TestCheckPR_NotifiesApproval(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewNone, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("alice", "COMMENTED"), review("alice", "APPROVED")}, + mergeableState: "blocked", // e.g. a second approval is required + }) + if len(events) != 1 || events[0].Kind != notify.EventKindReview || events[0].PreviousState != "none" || events[0].CurrentState != "approved" { + t.Fatalf("events = %+v, want one none → approved review event", events) + } + if pr.LastReviewState != github.ReviewApproved || pr.LastMergeState != github.MergeBlocked { + t.Errorf("stored review=%q merge=%q", pr.LastReviewState, pr.LastMergeState) + } +} + +func TestCheckPR_NotifiesChangesRequested(t *testing.T) { + _, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewApproved, lastMerge: github.MergeReady, + reviews: []github.Review{review("alice", "APPROVED"), review("bob", "CHANGES_REQUESTED")}, + mergeableState: "clean", + }) + if len(events) != 1 || events[0].CurrentState != github.ReviewChangesRequested { + t.Fatalf("events = %+v, want approved → changes_requested", events) + } +} + +func TestCheckPR_FirstObservationDoesNotNotify(t *testing.T) { + // Existing watch lists (and newly added PRs) have no stored review/merge state yet. + pr, events := runPREventsCheck(t, prEventsCase{ + reviews: []github.Review{review("alice", "APPROVED")}, + mergeableState: "dirty", + }) + if len(events) != 0 { + t.Errorf("events = %+v, want none on the first check", events) + } + if pr.LastReviewState != github.ReviewApproved || pr.LastMergeState != github.MergeConflict { + t.Errorf("stored review=%q merge=%q, want them recorded", pr.LastReviewState, pr.LastMergeState) + } +} + +func TestCheckPR_NotifiesNewConflict(t *testing.T) { + _, events := runPREventsCheck(t, prEventsCase{lastReview: github.ReviewNone, lastMerge: github.MergeReady, mergeableState: "dirty"}) + if len(events) != 1 || events[0].Kind != notify.EventKindMerge || events[0].CurrentState != github.MergeConflict { + t.Fatalf("events = %+v, want ready → conflict", events) + } +} + +func TestCheckPR_ReadyToMergeAfterBranchProtectionIsSatisfied(t *testing.T) { + // The required approval came from someone whose review was already counted, + // or a required reviewer was satisfied by CODEOWNERS: only the merge state changes. + _, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewApproved, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("alice", "APPROVED")}, + mergeableState: "clean", + }) + if len(events) != 1 || events[0].CurrentState != github.MergeReady || events[0].PreviousState != github.MergeBlocked { + t.Fatalf("events = %+v, want blocked → ready", events) + } +} + +func TestCheckPR_ReadyIsNotSentWithTheApprovalThatCausedIt(t *testing.T) { + _, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewNone, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("alice", "APPROVED")}, + mergeableState: "clean", + }) + if len(events) != 1 || events[0].Kind != notify.EventKindReview { + t.Fatalf("events = %+v, want only the approval", events) + } +} + +func TestCheckPR_UnstableToReadyIsLeftToTheCINotification(t *testing.T) { + // No branch protection: the PR is "unstable" while checks run and "clean" once they + // pass, which the CI success notification already announces. + _, events := runPREventsCheck(t, prEventsCase{lastReview: github.ReviewNone, lastMerge: github.MergeUnstable, mergeableState: "clean"}) + if len(events) != 0 { + t.Errorf("events = %+v, want none", events) + } +} + +func TestCheckPR_UnknownMergeStateKeepsTheLastOne(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{lastReview: github.ReviewNone, lastMerge: github.MergeConflict, mergeableState: "unknown"}) + if len(events) != 0 || pr.LastMergeState != github.MergeConflict { + t.Errorf("events=%+v merge=%q; while GitHub computes mergeability nothing changes", events, pr.LastMergeState) + } +} + +func TestCheckPR_DismissedReviewWithdrawsTheVerdict(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewChangesRequested, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("bob", "CHANGES_REQUESTED"), review("bob", "DISMISSED")}, + mergeableState: "blocked", + }) + if len(events) != 0 || pr.LastReviewState != github.ReviewNone { + t.Errorf("events=%+v review=%q, want silently back to none", events, pr.LastReviewState) + } +} + +func TestCheckPR_PREventFilters(t *testing.T) { + tests := []struct { + filter string + c prEventsCase + want int + }{ + {config.NotificationFilterFail, prEventsCase{lastReview: "none", lastMerge: "blocked", reviews: []github.Review{review("a", "APPROVED")}, mergeableState: "blocked"}, 0}, + {config.NotificationFilterFail, prEventsCase{lastReview: "none", lastMerge: "ready", mergeableState: "dirty"}, 1}, + {config.NotificationFilterSuccess, prEventsCase{lastReview: "none", lastMerge: "ready", mergeableState: "dirty"}, 0}, + {config.NotificationFilterSuccess, prEventsCase{lastReview: "approved", lastMerge: "blocked", reviews: []github.Review{review("a", "APPROVED")}, mergeableState: "clean"}, 1}, + } + for _, tt := range tests { + tt.c.filter = tt.filter + if _, events := runPREventsCheck(t, tt.c); len(events) != tt.want { + t.Errorf("filter %s, %+v: %d events, want %d", tt.filter, tt.c, len(events), tt.want) + } + } +} + +func TestCheckPR_PREventsCanBeDisabled(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{lastReview: "none", lastMerge: "ready", mergeableState: "dirty", disabled: true}) + if len(events) != 0 || pr.LastMergeState != "ready" { + t.Errorf("events=%+v merge=%q, want pr_events=false to skip review/merge checks", events, pr.LastMergeState) + } +} diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 53aa364..70d572f 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -15,6 +15,7 @@ type GitHubClient interface { GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) + GetReviews(ctx context.Context, owner, repo string, number int) ([]github.Review, error) } // ConfigStore defines the interface for config persistence. @@ -236,8 +237,10 @@ func (w *Watcher) checkPR(ctx context.Context, pr *config.WatchedPR) (closed boo // Check if status changed. Finding out that a PR has no CI is not a CI result, so // it isn't announced (it would otherwise ping once for every such PR). + notifiedCI := false if previousState != "" && previousState != currentState && currentState != github.StateNoCI && shouldNotify(w.config.NotificationFilter, currentState) { + notifiedCI = true event := ¬ify.StatusChangeEvent{ Owner: pr.Owner, Repo: pr.Repo, @@ -260,9 +263,91 @@ func (w *Watcher) checkPR(ctx context.Context, pr *config.WatchedPR) (closed boo pr.LastKnownState = currentState pr.LastChecked = time.Now() + if !w.config.DisablePREvents { + if err := w.checkPREvents(ctx, pr, ghPR, notifiedCI); err != nil { + return false, err + } + } return false, nil } +// checkPREvents notifies about changes of the PR's review decision and merge state. +// notifiedCI tells whether a CI notification was just sent for this PR. Nothing is +// announced the first time a PR is seen (the previous state is empty). +func (w *Watcher) checkPREvents(ctx context.Context, pr *config.WatchedPR, ghPR *github.PullRequest, notifiedCI bool) error { + reviews, err := w.client.GetReviews(ctx, pr.Owner, pr.Repo, pr.Number) + if err != nil { + return fmt.Errorf("failed to fetch reviews: %w", err) + } + notified := notifiedCI + + review := github.ReviewDecision(reviews) + if prev := pr.LastReviewState; prev != "" && prev != review && review != github.ReviewNone && + shouldNotifyPREvent(w.config.NotificationFilter, review) { + w.notifyPREvent(pr, ghPR, notify.EventKindReview, prev, review) + notified = true + } + pr.LastReviewState = review + + merge := github.MergeStatus(ghPR) + if merge == "" { + return nil // GitHub is still computing it; keep the last known state + } + if prev := pr.LastMergeState; prev != "" && prev != merge && mergeWorthNotifying(prev, merge, notified) && + shouldNotifyPREvent(w.config.NotificationFilter, merge) { + w.notifyPREvent(pr, ghPR, notify.EventKindMerge, prev, merge) + } + pr.LastMergeState = merge + return nil +} + +// mergeWorthNotifying decides which merge state changes are announced: new conflicts, +// and a PR becoming ready to merge after something other than its CI was holding it +// back (branch protection, conflicts, an outdated branch). Without branch protection a +// PR goes from unstable to ready as soon as CI passes, which the CI notification +// already says; the same goes for an approval that unblocks it, so "ready" isn't sent +// together with another notification. +func mergeWorthNotifying(prev, merge string, notified bool) bool { + switch merge { + case github.MergeConflict: + return true + case github.MergeReady: + return !notified && (prev == github.MergeBlocked || prev == github.MergeConflict || prev == github.MergeBehind) + } + return false +} + +func (w *Watcher) notifyPREvent(pr *config.WatchedPR, ghPR *github.PullRequest, kind, prev, current string) { + event := ¬ify.StatusChangeEvent{ + Kind: kind, + Owner: pr.Owner, + Repo: pr.Repo, + Number: pr.Number, + Title: pr.Title, + PreviousState: prev, + CurrentState: current, + SHA: ghPR.Head.SHA, + Timestamp: time.Now(), + } + if err := w.notifier.Notify(event); err != nil { + fmt.Printf("Warning: notification failed: %v\n", err) + } +} + +// shouldNotifyPREvent applies the notification filter to review and merge events: +// "fail" keeps the ones that need action (changes requested, conflicts), "success" +// the good news (approved, ready to merge). +func shouldNotifyPREvent(filter, state string) bool { + switch config.NormalizeNotificationFilter(filter) { + case config.NotificationFilterFail: + return state == github.ReviewChangesRequested || state == github.MergeConflict + case config.NotificationFilterSuccess: + return state == github.ReviewApproved || state == github.MergeReady + default: + return true + } +} + func shouldNotify(filter, currentState string) bool { if !config.IsValidNotificationFilter(filter) { filter = config.NotificationFilterChange diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 53b45d8..c03a2e0 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -17,9 +17,17 @@ type mockGitHubClient struct { prs map[string]*github.PullRequest statuses map[string]*github.CombinedStatus checkRuns map[string][]github.CheckRun + reviews map[string][]github.Review // keyed like prs err error } +func (m *mockGitHubClient) GetReviews(ctx context.Context, owner, repo string, number int) ([]github.Review, error) { + if m.err != nil { + return nil, m.err + } + return m.reviews[fmt.Sprintf("%s/%s/%d", owner, repo, number)], nil +} + func (m *mockGitHubClient) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) { if m.err != nil { return nil, m.err