From 3880378d9067de2e9a6c62e35943f27b11e66181 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 00:36:34 -0300 Subject: [PATCH 01/10] fix: report `no_ci` instead of `pending` forever for PRs without CI A PR with no checks or statuses at all was reported as "pending" indefinitely, so `prw wait` and the MCP wait_for_ci tool blocked until their timeout on repositories without CI or on stacked PRs whose base branch runs none (14 of 18 real PRs in testing). Zero checks is also what every PR looks like right after a push, so a PR is only reported as no_ci once it has had no checks or statuses and hasn't been updated (PR updated_at, which every push bumps) for 3 minutes; a zero/unknown updated_at keeps it pending. - github: PullRequest.UpdatedAt, StateNoCI, ApplyNoCI, applied by run, wait and broadcast. - prw wait exits 4 for no_ci (not 0: `prw wait && deploy` must not deploy unverified PRs); MCP summaries and tool descriptions cover it. - prw run doesn't notify on a transition into no_ci, and notifies normally if CI shows up later. Verified live: 11 real PRs moved from pending to no_ci; `prw wait` on one returns in 2s with exit 4 instead of waiting 30 minutes. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + README.md | 7 ++- TASKS.md | 3 +- cmd/prw/broadcast.go | 1 + cmd/prw/mcp.go | 8 ++- cmd/prw/noci_test.go | 91 +++++++++++++++++++++++++++++++ cmd/prw/wait.go | 11 ++++ internal/github/client.go | 25 +++++++++ internal/github/noci_test.go | 64 ++++++++++++++++++++++ internal/notify/webhook_format.go | 5 ++ internal/watcher/noci_test.go | 66 ++++++++++++++++++++++ internal/watcher/watcher.go | 7 ++- 12 files changed, 281 insertions(+), 8 deletions(-) create mode 100644 cmd/prw/noci_test.go create mode 100644 internal/github/noci_test.go create mode 100644 internal/watcher/noci_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index b989c4a..dc71faa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Integration tests against the real GitHub API (`make integration`, build tag `integration`), run nightly by a new `Integration` workflow. They fail against the old status-only logic. ### Fixed +- PRs without any CI are reported as `no_ci` instead of `pending` forever. Previously `prw wait` and the MCP `wait_for_ci` tool blocked until their timeout on such PRs (repositories without CI, or stacked PRs whose base branch runs none). A PR is `no_ci` once it has no checks or statuses at all and hasn't been updated for 3 minutes, so CI that is still registering after a push isn't mistaken for missing CI. `prw wait` exits `4` for it; `prw run` doesn't notify about it. - PR owner and repository names are compared case-insensitively (as on GitHub), so `DevBlac/PRW#5` and `devblac/prw#5` are no longer two separate watch-list entries. - Requests without a token no longer send an empty `Authorization: Bearer ` header, which GitHub rejects with 401 even for public repositories. - `MultiNotifier` (used whenever more than one notification channel is enabled) stopped at the first failing notifier. A down webhook silently suppressed the console and native notifications that were supposed to run after it; now every notifier always runs and their errors are combined. diff --git a/README.md b/README.md index 669de8f..f54da96 100644 --- a/README.md +++ b/README.md @@ -238,6 +238,7 @@ prw wait "$PR_URL" --timeout 15m --json | `1` | CI failed (`failure` or `error`) | | `2` | Timed out (`--timeout`, default 30m) while CI was still pending | | `3` | Other error: invalid URL, API error, missing token, PR closed without merging | +| `4` | The PR has **no CI**: no checks or statuses were reported within 3 minutes of its last update (see below) | | `130` | Interrupted (Ctrl+C) | It fails fast — as soon as one check fails it exits `1` with the failed checks and their links, without waiting for the rest. If someone pushes to the PR meanwhile, it follows the new head commit. Transient errors (network, 5xx, rate limits) are retried up to 3 times in a row. Polls every 15s by default (`--interval`). @@ -260,7 +261,9 @@ With `--json`, stdout is a single object: } ``` -`state` is one of `success`, `failure`, `error`, `pending` (with `timed_out` or `interrupted` set), `merged`, or `closed`. +`state` is one of `success`, `failure`, `error`, `pending` (with `timed_out` or `interrupted` set), `merged`, `closed`, or `no_ci`. + +**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). ## Using prw from AI agents: `prw mcp` @@ -286,7 +289,7 @@ Tools: | Tool | What it does | |------|--------------| -| `get_ci_status` | Current state of a PR (`success`, `failure`, `error`, `pending`, `merged`, `closed`), check counts, failed checks with links. Returns immediately. | +| `get_ci_status` | Current state of a PR (`success`, `failure`, `error`, `pending`, `merged`, `closed`, `no_ci`), check counts, failed checks with links. Returns immediately. | | `wait_for_ci` | Blocks until CI finishes (`timeout_seconds` default 600, max 3600). Fails fast on the first failed check, follows new pushes, sends progress notifications. On timeout it returns `pending` with `timed_out: true` so the agent can call it again. | | `list_watched` | PRs on the local watch list with their last known state. | | `watch_pr` / `unwatch_pr` | Add or remove a PR from the watch list (a running `prw run` picks it up). | diff --git a/TASKS.md b/TASKS.md index fdbec24..80ba30e 100644 --- a/TASKS.md +++ b/TASKS.md @@ -89,7 +89,8 @@ Legend — Impact: 1–5 · Effort: S (≤½ day), M (1–2 days), L (3+ days) - ✅ #14 MCP server — done (`prw mcp`: 5 tools, progress + cancellation, protocol 2025-06-18/2025-03-26/2024-11-05; verified end-to-end over stdio against the real API with the token from `gh auth token`). Not yet tried inside a real MCP client. - ✅ #10 `prw watch --mine` / `--review-requested` — done (search-backed subscriptions refreshed every 5 min by `prw run`; verified live: 18 open PRs across 7 repos found, CI checked, cleanly removed by `unwatch --mine`) - 🆕 #18 **Report "no CI" instead of `pending` forever** (found while testing #10: 14 of 18 real PRs have zero checks — repos without CI, or stacked PRs whose base isn't `main`). Today `prw wait` / `wait_for_ci` would wait until the timeout on them. Needs a grace period after the last push (CI takes a moment to register checks) before concluding "no CI". Impact 4, effort S–M. -- Next: #18, then #11 review & mergeability events +- ✅ #18 `no_ci` state — done (0 checks + PR not updated for 3 min → `no_ci`; `prw wait` exits 4; no notification; verified live: 11 real PRs went from `pending` to `no_ci`, `prw wait` on one returns in 2s instead of the 30-min timeout) +- Next: #11 review & mergeability events ## Planned diff --git a/cmd/prw/broadcast.go b/cmd/prw/broadcast.go index bf2138d..856ba5c 100644 --- a/cmd/prw/broadcast.go +++ b/cmd/prw/broadcast.go @@ -81,6 +81,7 @@ var broadcastCmd = &cobra.Command{ fmt.Printf("Error fetching status for %s/%s#%d: %v\n", pr.Owner, pr.Repo, pr.Number, err) continue } + github.ApplyNoCI(ci, ghPR.UpdatedAt, time.Now()) currentState := ci.State previousState := github.NormalizeState(pr.LastKnownState) diff --git a/cmd/prw/mcp.go b/cmd/prw/mcp.go index 44d7240..89792ca 100644 --- a/cmd/prw/mcp.go +++ b/cmd/prw/mcp.go @@ -43,7 +43,7 @@ const ( mcpInstructions = `prw reports the CI status of GitHub pull requests, aggregating GitHub Actions check runs and legacy commit statuses. Use get_ci_status for a one-off check, and wait_for_ci after pushing to a PR to block until CI finishes (it fails fast and lists failed checks with links). -States: success, failure, error, pending, merged, closed.` +States: success, failure, error, pending, merged, closed, and no_ci (the PR has no checks or statuses at all; waiting longer will not help).` ) func init() { @@ -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), 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, and failed checks with links. Aggregates GitHub Actions check runs and legacy commit statuses. Returns immediately.", "inputSchema": map[string]interface{}{ "type": "object", "properties": map[string]interface{}{"pr_url": prURLSchema}, @@ -440,7 +440,7 @@ var mcpTools = []map[string]interface{}{ { "name": "wait_for_ci", "title": "Wait for PR CI", - "description": "Block until a GitHub pull request's CI finishes, then return the result. Fails fast: returns as soon as any check fails, with the failed checks and links. Follows new pushes to the PR. If the timeout is reached first, returns state 'pending' with timed_out=true (call again to keep waiting). A merge while waiting returns state 'merged'.", + "description": "Block until a GitHub pull request's CI finishes, then return the result. Fails fast: returns as soon as any check fails, with the failed checks and links. Follows new pushes to the PR. If the timeout is reached first, returns state 'pending' with timed_out=true (call again to keep waiting). A merge while waiting returns state 'merged'; a PR with no checks or statuses at all returns 'no_ci' instead of waiting until the timeout.", "inputSchema": map[string]interface{}{ "type": "object", "properties": map[string]interface{}{ @@ -545,6 +545,8 @@ func ciSummary(r *waitResult) string { return fmt.Sprintf("CI of %s is pending: %s.", pr, pendingSummary(r)) case r.State == "success": return fmt.Sprintf("CI of %s passed (%d check(s)).", pr, r.TotalChecks) + case r.State == github.StateNoCI: + return fmt.Sprintf("%s has no CI: no checks or statuses were reported within %s of its last update, so nothing verified it.", pr, github.NoCIGracePeriod) } names := make([]string, len(r.FailedChecks)) for i, c := range r.FailedChecks { diff --git a/cmd/prw/noci_test.go b/cmd/prw/noci_test.go new file mode 100644 index 0000000..221fd01 --- /dev/null +++ b/cmd/prw/noci_test.go @@ -0,0 +1,91 @@ +package main + +import ( + "bytes" + "context" + "fmt" + "net/http" + "strings" + "testing" + "time" + + "github.com/devblac/prw/internal/github" +) + +func staleOpenPR(sha string, quietFor time.Duration) *github.PullRequest { + pr := openPR(sha) + pr.UpdatedAt = time.Now().Add(-quietFor) + return pr +} + +func TestWaitForCI_NoCIStopsInsteadOfWaitingForTheTimeout(t *testing.T) { + noSleep(t) + client := &scriptedClient{steps: []pollStep{{pr: staleOpenPR("sha", time.Hour)}}} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.State != github.StateNoCI || client.polls != 1 { + t.Errorf("state=%q polls=%d, want no_ci after one poll", result.State, client.polls) + } + if code := exitCode(result.exitErr()); code != exitWaitNoCI { + t.Errorf("exit code = %d, want 4 (no CI must not look like success)", code) + } +} + +func TestWaitForCI_WaitsForCIToRegisterAfterAPush(t *testing.T) { + noSleep(t) + client := &scriptedClient{steps: []pollStep{ + {pr: staleOpenPR("new", 10*time.Second)}, // just pushed, nothing registered yet + {pr: staleOpenPR("new", 20*time.Second), runs: []github.CheckRun{run("Test", "queued", "")}}, + {pr: staleOpenPR("new", 30*time.Second), runs: []github.CheckRun{run("Test", "completed", "success")}}, + }} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.State != "success" || client.polls != 3 { + t.Errorf("state=%q polls=%d, want success after 3 polls", result.State, client.polls) + } +} + +func TestPrintWaitResult_NoCI(t *testing.T) { + var buf bytes.Buffer + printWaitResult(&buf, &waitResult{Owner: "o", Repo: "r", Number: 1, State: github.StateNoCI, URL: "https://github.com/o/r/pull/1"}, false) + if !strings.Contains(buf.String(), "⚪ o/r#1") || !strings.Contains(buf.String(), "No CI: no checks or statuses") { + t.Errorf("output:\n%s", buf.String()) + } +} + +func TestCISummary_NoCI(t *testing.T) { + got := ciSummary(&waitResult{Owner: "o", Repo: "r", Number: 1, State: github.StateNoCI}) + if !strings.HasPrefix(got, "o/r#1 has no CI") { + t.Errorf("summary = %q", got) + } +} + +func TestWaitCmd_NoCIExitCode(t *testing.T) { + updated := time.Now().Add(-time.Hour).UTC().Format(time.RFC3339) + setupWaitCommand(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/pulls/1"): + fmt.Fprintf(w, `{"number":1,"title":"Stacked","state":"open","updated_at":%q,"head":{"sha":"abc1234"}}`, updated) + 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":[]}`) + } + }) + + out, err := captureStdout(func() error { + return waitCmd.RunE(waitCmd, []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, "No CI") { + t.Errorf("stdout:\n%s", out) + } +} diff --git a/cmd/prw/wait.go b/cmd/prw/wait.go index 7c8e4b1..8f44af2 100644 --- a/cmd/prw/wait.go +++ b/cmd/prw/wait.go @@ -24,6 +24,7 @@ const ( exitWaitFailed = 1 exitWaitTimeout = 2 exitWaitError = 3 + exitWaitNoCI = 4 exitWaitInterrupted = 130 // maxConsecutiveWaitErrors is how many transient API errors in a row `prw wait` @@ -82,6 +83,8 @@ Exit codes: 1 CI failed (failure or error) 2 timed out while CI was still pending 3 other error (invalid URL, API error, PR closed without merging) + 4 the PR has no CI: no checks or statuses reported within 3 minutes of its + last update (e.g. the repository has no CI, or none runs for its base branch) 130 interrupted (Ctrl+C) Progress goes to stderr; the result goes to stdout (as JSON with --json).`, @@ -177,6 +180,9 @@ func (r *waitResult) exitErr() error { return waitErrorf(exitWaitFailed, "CI of %s finished with %s (%d failed check(s))", pr, r.State, len(r.FailedChecks)) case "closed": return waitErrorf(exitWaitError, "%s was closed without merging", pr) + case github.StateNoCI: + // Not a success: `prw wait && deploy` must not deploy a PR that no CI looked at. + return waitErrorf(exitWaitNoCI, "%s has no CI: no checks or statuses were reported", pr) } return waitErrorf(exitWaitError, "unexpected state %q for %s", r.State, pr) } @@ -267,6 +273,7 @@ func pollOnce(client ciClient, result *waitResult) (string, error) { if err != nil { return "", err } + github.ApplyNoCI(ci, pr.UpdatedAt, time.Now()) result.State = ci.State result.FailedChecks = ci.FailedChecks result.TotalChecks = ci.Total @@ -313,6 +320,8 @@ func printWaitResult(w io.Writer, r *waitResult, asJSON bool) error { icon = "✅" case "failure", "error", "closed": icon = "❌" + case github.StateNoCI: + icon = "⚪" } header := fmt.Sprintf("%s %s/%s#%d", icon, r.Owner, r.Repo, r.Number) if r.Title != "" { @@ -328,6 +337,8 @@ func printWaitResult(w io.Writer, r *waitResult, asJSON bool) error { fmt.Fprintf(w, "Timed out after %s; CI is still %s\n", elapsed, r.State) case r.State == "merged" || r.State == "closed": fmt.Fprintf(w, "PR was %s (after %s)\n", r.State, elapsed) + case r.State == github.StateNoCI: + fmt.Fprintf(w, "No CI: no checks or statuses were reported for this PR (after %s)\n", elapsed) default: fmt.Fprintf(w, "CI: %s (%d check(s), %s)\n", r.State, r.TotalChecks, elapsed) } diff --git a/internal/github/client.go b/internal/github/client.go index 2a771c3..cceccf2 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -108,6 +108,31 @@ type PullRequest struct { Head struct { SHA string `json:"sha"` } `json:"head"` + // 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"` +} + +// StateNoCI is reported instead of "pending" for a PR that has no checks or +// statuses at all and wasn't updated for NoCIGracePeriod, i.e. one whose repository +// (or base branch) runs no CI for it. +const StateNoCI = "no_ci" + +// NoCIGracePeriod is how long after the last update of a PR prw keeps waiting for +// CI to report its first check before concluding the PR has none. GitHub Actions +// usually registers checks within seconds; external CI can take longer. +const NoCIGracePeriod = 3 * time.Minute + +// ApplyNoCI turns a "pending" status with no checks or statuses at all into StateNoCI +// once the PR has gone NoCIGracePeriod without updates. updatedAt is the PR's +// UpdatedAt; when it's unknown (zero), the status is left as pending. +func ApplyNoCI(ci *CIStatus, updatedAt, now time.Time) { + if ci.State != "pending" || ci.Total != 0 || updatedAt.IsZero() { + return + } + if now.Sub(updatedAt) >= NoCIGracePeriod { + ci.State = StateNoCI + } } // IsClosed reports whether the PR is closed, merged or not (GitHub reports both as diff --git a/internal/github/noci_test.go b/internal/github/noci_test.go new file mode 100644 index 0000000..8a89082 --- /dev/null +++ b/internal/github/noci_test.go @@ -0,0 +1,64 @@ +package github + +import ( + "encoding/json" + "testing" + "time" +) + +func TestApplyNoCI(t *testing.T) { + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + tests := []struct { + name string + ci CIStatus + updatedAt time.Time + want string + }{ + {"no checks, quiet past the grace period", CIStatus{State: "pending"}, now.Add(-NoCIGracePeriod), StateNoCI}, + {"no checks, quiet for days", CIStatus{State: "pending"}, now.Add(-72 * time.Hour), StateNoCI}, + {"no checks yet, just pushed", CIStatus{State: "pending"}, now.Add(-30 * time.Second), "pending"}, + {"no checks, just short of the grace period", CIStatus{State: "pending"}, now.Add(-NoCIGracePeriod + time.Second), "pending"}, + {"no checks, unknown update time", CIStatus{State: "pending"}, time.Time{}, "pending"}, + {"checks still running", CIStatus{State: "pending", Total: 2, Pending: 1}, now.Add(-time.Hour), "pending"}, + {"success is left alone", CIStatus{State: "success", Total: 1}, now.Add(-time.Hour), "success"}, + {"failure is left alone", CIStatus{State: "failure", Total: 1}, now.Add(-time.Hour), "failure"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ci := tt.ci + ApplyNoCI(&ci, tt.updatedAt, now) + if ci.State != tt.want { + t.Errorf("state = %q, want %q", ci.State, tt.want) + } + }) + } +} + +func TestApplyNoCI_WithSummarizeCI(t *testing.T) { + now := time.Now() + old := now.Add(-time.Hour) + + // The shape the real API returns for a commit nothing reported on. + none := SummarizeCI(&CombinedStatus{State: "pending", TotalCount: 0}, nil) + ApplyNoCI(none, old, now) + if none.State != StateNoCI { + t.Errorf("empty CI = %q, want no_ci", none.State) + } + + // A legacy status that is still pending is a real signal, not "no CI". + legacy := SummarizeCI(&CombinedStatus{State: "pending", TotalCount: 1, Statuses: []CommitStatus{{Context: "ci", State: "pending"}}}, nil) + ApplyNoCI(legacy, old, now) + if legacy.State != "pending" { + t.Errorf("pending legacy status = %q, want pending", legacy.State) + } +} + +func TestPullRequest_DecodesUpdatedAt(t *testing.T) { + var pr PullRequest + if err := json.Unmarshal([]byte(`{"number":7,"updated_at":"2026-10-18T03:27:41Z"}`), &pr); err != nil { + t.Fatal(err) + } + if want := time.Date(2026, 10, 18, 3, 27, 41, 0, time.UTC); !pr.UpdatedAt.Equal(want) { + t.Errorf("UpdatedAt = %v, want %v", pr.UpdatedAt, want) + } +} diff --git a/internal/notify/webhook_format.go b/internal/notify/webhook_format.go index c91e6c3..4481065 100644 --- a/internal/notify/webhook_format.go +++ b/internal/notify/webhook_format.go @@ -29,6 +29,7 @@ const ( colorSuccess = 0x2EA043 colorFailure = 0xD1242F colorPending = 0xDBAB09 + colorNoCI = 0x8C959F ) // IsValidWebhookFormat reports whether value is a supported webhook format. @@ -110,6 +111,8 @@ func stateEmoji(state string) string { return "✅" case "failure", "error": return "❌" + case github.StateNoCI: + return "⚪" default: return "⏳" } @@ -121,6 +124,8 @@ func stateColor(state string) int { return colorSuccess case "failure", "error": return colorFailure + case github.StateNoCI: + return colorNoCI default: return colorPending } diff --git a/internal/watcher/noci_test.go b/internal/watcher/noci_test.go new file mode 100644 index 0000000..8dc8ed8 --- /dev/null +++ b/internal/watcher/noci_test.go @@ -0,0 +1,66 @@ +package watcher + +import ( + "testing" + "time" + + "github.com/devblac/prw/internal/config" + "github.com/devblac/prw/internal/github" +) + +func quietPR(number int, sha string, quietFor time.Duration, now time.Time) *github.PullRequest { + pr := newPR(number, sha) + pr.State = "open" + pr.UpdatedAt = now.Add(-quietFor) + return pr +} + +func TestCheckPR_ReportsNoCIWithoutNotifying(t *testing.T) { + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha1", time.Hour, now)}} + notifier := &mockNotifier{} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "pending")}} + w := New(client, cfg, notifier) + w.now = func() time.Time { return now } + + if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + t.Fatal(err) + } + if got := cfg.WatchedPRs[0].LastKnownState; got != github.StateNoCI { + t.Errorf("state = %q, want no_ci", got) + } + if len(notifier.events) != 0 { + t.Errorf("pending → no_ci must not notify, got %+v", notifier.events[0]) + } +} + +func TestCheckPR_StaysPendingDuringGracePeriod(t *testing.T) { + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha1", 20*time.Second, now)}} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "")}} + w := New(client, cfg, &mockNotifier{}) + w.now = func() time.Time { return now } + + w.checkPR(&cfg.WatchedPRs[0]) + if got := cfg.WatchedPRs[0].LastKnownState; got != "pending" { + t.Errorf("a PR pushed 20s ago must stay pending while CI registers, got %q", got) + } +} + +func TestCheckPR_NotifiesWhenCIShowsUpLater(t *testing.T) { + // e.g. a stacked PR is retargeted to main and CI starts running. + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{ + prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha1", time.Minute, now)}, + checkRuns: map[string][]github.CheckRun{"sha1": {{Name: "Test", Status: "completed", Conclusion: "success"}}}, + } + notifier := &mockNotifier{} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, github.StateNoCI)}} + w := New(client, cfg, notifier) + w.now = func() time.Time { return now } + + w.checkPR(&cfg.WatchedPRs[0]) + if len(notifier.events) != 1 || notifier.events[0].PreviousState != github.StateNoCI || notifier.events[0].CurrentState != "success" { + t.Errorf("events = %+v, want one no_ci → success", notifier.events) + } +} diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 1dc9ada..d51eac0 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -219,12 +219,15 @@ func (w *Watcher) checkPR(pr *config.WatchedPR) (closed bool, err error) { if err != nil { return false, fmt.Errorf("failed to fetch status: %w", err) } + github.ApplyNoCI(ci, ghPR.UpdatedAt, w.now()) currentState := ci.State previousState := github.NormalizeState(pr.LastKnownState) - // Check if status changed - if previousState != "" && previousState != currentState && shouldNotify(w.config.NotificationFilter, currentState) { + // 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). + if previousState != "" && previousState != currentState && currentState != github.StateNoCI && + shouldNotify(w.config.NotificationFilter, currentState) { event := ¬ify.StatusChangeEvent{ Owner: pr.Owner, Repo: pr.Repo, From 1cd47b94e098820a1dbf73ca88ea3e805bec5150 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:23:37 -0300 Subject: [PATCH 02/10] fix: ignore check runs superseded by a newer run of the same check When a workflow runs more than once for the same commit (e.g. an older run cancelled by `concurrency: cancel-in-progress`), the check-runs API returns one run per check suite, so the cancelled one kept the PR "failed" forever even after the newer run passed. Only the newest run per app and check name counts now, like GitHub's merge box and required checks. Verified live on zephyrproject-rtos/zephyr#105183: previously reported as failure ("Prevent Merging" cancelled), now success. Addresses Codex review on #6. Co-Authored-By: Claude Opus 5.5 --- internal/github/client.go | 35 +++++++++++++++++++- internal/github/superseded_test.go | 52 ++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 internal/github/superseded_test.go diff --git a/internal/github/client.go b/internal/github/client.go index cceccf2..47727b4 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -159,10 +159,14 @@ type CommitStatus struct { // CheckRun represents a single check run (e.g. a GitHub Actions job). type CheckRun struct { + ID int64 `json:"id"` Name string `json:"name"` Status string `json:"status"` // queued, in_progress, completed, ... Conclusion string `json:"conclusion"` // success, failure, neutral, cancelled, ... HTMLURL string `json:"html_url"` + App struct { + ID int64 `json:"id"` + } `json:"app"` } type checkRunsResponse struct { @@ -377,7 +381,7 @@ func SummarizeCI(status *CombinedStatus, runs []CheckRun) *CIStatus { } } - for _, run := range runs { + for _, run := range latestCheckRuns(runs) { total++ if NormalizeState(run.Status) != "completed" { hasPending = true @@ -407,6 +411,35 @@ func SummarizeCI(status *CombinedStatus, runs []CheckRun) *CIStatus { return &CIStatus{State: state, FailedChecks: failed, Total: total, Pending: pending} } +// latestCheckRuns drops check runs superseded by a newer run of the same check (same +// app and name). The API returns one run per check suite, so a workflow that ran more +// than once for the same commit (e.g. an older run cancelled by `concurrency: +// cancel-in-progress`) shows up twice; like GitHub's merge box and required checks, +// only the newest one counts. Run IDs grow with creation time. +func latestCheckRuns(runs []CheckRun) []CheckRun { + type checkKey struct { + app int64 + name string + } + newest := make(map[checkKey]int64, len(runs)) + for _, run := range runs { + key := checkKey{run.App.ID, run.Name} + if id, seen := newest[key]; !seen || run.ID > id { + newest[key] = run.ID + } + } + if len(newest) == len(runs) { + return runs + } + latest := make([]CheckRun, 0, len(newest)) + for _, run := range runs { + if newest[checkKey{run.App.ID, run.Name}] == run.ID { + latest = append(latest, run) + } + } + return latest +} + func hasCISignal(status *CombinedStatus, runs []CheckRun) bool { if len(runs) > 0 { return true diff --git a/internal/github/superseded_test.go b/internal/github/superseded_test.go new file mode 100644 index 0000000..0de87b4 --- /dev/null +++ b/internal/github/superseded_test.go @@ -0,0 +1,52 @@ +package github + +import "testing" + +func actionsRun(id int64, name, status, conclusion string) CheckRun { + run := CheckRun{ID: id, Name: name, Status: status, Conclusion: conclusion} + run.App.ID = 15368 // github-actions + return run +} + +func TestSummarizeCI_IgnoresRunSupersededByNewerRun(t *testing.T) { + // Shape of zephyrproject-rtos/zephyr#105183: a workflow with cancel-in-progress ran + // twice for the same commit, in two check suites; the API returns both runs. + runs := []CheckRun{ + actionsRun(66407602784, "Prevent Merging", "completed", "success"), + actionsRun(66407483043, "Prevent Merging", "completed", "cancelled"), + actionsRun(66407400000, "Build", "completed", "success"), + } + ci := SummarizeCI(&CombinedStatus{State: "pending"}, runs) + if ci.State != "success" || len(ci.FailedChecks) != 0 || ci.Total != 2 { + t.Errorf("got state=%q failed=%v total=%d, want success with 2 checks", ci.State, ci.FailedChecks, ci.Total) + } +} + +func TestSummarizeCI_NewerFailureStillCounts(t *testing.T) { + runs := []CheckRun{ + actionsRun(1, "Test", "completed", "success"), + actionsRun(2, "Test", "completed", "failure"), + } + if ci := SummarizeCI(nil, runs); ci.State != "failure" || len(ci.FailedChecks) != 1 { + t.Errorf("got state=%q failed=%v, want the newer failure", ci.State, ci.FailedChecks) + } +} + +func TestSummarizeCI_NewerRunStillInProgress(t *testing.T) { + runs := []CheckRun{ + actionsRun(1, "Test", "completed", "failure"), + actionsRun(2, "Test", "in_progress", ""), + } + if ci := SummarizeCI(nil, runs); ci.State != "pending" || ci.Pending != 1 || len(ci.FailedChecks) != 0 { + t.Errorf("got state=%q pending=%d failed=%v, want pending on the re-run", ci.State, ci.Pending, ci.FailedChecks) + } +} + +func TestSummarizeCI_SameNameFromDifferentAppsIsKept(t *testing.T) { + other := CheckRun{ID: 1, Name: "Test", Status: "completed", Conclusion: "failure"} + other.App.ID = 999 // e.g. an external CI that also names its check "Test" + runs := []CheckRun{other, actionsRun(2, "Test", "completed", "success")} + if ci := SummarizeCI(nil, runs); ci.State != "failure" { + t.Errorf("state = %q; a failure from another app must not be hidden", ci.State) + } +} From 702becdd0fc0ca014630eb23c81d84cb408027f6 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:24:16 -0300 Subject: [PATCH 03/10] fix: detect GovSlack webhook URLs (hooks.slack-gov.com) as Slack GovSlack serves incoming webhooks from its own slack-gov.com domain, so auto mode sent them the generic payload, which Slack rejects. Addresses Codex review on #7. Co-Authored-By: Claude Opus 5.5 --- internal/notify/webhook_format.go | 2 +- internal/notify/webhook_format_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/notify/webhook_format.go b/internal/notify/webhook_format.go index 4481065..7f7f29a 100644 --- a/internal/notify/webhook_format.go +++ b/internal/notify/webhook_format.go @@ -59,7 +59,7 @@ func DetectWebhookFormat(rawURL string) string { } host := strings.ToLower(u.Hostname()) switch { - case host == "hooks.slack.com": + case host == "hooks.slack.com" || host == "hooks.slack-gov.com": // GovSlack uses its own domain return WebhookFormatSlack case (host == "discord.com" || host == "discordapp.com" || strings.HasSuffix(host, ".discord.com") || strings.HasSuffix(host, ".discordapp.com")) && strings.HasPrefix(u.Path, "/api/webhooks/"): diff --git a/internal/notify/webhook_format_test.go b/internal/notify/webhook_format_test.go index ce37e3b..fcd175e 100644 --- a/internal/notify/webhook_format_test.go +++ b/internal/notify/webhook_format_test.go @@ -35,6 +35,7 @@ func TestDetectWebhookFormat(t *testing.T) { want string }{ {"https://hooks.slack.com/services/T000/B000/XXXX", WebhookFormatSlack}, + {"https://hooks.slack-gov.com/services/T000/B000/XXXX", WebhookFormatSlack}, {"https://HOOKS.SLACK.COM/services/T000/B000/XXXX", WebhookFormatSlack}, {"https://discord.com/api/webhooks/123/token", WebhookFormatDiscord}, {"https://discordapp.com/api/webhooks/123/token", WebhookFormatDiscord}, From 423a135d00c4281731c38d42e2951a6eefe5b035 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:26:30 -0300 Subject: [PATCH 04/10] fix: keep the merged/closed notification for review-request PRs A PR auto-watched with --review-requested that was merged or closed while prw was stopped no longer matched the is:open search, so the startup sync removed it as a withdrawn request before it was checked, and its final merged/closed notification was lost. Before removing review-request PRs that stopped matching, the sync now looks them up and keeps the closed ones; the regular check then notifies and drops them. If the lookup fails, the PR is removed as before. Addresses Codex review on #16. Co-Authored-By: Claude Opus 5.5 --- internal/watcher/autowatch.go | 44 ++++++++++++++++++++++++++---- internal/watcher/autowatch_test.go | 28 +++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/internal/watcher/autowatch.go b/internal/watcher/autowatch.go index 297fdd5..3eeec9b 100644 --- a/internal/watcher/autowatch.go +++ b/internal/watcher/autowatch.go @@ -26,9 +26,11 @@ var AutoWatchDescription = map[string]string{ config.AutoWatchReviewRequested: "PRs where your review is requested", } -// Searcher runs GitHub pull request searches. +// Searcher runs GitHub pull request searches. GetPullRequest tells a review request +// that was withdrawn apart from a PR that was merged or closed. type Searcher interface { SearchPullRequests(query string) (*github.SearchResult, error) + GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) } // SyncResult reports what SyncAutoWatch changed. @@ -42,9 +44,9 @@ type SyncResult struct { // SyncAutoWatch runs the search of every source in sources and updates the watch list: // matching PRs that aren't watched yet are added (tagged with their source), and PRs // added for "review-requested" that no longer match are removed, since the request -// was withdrawn or answered. PRs added for "mine" are not removed when they stop -// matching: that means they were merged or closed, and `prw run` should still notify -// about it before dropping them. PRs watched by hand are never touched. +// was withdrawn or answered. PRs that stopped matching because they were merged or +// closed are kept instead (for "mine" that's always the reason), so that `prw run` +// still notifies about it before dropping them. PRs watched by hand are never touched. // // It returns an error only if every search failed; other problems are Warnings. func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { @@ -73,6 +75,7 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { if len(found) == 0 && lastErr != nil { return result, lastErr } + closed := closedReviewRequests(client, found) err := config.Update(func(c *config.Config) error { changed := false @@ -97,7 +100,8 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { } kept := c.WatchedPRs[:0] for _, pr := range c.WatchedPRs { - if pr.Source == source && !matches[prKey(pr.Owner, pr.Repo, pr.Number)] { + key := prKey(pr.Owner, pr.Repo, pr.Number) + if pr.Source == source && !matches[key] && !closed[key] { result.Removed = append(result.Removed, pr) changed = true continue @@ -117,6 +121,36 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { return result, nil } +// closedReviewRequests finds the review-request PRs that no longer match their search +// because they were merged or closed: the search only returns open PRs, so they look +// just like withdrawn requests. It runs before the config is locked, as it makes API +// calls. A PR whose lookup fails is not reported, i.e. it's removed as before. +func closedReviewRequests(client Searcher, found map[string]*github.SearchResult) map[string]bool { + res, ok := found[config.AutoWatchReviewRequested] + if !ok || res.Incomplete { + return nil // nothing will be removed + } + cfg, err := config.Load() + if err != nil { + return nil + } + matches := make(map[string]bool, len(res.PRs)) + for _, pr := range res.PRs { + matches[prKey(pr.Owner, pr.Repo, pr.Number)] = true + } + closed := map[string]bool{} + for _, pr := range cfg.WatchedPRs { + key := prKey(pr.Owner, pr.Repo, pr.Number) + if pr.Source != config.AutoWatchReviewRequested || matches[key] { + continue + } + if ghPR, err := client.GetPullRequest(pr.Owner, pr.Repo, pr.Number); err == nil && ghPR.IsClosed() { + closed[key] = true + } + } + return closed +} + func prKey(owner, repo string, number int) string { return strings.ToLower(owner) + "/" + strings.ToLower(repo) + "#" + fmt.Sprint(number) } diff --git a/internal/watcher/autowatch_test.go b/internal/watcher/autowatch_test.go index 08fbe63..889a776 100644 --- a/internal/watcher/autowatch_test.go +++ b/internal/watcher/autowatch_test.go @@ -301,3 +301,31 @@ func TestRun_PicksUpAutoWatchEnabledWhileRunning(t *testing.T) { t.Errorf("watch list = %s", got) } } + +func TestRunOnce_NotifiesReviewRequestMergedWhileStopped(t *testing.T) { + // The PR was merged while prw wasn't running: the is:open search no longer returns + // it, which must not be mistaken for a withdrawn review request. + useTempConfig(t) + cfg := &config.Config{ + AutoWatch: []string{config.AutoWatchReviewRequested}, + WatchedPRs: []config.WatchedPR{ + {Owner: "owner", Repo: "repo", Number: 20, Source: config.AutoWatchReviewRequested, LastKnownState: "success"}, + }, + } + saveConfig(t, cfg) + merged := newPR(20, "sha20") + merged.State, merged.Merged = "closed", true + s := &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: found()}} + s.prs = map[string]*github.PullRequest{"owner/repo/20": merged} + notifier := &mockNotifier{} + + if err := New(s, cfg, notifier).RunOnce(context.Background()); err != nil { + t.Fatal(err) + } + if len(notifier.events) != 1 || notifier.events[0].CurrentState != "merged" { + t.Errorf("events = %+v, want one merged notification", notifier.events) + } + if got := watchList(t); got != "" { + t.Errorf("watch list = %q, want the merged PR dropped after notifying", got) + } +} From 35784403cd2a7e9f5175af5759036bf1ff3c16e2 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:27:08 -0300 Subject: [PATCH 05/10] fix: don't flap a `no_ci` PR to `pending` on comments and label changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Any comment or label change bumps a PR's updated_at, which restarted the no-CI grace period: `prw run` stored `pending` and, with the default `change` filter, notified "no_ci → pending" although no CI had appeared, then silently went back to no_ci three minutes later. A PR that was no_ci now stays no_ci until an actual check or status shows up. Addresses Codex review on #17. Co-Authored-By: Claude Opus 5.5 --- internal/watcher/noci_test.go | 37 +++++++++++++++++++++++++++++++++++ internal/watcher/watcher.go | 5 +++++ 2 files changed, 42 insertions(+) diff --git a/internal/watcher/noci_test.go b/internal/watcher/noci_test.go index 8dc8ed8..57b5ff0 100644 --- a/internal/watcher/noci_test.go +++ b/internal/watcher/noci_test.go @@ -64,3 +64,40 @@ func TestCheckPR_NotifiesWhenCIShowsUpLater(t *testing.T) { t.Errorf("events = %+v, want one no_ci → success", notifier.events) } } + +func TestCheckPR_StaysNoCIWhenOnlyMetadataChanges(t *testing.T) { + // A comment or label bumps updated_at, but no check or status appeared. + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha1", 10*time.Second, now)}} + notifier := &mockNotifier{} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, github.StateNoCI)}} + w := New(client, cfg, notifier) + w.now = func() time.Time { return now } + + if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + t.Fatal(err) + } + if got := cfg.WatchedPRs[0].LastKnownState; got != github.StateNoCI { + t.Errorf("state = %q, want no_ci", got) + } + if len(notifier.events) != 0 { + t.Errorf("a comment must not notify no_ci → pending, got %+v", notifier.events[0]) + } +} + +func TestCheckPR_NoCIToPendingWhenCIStartsAfterAPush(t *testing.T) { + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{ + prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha2", 10*time.Second, now)}, + checkRuns: map[string][]github.CheckRun{"sha2": {{Name: "Test", Status: "queued"}}}, + } + notifier := &mockNotifier{} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, github.StateNoCI)}} + w := New(client, cfg, notifier) + w.now = func() time.Time { return now } + + w.checkPR(&cfg.WatchedPRs[0]) + if got := cfg.WatchedPRs[0].LastKnownState; got != "pending" || len(notifier.events) != 1 { + t.Errorf("state = %q events = %d, want pending with one notification once a check exists", got, len(notifier.events)) + } +} diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index d51eac0..31085a9 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -223,6 +223,11 @@ func (w *Watcher) checkPR(pr *config.WatchedPR) (closed bool, err error) { currentState := ci.State previousState := github.NormalizeState(pr.LastKnownState) + if previousState == github.StateNoCI && currentState == "pending" && ci.Total == 0 { + // Comments and label changes also bump updated_at, which restarts the grace + // period; until an actual check or status shows up, the PR still has no CI. + currentState = github.StateNoCI + } // Check if status changed. Finding out that a PR has no CI is not a CI result, so // it isn't announced (it would otherwise ping once for every such PR). From 94e876efac9e04a41d02af07a3978c7fc35f92ed Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:35:53 -0300 Subject: [PATCH 06/10] fix: cancel in-flight GitHub requests on timeout/Ctrl-C; confirm head before returning Context: the GitHub client methods now take a context.Context, passed down to each HTTP request. Previously `prw wait --timeout`, MCP `wait_for_ci` `timeout_seconds` and client cancellation, and Ctrl-C were only noticed between polls: a stalled network could keep a poll (PR + status + check-run pages, each with a 15s HTTP timeout) running well past the deadline, and Ctrl-C did nothing until it finished. A poll cancelled this way now ends as timed out (exit 2) or interrupted (exit 130) instead of being retried as a transient error. `prw run` also stops immediately on Ctrl-C, leaving the PRs it hadn't checked unchanged. Head re-check: `prw wait` / `wait_for_ci` look up the PR and then its CI, so a commit pushed in between made them return the final result of the previous commit (e.g. `prw wait && deploy` deploying unchecked code). Before returning a final result, the PR is read again (a free 304 when unchanged); if the head moved, waiting continues on the new commit. Addresses Codex reviews on #13 and #15. Co-Authored-By: Claude Opus 5.5 --- cmd/prw/broadcast.go | 5 +- cmd/prw/main.go | 4 +- cmd/prw/mcp.go | 4 +- cmd/prw/wait.go | 45 ++++++++--- cmd/prw/wait_ctx_test.go | 97 ++++++++++++++++++++++++ cmd/prw/wait_test.go | 38 ++++++++-- internal/github/client.go | 31 ++++---- internal/github/client_test.go | 29 +++---- internal/github/efficiency_test.go | 25 +++--- internal/github/integration_test.go | 15 ++-- internal/github/search_test.go | 11 +-- internal/watcher/autowatch.go | 19 ++--- internal/watcher/autowatch_test.go | 16 ++-- internal/watcher/closed_pr_test.go | 13 ++-- internal/watcher/noci_test.go | 11 +-- internal/watcher/ratelimit_test.go | 8 +- internal/watcher/watcher.go | 33 ++++---- internal/watcher/watcher_persist_test.go | 32 +++++++- internal/watcher/watcher_test.go | 28 +++---- 19 files changed, 323 insertions(+), 141 deletions(-) create mode 100644 cmd/prw/wait_ctx_test.go diff --git a/cmd/prw/broadcast.go b/cmd/prw/broadcast.go index 856ba5c..312a463 100644 --- a/cmd/prw/broadcast.go +++ b/cmd/prw/broadcast.go @@ -1,6 +1,7 @@ package main import ( + "context" "fmt" "strings" "time" @@ -70,13 +71,13 @@ var broadcastCmd = &cobra.Command{ for i := range cfg.WatchedPRs { pr := &cfg.WatchedPRs[i] - ghPR, err := client.GetPullRequest(pr.Owner, pr.Repo, pr.Number) + ghPR, err := client.GetPullRequest(context.Background(), pr.Owner, pr.Repo, pr.Number) if err != nil { fmt.Printf("Error fetching PR %s/%s#%d: %v\n", pr.Owner, pr.Repo, pr.Number, err) continue } - ci, err := github.FetchCIStatus(client, pr.Owner, pr.Repo, ghPR.Head.SHA) + ci, err := github.FetchCIStatus(context.Background(), client, pr.Owner, pr.Repo, ghPR.Head.SHA) if err != nil { fmt.Printf("Error fetching status for %s/%s#%d: %v\n", pr.Owner, pr.Repo, pr.Number, err) continue diff --git a/cmd/prw/main.go b/cmd/prw/main.go index a7a7665..bb10c5c 100644 --- a/cmd/prw/main.go +++ b/cmd/prw/main.go @@ -157,7 +157,7 @@ func watchSinglePR(prURL string) error { // Try to fetch the PR to validate it exists and get title client := newGitHubClient(token) - pr, err := client.GetPullRequest(owner, repo, number) + pr, err := client.GetPullRequest(context.Background(), owner, repo, number) if err != nil { return fmt.Errorf("failed to fetch PR: %w", err) } @@ -229,7 +229,7 @@ func enableAutoWatch(sources []string) error { } // Run the searches now so the user sees the result immediately. - res, err := watcher.SyncAutoWatch(newGitHubClient(token), sources) + res, err := watcher.SyncAutoWatch(context.Background(), newGitHubClient(token), sources) for _, warning := range res.Warnings { fmt.Printf("Warning: %s\n", warning) } diff --git a/cmd/prw/mcp.go b/cmd/prw/mcp.go index 89792ca..2a434a5 100644 --- a/cmd/prw/mcp.go +++ b/cmd/prw/mcp.go @@ -569,7 +569,7 @@ func toolGetCIStatus(ctx context.Context, s *mcpServer, args json.RawMessage, _ return toolError("%v", err) } result := &waitResult{Owner: owner, Repo: repo, Number: number, URL: github.FormatPRURL(owner, repo, number)} - if _, err := pollOnce(client, result); err != nil { + if _, err := pollOnce(ctx, client, result); err != nil { return toolError("failed to get CI status of %s/%s#%d: %v", owner, repo, number, err) } return toolOK(ciSummary(result), result) @@ -666,7 +666,7 @@ func toolWatchPR(ctx context.Context, s *mcpServer, args json.RawMessage, _ io.W if err != nil { return toolError("%v", err) } - pr, err := client.GetPullRequest(owner, repo, number) + pr, err := client.GetPullRequest(ctx, owner, repo, number) if err != nil { return toolError("failed to fetch %s/%s#%d: %v", owner, repo, number, err) } diff --git a/cmd/prw/wait.go b/cmd/prw/wait.go index 8f44af2..77edee6 100644 --- a/cmd/prw/wait.go +++ b/cmd/prw/wait.go @@ -189,7 +189,7 @@ func (r *waitResult) exitErr() error { // ciClient is the subset of the GitHub client used by waitForCI. type ciClient interface { - GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) + GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) github.CIFetcher } @@ -203,11 +203,23 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number return result } + stopped := func() *waitResult { + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + result.TimedOut = true + } else { + result.Interrupted = true + } + return finish() + } + var lastLine string consecutiveErrors := 0 for { delay := interval - state, err := pollOnce(client, result) + state, err := pollOnce(ctx, client, result) + if err != nil && ctx.Err() != nil { + return stopped(), nil // the timeout or Ctrl-C cancelled the poll's requests + } if limited, resetAt := github.IsRateLimited(err); limited { // Not a failure of the PR or the network: wait for the quota to reset // (still bounded by --timeout) instead of burning the retry budget. @@ -228,6 +240,10 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number } else { consecutiveErrors = 0 if state != "pending" { + if headMoved(ctx, client, result) { + fmt.Fprintf(progress, "↻ %s/%s#%d: new commit pushed; waiting for its CI\n", owner, repo, number) + continue + } return finish(), nil } line := fmt.Sprintf("⏳ %s/%s#%d: %s", owner, repo, number, pendingSummary(result)) @@ -239,19 +255,26 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number sleepFunc(ctx, delay) if ctx.Err() != nil { - if errors.Is(ctx.Err(), context.DeadlineExceeded) { - result.TimedOut = true - } else { - result.Interrupted = true - } - return finish(), nil + return stopped(), nil } } } +// headMoved reports whether a new commit was pushed to the PR since result was +// polled. The CI lookup follows the PR lookup, so a push in between would otherwise +// return the result of the previous commit. Merged or closed PRs don't move; if the +// check fails, the result stands. Unchanged PRs answer 304, so this costs no quota. +func headMoved(ctx context.Context, client ciClient, result *waitResult) bool { + if result.State == "merged" || result.State == "closed" { + return false + } + pr, err := client.GetPullRequest(ctx, result.Owner, result.Repo, result.Number) + return err == nil && pr.Head.SHA != result.SHA +} + // pollOnce refreshes result from GitHub and returns the current state. -func pollOnce(client ciClient, result *waitResult) (string, error) { - pr, err := client.GetPullRequest(result.Owner, result.Repo, result.Number) +func pollOnce(ctx context.Context, client ciClient, result *waitResult) (string, error) { + pr, err := client.GetPullRequest(ctx, result.Owner, result.Repo, result.Number) if err != nil { return "", fmt.Errorf("failed to fetch PR: %w", err) } @@ -269,7 +292,7 @@ func pollOnce(client ciClient, result *waitResult) (string, error) { return result.State, nil } - ci, err := github.FetchCIStatus(client, result.Owner, result.Repo, pr.Head.SHA) + ci, err := github.FetchCIStatus(ctx, client, result.Owner, result.Repo, pr.Head.SHA) if err != nil { return "", err } diff --git a/cmd/prw/wait_ctx_test.go b/cmd/prw/wait_ctx_test.go new file mode 100644 index 0000000..fe67c12 --- /dev/null +++ b/cmd/prw/wait_ctx_test.go @@ -0,0 +1,97 @@ +package main + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/devblac/prw/internal/github" +) + +func TestWaitForCI_FollowsPushThatLandsDuringThePoll(t *testing.T) { + // The PR is read at "old", then "new" is pushed before the CI lookup of "old" + // returns success: that result must not be reported for the PR. + noSleep(t) + client := &scriptedClient{steps: []pollStep{ + {pr: openPR("old"), runs: []github.CheckRun{run("Test", "completed", "success")}, movedTo: openPR("new")}, + {pr: openPR("new"), runs: []github.CheckRun{run("Test", "in_progress", "")}}, + {pr: openPR("new"), runs: []github.CheckRun{run("Test", "completed", "failure")}}, + }} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.SHA != "new" || result.State != "failure" { + t.Errorf("got %s on %s, want the failure of the new head", result.State, result.SHA) + } +} + +func TestWaitForCI_ConfirmsHeadBeforeReturning(t *testing.T) { + noSleep(t) + client := &scriptedClient{steps: []pollStep{{pr: openPR("sha"), runs: []github.CheckRun{run("Test", "completed", "success")}}}} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.State != "success" || client.polls != 1 || client.revalidations != 1 { + t.Errorf("state=%q polls=%d revalidations=%d, want success after one poll and one head check", + result.State, client.polls, client.revalidations) + } +} + +// hangingClient is a real client against a server that never answers until the +// request is cancelled, like a stalled network. +func hangingClient(t *testing.T) *github.Client { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): + case <-time.After(10 * time.Second): + } + })) + t.Cleanup(srv.Close) + client := github.NewClient("token") + client.BaseURL = srv.URL + return client +} + +func TestWaitForCI_TimeoutCancelsTheRequestInFlight(t *testing.T) { + noSleep(t) + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + start := time.Now() + result, err := waitForCI(ctx, hangingClient(t), "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Errorf("returned after %s; the timeout must cancel the hanging request", elapsed) + } + if !result.TimedOut || exitCode(result.exitErr()) != exitWaitTimeout { + t.Errorf("result = %+v, want timed out (exit 2)", result) + } +} + +func TestWaitForCI_InterruptCancelsTheRequestInFlight(t *testing.T) { + noSleep(t) + ctx, cancel := context.WithCancel(context.Background()) + time.AfterFunc(100*time.Millisecond, cancel) // Ctrl-C + + start := time.Now() + result, err := waitForCI(ctx, hangingClient(t), "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Errorf("returned after %s; Ctrl-C must cancel the hanging request", elapsed) + } + if !result.Interrupted || exitCode(result.exitErr()) != exitWaitInterrupted { + t.Errorf("result = %+v, want interrupted (exit 130)", result) + } +} diff --git a/cmd/prw/wait_test.go b/cmd/prw/wait_test.go index 295b300..845e346 100644 --- a/cmd/prw/wait_test.go +++ b/cmd/prw/wait_test.go @@ -25,16 +25,39 @@ type pollStep struct { status *github.CombinedStatus runs []github.CheckRun ciErr error + // movedTo, if set, is what the PR looks like when waitForCI re-reads it to + // confirm the head hasn't moved, i.e. a push that landed during this poll. + movedTo *github.PullRequest } -// scriptedClient returns steps[i] on the i-th poll (repeating the last one). +// scriptedClient returns steps[i] on the i-th poll (repeating the last one). A PR +// lookup right after a poll that found a final CI result is the head re-check, not a +// new poll. type scriptedClient struct { - steps []pollStep - polls int - current pollStep + steps []pollStep + polls int + revalidations int + current pollStep + afterCI bool } -func (c *scriptedClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { +func (c *scriptedClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { + if c.afterCI { + c.afterCI = false + status := c.current.status + if status == nil { + status = &github.CombinedStatus{State: "pending"} + } + ci := github.SummarizeCI(status, c.current.runs) + github.ApplyNoCI(ci, c.current.pr.UpdatedAt, time.Now()) + if ci.State != "pending" { + c.revalidations++ + if c.current.movedTo != nil { + return c.current.movedTo, nil + } + return c.current.pr, nil + } + } i := c.polls if i >= len(c.steps) { i = len(c.steps) - 1 @@ -44,7 +67,7 @@ func (c *scriptedClient) GetPullRequest(owner, repo string, number int) (*github return c.current.pr, c.current.prErr } -func (c *scriptedClient) GetCombinedStatus(owner, repo, ref string) (*github.CombinedStatus, error) { +func (c *scriptedClient) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) { if c.current.ciErr != nil { return nil, c.current.ciErr } @@ -54,7 +77,8 @@ func (c *scriptedClient) GetCombinedStatus(owner, repo, ref string) (*github.Com return &github.CombinedStatus{State: "pending"}, nil } -func (c *scriptedClient) GetCheckRuns(owner, repo, ref string) ([]github.CheckRun, error) { +func (c *scriptedClient) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) { + c.afterCI = true return c.current.runs, nil } diff --git a/internal/github/client.go b/internal/github/client.go index 47727b4..2b6adf3 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -1,6 +1,7 @@ package github import ( + "context" "encoding/json" "errors" "fmt" @@ -192,8 +193,8 @@ type CIStatus struct { // CIFetcher is the subset of GitHub operations needed to compute CI status. type CIFetcher interface { - GetCombinedStatus(owner, repo, ref string) (*CombinedStatus, error) - GetCheckRuns(owner, repo, ref string) ([]CheckRun, error) + GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*CombinedStatus, error) + GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]CheckRun, error) } // ParsePRURL extracts owner, repo, and PR number from a GitHub PR URL. @@ -216,36 +217,36 @@ func ParsePRURL(prURL string) (owner, repo string, number int, err error) { } // GetPullRequest fetches a pull request by owner, repo, and PR number. -func (c *Client) GetPullRequest(owner, repo string, number int) (*PullRequest, error) { +func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) { path := fmt.Sprintf("/repos/%s/%s/pulls/%d", owner, repo, number) var pr PullRequest - if err := c.getJSON(path, &pr); err != nil { + if err := c.getJSON(ctx, path, &pr); err != nil { return nil, err } return &pr, nil } // GetCombinedStatus fetches the combined legacy commit status for a commit. -func (c *Client) GetCombinedStatus(owner, repo, ref string) (*CombinedStatus, error) { +func (c *Client) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*CombinedStatus, error) { path := fmt.Sprintf("/repos/%s/%s/commits/%s/status", owner, repo, url.PathEscape(ref)) var status CombinedStatus - if err := c.getJSON(path, &status); err != nil { + if err := c.getJSON(ctx, path, &status); err != nil { return nil, err } return &status, nil } // GetCheckRuns fetches the latest check runs (e.g. GitHub Actions jobs) for a commit. -func (c *Client) GetCheckRuns(owner, repo, ref string) ([]CheckRun, error) { +func (c *Client) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]CheckRun, error) { var runs []CheckRun for page := 1; page <= maxCheckRunPages; page++ { path := fmt.Sprintf("/repos/%s/%s/commits/%s/check-runs?per_page=%d&page=%d", owner, repo, url.PathEscape(ref), checkRunsPerPage, page) var resp checkRunsResponse - if err := c.getJSON(path, &resp); err != nil { + if err := c.getJSON(ctx, path, &resp); err != nil { var apiErr *APIError if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusForbidden && !apiErr.RateLimited { return nil, fmt.Errorf("%w (fine-grained tokens need the \"Checks: read\" permission)", err) @@ -298,12 +299,12 @@ type searchIssuesResponse struct { // SearchPullRequests runs a GitHub issue search (e.g. "is:pr is:open author:@me") and // returns the pull requests it finds, up to 300. Qualifiers like @me need a token. -func (c *Client) SearchPullRequests(query string) (*SearchResult, error) { +func (c *Client) SearchPullRequests(ctx context.Context, query string) (*SearchResult, error) { result := &SearchResult{} for page := 1; page <= maxSearchPages; page++ { path := fmt.Sprintf("/search/issues?q=%s&per_page=%d&page=%d", url.QueryEscape(query), searchPerPage, page) var resp searchIssuesResponse - if err := c.getJSON(path, &resp); err != nil { + if err := c.getJSON(ctx, path, &resp); err != nil { return nil, err } result.Total = resp.TotalCount @@ -331,13 +332,13 @@ func (c *Client) SearchPullRequests(query string) (*SearchResult, error) { } // FetchCIStatus fetches commit statuses and check runs for a commit and aggregates them. -func FetchCIStatus(c CIFetcher, owner, repo, ref string) (*CIStatus, error) { - status, err := c.GetCombinedStatus(owner, repo, ref) +func FetchCIStatus(ctx context.Context, c CIFetcher, owner, repo, ref string) (*CIStatus, error) { + status, err := c.GetCombinedStatus(ctx, owner, repo, ref) if err != nil { return nil, fmt.Errorf("failed to fetch commit status: %w", err) } - runs, err := c.GetCheckRuns(owner, repo, ref) + runs, err := c.GetCheckRuns(ctx, owner, repo, ref) if err != nil { return nil, fmt.Errorf("failed to fetch check runs: %w", err) } @@ -452,8 +453,8 @@ func hasCISignal(status *CombinedStatus, runs []CheckRun) bool { } // getJSON performs an authenticated GET request and decodes the JSON response into v. -func (c *Client) getJSON(path string, v interface{}) error { - req, err := http.NewRequest("GET", c.BaseURL+path, nil) +func (c *Client) getJSON(ctx context.Context, path string, v interface{}) error { + req, err := http.NewRequestWithContext(ctx, "GET", c.BaseURL+path, nil) if err != nil { return err } diff --git a/internal/github/client_test.go b/internal/github/client_test.go index 25b688f..0f7168f 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -1,6 +1,7 @@ package github import ( + "context" "fmt" "io" "net/http" @@ -144,7 +145,7 @@ func TestGetPullRequest(t *testing.T) { }, } - pr, err := client.GetPullRequest("owner", "repo", 123) + pr, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if tt.wantErr { if err == nil { t.Error("expected error, got nil") @@ -228,7 +229,7 @@ func TestGetCombinedStatus(t *testing.T) { }, } - status, err := client.GetCombinedStatus("owner", "repo", "abc123") + status, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if tt.wantErr { if err == nil { t.Error("expected error, got nil") @@ -333,7 +334,7 @@ func TestGetPullRequest_NetworkError(t *testing.T) { }, } - _, err := client.GetPullRequest("owner", "repo", 123) + _, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if err == nil { t.Error("expected error for network failure, got nil") } @@ -355,7 +356,7 @@ func TestGetCombinedStatus_NetworkError(t *testing.T) { }, } - _, err := client.GetCombinedStatus("owner", "repo", "abc123") + _, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err == nil { t.Error("expected error for network failure, got nil") } @@ -381,7 +382,7 @@ func TestGetPullRequest_InvalidJSON(t *testing.T) { }, } - _, err := client.GetPullRequest("owner", "repo", 123) + _, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if err == nil { t.Error("expected error for invalid JSON, got nil") } @@ -407,7 +408,7 @@ func TestGetCombinedStatus_InvalidJSON(t *testing.T) { }, } - _, err := client.GetCombinedStatus("owner", "repo", "abc123") + _, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err == nil { t.Error("expected error for invalid JSON, got nil") } @@ -433,7 +434,7 @@ func TestGetPullRequest_ErrorResponse(t *testing.T) { }, } - _, err := client.GetPullRequest("owner", "repo", 123) + _, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if err == nil { t.Error("expected error for 403 response, got nil") } @@ -459,7 +460,7 @@ func TestGetCombinedStatus_ErrorResponse(t *testing.T) { }, } - _, err := client.GetCombinedStatus("owner", "repo", "abc123") + _, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err == nil { t.Error("expected error for 404 response, got nil") } @@ -539,7 +540,7 @@ func TestGetCombinedStatus_EmptyState(t *testing.T) { }, } - status, err := client.GetCombinedStatus("owner", "repo", "abc123") + status, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -570,7 +571,7 @@ func TestGetJSON_OmitsAuthorizationWithoutToken(t *testing.T) { }, } - if _, err := client.GetPullRequest("owner", "repo", 1); err != nil { + if _, err := client.GetPullRequest(context.Background(), "owner", "repo", 1); err != nil { t.Fatalf("unexpected error: %v", err) } if len(gotAuth) != 0 { @@ -607,7 +608,7 @@ func TestGetCheckRuns_Paginates(t *testing.T) { }, } - runs, err := client.GetCheckRuns("owner", "repo", "abc123") + runs, err := client.GetCheckRuns(context.Background(), "owner", "repo", "abc123") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -632,7 +633,7 @@ func TestGetCheckRuns_ForbiddenHint(t *testing.T) { }, } - _, err := client.GetCheckRuns("owner", "repo", "abc123") + _, err := client.GetCheckRuns(context.Background(), "owner", "repo", "abc123") if err == nil { t.Fatal("expected error for 403 response, got nil") } @@ -662,7 +663,7 @@ func TestFetchCIStatus_ActionsOnlyRepo(t *testing.T) { }, } - ci, err := FetchCIStatus(client, "owner", "repo", "abc123") + ci, err := FetchCIStatus(context.Background(), client, "owner", "repo", "abc123") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -690,7 +691,7 @@ func TestFetchCIStatus_PropagatesErrors(t *testing.T) { }, } - _, err := FetchCIStatus(client, "owner", "repo", "abc123") + _, err := FetchCIStatus(context.Background(), client, "owner", "repo", "abc123") if err == nil || !strings.Contains(err.Error(), "check runs") { t.Errorf("expected check runs error, got %v", err) } diff --git a/internal/github/efficiency_test.go b/internal/github/efficiency_test.go index 8e79432..1fc6866 100644 --- a/internal/github/efficiency_test.go +++ b/internal/github/efficiency_test.go @@ -1,6 +1,7 @@ package github import ( + "context" "errors" "fmt" "net/http" @@ -37,7 +38,7 @@ func TestGetJSON_UsesETagAndServesCachedBodyOn304(t *testing.T) { }) for i := 0; i < 3; i++ { - pr, err := c.GetPullRequest("o", "r", 7) + pr, err := c.GetPullRequest(context.Background(), "o", "r", 7) if err != nil { t.Fatalf("request %d: %v", i, err) } @@ -63,13 +64,13 @@ func TestGetJSON_RefreshesCacheWhenResourceChanges(t *testing.T) { fmt.Fprintf(w, `{"state":"pending","total_count":%d}`, v) }) - s, _ := c.GetCombinedStatus("o", "r", "sha") + s, _ := c.GetCombinedStatus(context.Background(), "o", "r", "sha") atomic.StoreInt32(&version, 2) - s2, err := c.GetCombinedStatus("o", "r", "sha") + s2, err := c.GetCombinedStatus(context.Background(), "o", "r", "sha") if err != nil { t.Fatal(err) } - s3, _ := c.GetCombinedStatus("o", "r", "sha") + s3, _ := c.GetCombinedStatus(context.Background(), "o", "r", "sha") if s.TotalCount != 1 || s2.TotalCount != 2 || s3.TotalCount != 2 { t.Errorf("got %d, %d, %d; want 1, 2, 2", s.TotalCount, s2.TotalCount, s3.TotalCount) } @@ -85,8 +86,8 @@ func TestGetJSON_CachesPerPath(t *testing.T) { fmt.Fprintf(w, `{"number":%s}`, n) }) - a, _ := c.GetPullRequest("o", "r", 1) - b, _ := c.GetPullRequest("o", "r", 2) + a, _ := c.GetPullRequest(context.Background(), "o", "r", 1) + b, _ := c.GetPullRequest(context.Background(), "o", "r", 2) if a.Number != 1 || b.Number != 2 { t.Errorf("got #%d and #%d", a.Number, b.Number) } @@ -99,15 +100,15 @@ func TestGetJSON_NoETagMeansNoConditionalRequest(t *testing.T) { } fmt.Fprint(w, `{"number":1}`) }) - c.GetPullRequest("o", "r", 1) - c.GetPullRequest("o", "r", 1) + c.GetPullRequest(context.Background(), "o", "r", 1) + c.GetPullRequest(context.Background(), "o", "r", 1) } func TestGetJSON_Unexpected304IsAnError(t *testing.T) { c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotModified) }) - if _, err := c.GetPullRequest("o", "r", 1); err == nil { + if _, err := c.GetPullRequest(context.Background(), "o", "r", 1); err == nil { t.Error("a 304 without a cached body must not decode as an empty PR") } } @@ -134,7 +135,7 @@ func TestClient_RecordsRateLimitHeaders(t *testing.T) { if c.RateLimit().Known { t.Error("rate limit should be unknown before any request") } - c.GetPullRequest("o", "r", 1) + c.GetPullRequest(context.Background(), "o", "r", 1) rl := c.RateLimit() if !rl.Known || rl.Limit != 5000 || rl.Remaining != 4321 || rl.Reset.Unix() != reset { t.Errorf("RateLimit() = %+v", rl) @@ -251,7 +252,7 @@ func TestGetCheckRuns_RateLimitHasNoPermissionHint(t *testing.T) { w.WriteHeader(http.StatusForbidden) fmt.Fprint(w, `{"message":"API rate limit exceeded"}`) }) - _, err := c.GetCheckRuns("o", "r", "sha") + _, err := c.GetCheckRuns(context.Background(), "o", "r", "sha") if limited, _ := IsRateLimited(err); !limited { t.Fatalf("expected a rate limit error, got %v", err) } @@ -273,7 +274,7 @@ func TestClient_ConcurrentRequestsAreSafe(t *testing.T) { wg.Add(1) go func(n int) { defer wg.Done() - c.GetPullRequest("o", "r", n%3) + c.GetPullRequest(context.Background(), "o", "r", n%3) c.RateLimit() }(i) } diff --git a/internal/github/integration_test.go b/internal/github/integration_test.go index 0c23377..26f4817 100644 --- a/internal/github/integration_test.go +++ b/internal/github/integration_test.go @@ -14,6 +14,7 @@ package github import ( + "context" "errors" "net/http" "os" @@ -40,7 +41,7 @@ func integrationClient(t *testing.T) *Client { func TestIntegration_ActionsOnlyCommitResolvesToSuccess(t *testing.T) { c := integrationClient(t) - status, err := c.GetCombinedStatus(fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) + status, err := c.GetCombinedStatus(context.Background(), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) if err != nil { t.Fatalf("GetCombinedStatus: %v", err) } @@ -49,7 +50,7 @@ func TestIntegration_ActionsOnlyCommitResolvesToSuccess(t *testing.T) { t.Logf("note: legacy status changed to state=%q total_count=%d", status.State, status.TotalCount) } - runs, err := c.GetCheckRuns(fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) + runs, err := c.GetCheckRuns(context.Background(), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) if err != nil { t.Fatalf("GetCheckRuns: %v", err) } @@ -65,7 +66,7 @@ func TestIntegration_ActionsOnlyCommitResolvesToSuccess(t *testing.T) { } func TestIntegration_FetchCIStatus(t *testing.T) { - ci, err := FetchCIStatus(integrationClient(t), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) + ci, err := FetchCIStatus(context.Background(), integrationClient(t), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) if err != nil { t.Fatalf("FetchCIStatus: %v", err) } @@ -78,7 +79,7 @@ func TestIntegration_FetchCIStatus(t *testing.T) { } func TestIntegration_MergedPullRequest(t *testing.T) { - pr, err := integrationClient(t).GetPullRequest(fixtureOwner, fixtureRepo, fixtureMergedPR) + pr, err := integrationClient(t).GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, fixtureMergedPR) if err != nil { t.Fatalf("GetPullRequest: %v", err) } @@ -116,13 +117,13 @@ func TestIntegration_ConditionalRequestsAreFree(t *testing.T) { rec := &statusRecorder{} c.HTTPClient.Transport = rec - first, err := c.GetPullRequest(fixtureOwner, fixtureRepo, fixtureMergedPR) + first, err := c.GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, fixtureMergedPR) if err != nil { t.Fatalf("first request: %v", err) } before := c.RateLimit() - second, err := c.GetPullRequest(fixtureOwner, fixtureRepo, fixtureMergedPR) + second, err := c.GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, fixtureMergedPR) if err != nil { t.Fatalf("second request: %v", err) } @@ -146,7 +147,7 @@ func TestIntegration_ConditionalRequestsAreFree(t *testing.T) { } func TestIntegration_MissingPullRequestIs404(t *testing.T) { - _, err := integrationClient(t).GetPullRequest(fixtureOwner, fixtureRepo, 999999) + _, err := integrationClient(t).GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, 999999) var apiErr *APIError if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusNotFound { t.Errorf("expected a 404 APIError, got %v", err) diff --git a/internal/github/search_test.go b/internal/github/search_test.go index 3fcc3aa..3416580 100644 --- a/internal/github/search_test.go +++ b/internal/github/search_test.go @@ -1,6 +1,7 @@ package github import ( + "context" "fmt" "net/http" "strconv" @@ -27,7 +28,7 @@ func TestSearchPullRequests_ParsesResultsAndSkipsIssues(t *testing.T) { searchItem("Alice", "api", 1, true), searchItem("bob", "web", 2, false), searchItem("bob", "web", 3, true)) }) - res, err := c.SearchPullRequests("is:pr is:open author:@me") + res, err := c.SearchPullRequests(context.Background(), "is:pr is:open author:@me") if err != nil { t.Fatal(err) } @@ -62,7 +63,7 @@ func TestSearchPullRequests_Paginates(t *testing.T) { fmt.Fprintf(w, `{"total_count":%d,"items":[%s]}`, searchPerPage+5, strings.Join(items, ",")) }) - res, err := c.SearchPullRequests("q") + res, err := c.SearchPullRequests(context.Background(), "q") if err != nil { t.Fatal(err) } @@ -82,7 +83,7 @@ func TestSearchPullRequests_CapsAndMarksIncomplete(t *testing.T) { fmt.Fprintf(w, `{"total_count":5000,"items":[%s]}`, strings.Join(items, ",")) }) - res, err := c.SearchPullRequests("q") + res, err := c.SearchPullRequests(context.Background(), "q") if err != nil { t.Fatal(err) } @@ -98,7 +99,7 @@ func TestSearchPullRequests_IncompleteFromGitHub(t *testing.T) { c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, `{"total_count":1,"incomplete_results":true,"items":[%s]}`, searchItem("o", "r", 1, true)) }) - res, err := c.SearchPullRequests("q") + res, err := c.SearchPullRequests(context.Background(), "q") if err != nil { t.Fatal(err) } @@ -111,7 +112,7 @@ func TestSearchPullRequests_Error(t *testing.T) { c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"message":"Validation Failed"}`, http.StatusUnprocessableEntity) }) - if _, err := c.SearchPullRequests("q"); err == nil || !strings.Contains(err.Error(), "422") { + if _, err := c.SearchPullRequests(context.Background(), "q"); err == nil || !strings.Contains(err.Error(), "422") { t.Errorf("expected a 422 error, got %v", err) } } diff --git a/internal/watcher/autowatch.go b/internal/watcher/autowatch.go index 3eeec9b..ed9955b 100644 --- a/internal/watcher/autowatch.go +++ b/internal/watcher/autowatch.go @@ -1,6 +1,7 @@ package watcher import ( + "context" "fmt" "strings" "time" @@ -29,8 +30,8 @@ var AutoWatchDescription = map[string]string{ // Searcher runs GitHub pull request searches. GetPullRequest tells a review request // that was withdrawn apart from a PR that was merged or closed. type Searcher interface { - SearchPullRequests(query string) (*github.SearchResult, error) - GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) + SearchPullRequests(ctx context.Context, query string) (*github.SearchResult, error) + GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) } // SyncResult reports what SyncAutoWatch changed. @@ -49,7 +50,7 @@ type SyncResult struct { // still notifies about it before dropping them. PRs watched by hand are never touched. // // It returns an error only if every search failed; other problems are Warnings. -func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { +func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*SyncResult, error) { result := &SyncResult{} found := map[string]*github.SearchResult{} var lastErr error @@ -60,7 +61,7 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { result.Warnings = append(result.Warnings, fmt.Sprintf("unknown auto-watch source %q ignored", source)) continue } - res, err := client.SearchPullRequests(query) + res, err := client.SearchPullRequests(ctx, query) if err != nil { lastErr = err result.Warnings = append(result.Warnings, fmt.Sprintf("search for %s failed: %v", AutoWatchDescription[source], err)) @@ -75,7 +76,7 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { if len(found) == 0 && lastErr != nil { return result, lastErr } - closed := closedReviewRequests(client, found) + closed := closedReviewRequests(ctx, client, found) err := config.Update(func(c *config.Config) error { changed := false @@ -125,7 +126,7 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { // because they were merged or closed: the search only returns open PRs, so they look // just like withdrawn requests. It runs before the config is locked, as it makes API // calls. A PR whose lookup fails is not reported, i.e. it's removed as before. -func closedReviewRequests(client Searcher, found map[string]*github.SearchResult) map[string]bool { +func closedReviewRequests(ctx context.Context, client Searcher, found map[string]*github.SearchResult) map[string]bool { res, ok := found[config.AutoWatchReviewRequested] if !ok || res.Incomplete { return nil // nothing will be removed @@ -144,7 +145,7 @@ func closedReviewRequests(client Searcher, found map[string]*github.SearchResult if pr.Source != config.AutoWatchReviewRequested || matches[key] { continue } - if ghPR, err := client.GetPullRequest(pr.Owner, pr.Repo, pr.Number); err == nil && ghPR.IsClosed() { + if ghPR, err := client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number); err == nil && ghPR.IsClosed() { closed[key] = true } } @@ -156,7 +157,7 @@ func prKey(owner, repo string, number int) string { } // syncAutoWatch refreshes the auto-watch searches during `prw run` and reports changes. -func (w *Watcher) syncAutoWatch() { +func (w *Watcher) syncAutoWatch(ctx context.Context) { sources := w.config.AutoWatch if fresh, err := config.Load(); err == nil { sources = fresh.AutoWatch // `prw watch --mine` may have run since startup @@ -170,7 +171,7 @@ func (w *Watcher) syncAutoWatch() { if !ok { return } - res, err := SyncAutoWatch(searcher, sources) + res, err := SyncAutoWatch(ctx, searcher, sources) for _, warning := range res.Warnings { fmt.Printf("Warning: auto-watch: %s\n", warning) } diff --git a/internal/watcher/autowatch_test.go b/internal/watcher/autowatch_test.go index 889a776..a2b23c1 100644 --- a/internal/watcher/autowatch_test.go +++ b/internal/watcher/autowatch_test.go @@ -22,7 +22,7 @@ type fakeSearcher struct { searches int } -func (f *fakeSearcher) SearchPullRequests(query string) (*github.SearchResult, error) { +func (f *fakeSearcher) SearchPullRequests(ctx context.Context, query string) (*github.SearchResult, error) { f.searches++ for source, q := range autoWatchQueries { if q == query { @@ -82,7 +82,7 @@ func TestSyncAutoWatch_AddsMatchesTaggedWithSource(t *testing.T) { config.AutoWatchReviewRequested: found(3), }} - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { t.Fatal(err) } @@ -116,7 +116,7 @@ func TestSyncAutoWatch_DropsWithdrawnReviewRequestsButKeepsMine(t *testing.T) { config.AutoWatchReviewRequested: found(21), }} - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { t.Fatal(err) } @@ -138,7 +138,7 @@ func TestSyncAutoWatch_IncompleteResultsNeverRemove(t *testing.T) { partial.Incomplete, partial.Total = true, 2 s := &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: partial}} - res, err := SyncAutoWatch(s, []string{config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchReviewRequested}) if err != nil { t.Fatal(err) } @@ -161,7 +161,7 @@ func TestSyncAutoWatch_OneFailedSearchDoesNotBlockTheOther(t *testing.T) { errs: map[string]error{config.AutoWatchReviewRequested: errors.New("boom")}, } - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { t.Fatalf("a partial failure is a warning, got %v", err) } @@ -179,7 +179,7 @@ func TestSyncAutoWatch_AllSearchesFailing(t *testing.T) { boom := errors.New("network down") s := &fakeSearcher{errs: map[string]error{config.AutoWatchMine: boom}} - if _, err := SyncAutoWatch(s, []string{config.AutoWatchMine}); !errors.Is(err, boom) { + if _, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine}); !errors.Is(err, boom) { t.Errorf("err = %v, want the search error", err) } } @@ -189,7 +189,7 @@ func TestSyncAutoWatch_SkipsSourceDisabledMeanwhile(t *testing.T) { saveConfig(t, &config.Config{}) // `prw unwatch --mine` ran after the caller read the config s := &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchMine: found(1)}} - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine}) if err != nil { t.Fatal(err) } @@ -201,7 +201,7 @@ func TestSyncAutoWatch_SkipsSourceDisabledMeanwhile(t *testing.T) { func TestSyncAutoWatch_UnknownSource(t *testing.T) { useTempConfig(t) saveConfig(t, &config.Config{AutoWatch: []string{"bogus"}}) - res, err := SyncAutoWatch(&fakeSearcher{}, []string{"bogus"}) + res, err := SyncAutoWatch(context.Background(), &fakeSearcher{}, []string{"bogus"}) if err != nil { t.Fatal(err) } diff --git a/internal/watcher/closed_pr_test.go b/internal/watcher/closed_pr_test.go index 6127ddc..f5e69bd 100644 --- a/internal/watcher/closed_pr_test.go +++ b/internal/watcher/closed_pr_test.go @@ -1,6 +1,7 @@ package watcher import ( + "context" "testing" "github.com/devblac/prw/internal/config" @@ -19,7 +20,7 @@ func TestCheckPR_MergedPRIsRemoved(t *testing.T) { cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "success")}} w := New(client, cfg, notifier) - closed, err := w.checkPR(&cfg.WatchedPRs[0]) + closed, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -44,7 +45,7 @@ func TestCheckPR_ClosedWithoutMergeIsRemoved(t *testing.T) { cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "failure")}} w := New(client, cfg, notifier) - closed, err := w.checkPR(&cfg.WatchedPRs[0]) + closed, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -62,7 +63,7 @@ func TestCheckPR_MergedNotifiesEvenWithFailFilter(t *testing.T) { cfg := &config.Config{NotificationFilter: config.NotificationFilterFail, WatchedPRs: []config.WatchedPR{watched(1, "success")}} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("unexpected error: %v", err) } if len(notifier.events) != 1 { @@ -80,7 +81,7 @@ func TestCheckPR_OpenPRIsNotRemoved(t *testing.T) { cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "pending")}} w := New(client, cfg, &mockNotifier{}) - closed, err := w.checkPR(&cfg.WatchedPRs[0]) + closed, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -107,7 +108,7 @@ func TestCheckAllPRs_RemovesOnlyClosedPRs(t *testing.T) { inMemory := &config.Config{PollIntervalSeconds: 20, WatchedPRs: []config.WatchedPR{watched(1, "pending"), watched(2, "success")}} w := New(client, inMemory, &mockNotifier{}) - w.checkAllPRs() + w.checkAllPRs(context.Background()) if len(w.config.WatchedPRs) != 1 || w.config.WatchedPRs[0].Number != 1 { t.Fatalf("in-memory watch list after checkAllPRs = %+v, want only #1", w.config.WatchedPRs) @@ -144,7 +145,7 @@ func TestCheckAllPRs_RemovalSurvivesConcurrentChange(t *testing.T) { t.Fatal(err) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, _ := config.Load() if len(loaded.WatchedPRs) != 1 || loaded.WatchedPRs[0].Number != 2 { diff --git a/internal/watcher/noci_test.go b/internal/watcher/noci_test.go index 57b5ff0..d5b32e9 100644 --- a/internal/watcher/noci_test.go +++ b/internal/watcher/noci_test.go @@ -1,6 +1,7 @@ package watcher import ( + "context" "testing" "time" @@ -23,7 +24,7 @@ func TestCheckPR_ReportsNoCIWithoutNotifying(t *testing.T) { w := New(client, cfg, notifier) w.now = func() time.Time { return now } - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatal(err) } if got := cfg.WatchedPRs[0].LastKnownState; got != github.StateNoCI { @@ -41,7 +42,7 @@ func TestCheckPR_StaysPendingDuringGracePeriod(t *testing.T) { w := New(client, cfg, &mockNotifier{}) w.now = func() time.Time { return now } - w.checkPR(&cfg.WatchedPRs[0]) + w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if got := cfg.WatchedPRs[0].LastKnownState; got != "pending" { t.Errorf("a PR pushed 20s ago must stay pending while CI registers, got %q", got) } @@ -59,7 +60,7 @@ func TestCheckPR_NotifiesWhenCIShowsUpLater(t *testing.T) { w := New(client, cfg, notifier) w.now = func() time.Time { return now } - w.checkPR(&cfg.WatchedPRs[0]) + w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if len(notifier.events) != 1 || notifier.events[0].PreviousState != github.StateNoCI || notifier.events[0].CurrentState != "success" { t.Errorf("events = %+v, want one no_ci → success", notifier.events) } @@ -74,7 +75,7 @@ func TestCheckPR_StaysNoCIWhenOnlyMetadataChanges(t *testing.T) { w := New(client, cfg, notifier) w.now = func() time.Time { return now } - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatal(err) } if got := cfg.WatchedPRs[0].LastKnownState; got != github.StateNoCI { @@ -96,7 +97,7 @@ func TestCheckPR_NoCIToPendingWhenCIStartsAfterAPush(t *testing.T) { w := New(client, cfg, notifier) w.now = func() time.Time { return now } - w.checkPR(&cfg.WatchedPRs[0]) + w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if got := cfg.WatchedPRs[0].LastKnownState; got != "pending" || len(notifier.events) != 1 { t.Errorf("state = %q events = %d, want pending with one notification once a check exists", got, len(notifier.events)) } diff --git a/internal/watcher/ratelimit_test.go b/internal/watcher/ratelimit_test.go index 146883f..d15e962 100644 --- a/internal/watcher/ratelimit_test.go +++ b/internal/watcher/ratelimit_test.go @@ -16,12 +16,12 @@ type countingClient struct { prErr error } -func (c *countingClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { +func (c *countingClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { c.calls++ if c.prErr != nil { return nil, c.prErr } - return c.mockGitHubClient.GetPullRequest(owner, repo, number) + return c.mockGitHubClient.GetPullRequest(context.Background(), owner, repo, number) } func TestCheckAllPRs_StopsAndPausesOnRateLimit(t *testing.T) { @@ -37,7 +37,7 @@ func TestCheckAllPRs_StopsAndPausesOnRateLimit(t *testing.T) { w := New(client, cfg, &mockNotifier{}) w.now = func() time.Time { return reset.Add(-10 * time.Minute) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) if client.calls != 1 { t.Errorf("must stop after the first rate-limited request, made %d", client.calls) @@ -95,7 +95,7 @@ func TestCheckAllPRs_OtherErrorsDoNotPause(t *testing.T) { } w := New(client, cfg, &mockNotifier{}) - w.checkAllPRs() + w.checkAllPRs(context.Background()) if client.calls != 2 { t.Errorf("a non-rate-limit error must not stop the round, made %d calls", client.calls) diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 31085a9..53aa364 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -12,9 +12,9 @@ import ( // GitHubClient defines the interface for GitHub operations. type GitHubClient interface { - GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) - GetCombinedStatus(owner, repo, ref string) (*github.CombinedStatus, error) - GetCheckRuns(owner, repo, ref string) ([]github.CheckRun, error) + GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) + GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) + GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) } // ConfigStore defines the interface for config persistence. @@ -70,10 +70,10 @@ func (w *Watcher) Run(ctx context.Context) error { // Check immediately on startup if len(w.config.AutoWatch) > 0 { - w.syncAutoWatch() + w.syncAutoWatch(ctx) w.reloadWatchedPRs() } - w.checkAllPRs() + w.checkAllPRs(ctx) for { select { @@ -86,10 +86,10 @@ func (w *Watcher) Run(ctx context.Context) error { } w.reloadWatchedPRs() if len(w.config.AutoWatch) > 0 && w.now().Sub(w.lastAutoWatch) >= AutoWatchInterval { - w.syncAutoWatch() + w.syncAutoWatch(ctx) w.reloadWatchedPRs() } - w.checkAllPRs() + w.checkAllPRs(ctx) } } } @@ -114,23 +114,28 @@ func (w *Watcher) reloadWatchedPRs() { func (w *Watcher) RunOnce(ctx context.Context) error { fmt.Printf("Running one-time check with %d second poll interval...\n", w.config.PollIntervalSeconds) if len(w.config.AutoWatch) > 0 { - w.syncAutoWatch() + w.syncAutoWatch(ctx) w.reloadWatchedPRs() } if len(w.config.WatchedPRs) == 0 { fmt.Println(noPRsMessage) return nil } - w.checkAllPRs() + w.checkAllPRs(ctx) return nil } -func (w *Watcher) checkAllPRs() { +func (w *Watcher) checkAllPRs(ctx context.Context) { var stillWatched, done []config.WatchedPR for i := range w.config.WatchedPRs { pr := w.config.WatchedPRs[i] - closed, err := w.checkPR(&pr) + closed, err := w.checkPR(ctx, &pr) + if err != nil && ctx.Err() != nil { + // Stopped mid-round (Ctrl-C): keep this and the remaining PRs unchanged. + stillWatched = append(stillWatched, w.config.WatchedPRs[i:]...) + break + } if err != nil { stillWatched = append(stillWatched, pr) // keep watching; preserve any partial update (e.g. title) if limited, resetAt := github.IsRateLimited(err); limited { @@ -176,9 +181,9 @@ func (w *Watcher) pauseForRateLimit(resetAt time.Time) { // checkPR checks one PR's CI status and reports whether it is now closed (merged or // not), in which case the caller should stop watching it. -func (w *Watcher) checkPR(pr *config.WatchedPR) (closed bool, err error) { +func (w *Watcher) checkPR(ctx context.Context, pr *config.WatchedPR) (closed bool, err error) { // Fetch the PR to get its state and current head SHA - ghPR, err := w.client.GetPullRequest(pr.Owner, pr.Repo, pr.Number) + ghPR, err := w.client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number) if err != nil { return false, fmt.Errorf("failed to fetch PR: %w", err) } @@ -215,7 +220,7 @@ func (w *Watcher) checkPR(pr *config.WatchedPR) (closed bool, err error) { currentSHA := ghPR.Head.SHA // Aggregate commit statuses and check runs (GitHub Actions) for the head commit - ci, err := github.FetchCIStatus(w.client, pr.Owner, pr.Repo, currentSHA) + ci, err := github.FetchCIStatus(ctx, w.client, pr.Owner, pr.Repo, currentSHA) if err != nil { return false, fmt.Errorf("failed to fetch status: %w", err) } diff --git a/internal/watcher/watcher_persist_test.go b/internal/watcher/watcher_persist_test.go index 2bdd92d..80b057e 100644 --- a/internal/watcher/watcher_persist_test.go +++ b/internal/watcher/watcher_persist_test.go @@ -1,6 +1,7 @@ package watcher import ( + "context" "os" "path/filepath" "testing" @@ -63,7 +64,7 @@ func TestWatcherReloadsWatchListEachTick(t *testing.T) { t.Fatalf("watcher did not pick up the new watch list: %+v", w.config.WatchedPRs) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, err := config.Load() if err != nil { @@ -106,7 +107,7 @@ func TestWatcherDoesNotResurrectUnwatchedPR(t *testing.T) { t.Fatal(err) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, _ := config.Load() if len(loaded.WatchedPRs) != 1 || loaded.WatchedPRs[0].Number != 1 { @@ -141,7 +142,7 @@ func TestWatcherKeepsPRAddedDuringPoll(t *testing.T) { t.Fatal(err) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, _ := config.Load() if len(loaded.WatchedPRs) != 2 { @@ -171,7 +172,7 @@ func TestWatcherDoesNotPersistRunOnlyOverrides(t *testing.T) { prs: map[string]*github.PullRequest{"owner/repo/1": newPR(1, "sha1")}, statuses: map[string]*github.CombinedStatus{"sha1": {State: "success", SHA: "sha1"}}, } - New(client, inMemory, &mockNotifier{}).checkAllPRs() + New(client, inMemory, &mockNotifier{}).checkAllPRs(context.Background()) loaded, _ := config.Load() if loaded.NotificationFilter != config.NotificationFilterChange { @@ -200,3 +201,26 @@ func TestWatcherReloadKeepsListOnLoadError(t *testing.T) { t.Errorf("a config read error must not empty the watch list: %+v", w.config.WatchedPRs) } } + +func TestCheckAllPRs_StoppedMidRoundKeepsEveryPR(t *testing.T) { + useTempConfig(t) + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "success"), watched(2, "failure")}} + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() // Ctrl-C: every request now fails with context.Canceled + client := &mockGitHubClient{err: ctx.Err()} + notifier := &mockNotifier{} + w := New(client, cfg, notifier) + + w.checkAllPRs(ctx) + + loaded, _ := config.Load() + if len(loaded.WatchedPRs) != 2 || loaded.WatchedPRs[0].LastKnownState != "success" || loaded.WatchedPRs[1].LastKnownState != "failure" { + t.Errorf("watch list = %+v, want both PRs unchanged", loaded.WatchedPRs) + } + if len(notifier.events) != 0 { + t.Errorf("events = %+v", notifier.events) + } +} diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 4cecfcd..53b45d8 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -20,14 +20,14 @@ type mockGitHubClient struct { err error } -func (m *mockGitHubClient) GetCheckRuns(owner, repo, ref string) ([]github.CheckRun, error) { +func (m *mockGitHubClient) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) { if m.err != nil { return nil, m.err } return m.checkRuns[ref], nil } -func (m *mockGitHubClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { +func (m *mockGitHubClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { if m.err != nil { return nil, m.err } @@ -39,7 +39,7 @@ func (m *mockGitHubClient) GetPullRequest(owner, repo string, number int) (*gith return pr, nil } -func (m *mockGitHubClient) GetCombinedStatus(owner, repo, ref string) (*github.CombinedStatus, error) { +func (m *mockGitHubClient) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) { if m.err != nil { return nil, m.err } @@ -95,7 +95,7 @@ func TestWatcherNotificationError(t *testing.T) { w := New(client, cfg, notifier) // Should not return error, just print warning - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Errorf("checkPR should not fail when notification fails: %v", err) } } @@ -133,7 +133,7 @@ func TestWatcherNoStatusChange(t *testing.T) { w := New(client, cfg, notifier) // Check the PR once - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -175,7 +175,7 @@ func TestWatcherStatusChange(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -233,7 +233,7 @@ func TestWatcherFirstCheck(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -263,7 +263,7 @@ func TestWatcherGitHubError(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - _, err := w.checkPR(&cfg.WatchedPRs[0]) + _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err == nil { t.Error("expected error when GitHub API fails") } @@ -386,7 +386,7 @@ func TestWatcherUpdateTitle(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -425,7 +425,7 @@ func TestWatcherStatusError(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - _, err := w.checkPR(&cfg.WatchedPRs[0]) + _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err == nil { t.Error("expected error when status fetch fails") } @@ -459,7 +459,7 @@ func TestWatcherCheckAllPRs(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - w.checkAllPRs() + w.checkAllPRs(context.Background()) // Both PRs should be notified if len(notifier.events) != 2 { @@ -502,7 +502,7 @@ func TestWatcherCheckAllPRsWithError(t *testing.T) { w := New(client, cfg, notifier) // This should handle errors gracefully and print warnings - w.checkAllPRs() + w.checkAllPRs(context.Background()) // Only one PR should be successfully checked if len(notifier.events) != 1 { @@ -560,7 +560,7 @@ func TestWatcherNotificationFilterFailOnly(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -600,7 +600,7 @@ func TestWatcherNotificationFilterSuccessOnly(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } From 20e5b1b3dfefd95d0eef189f0e7c73b29f806075 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:37:36 -0300 Subject: [PATCH 07/10] fix: evict one ETag cache entry instead of clearing the whole cache When the 512-entry cache was full, every new 200 response wiped it. With more paths polled in rotation than fit (~170 PRs, fewer with paginated check runs) it was cleared over and over and nearly every request became unconditional, losing the free 304s. A new path now evicts a single random entry (refreshing a cached path evicts nothing). Random rather than LRU on purpose: cyclic polling is LRU's worst case. Simulated with 640 paths: 64% hits, was 0%. Addresses Codex review on #14. Co-Authored-By: Claude Opus 5.5 --- internal/github/client.go | 12 ++++++++- internal/github/efficiency_test.go | 42 ++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/internal/github/client.go b/internal/github/client.go index 2b6adf3..a5b9069 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -506,9 +506,19 @@ func (c *Client) getJSON(ctx context.Context, path string, v interface{}) error func (c *Client) storeCache(path string, entry cachedResponse) { c.mu.Lock() defer c.mu.Unlock() - if c.cache == nil || len(c.cache) >= maxCacheEntries { + if c.cache == nil { c.cache = make(map[string]cachedResponse) } + if _, exists := c.cache[path]; !exists && len(c.cache) >= maxCacheEntries { + // Make room by evicting one entry at random (map iteration order is random). + // Clearing the whole cache, or LRU, would make every request unconditional + // once more paths are polled in rotation than fit: polling cycles through + // the same paths, which is LRU's worst case. Random eviction keeps most hits. + for victim := range c.cache { + delete(c.cache, victim) + break + } + } c.cache[path] = entry } diff --git a/internal/github/efficiency_test.go b/internal/github/efficiency_test.go index 1fc6866..8c6ac79 100644 --- a/internal/github/efficiency_test.go +++ b/internal/github/efficiency_test.go @@ -280,3 +280,45 @@ func TestClient_ConcurrentRequestsAreSafe(t *testing.T) { } wg.Wait() } + +func TestStoreCache_FullCacheKeepsOtherEntries(t *testing.T) { + c := NewClient("") + for i := 0; i < maxCacheEntries; i++ { + c.storeCache(fmt.Sprintf("/p/%d", i), cachedResponse{etag: "e"}) + } + c.storeCache("/p/0", cachedResponse{etag: "refreshed"}) // a changed resource + if n := len(c.cache); n != maxCacheEntries || c.cache["/p/0"].etag != "refreshed" { + t.Errorf("refreshing a cached path must not evict anything: %d entries", n) + } + c.storeCache("/new", cachedResponse{etag: "e"}) + if n := len(c.cache); n != maxCacheEntries { + t.Errorf("a new path must evict exactly one entry, cache has %d", n) + } +} + +func TestStoreCache_PollingMorePathsThanFitStillHits(t *testing.T) { + // e.g. ~200 watched PRs, 3 paths each, polled in rotation. + c := NewClient("") + paths := maxCacheEntries + maxCacheEntries/4 + hits, lookups := 0, 0 + for round := 0; round < 20; round++ { + for i := 0; i < paths; i++ { + path := fmt.Sprintf("/p/%d", i) + if _, ok := c.cache[path]; ok { + if round > 0 { + hits++ + } + } else { + c.storeCache(path, cachedResponse{etag: "e"}) + } + if round > 0 { + lookups++ + } + } + } + rate := float64(hits) / float64(lookups) + t.Logf("hit rate %.0f%% polling %d paths through a %d-entry cache", rate*100, paths, maxCacheEntries) + if rate < 0.5 { + t.Errorf("hit rate %.0f%%; overflowing the cache must not make every request unconditional", rate*100) + } +} From 2614d38b1723496e2c29ac92ada246e9642ab8eb Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sun, 18 Oct 2026 02:38:01 -0300 Subject: [PATCH 08/10] docs: changelog, README and tasks for the Codex review fixes Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 +++++++ README.md | 2 +- TASKS.md | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc71faa..4a90e9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Integration tests against the real GitHub API (`make integration`, build tag `integration`), run nightly by a new `Integration` workflow. They fail against the old status-only logic. ### Fixed +- A check whose workflow ran more than once for the same commit (e.g. an older run cancelled by `concurrency: cancel-in-progress`) kept the PR failed even after the newer run passed. Only the newest run of each check counts now, like on GitHub. +- `prw wait` and the MCP `wait_for_ci` tool could return the final result of the previous commit when a new one was pushed during a poll. The PR head is now re-checked before returning, and waiting continues on the new commit. +- `prw wait --timeout`, the MCP `timeout_seconds` / cancellation and Ctrl-C now cancel the GitHub requests in flight; on a stalled network they used to be noticed only after the whole poll finished (up to 15 seconds per request). `prw run` also stops at once on Ctrl-C. +- PRs auto-watched with `--review-requested` that were merged or closed while `prw` wasn't running are no longer dropped as withdrawn review requests: they get their final `merged`/`closed` notification first. +- A `no_ci` PR no longer flaps to `pending` (with a "no_ci → pending" notification) when someone comments or changes a label; it stays `no_ci` until an actual check or status appears. +- The ETag cache no longer clears itself entirely when full, which made almost every request unconditional when watching many PRs; it evicts one entry instead. +- GovSlack webhook URLs (`hooks.slack-gov.com`) are detected as Slack. - PRs without any CI are reported as `no_ci` instead of `pending` forever. Previously `prw wait` and the MCP `wait_for_ci` tool blocked until their timeout on such PRs (repositories without CI, or stacked PRs whose base branch runs none). A PR is `no_ci` once it has no checks or statuses at all and hasn't been updated for 3 minutes, so CI that is still registering after a push isn't mistaken for missing CI. `prw wait` exits `4` for it; `prw run` doesn't notify about it. - PR owner and repository names are compared case-insensitively (as on GitHub), so `DevBlac/PRW#5` and `devblac/prw#5` are no longer two separate watch-list entries. - Requests without a token no longer send an empty `Authorization: Bearer ` header, which GitHub rejects with 401 even for public repositories. diff --git a/README.md b/README.md index f54da96..739db12 100644 --- a/README.md +++ b/README.md @@ -387,7 +387,7 @@ If you configure a `webhook_url`, `prw` will POST to it on every status change. | URL | Format | What you get | |-----|--------|--------------| -| `https://hooks.slack.com/...` | `slack` | A message with the PR link, status transition and failed checks | +| `https://hooks.slack.com/...` (or GovSlack `hooks.slack-gov.com`) | `slack` | A message with the PR link, status transition and failed checks | | `https://discord.com/api/webhooks/...` | `discord` | A colored embed (green / red / yellow) with the same details | | anything else | `generic` | Raw JSON (below), for your own services | diff --git a/TASKS.md b/TASKS.md index 80ba30e..6c34e5a 100644 --- a/TASKS.md +++ b/TASKS.md @@ -90,6 +90,7 @@ Legend — Impact: 1–5 · Effort: S (≤½ day), M (1–2 days), L (3+ days) - ✅ #10 `prw watch --mine` / `--review-requested` — done (search-backed subscriptions refreshed every 5 min by `prw run`; verified live: 18 open PRs across 7 repos found, CI checked, cleanly removed by `unwatch --mine`) - 🆕 #18 **Report "no CI" instead of `pending` forever** (found while testing #10: 14 of 18 real PRs have zero checks — repos without CI, or stacked PRs whose base isn't `main`). Today `prw wait` / `wait_for_ci` would wait until the timeout on them. Needs a grace period after the last push (CI takes a moment to register checks) before concluding "no CI". Impact 4, effort S–M. - ✅ #18 `no_ci` state — done (0 checks + PR not updated for 3 min → `no_ci`; `prw wait` exits 4; no notification; verified live: 11 real PRs went from `pending` to `no_ci`, `prw wait` on one returns in 2s instead of the 30-min timeout) +- ✅ Codex review feedback on #6, #7, #13–#17 — done in `fix/codex-review`: superseded check runs (verified live on zephyrproject-rtos/zephyr#105183: was `failure`, now `success`), head re-check in `prw wait`, context propagation into every API request, closed review-request PRs keep their notification, no `no_ci` flapping, ETag cache eviction, GovSlack. - Next: #11 review & mergeability events ## Planned From 8f217bd24153e1e89a25c2f9fd6bd6a8cd430454 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sat, 24 Oct 2026 15:34:23 -0300 Subject: [PATCH 09/10] fix: don't take a failed head re-check as "head unchanged" If the request confirming the PR head failed, `prw wait` returned the final result of the commit it had polled, even if a push had just landed, and a timeout or Ctrl-C during that request ended as success/failure instead of timed out / interrupted. Its errors now go through the same path as poll errors: retried when transient, reported as timeout or interruption when the context is done. Addresses Codex review on #18. Co-Authored-By: Claude Opus 5.5 --- cmd/prw/wait.go | 27 ++++++++++++++++++--------- cmd/prw/wait_ctx_test.go | 38 ++++++++++++++++++++++++++++++++++++++ cmd/prw/wait_test.go | 7 +++++++ 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/cmd/prw/wait.go b/cmd/prw/wait.go index 77edee6..32895a0 100644 --- a/cmd/prw/wait.go +++ b/cmd/prw/wait.go @@ -217,6 +217,12 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number for { delay := interval state, err := pollOnce(ctx, client, result) + moved := false + if err == nil && state != "pending" { + // Errors confirming the head are handled like poll errors: retried, or + // reported as a timeout / interruption, never taken as "head unchanged". + moved, err = headMoved(ctx, client, result) + } if err != nil && ctx.Err() != nil { return stopped(), nil // the timeout or Ctrl-C cancelled the poll's requests } @@ -239,11 +245,11 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number fmt.Fprintf(progress, "⚠️ %v (retrying)\n", err) } else { consecutiveErrors = 0 + if moved { + fmt.Fprintf(progress, "↻ %s/%s#%d: new commit pushed; waiting for its CI\n", owner, repo, number) + continue + } if state != "pending" { - if headMoved(ctx, client, result) { - fmt.Fprintf(progress, "↻ %s/%s#%d: new commit pushed; waiting for its CI\n", owner, repo, number) - continue - } return finish(), nil } line := fmt.Sprintf("⏳ %s/%s#%d: %s", owner, repo, number, pendingSummary(result)) @@ -262,14 +268,17 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number // headMoved reports whether a new commit was pushed to the PR since result was // polled. The CI lookup follows the PR lookup, so a push in between would otherwise -// return the result of the previous commit. Merged or closed PRs don't move; if the -// check fails, the result stands. Unchanged PRs answer 304, so this costs no quota. -func headMoved(ctx context.Context, client ciClient, result *waitResult) bool { +// return the result of the previous commit. Merged or closed PRs don't move. +// Unchanged PRs answer 304, so this costs no quota. +func headMoved(ctx context.Context, client ciClient, result *waitResult) (bool, error) { if result.State == "merged" || result.State == "closed" { - return false + return false, nil } pr, err := client.GetPullRequest(ctx, result.Owner, result.Repo, result.Number) - return err == nil && pr.Head.SHA != result.SHA + if err != nil { + return false, fmt.Errorf("failed to confirm the PR head: %w", err) + } + return pr.Head.SHA != result.SHA, nil } // pollOnce refreshes result from GitHub and returns the current state. diff --git a/cmd/prw/wait_ctx_test.go b/cmd/prw/wait_ctx_test.go index fe67c12..e2db5a7 100644 --- a/cmd/prw/wait_ctx_test.go +++ b/cmd/prw/wait_ctx_test.go @@ -3,6 +3,7 @@ package main import ( "bytes" "context" + "errors" "net/http" "net/http/httptest" "testing" @@ -44,6 +45,43 @@ func TestWaitForCI_ConfirmsHeadBeforeReturning(t *testing.T) { } } +func TestWaitForCI_RetriesWhenTheHeadCheckFails(t *testing.T) { + // A push landed during the poll, and the request that would notice it failed: the + // old commit's result must not be returned. + noSleep(t) + client := &scriptedClient{steps: []pollStep{ + {pr: openPR("old"), runs: []github.CheckRun{run("Test", "completed", "success")}, + revalidate: func() error { return errors.New("connection reset") }}, + {pr: openPR("new"), runs: []github.CheckRun{run("Test", "completed", "failure")}}, + }} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.SHA != "new" || result.State != "failure" { + t.Errorf("got %s on %s, want the failure of the new head", result.State, result.SHA) + } +} + +func TestWaitForCI_InterruptDuringTheHeadCheck(t *testing.T) { + noSleep(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + client := &scriptedClient{steps: []pollStep{{ + pr: openPR("sha"), runs: []github.CheckRun{run("Test", "completed", "success")}, + revalidate: func() error { cancel(); return ctx.Err() }, // Ctrl-C while it's in flight + }}} + + result, err := waitForCI(ctx, client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if !result.Interrupted || exitCode(result.exitErr()) != exitWaitInterrupted { + t.Errorf("result = %+v, want interrupted (exit 130), not success", result) + } +} + // hangingClient is a real client against a server that never answers until the // request is cancelled, like a stalled network. func hangingClient(t *testing.T) *github.Client { diff --git a/cmd/prw/wait_test.go b/cmd/prw/wait_test.go index 845e346..c3da461 100644 --- a/cmd/prw/wait_test.go +++ b/cmd/prw/wait_test.go @@ -28,6 +28,8 @@ type pollStep struct { // movedTo, if set, is what the PR looks like when waitForCI re-reads it to // confirm the head hasn't moved, i.e. a push that landed during this poll. movedTo *github.PullRequest + // revalidate, if set, runs on that re-read and its error is returned. + revalidate func() error } // scriptedClient returns steps[i] on the i-th poll (repeating the last one). A PR @@ -52,6 +54,11 @@ func (c *scriptedClient) GetPullRequest(ctx context.Context, owner, repo string, github.ApplyNoCI(ci, c.current.pr.UpdatedAt, time.Now()) if ci.State != "pending" { c.revalidations++ + if c.current.revalidate != nil { + if err := c.current.revalidate(); err != nil { + return nil, err + } + } if c.current.movedTo != nil { return c.current.movedTo, nil } From 3ab10ed5b67e66dbb0f253a100e5fc2f1e675ad6 Mon Sep 17 00:00:00 2001 From: Julian Blacher Date: Sat, 24 Oct 2026 15:36:24 -0300 Subject: [PATCH 10/10] fix: only drop review-request PRs once they're proven withdrawn A review-request PR that stopped matching its search was dropped whenever the lookup meant to tell "merged/closed" apart from "request withdrawn" failed. Interrupting `prw run` (Ctrl-C) during that lookup, a network error or a rate limit therefore silently removed PRs together with their final merged/closed notification. Now a PR is only dropped when it's confirmed open (the request was withdrawn or answered) or GitHub answers 404; otherwise it's kept until the next sync. A cancelled sync returns before touching the config. Addresses Codex review on #18. Co-Authored-By: Claude Opus 5.5 --- internal/watcher/autowatch.go | 41 ++++++++++----- internal/watcher/autowatch_test.go | 83 ++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 12 deletions(-) diff --git a/internal/watcher/autowatch.go b/internal/watcher/autowatch.go index ed9955b..bf53781 100644 --- a/internal/watcher/autowatch.go +++ b/internal/watcher/autowatch.go @@ -2,7 +2,9 @@ package watcher import ( "context" + "errors" "fmt" + "net/http" "strings" "time" @@ -49,7 +51,8 @@ type SyncResult struct { // closed are kept instead (for "mine" that's always the reason), so that `prw run` // still notifies about it before dropping them. PRs watched by hand are never touched. // -// It returns an error only if every search failed; other problems are Warnings. +// It returns an error if every search failed or ctx was cancelled (then nothing is +// changed); other problems are Warnings. func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*SyncResult, error) { result := &SyncResult{} found := map[string]*github.SearchResult{} @@ -76,7 +79,10 @@ func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*Syn if len(found) == 0 && lastErr != nil { return result, lastErr } - closed := closedReviewRequests(ctx, client, found) + withdrawn := withdrawnReviewRequests(ctx, client, found) + if err := ctx.Err(); err != nil { + return result, err // interrupted: lookups that didn't finish prove nothing + } err := config.Update(func(c *config.Config) error { changed := false @@ -102,7 +108,7 @@ func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*Syn kept := c.WatchedPRs[:0] for _, pr := range c.WatchedPRs { key := prKey(pr.Owner, pr.Repo, pr.Number) - if pr.Source == source && !matches[key] && !closed[key] { + if pr.Source == source && !matches[key] && withdrawn[key] { result.Removed = append(result.Removed, pr) changed = true continue @@ -122,11 +128,14 @@ func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*Syn return result, nil } -// closedReviewRequests finds the review-request PRs that no longer match their search -// because they were merged or closed: the search only returns open PRs, so they look -// just like withdrawn requests. It runs before the config is locked, as it makes API -// calls. A PR whose lookup fails is not reported, i.e. it's removed as before. -func closedReviewRequests(ctx context.Context, client Searcher, found map[string]*github.SearchResult) map[string]bool { +// withdrawnReviewRequests finds the review-request PRs that can be dropped: they no +// longer match their search and are still open (so the request was withdrawn or +// answered), or are gone (404). The search only returns open PRs, so a merged or +// closed PR looks the same until it's looked up; those are kept for the regular check +// to notify about. A failed lookup (network, rate limit, cancellation) proves nothing, +// so that PR is kept until the next sync. It runs before the config is locked, as it +// makes API calls. +func withdrawnReviewRequests(ctx context.Context, client Searcher, found map[string]*github.SearchResult) map[string]bool { res, ok := found[config.AutoWatchReviewRequested] if !ok || res.Incomplete { return nil // nothing will be removed @@ -139,17 +148,22 @@ func closedReviewRequests(ctx context.Context, client Searcher, found map[string for _, pr := range res.PRs { matches[prKey(pr.Owner, pr.Repo, pr.Number)] = true } - closed := map[string]bool{} + withdrawn := map[string]bool{} for _, pr := range cfg.WatchedPRs { key := prKey(pr.Owner, pr.Repo, pr.Number) if pr.Source != config.AutoWatchReviewRequested || matches[key] { continue } - if ghPR, err := client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number); err == nil && ghPR.IsClosed() { - closed[key] = true + ghPR, err := client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number) + var apiErr *github.APIError + switch { + case err == nil: + withdrawn[key] = !ghPR.IsClosed() + case errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound: + withdrawn[key] = true // deleted, or no longer visible to us } } - return closed + return withdrawn } func prKey(owner, repo string, number int) string { @@ -172,6 +186,9 @@ func (w *Watcher) syncAutoWatch(ctx context.Context) { return } res, err := SyncAutoWatch(ctx, searcher, sources) + if ctx.Err() != nil { + return // stopping; the failed searches and lookups aren't worth reporting + } for _, warning := range res.Warnings { fmt.Printf("Warning: auto-watch: %s\n", warning) } diff --git a/internal/watcher/autowatch_test.go b/internal/watcher/autowatch_test.go index a2b23c1..d08bf4e 100644 --- a/internal/watcher/autowatch_test.go +++ b/internal/watcher/autowatch_test.go @@ -115,6 +115,7 @@ func TestSyncAutoWatch_DropsWithdrawnReviewRequestsButKeepsMine(t *testing.T) { config.AutoWatchMine: found(), config.AutoWatchReviewRequested: found(21), }} + s.prs = map[string]*github.PullRequest{"owner/repo/20": openReviewPR(20)} res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { @@ -329,3 +330,85 @@ func TestRunOnce_NotifiesReviewRequestMergedWhileStopped(t *testing.T) { t.Errorf("watch list = %q, want the merged PR dropped after notifying", got) } } + +func openReviewPR(number int) *github.PullRequest { + pr := newPR(number, fmt.Sprintf("sha%d", number)) + pr.State = "open" + return pr +} + +// failingLookups answers GetPullRequest with errs (keyed like prs) for the given PRs. +type failingLookups struct { + *fakeSearcher + errs map[string]error +} + +func (f *failingLookups) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { + if err := f.errs[fmt.Sprintf("%s/%s/%d", owner, repo, number)]; err != nil { + return nil, err + } + return f.fakeSearcher.GetPullRequest(ctx, owner, repo, number) +} + +func TestSyncAutoWatch_RemovesReviewRequestsOnlyWhenProvenWithdrawn(t *testing.T) { + useTempConfig(t) + reviewPR := func(n int) config.WatchedPR { + return config.WatchedPR{Owner: "owner", Repo: "repo", Number: n, Source: config.AutoWatchReviewRequested} + } + saveConfig(t, &config.Config{ + AutoWatch: []string{config.AutoWatchReviewRequested}, + WatchedPRs: []config.WatchedPR{reviewPR(1), reviewPR(2), reviewPR(3)}, + }) + s := &failingLookups{ + fakeSearcher: &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: found()}}, + errs: map[string]error{ + "owner/repo/2": &github.APIError{StatusCode: 404}, // deleted or no access + "owner/repo/3": errors.New("connection reset"), + }, + } + s.prs = map[string]*github.PullRequest{"owner/repo/1": openReviewPR(1)} + + if _, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchReviewRequested}); err != nil { + t.Fatal(err) + } + if got := watchList(t); got != "3:review-requested" { + t.Errorf("watch list = %q, want #1 (open) and #2 (404) dropped, #3 (lookup failed) kept", got) + } +} + +func TestSyncAutoWatch_InterruptedDuringLookupsChangesNothing(t *testing.T) { + useTempConfig(t) + saveConfig(t, &config.Config{ + AutoWatch: []string{config.AutoWatchReviewRequested}, + WatchedPRs: []config.WatchedPR{ + {Owner: "owner", Repo: "repo", Number: 1, Source: config.AutoWatchReviewRequested}, + {Owner: "owner", Repo: "repo", Number: 2, Source: config.AutoWatchReviewRequested}, + }, + }) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + s := &failingLookups{ + fakeSearcher: &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: found(9)}}, + } + s.prs = map[string]*github.PullRequest{"owner/repo/1": openReviewPR(1)} + s.errs = map[string]error{"owner/repo/2": context.Canceled} + cancelOnLookup := &cancellingSearcher{failingLookups: s, cancel: cancel} + + if _, err := SyncAutoWatch(ctx, cancelOnLookup, []string{config.AutoWatchReviewRequested}); !errors.Is(err, context.Canceled) { + t.Fatalf("err = %v, want context.Canceled", err) + } + if got := watchList(t); got != "1:review-requested 2:review-requested" { + t.Errorf("watch list = %q, want it untouched (no removals, no additions)", got) + } +} + +// cancellingSearcher cancels the context (Ctrl-C) when the first PR lookup starts. +type cancellingSearcher struct { + *failingLookups + cancel context.CancelFunc +} + +func (c *cancellingSearcher) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { + c.cancel() + return c.failingLookups.GetPullRequest(ctx, owner, repo, number) +}