diff --git a/backend/spi_api.go b/backend/spi_api.go index b44f996..0bef731 100644 --- a/backend/spi_api.go +++ b/backend/spi_api.go @@ -2,12 +2,14 @@ package main import ( "encoding/json" + "errors" "fmt" "net/url" "strconv" "strings" "time" + plugin "github.com/Paca-AI/plugin-sdk-go" gp "github.com/Paca-AI/plugin-sdk-go/gitprovider" ) @@ -67,7 +69,7 @@ func (a *ghAPI) doRaw(method, path string, body any, accept string) (int, string } resp, err := fetch(method, rawURL, h, payload) if err != nil { - return 0, "", nil, gp.Errorf(gp.CodeTransient, "GitHub is unreachable") + return 0, "", nil, fetchFailure(err) } if resp.Status == 401 && a.refresh != nil { refresh := a.refresh @@ -81,7 +83,7 @@ func (a *ghAPI) doRaw(method, path string, body any, accept string) (int, string a.token = tok h["Authorization"] = "Bearer " + tok if resp, err = fetch(method, rawURL, h, payload); err != nil { - return 0, "", nil, gp.Errorf(gp.CodeTransient, "GitHub is unreachable") + return 0, "", nil, fetchFailure(err) } } if resp.Status >= 400 { @@ -90,6 +92,16 @@ func (a *ghAPI) doRaw(method, path string, body any, accept string) (int, string return resp.Status, resp.Body, resp.Headers, nil } +// fetchFailure maps a failed host fetch to a *gp.Error. A response too large +// for the plugin to receive is a size problem, not an outage, so it must not +// be reported (or retried) as "GitHub is unreachable". +func fetchFailure(err error) error { + if errors.Is(err, plugin.ErrResponseTooLarge) { + return gp.Errorf(gp.CodeInternal, "GitHub's response was too large for the plugin to receive") + } + return gp.Errorf(gp.CodeTransient, "GitHub is unreachable") +} + func (a *ghAPI) get(path string, out any) error { text, err := a.do("GET", path, nil, "") if err != nil { diff --git a/backend/spi_issues_test.go b/backend/spi_issues_test.go index 8f1a398..5c2f289 100644 --- a/backend/spi_issues_test.go +++ b/backend/spi_issues_test.go @@ -2,7 +2,10 @@ package main import ( "encoding/json" + "errors" + "fmt" "net/url" + "strings" "testing" plugin "github.com/Paca-AI/plugin-sdk-go" @@ -11,6 +14,16 @@ import ( func TestListIssuesFiltersPullRequestsWithoutLosingPages(t *testing.T) { p, g, _ := seededProvider(t) + // Page 1 is a full page (100 rows) holding a single real issue among pull + // requests; page 2 is short, so it is the last. + fullPage := func() []map[string]any { + rows := make([]map[string]any, listPerPage) + for i := range rows { + rows[i] = map[string]any{"number": i + 1, "pull_request": map[string]any{"url": "x"}} + } + rows[0] = map[string]any{"number": 101, "title": "Bug", "body": "Details", "state": "closed", "html_url": "https://github.com/acme/web/issues/101", "user": map[string]string{"login": "alice"}, "labels": []map[string]string{{"name": "bug"}}, "assignees": []map[string]string{{"login": "bob"}}} + return rows + } fetch = func(method, rawURL string, headers map[string]string, body string) (*plugin.FetchResponse, error) { u, err := url.Parse(rawURL) if err != nil { @@ -19,39 +32,56 @@ func TestListIssuesFiltersPullRequestsWithoutLosingPages(t *testing.T) { if u.Path != "/repos/acme/web/issues" { return g.fetch(method, rawURL, headers, body) } - if method != "GET" || u.Query().Get("state") != "closed" || u.Query().Get("per_page") != "1" { + if method != "GET" || u.Query().Get("state") != "closed" || u.Query().Get("per_page") != "100" { t.Fatalf("unexpected request: %s %s", method, rawURL) } switch u.Query().Get("page") { case "1": - return resp(200, []map[string]any{{"number": 1, "pull_request": map[string]any{"url": "x"}}}) + return resp(200, fullPage()) case "2": - return resp(200, []map[string]any{{"number": 101, "title": "Bug", "body": "Details", "state": "closed", "html_url": "https://github.com/acme/web/issues/101", "user": map[string]string{"login": "alice"}, "labels": []map[string]string{{"name": "bug"}}, "assignees": []map[string]string{{"login": "bob"}}}}) - case "3": - return resp(200, []map[string]any{}) + return resp(200, []map[string]any{{"number": 202, "pull_request": map[string]any{"url": "x"}}}) default: t.Fatalf("unexpected page: %s", rawURL) return nil, nil } } first, err := p.ListIssues(bg, gp.ListIssuesRequest{Target: webTarget(), State: "closed"}) - if err != nil || len(first.Items) != 0 || first.NextCursor != "2" { + if err != nil || len(first.Items) != 1 || first.NextCursor != "2" { t.Fatalf("first page: %+v, %v", first, err) } second, err := p.ListIssues(bg, gp.ListIssuesRequest{Target: webTarget(), State: "closed", Cursor: first.NextCursor}) - if err != nil || len(second.Items) != 1 || second.NextCursor != "3" { + if err != nil || len(second.Items) != 0 || second.NextCursor != "" { t.Fatalf("second page: %+v, %v", second, err) } - third, err := p.ListIssues(bg, gp.ListIssuesRequest{Target: webTarget(), State: "closed", Cursor: second.NextCursor}) - if err != nil || len(third.Items) != 0 || third.NextCursor != "" { - t.Fatalf("third page: %+v, %v", third, err) - } - i := second.Items[0] + i := first.Items[0] if i.Number != "101" || i.Title != "Bug" || i.Body != "Details" || i.State != "closed" || i.Author != "alice" || i.URL != "https://github.com/acme/web/issues/101" || len(i.Labels) != 1 || i.Labels[0] != "bug" || len(i.Assignees) != 1 || i.Assignees[0] != "bob" { t.Fatalf("issue fields: %+v", i) } } +// A response the SDK could not deliver is a size problem; it must not be +// reported as GitHub being unreachable (which core would treat as retryable). +func TestFetchTooLargeIsNotReportedAsUnreachable(t *testing.T) { + p, g, _ := seededProvider(t) + fetch = func(method, rawURL string, headers map[string]string, body string) (*plugin.FetchResponse, error) { + if u, _ := url.Parse(rawURL); u.Path == "/repos/acme/web/issues" { + return nil, fmt.Errorf("plugin: fetch: %w: too big", plugin.ErrResponseTooLarge) + } + return g.fetch(method, rawURL, headers, body) + } + _, err := p.ListIssues(bg, gp.ListIssuesRequest{Target: webTarget()}) + if codeOf(t, err) != gp.CodeInternal || strings.Contains(err.Error(), "unreachable") { + t.Fatalf("want a non-retryable size error, got: %v", err) + } + fetch = func(string, string, map[string]string, string) (*plugin.FetchResponse, error) { + return nil, errors.New("dial failed") + } + _, err = p.ListIssues(bg, gp.ListIssuesRequest{Target: webTarget()}) + if codeOf(t, err) != gp.CodeTransient { + t.Fatalf("a genuine transport failure stays transient, got: %v", err) + } +} + func TestListIssuesValidatesAndPropagatesErrors(t *testing.T) { p, g, _ := seededProvider(t) for _, req := range []gp.ListIssuesRequest{ diff --git a/backend/spi_reads.go b/backend/spi_reads.go index 73b4fc8..ef218f1 100644 --- a/backend/spi_reads.go +++ b/backend/spi_reads.go @@ -419,14 +419,12 @@ func (p *ghProvider) ListIssues(_ context.Context, req gp.ListIssuesRequest) (gp return gp.IssuePage{}, err } var issues []ghIssue - // shortcut: one large response can still exceed the WASM arena; upgrade the SDK transport limit handling. - if err := api.get(repo+"/issues?state="+state+"&per_page=1&page="+strconv.Itoa(pg), &issues); err != nil { + if err := api.get(repo+"/issues?state="+state+"&per_page="+strconv.Itoa(listPerPage)+"&page="+strconv.Itoa(pg), &issues); err != nil { return gp.IssuePage{}, err } - out := gp.IssuePage{Items: []gp.Issue{}} - if len(issues) > 0 { - out.NextCursor = strconv.Itoa(pg + 1) - } + // GitHub's issues endpoint also returns pull requests, so the cursor must + // follow the raw page size, not the filtered item count. + out := gp.IssuePage{Items: []gp.Issue{}, NextCursor: nextPage(len(issues), pg)} for _, issue := range issues { if len(issue.PullRequest) == 0 || string(issue.PullRequest) == "null" { out.Items = append(out.Items, issue.toIssue())