From 04ad98581512eb1263d74f35d6dd482ca2e7b220 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 03:11:36 -0300 Subject: [PATCH 1/5] feat: notify about reviews and merge conflicts, not just CI `prw run` now also tells you when a watched PR is approved, gets changes requested, runs into merge conflicts, or becomes ready to merge after branch protection, conflicts or an outdated branch were holding it back. - Review decision from /pulls/{n}/reviews: each reviewer's latest approval or change request counts, comments don't, dismissals withdraw it. One extra request per PR per poll, a free 304 when nothing changed. - Merge state from the PR's mergeable_state (already fetched); "unknown" while GitHub computes it keeps the last value. - Quiet by design: nothing on the first check of a PR; "ready" is not sent with the CI or approval notification that caused it, nor for repositories without branch protection (unstable -> clean is just CI passing). - Events carry a Kind; console, native, Slack/Discord ("Review:" / "Merge:") and generic webhooks ("pr_review_change" / "pr_merge_state_change") render them. --on fail keeps changes_requested and conflict, --on success approved and ready. - `prw list` gains REVIEW and MERGE columns (review/merge in --json and MCP list_watched); `prw wait --json` and the MCP tools report merge_state. - `prw config set pr_events false` turns it off. Verified live: devblac/prw#12 ready, cli/cli#14727 blocked; a simulated blocked -> ready notified once, ready -> blocked stayed silent. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + README.md | 25 ++++ TASKS.md | 3 +- cmd/prw/main.go | 30 +++- cmd/prw/mcp.go | 6 +- cmd/prw/wait.go | 30 ++-- internal/config/config.go | 9 ++ internal/github/client.go | 4 + internal/github/review.go | 102 ++++++++++++++ internal/github/review_test.go | 83 +++++++++++ internal/notify/native.go | 2 +- internal/notify/notify.go | 22 ++- internal/notify/pr_events_format_test.go | 75 ++++++++++ internal/notify/webhook_format.go | 45 +++++- internal/watcher/pr_events_test.go | 171 +++++++++++++++++++++++ internal/watcher/watcher.go | 85 +++++++++++ internal/watcher/watcher_test.go | 8 ++ 17 files changed, 671 insertions(+), 30 deletions(-) create mode 100644 internal/github/review.go create mode 100644 internal/github/review_test.go create mode 100644 internal/notify/pr_events_format_test.go create mode 100644 internal/watcher/pr_events_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a90e9c..5ebb476 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `prw run` now notifies once (`merged` or `closed`) and automatically stops watching a PR once it is merged or closed, instead of polling it forever. This notification always fires, regardless of `--on`/`notification_filter`. ### Added +- Review and merge events: `prw run` notifies when a PR is approved, gets changes requested, runs into merge conflicts, or becomes ready to merge after branch protection was holding it back. Shown in console, native, Slack/Discord and generic webhooks (`type` `pr_review_change` / `pr_merge_state_change`), filtered by `--on` (`fail`: changes requested and conflicts; `success`: approved and ready). `prw list` gains `REVIEW` and `MERGE` columns (`review` / `merge` in `--json` and MCP `list_watched`); `prw wait --json` and the MCP tools report `merge_state`. Disable with `prw config set pr_events false`. - Auto-watch: `prw watch --mine` follows all your open PRs and `prw watch --review-requested` the PRs where your review is requested, across every repository. Matches are added immediately; `prw run` refreshes the searches every 5 minutes. Review-request PRs are dropped when the request goes away; PRs watched by hand are never touched by it. `prw unwatch --mine` / `--review-requested` turns it off and removes the PRs it added. `prw list` gains an `ADDED BY` column (and `source` in `--json`); `prw config show` shows `auto_watch`. - `prw mcp`: a Model Context Protocol server over stdio for AI coding agents, with tools `get_ci_status`, `wait_for_ci` (progress notifications, cancellation, fail-fast), `list_watched`, `watch_pr` and `unwatch_pr`. Implemented without new dependencies (the official Go SDK needs Go 1.25). - Conditional requests (ETags): repeated polls of unchanged resources are answered with `304 Not Modified`, which doesn't count against the authenticated rate limit. Measured against the real API: after the first poll, 5 further polls of a PR (15 requests) consumed 0 quota. Previously each watched PR cost 3 requests per poll (~540/hour at the default 20s interval), so about 9 PRs exhausted the 5,000/hour limit. diff --git a/README.md b/README.md index 739db12..76f651b 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,7 @@ The tool is deliberately minimal. It does one thing well: watch PRs and tell you - **Auto-cleanup**: merged/closed PRs are notified once and dropped from the watch list by `prw run` - **Watch multiple PRs** from different repositories simultaneously - **Instant notifications** when CI status changes (pending, success, failure, error) +- **Review & merge events**: approved, changes requested, merge conflicts, ready to merge - **Chat-ops broadcast**: one command to push current PR status to Slack/Discord (`prw broadcast`) - **Webhook support** for Slack, Discord, or custom integrations - **Native OS notifications** via system toasts (macOS/Linux/Windows) @@ -345,6 +346,7 @@ prw config set webhook_url https://hooks.slack.com/services/YOUR/WEBHOOK/URL - **`webhook_url`**: Optional HTTP endpoint for notifications - **`webhook_format`**: Payload format: `auto` (default, detected from the URL), `slack`, `discord`, or `generic` - **`notification_native`**: Enable native OS notifications (true/false, default: false) +- **`pr_events`**: Also notify about reviews and merge conflicts / readiness (true/false, default: true) - **`github_token`**: GitHub Personal Access Token (prefer env var `GITHUB_TOKEN`) ## Notifications @@ -432,6 +434,29 @@ prw run --on change The same setting can be persisted via `prw config set notification_filter `. +### Reviews and merge conflicts + +Besides CI, `prw run` notifies when a PR's review decision or merge state changes: + +| Event | When | +|-------|------| +| `approved` | A reviewer approved and nobody's latest review requests changes | +| `changes_requested` | A reviewer's latest review requests changes | +| `conflict` | The PR has merge conflicts with its base branch | +| `ready` | GitHub says the PR can be merged, after branch protection, conflicts or an outdated branch were holding it back | + +``` +๐Ÿ”” Review Update! + PR: kubernetes/kubernetes#12345 + Review: none โ†’ approved +``` + +Webhooks get the same shapes as CI changes, with `"type": "pr_review_change"` or `"pr_merge_state_change"` in the generic payload (and `Review:` / `Merge:` instead of `Status:` in Slack and Discord messages). With `--on fail` you only get `changes_requested` and `conflict`; with `--on success`, `approved` and `ready`. + +A few rules keep this quiet: nothing is announced the first time a PR is checked; `ready` isn't sent together with the CI or approval notification that caused it, and not at all for repositories without branch protection (there, "CI passed" already means it); dismissed reviews and new review rounds don't notify until someone approves or requests changes. `prw list` shows the last review and merge state in the `REVIEW` and `MERGE` columns, `prw wait --json` and the MCP tools include `merge_state`. + +Turn these off with `prw config set pr_events false`. They cost one extra request per PR per poll (the reviews), answered with a free `304` when nothing changed. + ### Merged and closed PRs Once `prw run` sees that a watched PR was merged or closed, it sends one final notification (`CurrentState` is `merged` or `closed`, so you'll see e.g. `success โ†’ merged`) and removes the PR from the watch list โ€” regardless of `--on`/`notification_filter`, since a merge is a lifecycle event, not a CI result. This keeps the watch list from growing forever. `prw list` and `prw broadcast` are unaffected: a PR only drops out once `prw run` has actually polled it. diff --git a/TASKS.md b/TASKS.md index 6c34e5a..2880380 100644 --- a/TASKS.md +++ b/TASKS.md @@ -91,7 +91,8 @@ Legend โ€” Impact: 1โ€“5 ยท Effort: S (โ‰คยฝ day), M (1โ€“2 days), L (3+ days) - ๐Ÿ†• #18 **Report "no CI" instead of `pending` forever** (found while testing #10: 14 of 18 real PRs have zero checks โ€” repos without CI, or stacked PRs whose base isn't `main`). Today `prw wait` / `wait_for_ci` would wait until the timeout on them. Needs a grace period after the last push (CI takes a moment to register checks) before concluding "no CI". Impact 4, effort Sโ€“M. - โœ… #18 `no_ci` state โ€” done (0 checks + PR not updated for 3 min โ†’ `no_ci`; `prw wait` exits 4; no notification; verified live: 11 real PRs went from `pending` to `no_ci`, `prw wait` on one returns in 2s instead of the 30-min timeout) - โœ… Codex review feedback on #6, #7, #13โ€“#17 โ€” done in `fix/codex-review`: superseded check runs (verified live on zephyrproject-rtos/zephyr#105183: was `failure`, now `success`), head re-check in `prw wait`, context propagation into every API request, closed review-request PRs keep their notification, no `no_ci` flapping, ETag cache eviction, GovSlack. -- Next: #11 review & mergeability events +- โœ… #11 Review & mergeability events โ€” done (approved / changes_requested / conflict / ready; verified live: devblac/prw#12 `ready`, cli/cli#14727 `blocked`, a simulated blocked โ†’ ready notified once and ready โ†’ blocked stayed silent) +- Next: #12 `prw checks ` ## Planned diff --git a/cmd/prw/main.go b/cmd/prw/main.go index bb10c5c..7719aa9 100644 --- a/cmd/prw/main.go +++ b/cmd/prw/main.go @@ -283,8 +283,8 @@ var listCmd = &cobra.Command{ } w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0) - fmt.Fprintln(w, "REPO\tPR\tSTATUS\tLAST CHECKED\tADDED BY\tTITLE") - fmt.Fprintln(w, "----\t--\t------\t------------\t--------\t-----") + fmt.Fprintln(w, "REPO\tPR\tSTATUS\tREVIEW\tMERGE\tLAST CHECKED\tADDED BY\tTITLE") + fmt.Fprintln(w, "----\t--\t------\t------\t-----\t------------\t--------\t-----") for _, pr := range cfg.WatchedPRs { repo := fmt.Sprintf("%s/%s", pr.Owner, pr.Repo) @@ -305,7 +305,7 @@ var listCmd = &cobra.Command{ if pr.Source != "" { addedBy = "--" + pr.Source } - fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\n", repo, prNum, status, lastChecked, addedBy, title) + fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n", repo, prNum, status, orDash(pr.LastReviewState), orDash(pr.LastMergeState), lastChecked, addedBy, title) } w.Flush() @@ -484,6 +484,7 @@ var configShowCmd = &cobra.Command{ fmt.Printf("webhook_format: %s\n", notify.NormalizeWebhookFormat(cfg.WebhookFormat)) fmt.Printf("notification_filter: %s\n", cfg.NotificationFilter) fmt.Printf("notification_native: %v\n", cfg.NotificationNative) + fmt.Printf("pr_events: %v\n", !cfg.DisablePREvents) _, tokenSource := cfg.ResolveToken() fmt.Printf("github_token: %s\n", tokenSource) @@ -510,7 +511,8 @@ Supported keys: - webhook_format: auto, slack, discord, or generic (default: auto, detected from the URL) - github_token: GitHub personal access token - notification_filter: change, fail, or success - - notification_native: enable native OS notifications (true/false)`, + - notification_native: enable native OS notifications (true/false) + - pr_events: notify about reviews and merge conflicts / readiness, not just CI (true/false, default: true)`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { key := args[0] @@ -557,6 +559,12 @@ func applyConfigSet(cfg *config.Config, key, value string) error { return fmt.Errorf("notification_native must be true or false") } cfg.NotificationNative = enabled + case "pr_events": + enabled, err := strconv.ParseBool(value) + if err != nil { + return fmt.Errorf("pr_events must be true or false") + } + cfg.DisablePREvents = !enabled default: return fmt.Errorf("unknown config key: %s", key) } @@ -595,6 +603,8 @@ func applyConfigUnset(cfg *config.Config, key string) error { cfg.NotificationFilter = config.NotificationFilterChange case "notification_native": cfg.NotificationNative = false + case "pr_events": + cfg.DisablePREvents = false default: return fmt.Errorf("unknown config key: %s", key) } @@ -623,6 +633,16 @@ type listPROutput struct { LastChecked *time.Time `json:"last_checked,omitempty"` Title string `json:"title,omitempty"` Source string `json:"source,omitempty"` // auto-watch source that added it; empty if added by hand + Review string `json:"review,omitempty"` // approved, changes_requested or none; empty until checked + Merge string `json:"merge,omitempty"` // ready, conflict, blocked, behind, unstable or draft +} + +// orDash shows a state that hasn't been observed yet as "-". +func orDash(s string) string { + if s == "" { + return "-" + } + return s } func outputJSONList(prs []config.WatchedPR) error { @@ -647,6 +667,8 @@ func outputJSONList(prs []config.WatchedPR) error { LastChecked: lastChecked, Title: pr.Title, Source: pr.Source, + Review: pr.LastReviewState, + Merge: pr.LastMergeState, }) } diff --git a/cmd/prw/mcp.go b/cmd/prw/mcp.go index 2a434a5..c126960 100644 --- a/cmd/prw/mcp.go +++ b/cmd/prw/mcp.go @@ -428,7 +428,7 @@ var mcpTools = []map[string]interface{}{ { "name": "get_ci_status", "title": "Get PR CI status", - "description": "Get the current CI status of a GitHub pull request: overall state (success, failure, error, pending, merged, closed, or no_ci when the PR has no checks at all), check counts, and failed checks with links. Aggregates GitHub Actions check runs and legacy commit statuses. Returns immediately.", + "description": "Get the current CI status of a GitHub pull request: overall state (success, failure, error, pending, merged, closed, or no_ci when the PR has no checks at all), check counts, failed checks with links, and merge_state (ready, conflict, blocked, behind, unstable, draft) telling whether the PR can be merged. Aggregates GitHub Actions check runs and legacy commit statuses. Returns immediately.", "inputSchema": map[string]interface{}{ "type": "object", "properties": map[string]interface{}{"pr_url": prURLSchema}, @@ -626,6 +626,8 @@ type watchedPROutput struct { Number int `json:"number"` Title string `json:"title,omitempty"` LastState string `json:"last_known_state"` + Review string `json:"review,omitempty"` // approved, changes_requested, none + Merge string `json:"merge,omitempty"` // ready, conflict, blocked, behind, unstable, draft LastChecked *time.Time `json:"last_checked,omitempty"` } @@ -642,7 +644,7 @@ func toolListWatched(ctx context.Context, s *mcpServer, _ json.RawMessage, _ io. } out := watchedPROutput{ URL: github.FormatPRURL(pr.Owner, pr.Repo, pr.Number), Owner: pr.Owner, Repo: pr.Repo, - Number: pr.Number, Title: pr.Title, LastState: state, + Number: pr.Number, Title: pr.Title, LastState: state, Review: pr.LastReviewState, Merge: pr.LastMergeState, } if !pr.LastChecked.IsZero() { t := pr.LastChecked diff --git a/cmd/prw/wait.go b/cmd/prw/wait.go index 77edee6..cc3e421 100644 --- a/cmd/prw/wait.go +++ b/cmd/prw/wait.go @@ -149,19 +149,22 @@ Progress goes to stderr; the result goes to stdout (as JSON with --json).`, // waitResult is the outcome of waitForCI; it is also the --json output. type waitResult struct { - Owner string `json:"owner"` - Repo string `json:"repo"` - Number int `json:"number"` - Title string `json:"title,omitempty"` - URL string `json:"url"` - SHA string `json:"sha,omitempty"` - State string `json:"state"` // success, failure, error, pending, merged, closed - FailedChecks []github.FailedCheck `json:"failed_checks,omitempty"` - TotalChecks int `json:"total_checks"` - PendingChecks int `json:"pending_checks"` - ElapsedSeconds float64 `json:"elapsed_seconds"` - TimedOut bool `json:"timed_out,omitempty"` - Interrupted bool `json:"interrupted,omitempty"` + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + Title string `json:"title,omitempty"` + URL string `json:"url"` + SHA string `json:"sha,omitempty"` + State string `json:"state"` // success, failure, error, pending, merged, closed + FailedChecks []github.FailedCheck `json:"failed_checks,omitempty"` + TotalChecks int `json:"total_checks"` + PendingChecks int `json:"pending_checks"` + // MergeState is GitHub's mergeability (ready, conflict, blocked, behind, unstable, + // draft); empty while GitHub is still computing it. + MergeState string `json:"merge_state,omitempty"` + ElapsedSeconds float64 `json:"elapsed_seconds"` + TimedOut bool `json:"timed_out,omitempty"` + Interrupted bool `json:"interrupted,omitempty"` } // exitErr maps the result to the command's exit code (nil means exit 0). @@ -282,6 +285,7 @@ func pollOnce(ctx context.Context, client ciClient, result *waitResult) (string, result.Title = pr.Title } result.SHA = pr.Head.SHA + result.MergeState = github.MergeStatus(pr) if pr.IsClosed() { result.State = "closed" diff --git a/internal/config/config.go b/internal/config/config.go index fb78d24..b722054 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,6 +27,9 @@ type Config struct { GitHubToken string `json:"github_token,omitempty"` NotificationFilter string `json:"notification_filter,omitempty"` NotificationNative bool `json:"notification_native,omitempty"` + // DisablePREvents turns off review (approved / changes requested) and merge + // (conflict / ready to merge) notifications; `pr_events` in `prw config`. + DisablePREvents bool `json:"disable_pr_events,omitempty"` // AutoWatch lists the searches (AutoWatchMine, AutoWatchReviewRequested) whose // matching PRs are added to the watch list automatically. @@ -101,6 +104,10 @@ type WatchedPR struct { LastKnownState string `json:"last_known_state,omitempty"` LastChecked time.Time `json:"last_checked,omitempty"` Title string `json:"title,omitempty"` + // LastReviewState and LastMergeState are the review decision and merge state + // (github.Review* / github.Merge*) seen on the last check; empty until observed. + LastReviewState string `json:"last_review_state,omitempty"` + LastMergeState string `json:"last_merge_state,omitempty"` // Source is the auto-watch source that added this PR, or empty if it was added by // hand with `prw watch `. Source string `json:"source,omitempty"` @@ -280,6 +287,8 @@ func (c *Config) ApplyPRState(pr WatchedPR) bool { w.LastKnownSHA = pr.LastKnownSHA w.LastKnownState = pr.LastKnownState w.LastChecked = pr.LastChecked + w.LastReviewState = pr.LastReviewState + w.LastMergeState = pr.LastMergeState if pr.Title != "" { w.Title = pr.Title } diff --git a/internal/github/client.go b/internal/github/client.go index a5b9069..ddbf810 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -112,6 +112,10 @@ type PullRequest struct { // UpdatedAt changes on every push (and on comments, labels, ...), so it is an // upper bound for how recently the head commit was pushed. UpdatedAt time.Time `json:"updated_at"` + // MergeableState is GitHub's verdict on whether the PR can be merged (clean, + // dirty, blocked, behind, unstable, draft, ...); "unknown" while it's computed. + // See MergeStatus. + MergeableState string `json:"mergeable_state"` } // StateNoCI is reported instead of "pending" for a PR that has no checks or diff --git a/internal/github/review.go b/internal/github/review.go new file mode 100644 index 0000000..a3de91e --- /dev/null +++ b/internal/github/review.go @@ -0,0 +1,102 @@ +package github + +import ( + "context" + "fmt" + "strings" +) + +// Review decisions, derived from the reviews of a PR. +const ( + ReviewApproved = "approved" + ReviewChangesRequested = "changes_requested" + ReviewNone = "none" +) + +// Merge states, derived from a PR's mergeable_state. +const ( + MergeReady = "ready" // clean: every requirement is met + MergeConflict = "conflict" // dirty: merge conflicts with the base branch + MergeBlocked = "blocked" // a branch protection requirement isn't met yet + MergeBehind = "behind" // the branch must be updated with the base first + MergeUnstable = "unstable" // mergeable, but non-required checks didn't pass + MergeDraft = "draft" +) + +const ( + reviewsPerPage = 100 + maxReviewsPages = 3 +) + +// Review is a single submitted pull request review. +type Review struct { + ID int64 `json:"id"` + User struct { + Login string `json:"login"` + } `json:"user"` + State string `json:"state"` // APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED, PENDING +} + +// GetReviews fetches the reviews of a pull request, oldest first. +func (c *Client) GetReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) { + var reviews []Review + for page := 1; page <= maxReviewsPages; page++ { + path := fmt.Sprintf("/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d", owner, repo, number, reviewsPerPage, page) + var batch []Review + if err := c.getJSON(ctx, path, &batch); err != nil { + return nil, err + } + reviews = append(reviews, batch...) + if len(batch) < reviewsPerPage { + break + } + } + return reviews, nil +} + +// ReviewDecision summarizes reviews like GitHub's review decision: changes requested +// if any reviewer's latest review requests changes, otherwise approved if any +// reviewer's latest review approves, otherwise none. Comments don't change a +// reviewer's verdict; a dismissed review withdraws it. Unlike GitHub's own decision, +// it doesn't know whether the approvals satisfy branch protection (MergeStatus does). +func ReviewDecision(reviews []Review) string { + verdict := map[string]string{} // reviewer โ†’ latest APPROVED / CHANGES_REQUESTED + for _, r := range reviews { + login := strings.ToLower(r.User.Login) + switch strings.ToUpper(r.State) { + case "APPROVED", "CHANGES_REQUESTED": + verdict[login] = strings.ToUpper(r.State) + case "DISMISSED": + delete(verdict, login) + } + } + decision := ReviewNone + for _, v := range verdict { + if v == "CHANGES_REQUESTED" { + return ReviewChangesRequested + } + decision = ReviewApproved + } + return decision +} + +// MergeStatus maps the PR's mergeable_state to one of the Merge* constants. It +// returns "" while GitHub is still computing it ("unknown"), when the previous value +// should be kept. +func MergeStatus(pr *PullRequest) string { + switch strings.ToLower(pr.MergeableState) { + case "clean", "has_hooks": + return MergeReady + case "dirty": + return MergeConflict + case "blocked": + return MergeBlocked + case "behind": + return MergeBehind + case "unstable": + return MergeUnstable + case "draft": + return MergeDraft + } + return "" +} diff --git a/internal/github/review_test.go b/internal/github/review_test.go new file mode 100644 index 0000000..20eb44a --- /dev/null +++ b/internal/github/review_test.go @@ -0,0 +1,83 @@ +package github + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func rev(login, state string) Review { + var r Review + r.User.Login = login + r.State = state + return r +} + +func TestReviewDecision(t *testing.T) { + tests := []struct { + name string + reviews []Review + want string + }{ + {"no reviews", nil, ReviewNone}, + {"only comments", []Review{rev("a", "COMMENTED")}, ReviewNone}, + {"approved", []Review{rev("a", "APPROVED")}, ReviewApproved}, + {"a comment after approving keeps the approval", []Review{rev("a", "APPROVED"), rev("a", "COMMENTED")}, ReviewApproved}, + {"changes requested wins over another approval", []Review{rev("a", "APPROVED"), rev("b", "CHANGES_REQUESTED")}, ReviewChangesRequested}, + {"approving after requesting changes", []Review{rev("b", "CHANGES_REQUESTED"), rev("b", "APPROVED")}, ReviewApproved}, + {"dismissed", []Review{rev("b", "CHANGES_REQUESTED"), rev("b", "DISMISSED")}, ReviewNone}, + {"logins are case-insensitive", []Review{rev("Bob", "CHANGES_REQUESTED"), rev("bob", "APPROVED")}, ReviewApproved}, + {"pending reviews don't count", []Review{rev("a", "PENDING")}, ReviewNone}, + } + for _, tt := range tests { + if got := ReviewDecision(tt.reviews); got != tt.want { + t.Errorf("%s: got %q, want %q", tt.name, got, tt.want) + } + } +} + +func TestMergeStatus(t *testing.T) { + for state, want := range map[string]string{ + "clean": MergeReady, "has_hooks": MergeReady, "dirty": MergeConflict, "blocked": MergeBlocked, + "behind": MergeBehind, "unstable": MergeUnstable, "draft": MergeDraft, "unknown": "", "": "", + } { + if got := MergeStatus(&PullRequest{MergeableState: state}); got != want { + t.Errorf("MergeStatus(%q) = %q, want %q", state, got, want) + } + } +} + +func TestGetReviews_Paginates(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.URL.Path, "/repos/o/r/pulls/7/reviews") { + http.NotFound(w, r) + return + } + count := reviewsPerPage + if r.URL.Query().Get("page") == "2" { + count = 1 + } + parts := make([]string, count) + for i := range parts { + parts[i] = fmt.Sprintf(`{"id":%d,"user":{"login":"u"},"state":"COMMENTED"}`, i) + } + if r.URL.Query().Get("page") == "2" { + parts[0] = `{"id":999,"user":{"login":"u"},"state":"APPROVED"}` + } + fmt.Fprint(w, "["+strings.Join(parts, ",")+"]") + })) + defer srv.Close() + c := NewClient("token") + c.BaseURL = srv.URL + + reviews, err := c.GetReviews(context.Background(), "o", "r", 7) + if err != nil { + t.Fatal(err) + } + if len(reviews) != reviewsPerPage+1 || ReviewDecision(reviews) != ReviewApproved { + t.Errorf("got %d reviews, decision %q; want the approval on page 2 to count", len(reviews), ReviewDecision(reviews)) + } +} diff --git a/internal/notify/native.go b/internal/notify/native.go index d2ad112..86f6f9b 100644 --- a/internal/notify/native.go +++ b/internal/notify/native.go @@ -55,7 +55,7 @@ func (n *NativeNotifier) Notify(event *StatusChangeEvent) error { return nil } - title := fmt.Sprintf("PR Status Change: %s/%s#%d", event.Owner, event.Repo, event.Number) + title := fmt.Sprintf("PR %s Change: %s/%s#%d", stateLabel(event.Kind), event.Owner, event.Repo, event.Number) message := fmt.Sprintf("%s โ†’ %s", event.PreviousState, event.CurrentState) if event.Title != "" { message = fmt.Sprintf("%s\n%s", event.Title, message) diff --git a/internal/notify/notify.go b/internal/notify/notify.go index ae8bf20..4196a9f 100644 --- a/internal/notify/notify.go +++ b/internal/notify/notify.go @@ -12,8 +12,17 @@ import ( "github.com/devblac/prw/internal/github" ) -// StatusChangeEvent represents a CI status change for a PR. +// Event kinds. CI status changes (and merged/closed) have an empty Kind, as before. +const ( + EventKindCI = "" + EventKindReview = "review" // states are github.Review* + EventKindMerge = "merge" // states are github.Merge* +) + +// StatusChangeEvent represents a change of a PR: its CI status by default, or its +// review decision or merge state, depending on Kind. type StatusChangeEvent struct { + Kind string Owner string Repo string Number int @@ -65,12 +74,19 @@ func NewConsoleNotifier() *ConsoleNotifier { func (c *ConsoleNotifier) Notify(event *StatusChangeEvent) error { prURL := github.FormatPRURL(event.Owner, event.Repo, event.Number) - fmt.Printf("\n๐Ÿ”” Status Change Detected!\n") + switch event.Kind { + case EventKindReview: + fmt.Printf("\n๐Ÿ”” Review Update!\n") + case EventKindMerge: + fmt.Printf("\n๐Ÿ”” Merge Status Change!\n") + default: + fmt.Printf("\n๐Ÿ”” Status Change Detected!\n") + } fmt.Printf(" PR: %s/%s#%d\n", event.Owner, event.Repo, event.Number) if event.Title != "" { fmt.Printf(" Title: %s\n", event.Title) } - fmt.Printf(" Status: %s โ†’ %s\n", event.PreviousState, event.CurrentState) + fmt.Printf(" %s: %s โ†’ %s\n", stateLabel(event.Kind), event.PreviousState, event.CurrentState) if len(event.FailedChecks) > 0 { fmt.Printf(" Failed checks:\n") for _, check := range event.FailedChecks { diff --git a/internal/notify/pr_events_format_test.go b/internal/notify/pr_events_format_test.go new file mode 100644 index 0000000..21ca17e --- /dev/null +++ b/internal/notify/pr_events_format_test.go @@ -0,0 +1,75 @@ +package notify + +import ( + "encoding/json" + "os/exec" + "strings" + "testing" + + "github.com/devblac/prw/internal/github" +) + +func prEvent(kind, prev, current string) *StatusChangeEvent { + return &StatusChangeEvent{Kind: kind, Owner: "o", Repo: "r", Number: 1, Title: "Add feature", PreviousState: prev, CurrentState: current} +} + +func TestPayloads_ReviewEvent(t *testing.T) { + event := prEvent(EventKindReview, github.ReviewNone, github.ReviewApproved) + + if text := slackPayload(event)["text"]; !strings.HasPrefix(text, "โœ… ") || !strings.Contains(text, "Review: `none โ†’ approved`") { + t.Errorf("slack text = %q", text) + } + embed := discordPayload(event)["embeds"].([]discordEmbed)[0] + if !strings.Contains(embed.Description, "Review: `none โ†’ approved`") || embed.Color != colorSuccess { + t.Errorf("discord embed = %+v", embed) + } + data, _ := json.Marshal(genericPayload(event)) + if !strings.Contains(string(data), `"type":"pr_review_change"`) || !strings.Contains(string(data), `"current_state":"approved"`) { + t.Errorf("generic payload = %s", data) + } +} + +func TestPayloads_MergeEvents(t *testing.T) { + conflict := prEvent(EventKindMerge, github.MergeReady, github.MergeConflict) + if text := slackPayload(conflict)["text"]; !strings.HasPrefix(text, "โš ๏ธ ") || !strings.Contains(text, "Merge: `ready โ†’ conflict`") { + t.Errorf("slack text = %q", text) + } + if c := discordPayload(conflict)["embeds"].([]discordEmbed)[0].Color; c != colorFailure { + t.Errorf("conflict color = %#x", c) + } + ready := prEvent(EventKindMerge, github.MergeBlocked, github.MergeReady) + if text := slackPayload(ready)["text"]; !strings.HasPrefix(text, "๐Ÿš€ ") { + t.Errorf("slack text = %q", text) + } + if data, _ := json.Marshal(genericPayload(ready)); !strings.Contains(string(data), `"type":"pr_merge_state_change"`) { + t.Errorf("generic payload = %s", data) + } +} + +func TestPayloads_CIEventsKeepTheirType(t *testing.T) { + event := prEvent(EventKindCI, "pending", "success") + if data, _ := json.Marshal(genericPayload(event)); !strings.Contains(string(data), `"type":"pr_status_change"`) { + t.Errorf("generic payload = %s", data) + } + if text := slackPayload(event)["text"]; !strings.Contains(text, "Status: `pending โ†’ success`") { + t.Errorf("slack text = %q", text) + } +} + +func TestNativeTitle_NamesTheKind(t *testing.T) { + var seen string + old := runNativeCommand + runNativeCommand = func(cmd *exec.Cmd) error { + seen = strings.Join(append(cmd.Args, cmd.Env...), "\n") // title is an arg or env var + return nil + } + t.Cleanup(func() { runNativeCommand = old }) + + n := &NativeNotifier{enabled: true} + if err := n.Notify(prEvent(EventKindReview, github.ReviewNone, github.ReviewChangesRequested)); err != nil { + t.Fatal(err) + } + if seen != "" && !strings.Contains(seen, "PR Review Change: o/r#1") { + t.Errorf("native notification doesn't name the review:\n%s", seen) + } +} diff --git a/internal/notify/webhook_format.go b/internal/notify/webhook_format.go index 7f7f29a..731d6e9 100644 --- a/internal/notify/webhook_format.go +++ b/internal/notify/webhook_format.go @@ -30,6 +30,7 @@ const ( colorFailure = 0xD1242F colorPending = 0xDBAB09 colorNoCI = 0x8C959F + colorChanges = 0xBC4C00 ) // IsValidWebhookFormat reports whether value is a supported webhook format. @@ -91,7 +92,7 @@ func buildWebhookBody(format string, event *StatusChangeEvent) ([]byte, error) { func genericPayload(event *StatusChangeEvent) WebhookPayload { return WebhookPayload{ - Type: "pr_status_change", + Type: payloadType(event.Kind), Owner: event.Owner, Repo: event.Repo, PRNumber: event.Number, @@ -105,14 +106,44 @@ func genericPayload(event *StatusChangeEvent) WebhookPayload { } } +// payloadType is the generic payload's "type" for each event kind. +func payloadType(kind string) string { + switch kind { + case EventKindReview: + return "pr_review_change" + case EventKindMerge: + return "pr_merge_state_change" + default: + return "pr_status_change" + } +} + +// stateLabel names what changed, e.g. "Status: pending โ†’ success". +func stateLabel(kind string) string { + switch kind { + case EventKindReview: + return "Review" + case EventKindMerge: + return "Merge" + default: + return "Status" + } +} + func stateEmoji(state string) string { switch state { - case "success": + case "success", github.ReviewApproved: return "โœ…" case "failure", "error": return "โŒ" case github.StateNoCI: return "โšช" + case github.ReviewChangesRequested: + return "๐Ÿ“" + case github.MergeConflict: + return "โš ๏ธ" + case github.MergeReady: + return "๐Ÿš€" default: return "โณ" } @@ -120,10 +151,12 @@ func stateEmoji(state string) string { func stateColor(state string) int { switch state { - case "success": + case "success", github.ReviewApproved, github.MergeReady: return colorSuccess - case "failure", "error": + case "failure", "error", github.MergeConflict: return colorFailure + case github.ReviewChangesRequested: + return colorChanges case github.StateNoCI: return colorNoCI default: @@ -156,7 +189,7 @@ func slackPayload(event *StatusChangeEvent) map[string]string { if event.Title != "" { fmt.Fprintf(&b, " โ€” %s", slackEscape(event.Title)) } - fmt.Fprintf(&b, "\nStatus: `%s`", transition(event)) + fmt.Fprintf(&b, "\n%s: `%s`", stateLabel(event.Kind), transition(event)) for i, check := range event.FailedChecks { if i == maxChecksListed { @@ -188,7 +221,7 @@ func discordPayload(event *StatusChangeEvent) map[string]interface{} { } var desc strings.Builder - fmt.Fprintf(&desc, "Status: `%s`", transition(event)) + fmt.Fprintf(&desc, "%s: `%s`", stateLabel(event.Kind), transition(event)) for i, check := range event.FailedChecks { if i == maxChecksListed { fmt.Fprintf(&desc, "\nโ€ฆ and %d more", len(event.FailedChecks)-maxChecksListed) diff --git a/internal/watcher/pr_events_test.go b/internal/watcher/pr_events_test.go new file mode 100644 index 0000000..d02b020 --- /dev/null +++ b/internal/watcher/pr_events_test.go @@ -0,0 +1,171 @@ +package watcher + +import ( + "context" + "testing" + + "github.com/devblac/prw/internal/config" + "github.com/devblac/prw/internal/github" + "github.com/devblac/prw/internal/notify" +) + +func review(login, state string) github.Review { + var r github.Review + r.User.Login = login + r.State = state + return r +} + +// prEventsCase is one check of PR #1 whose CI stays green, starting from the given +// stored review and merge states. +type prEventsCase struct { + lastReview, lastMerge string + reviews []github.Review + mergeableState string + filter string + disabled bool +} + +func runPREventsCheck(t *testing.T, c prEventsCase) (*config.WatchedPR, []*notify.StatusChangeEvent) { + t.Helper() + pr := newPR(1, "sha1") + pr.State = "open" + pr.MergeableState = c.mergeableState + client := &mockGitHubClient{ + prs: map[string]*github.PullRequest{"owner/repo/1": pr}, + statuses: map[string]*github.CombinedStatus{"sha1": {State: "success", TotalCount: 1}}, + reviews: map[string][]github.Review{"owner/repo/1": c.reviews}, + } + watchedPR := watched(1, "success") + watchedPR.LastReviewState, watchedPR.LastMergeState = c.lastReview, c.lastMerge + cfg := &config.Config{NotificationFilter: c.filter, DisablePREvents: c.disabled, WatchedPRs: []config.WatchedPR{watchedPR}} + notifier := &mockNotifier{} + + if _, err := New(client, cfg, notifier).checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { + t.Fatal(err) + } + return &cfg.WatchedPRs[0], notifier.events +} + +func TestCheckPR_NotifiesApproval(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewNone, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("alice", "COMMENTED"), review("alice", "APPROVED")}, + mergeableState: "blocked", // e.g. a second approval is required + }) + if len(events) != 1 || events[0].Kind != notify.EventKindReview || events[0].PreviousState != "none" || events[0].CurrentState != "approved" { + t.Fatalf("events = %+v, want one none โ†’ approved review event", events) + } + if pr.LastReviewState != github.ReviewApproved || pr.LastMergeState != github.MergeBlocked { + t.Errorf("stored review=%q merge=%q", pr.LastReviewState, pr.LastMergeState) + } +} + +func TestCheckPR_NotifiesChangesRequested(t *testing.T) { + _, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewApproved, lastMerge: github.MergeReady, + reviews: []github.Review{review("alice", "APPROVED"), review("bob", "CHANGES_REQUESTED")}, + mergeableState: "clean", + }) + if len(events) != 1 || events[0].CurrentState != github.ReviewChangesRequested { + t.Fatalf("events = %+v, want approved โ†’ changes_requested", events) + } +} + +func TestCheckPR_FirstObservationDoesNotNotify(t *testing.T) { + // Existing watch lists (and newly added PRs) have no stored review/merge state yet. + pr, events := runPREventsCheck(t, prEventsCase{ + reviews: []github.Review{review("alice", "APPROVED")}, + mergeableState: "dirty", + }) + if len(events) != 0 { + t.Errorf("events = %+v, want none on the first check", events) + } + if pr.LastReviewState != github.ReviewApproved || pr.LastMergeState != github.MergeConflict { + t.Errorf("stored review=%q merge=%q, want them recorded", pr.LastReviewState, pr.LastMergeState) + } +} + +func TestCheckPR_NotifiesNewConflict(t *testing.T) { + _, events := runPREventsCheck(t, prEventsCase{lastReview: github.ReviewNone, lastMerge: github.MergeReady, mergeableState: "dirty"}) + if len(events) != 1 || events[0].Kind != notify.EventKindMerge || events[0].CurrentState != github.MergeConflict { + t.Fatalf("events = %+v, want ready โ†’ conflict", events) + } +} + +func TestCheckPR_ReadyToMergeAfterBranchProtectionIsSatisfied(t *testing.T) { + // The required approval came from someone whose review was already counted, + // or a required reviewer was satisfied by CODEOWNERS: only the merge state changes. + _, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewApproved, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("alice", "APPROVED")}, + mergeableState: "clean", + }) + if len(events) != 1 || events[0].CurrentState != github.MergeReady || events[0].PreviousState != github.MergeBlocked { + t.Fatalf("events = %+v, want blocked โ†’ ready", events) + } +} + +func TestCheckPR_ReadyIsNotSentWithTheApprovalThatCausedIt(t *testing.T) { + _, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewNone, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("alice", "APPROVED")}, + mergeableState: "clean", + }) + if len(events) != 1 || events[0].Kind != notify.EventKindReview { + t.Fatalf("events = %+v, want only the approval", events) + } +} + +func TestCheckPR_UnstableToReadyIsLeftToTheCINotification(t *testing.T) { + // No branch protection: the PR is "unstable" while checks run and "clean" once they + // pass, which the CI success notification already announces. + _, events := runPREventsCheck(t, prEventsCase{lastReview: github.ReviewNone, lastMerge: github.MergeUnstable, mergeableState: "clean"}) + if len(events) != 0 { + t.Errorf("events = %+v, want none", events) + } +} + +func TestCheckPR_UnknownMergeStateKeepsTheLastOne(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{lastReview: github.ReviewNone, lastMerge: github.MergeConflict, mergeableState: "unknown"}) + if len(events) != 0 || pr.LastMergeState != github.MergeConflict { + t.Errorf("events=%+v merge=%q; while GitHub computes mergeability nothing changes", events, pr.LastMergeState) + } +} + +func TestCheckPR_DismissedReviewWithdrawsTheVerdict(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{ + lastReview: github.ReviewChangesRequested, lastMerge: github.MergeBlocked, + reviews: []github.Review{review("bob", "CHANGES_REQUESTED"), review("bob", "DISMISSED")}, + mergeableState: "blocked", + }) + if len(events) != 0 || pr.LastReviewState != github.ReviewNone { + t.Errorf("events=%+v review=%q, want silently back to none", events, pr.LastReviewState) + } +} + +func TestCheckPR_PREventFilters(t *testing.T) { + tests := []struct { + filter string + c prEventsCase + want int + }{ + {config.NotificationFilterFail, prEventsCase{lastReview: "none", lastMerge: "blocked", reviews: []github.Review{review("a", "APPROVED")}, mergeableState: "blocked"}, 0}, + {config.NotificationFilterFail, prEventsCase{lastReview: "none", lastMerge: "ready", mergeableState: "dirty"}, 1}, + {config.NotificationFilterSuccess, prEventsCase{lastReview: "none", lastMerge: "ready", mergeableState: "dirty"}, 0}, + {config.NotificationFilterSuccess, prEventsCase{lastReview: "approved", lastMerge: "blocked", reviews: []github.Review{review("a", "APPROVED")}, mergeableState: "clean"}, 1}, + } + for _, tt := range tests { + tt.c.filter = tt.filter + if _, events := runPREventsCheck(t, tt.c); len(events) != tt.want { + t.Errorf("filter %s, %+v: %d events, want %d", tt.filter, tt.c, len(events), tt.want) + } + } +} + +func TestCheckPR_PREventsCanBeDisabled(t *testing.T) { + pr, events := runPREventsCheck(t, prEventsCase{lastReview: "none", lastMerge: "ready", mergeableState: "dirty", disabled: true}) + if len(events) != 0 || pr.LastMergeState != "ready" { + t.Errorf("events=%+v merge=%q, want pr_events=false to skip review/merge checks", events, pr.LastMergeState) + } +} diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 53aa364..70d572f 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -15,6 +15,7 @@ type GitHubClient interface { GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) + GetReviews(ctx context.Context, owner, repo string, number int) ([]github.Review, error) } // ConfigStore defines the interface for config persistence. @@ -236,8 +237,10 @@ func (w *Watcher) checkPR(ctx context.Context, pr *config.WatchedPR) (closed boo // Check if status changed. Finding out that a PR has no CI is not a CI result, so // it isn't announced (it would otherwise ping once for every such PR). + notifiedCI := false if previousState != "" && previousState != currentState && currentState != github.StateNoCI && shouldNotify(w.config.NotificationFilter, currentState) { + notifiedCI = true event := ¬ify.StatusChangeEvent{ Owner: pr.Owner, Repo: pr.Repo, @@ -260,9 +263,91 @@ func (w *Watcher) checkPR(ctx context.Context, pr *config.WatchedPR) (closed boo pr.LastKnownState = currentState pr.LastChecked = time.Now() + if !w.config.DisablePREvents { + if err := w.checkPREvents(ctx, pr, ghPR, notifiedCI); err != nil { + return false, err + } + } return false, nil } +// checkPREvents notifies about changes of the PR's review decision and merge state. +// notifiedCI tells whether a CI notification was just sent for this PR. Nothing is +// announced the first time a PR is seen (the previous state is empty). +func (w *Watcher) checkPREvents(ctx context.Context, pr *config.WatchedPR, ghPR *github.PullRequest, notifiedCI bool) error { + reviews, err := w.client.GetReviews(ctx, pr.Owner, pr.Repo, pr.Number) + if err != nil { + return fmt.Errorf("failed to fetch reviews: %w", err) + } + notified := notifiedCI + + review := github.ReviewDecision(reviews) + if prev := pr.LastReviewState; prev != "" && prev != review && review != github.ReviewNone && + shouldNotifyPREvent(w.config.NotificationFilter, review) { + w.notifyPREvent(pr, ghPR, notify.EventKindReview, prev, review) + notified = true + } + pr.LastReviewState = review + + merge := github.MergeStatus(ghPR) + if merge == "" { + return nil // GitHub is still computing it; keep the last known state + } + if prev := pr.LastMergeState; prev != "" && prev != merge && mergeWorthNotifying(prev, merge, notified) && + shouldNotifyPREvent(w.config.NotificationFilter, merge) { + w.notifyPREvent(pr, ghPR, notify.EventKindMerge, prev, merge) + } + pr.LastMergeState = merge + return nil +} + +// mergeWorthNotifying decides which merge state changes are announced: new conflicts, +// and a PR becoming ready to merge after something other than its CI was holding it +// back (branch protection, conflicts, an outdated branch). Without branch protection a +// PR goes from unstable to ready as soon as CI passes, which the CI notification +// already says; the same goes for an approval that unblocks it, so "ready" isn't sent +// together with another notification. +func mergeWorthNotifying(prev, merge string, notified bool) bool { + switch merge { + case github.MergeConflict: + return true + case github.MergeReady: + return !notified && (prev == github.MergeBlocked || prev == github.MergeConflict || prev == github.MergeBehind) + } + return false +} + +func (w *Watcher) notifyPREvent(pr *config.WatchedPR, ghPR *github.PullRequest, kind, prev, current string) { + event := ¬ify.StatusChangeEvent{ + Kind: kind, + Owner: pr.Owner, + Repo: pr.Repo, + Number: pr.Number, + Title: pr.Title, + PreviousState: prev, + CurrentState: current, + SHA: ghPR.Head.SHA, + Timestamp: time.Now(), + } + if err := w.notifier.Notify(event); err != nil { + fmt.Printf("Warning: notification failed: %v\n", err) + } +} + +// shouldNotifyPREvent applies the notification filter to review and merge events: +// "fail" keeps the ones that need action (changes requested, conflicts), "success" +// the good news (approved, ready to merge). +func shouldNotifyPREvent(filter, state string) bool { + switch config.NormalizeNotificationFilter(filter) { + case config.NotificationFilterFail: + return state == github.ReviewChangesRequested || state == github.MergeConflict + case config.NotificationFilterSuccess: + return state == github.ReviewApproved || state == github.MergeReady + default: + return true + } +} + func shouldNotify(filter, currentState string) bool { if !config.IsValidNotificationFilter(filter) { filter = config.NotificationFilterChange diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 53b45d8..c03a2e0 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -17,9 +17,17 @@ type mockGitHubClient struct { prs map[string]*github.PullRequest statuses map[string]*github.CombinedStatus checkRuns map[string][]github.CheckRun + reviews map[string][]github.Review // keyed like prs err error } +func (m *mockGitHubClient) GetReviews(ctx context.Context, owner, repo string, number int) ([]github.Review, error) { + if m.err != nil { + return nil, m.err + } + return m.reviews[fmt.Sprintf("%s/%s/%d", owner, repo, number)], nil +} + func (m *mockGitHubClient) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) { if m.err != nil { return nil, m.err From f9b6f410f305e69f177635ffd1370ee33fe86aee Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sat, 24 Oct 2026 15:37:26 -0300 Subject: [PATCH 2/5] fix: read every page of reviews, not just the first 300 Reviews come oldest first, so the 3-page cap dropped the newest approvals, change requests and dismissals on PRs with more than 300 reviews, and the review state stayed stale. Pagination now runs to the last page, with a 100-page cap only as a runaway guard. Addresses Codex review on #19. Co-Authored-By: Claude Opus 5.5 --- internal/github/review.go | 7 +++++-- internal/github/review_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/internal/github/review.go b/internal/github/review.go index a3de91e..0e53ee5 100644 --- a/internal/github/review.go +++ b/internal/github/review.go @@ -24,8 +24,11 @@ const ( ) const ( - reviewsPerPage = 100 - maxReviewsPages = 3 + reviewsPerPage = 100 + // maxReviewsPages only guards against a runaway loop: reviews come oldest first, + // so stopping early would drop the newest verdicts. Every page is fetched (with a + // free 304 when unchanged), so 10,000 reviews is far beyond any real PR. + maxReviewsPages = 100 ) // Review is a single submitted pull request review. diff --git a/internal/github/review_test.go b/internal/github/review_test.go index 20eb44a..5e473d4 100644 --- a/internal/github/review_test.go +++ b/internal/github/review_test.go @@ -50,6 +50,40 @@ func TestMergeStatus(t *testing.T) { } } +func TestGetReviews_ReadsPastThreeHundredReviews(t *testing.T) { + // 350 comments, then a change request on page 4: the newest verdict must count. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var page int + fmt.Sscan(r.URL.Query().Get("page"), &page) + count := reviewsPerPage + if page == 4 { + count = 51 + } + parts := make([]string, count) + for i := range parts { + parts[i] = `{"user":{"login":"u"},"state":"COMMENTED"}` + } + if page == 1 { + parts[0] = `{"user":{"login":"a"},"state":"APPROVED"}` + } + if page == 4 { + parts[50] = `{"user":{"login":"b"},"state":"CHANGES_REQUESTED"}` + } + fmt.Fprint(w, "["+strings.Join(parts, ",")+"]") + })) + defer srv.Close() + c := NewClient("token") + c.BaseURL = srv.URL + + reviews, err := c.GetReviews(context.Background(), "o", "r", 7) + if err != nil { + t.Fatal(err) + } + if len(reviews) != 351 || ReviewDecision(reviews) != ReviewChangesRequested { + t.Errorf("got %d reviews, decision %q; want 351 and the change request on page 4", len(reviews), ReviewDecision(reviews)) + } +} + func TestGetReviews_Paginates(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !strings.HasSuffix(r.URL.Path, "/repos/o/r/pulls/7/reviews") { From 9d6f230797382133c43f0eec0c2e7414211ae5d5 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sat, 24 Oct 2026 15:40:20 -0300 Subject: [PATCH 3/5] feat: add `prw checks` to list every check of a PR `prw checks ` shows each check run and commit status of the PR's head commit with its result, how long it took and a link: failures first, then cancelled and running ones, then passed, neutral and skipped. Superseded runs of the same check are left out, as they don't count towards the result. The header has the short SHA, overall state, counts per bucket and merge state. `--json` prints the same as an object with a `checks` array. Exit codes match `prw wait` (0 passed, 1 failed, 3 error, 4 no CI), plus 8 while checks are still running, like `gh pr checks`. Verified live on devblac/docker-lint#5, zephyrproject-rtos/zephyr#105183 (35 checks; the cancelled run superseded by a newer one is hidden) and devblac/prw#13. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + README.md | 19 +++ TASKS.md | 4 +- cmd/prw/checks.go | 294 ++++++++++++++++++++++++++++++++++++++ cmd/prw/checks_test.go | 163 +++++++++++++++++++++ internal/github/client.go | 11 +- 6 files changed, 487 insertions(+), 5 deletions(-) create mode 100644 cmd/prw/checks.go create mode 100644 cmd/prw/checks_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ebb476..96b8283 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 +- `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). diff --git a/README.md b/README.md index 76f651b..767b2c2 100644 --- a/README.md +++ b/README.md @@ -31,6 +31,7 @@ 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 @@ -266,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, `1` failed, `3` error, `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. diff --git a/TASKS.md b/TASKS.md index 2880380..e8673ed 100644 --- a/TASKS.md +++ b/TASKS.md @@ -92,7 +92,9 @@ Legend โ€” Impact: 1โ€“5 ยท Effort: S (โ‰คยฝ day), M (1โ€“2 days), L (3+ days) - โœ… #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. - โœ… #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 ` +- โœ… 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..e452ea6 --- /dev/null +++ b/cmd/prw/checks.go @@ -0,0 +1,294 @@ +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) + 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", "closed": + return nil + 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..3f1c094 --- /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, "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/internal/github/client.go b/internal/github/client.go index ddbf810..87ba773 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -169,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"` } @@ -386,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 @@ -416,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 From 92eac0c78eb8e1ace3b4c7f29b91a6079de195aa Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sat, 24 Oct 2026 15:58:35 -0300 Subject: [PATCH 4/5] fix: `prw checks` exits 3 for a PR closed without merging It exited 0, so `prw checks "$PR" && ...` went ahead on an abandoned PR, and the exit codes didn't match `prw wait` as documented. Addresses Codex review on #20. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 767b2c2..211264e 100644 --- a/README.md +++ b/README.md @@ -283,7 +283,7 @@ abc1234 ยท failure ยท 1 fail, 1 pending, 3 pass ยท merge: blocked โœ… 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, `1` failed, `3` error, `4` no CI โ€” plus `8` while checks are still running. +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` From 71fd25dfd6b7518a689f5e0c92962252ee9ed3fa Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sat, 24 Oct 2026 15:59:15 -0300 Subject: [PATCH 5/5] fix: actually return exit 3 from `prw checks` for closed PRs The previous commit only updated the README; this is the code change: a PR closed without merging now exits 3 (like `prw wait`) instead of 0. Addresses Codex review on #20. Co-Authored-By: Claude Opus 5.5 --- cmd/prw/checks.go | 7 +++++-- cmd/prw/checks_test.go | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/cmd/prw/checks.go b/cmd/prw/checks.go index e452ea6..8266324 100644 --- a/cmd/prw/checks.go +++ b/cmd/prw/checks.go @@ -48,7 +48,7 @@ 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) + 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 @@ -208,8 +208,11 @@ func checkRunBucket(run github.CheckRun) string { func (r *checksReport) exitErr() error { pr := fmt.Sprintf("%s/%s#%d", r.Owner, r.Repo, r.Number) switch r.State { - case "success", "merged", "closed": + 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: diff --git a/cmd/prw/checks_test.go b/cmd/prw/checks_test.go index 3f1c094..4d343b3 100644 --- a/cmd/prw/checks_test.go +++ b/cmd/prw/checks_test.go @@ -93,7 +93,7 @@ func TestPrintChecks(t *testing.T) { func TestChecksExitCodes(t *testing.T) { for state, want := range map[string]int{ - "success": 0, "merged": 0, "failure": exitWaitFailed, "error": exitWaitFailed, + "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}