Skip to content

Commit 7b08c81

Browse files
authored
feat(github): autopilot parity — typed RateLimitError/AuthError, GetOpenSubIssueNumbers, failed-retry labels, exported ParseParentIssueNumber (#77)
Gaps found while wiring Pilot M7 4d (autopilot cutover to the SDK client): - doRequest now returns *RateLimitError on 429 AND header/message-classified 403 rate limits (with parsed Retry-After/X-RateLimit-Reset), and *AuthError on 401 — hosts use errors.As on these (ported verbatim from the pre- extraction client). Retry layer gains typed fast paths (403-rate-limits are now correctly retryable; AuthError is not; RetryAfter honored without string parsing). - GetOpenSubIssueNumbers: list-returning sibling of GetOpenSubIssueCount — hosts cross-check each open child, a count is insufficient. - LabelPilot + pilot-failed-retry-* constants + FailedRetryStateLabels. - ParseParentIssueNumber exported (delegates to the poller-internal parser). sdk/core untouched; api.golden byte-identical.
1 parent b86a7fd commit 7b08c81

6 files changed

Lines changed: 351 additions & 1 deletion

File tree

sdk/integrations/github/client.go

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,31 @@ func (c *Client) doRequest(ctx context.Context, method, path string, body interf
203203
}
204204

205205
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
206-
return fmt.Errorf("API error (status %d): %s", resp.StatusCode, string(respBody))
206+
msg := string(respBody)
207+
if resp.StatusCode == http.StatusUnauthorized {
208+
return &AuthError{Message: msg}
209+
}
210+
if resp.StatusCode == http.StatusTooManyRequests {
211+
return &RateLimitError{
212+
StatusCode: http.StatusTooManyRequests,
213+
RetryAfter: parseRetryAfterHeader(resp.Header),
214+
Message: msg,
215+
}
216+
}
217+
if resp.StatusCode == http.StatusForbidden {
218+
msgLower := strings.ToLower(msg)
219+
isRateLimit := resp.Header.Get("X-RateLimit-Remaining") == "0" ||
220+
strings.Contains(msgLower, "secondary rate limit") ||
221+
strings.Contains(msgLower, "rate limit exceeded")
222+
if isRateLimit {
223+
return &RateLimitError{
224+
StatusCode: http.StatusForbidden,
225+
RetryAfter: parseRetryAfterHeader(resp.Header),
226+
Message: msg,
227+
}
228+
}
229+
}
230+
return fmt.Errorf("API error (status %d): %s", resp.StatusCode, msg)
207231
}
208232

209233
if result != nil && len(respBody) > 0 {
@@ -1128,6 +1152,58 @@ func (c *Client) LinkSubIssue(ctx context.Context, owner, repo string, parentNum
11281152
return c.ExecuteGraphQL(ctx, mutation, variables, nil)
11291153
}
11301154

1155+
// GetOpenSubIssueNumbers queries native GitHub sub-issues for a parent issue
1156+
// and returns the numbers of sub-issues in OPEN state plus whether the parent
1157+
// has any native sub-issue links at all. Callers that need to cross-check each
1158+
// open child individually (rather than just count them) use this variant.
1159+
func (c *Client) GetOpenSubIssueNumbers(ctx context.Context, owner, repo string, parentNum int) (numbers []int, hasNativeLinks bool, err error) {
1160+
parentID, err := c.GetIssueNodeID(ctx, owner, repo, parentNum)
1161+
if err != nil {
1162+
return nil, false, fmt.Errorf("resolve parent node ID: %w", err)
1163+
}
1164+
1165+
const query = `query($issueID: ID!) {
1166+
node(id: $issueID) {
1167+
... on Issue {
1168+
subIssues(first: 100) {
1169+
totalCount
1170+
nodes {
1171+
number
1172+
state
1173+
}
1174+
}
1175+
}
1176+
}
1177+
}`
1178+
1179+
var result struct {
1180+
Node struct {
1181+
SubIssues struct {
1182+
TotalCount int `json:"totalCount"`
1183+
Nodes []struct {
1184+
Number int `json:"number"`
1185+
State string `json:"state"`
1186+
} `json:"nodes"`
1187+
} `json:"subIssues"`
1188+
} `json:"node"`
1189+
}
1190+
1191+
if err := c.ExecuteGraphQL(ctx, query, map[string]interface{}{"issueID": parentID}, &result); err != nil {
1192+
return nil, false, fmt.Errorf("query sub-issues for %s/%s#%d: %w", owner, repo, parentNum, err)
1193+
}
1194+
1195+
if result.Node.SubIssues.TotalCount == 0 {
1196+
return nil, false, nil
1197+
}
1198+
1199+
for _, n := range result.Node.SubIssues.Nodes {
1200+
if n.State == "OPEN" {
1201+
numbers = append(numbers, n.Number)
1202+
}
1203+
}
1204+
return numbers, true, nil
1205+
}
1206+
11311207
// GetOpenSubIssueCount queries native GitHub sub-issues for a parent issue and returns:
11321208
// - count: number of sub-issues in OPEN state
11331209
// - hasNativeLinks: true when the parent has at least one native sub-issue link

sdk/integrations/github/errors.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
package github
2+
3+
import (
4+
"fmt"
5+
"net/http"
6+
"strconv"
7+
"time"
8+
)
9+
10+
// RateLimitError is returned by doRequest when GitHub signals a rate limit via
11+
// a 403 or 429 response. It carries the parsed Retry-After duration so the
12+
// retry loop can honor it without regexing the error string.
13+
type RateLimitError struct {
14+
StatusCode int
15+
RetryAfter time.Duration
16+
Message string
17+
}
18+
19+
func (e *RateLimitError) Error() string {
20+
return fmt.Sprintf("API error (status %d): %s", e.StatusCode, e.Message)
21+
}
22+
23+
// AuthError is returned by doRequest when GitHub rejects the token itself
24+
// (401 Unauthorized) — as opposed to a 403 which can mean rate-limited or
25+
// forbidden-but-authenticated. Callers use errors.As to distinguish a dead/
26+
// revoked token from other failures.
27+
type AuthError struct {
28+
Message string
29+
}
30+
31+
func (e *AuthError) Error() string {
32+
return fmt.Sprintf("API error (status %d): %s", http.StatusUnauthorized, e.Message)
33+
}
34+
35+
// parseRetryAfterHeader reads Retry-After and X-RateLimit-Reset headers and
36+
// returns the delay duration. Returns 0 when neither header is present.
37+
func parseRetryAfterHeader(h http.Header) time.Duration {
38+
if v := h.Get("Retry-After"); v != "" {
39+
if secs, err := strconv.Atoi(v); err == nil && secs > 0 {
40+
return time.Duration(secs) * time.Second
41+
}
42+
}
43+
if v := h.Get("X-RateLimit-Reset"); v != "" {
44+
if unix, err := strconv.ParseInt(v, 10, 64); err == nil {
45+
if d := time.Until(time.Unix(unix, 0)); d > 0 {
46+
return d
47+
}
48+
}
49+
}
50+
return 0
51+
}
Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
package github
2+
3+
import (
4+
"context"
5+
"errors"
6+
"net/http"
7+
"net/http/httptest"
8+
"strconv"
9+
"testing"
10+
"time"
11+
12+
"github.com/qf-studio/studio-sdk/sdk/testutil"
13+
)
14+
15+
func newErrClient(t *testing.T, status int, body string, headers map[string]string) *Client {
16+
t.Helper()
17+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
18+
for k, v := range headers {
19+
w.Header().Set(k, v)
20+
}
21+
w.WriteHeader(status)
22+
_, _ = w.Write([]byte(body))
23+
}))
24+
t.Cleanup(srv.Close)
25+
return NewClientWithBaseURL(testutil.FakeGitHubToken, srv.URL)
26+
}
27+
28+
func TestDoRequest_429_ReturnsTypedRateLimitError(t *testing.T) {
29+
c := newErrClient(t, http.StatusTooManyRequests, `{"message":"API rate limit exceeded"}`,
30+
map[string]string{"Retry-After": "42"})
31+
32+
_, err := c.GetIssue(context.Background(), "o", "r", 1)
33+
if err == nil {
34+
t.Fatal("expected error")
35+
}
36+
var rlErr *RateLimitError
37+
if !errors.As(err, &rlErr) {
38+
t.Fatalf("error is %T, want *RateLimitError (errors.As must match for autopilot handling)", err)
39+
}
40+
if rlErr.StatusCode != http.StatusTooManyRequests {
41+
t.Errorf("StatusCode = %d, want 429", rlErr.StatusCode)
42+
}
43+
if rlErr.RetryAfter != 42*time.Second {
44+
t.Errorf("RetryAfter = %v, want 42s", rlErr.RetryAfter)
45+
}
46+
}
47+
48+
func TestDoRequest_403RateLimit_ReturnsTypedRateLimitError(t *testing.T) {
49+
tests := []struct {
50+
name string
51+
body string
52+
headers map[string]string
53+
}{
54+
{
55+
name: "remaining zero header",
56+
body: `{"message":"forbidden"}`,
57+
headers: map[string]string{"X-RateLimit-Remaining": "0", "X-RateLimit-Reset": strconv.FormatInt(time.Now().Add(90*time.Second).Unix(), 10)},
58+
},
59+
{
60+
name: "secondary rate limit message",
61+
body: `{"message":"You have exceeded a secondary rate limit"}`,
62+
},
63+
}
64+
for _, tt := range tests {
65+
t.Run(tt.name, func(t *testing.T) {
66+
c := newErrClient(t, http.StatusForbidden, tt.body, tt.headers)
67+
_, err := c.GetIssue(context.Background(), "o", "r", 1)
68+
var rlErr *RateLimitError
69+
if !errors.As(err, &rlErr) {
70+
t.Fatalf("error is %T, want *RateLimitError", err)
71+
}
72+
if rlErr.StatusCode != http.StatusForbidden {
73+
t.Errorf("StatusCode = %d, want 403", rlErr.StatusCode)
74+
}
75+
})
76+
}
77+
}
78+
79+
func TestDoRequest_403Plain_IsNotRateLimitError(t *testing.T) {
80+
c := newErrClient(t, http.StatusForbidden, `{"message":"Resource not accessible by integration"}`, nil)
81+
_, err := c.GetIssue(context.Background(), "o", "r", 1)
82+
if err == nil {
83+
t.Fatal("expected error")
84+
}
85+
var rlErr *RateLimitError
86+
if errors.As(err, &rlErr) {
87+
t.Error("plain 403 must NOT be classified as a rate limit")
88+
}
89+
}
90+
91+
func TestDoRequest_401_ReturnsTypedAuthError(t *testing.T) {
92+
c := newErrClient(t, http.StatusUnauthorized, `{"message":"Bad credentials"}`, nil)
93+
_, err := c.GetIssue(context.Background(), "o", "r", 1)
94+
var authErr *AuthError
95+
if !errors.As(err, &authErr) {
96+
t.Fatalf("error is %T, want *AuthError", err)
97+
}
98+
}
99+
100+
func TestIsRetryable_TypedErrors(t *testing.T) {
101+
if !isRetryableError(&RateLimitError{StatusCode: 403, Message: "secondary rate limit"}) {
102+
t.Error("403-classified RateLimitError must be retryable (string path would miss it)")
103+
}
104+
if isRetryableError(&AuthError{Message: "Bad credentials"}) {
105+
t.Error("AuthError must not be retryable")
106+
}
107+
}
108+
109+
func TestExtractRetryAfter_TypedFastPath(t *testing.T) {
110+
got := extractRetryAfter(&RateLimitError{StatusCode: 429, RetryAfter: 17 * time.Second})
111+
if got != 17*time.Second {
112+
t.Errorf("extractRetryAfter = %v, want 17s", got)
113+
}
114+
}
115+
116+
func TestParseParentIssueNumber_Exported(t *testing.T) {
117+
if n := ParseParentIssueNumber("Parent: #37\n\nBody"); n != 37 {
118+
t.Errorf("ParseParentIssueNumber = %d, want 37", n)
119+
}
120+
if n := ParseParentIssueNumber("Parent: GH-42"); n != 42 {
121+
t.Errorf("ParseParentIssueNumber = %d, want 42", n)
122+
}
123+
if n := ParseParentIssueNumber("no parent here"); n != 0 {
124+
t.Errorf("ParseParentIssueNumber = %d, want 0", n)
125+
}
126+
}
127+
128+
func TestFailedRetryStateLabels(t *testing.T) {
129+
want := []string{"pilot-failed-retry-1", "pilot-failed-retry-2", "pilot-failed-retry-exhausted"}
130+
if len(FailedRetryStateLabels) != len(want) {
131+
t.Fatalf("FailedRetryStateLabels = %v", FailedRetryStateLabels)
132+
}
133+
for i, w := range want {
134+
if FailedRetryStateLabels[i] != w {
135+
t.Errorf("FailedRetryStateLabels[%d] = %q, want %q", i, FailedRetryStateLabels[i], w)
136+
}
137+
}
138+
if LabelPilot != "pilot" {
139+
t.Errorf("LabelPilot = %q, want pilot", LabelPilot)
140+
}
141+
}
142+
143+
func TestGetOpenSubIssueNumbers(t *testing.T) {
144+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
145+
w.Header().Set("Content-Type", "application/json")
146+
if r.URL.Path == "/graphql" {
147+
_, _ = w.Write([]byte(`{"data":{"node":{"subIssues":{"totalCount":3,"nodes":[
148+
{"number":11,"state":"OPEN"},{"number":12,"state":"CLOSED"},{"number":13,"state":"OPEN"}]}}}}`))
149+
return
150+
}
151+
_, _ = w.Write([]byte(`{"node_id":"I_parent","number":10}`))
152+
}))
153+
defer srv.Close()
154+
155+
c := NewClientWithBaseURL(testutil.FakeGitHubToken, srv.URL)
156+
numbers, hasLinks, err := c.GetOpenSubIssueNumbers(context.Background(), "o", "r", 10)
157+
if err != nil {
158+
t.Fatalf("GetOpenSubIssueNumbers: %v", err)
159+
}
160+
if !hasLinks {
161+
t.Error("hasNativeLinks = false, want true (totalCount 3)")
162+
}
163+
if len(numbers) != 2 || numbers[0] != 11 || numbers[1] != 13 {
164+
t.Errorf("numbers = %v, want [11 13]", numbers)
165+
}
166+
}
167+
168+
func TestGetOpenSubIssueNumbers_NoNativeLinks(t *testing.T) {
169+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
170+
w.Header().Set("Content-Type", "application/json")
171+
if r.URL.Path == "/graphql" {
172+
_, _ = w.Write([]byte(`{"data":{"node":{"subIssues":{"totalCount":0,"nodes":[]}}}}`))
173+
return
174+
}
175+
_, _ = w.Write([]byte(`{"node_id":"I_parent","number":10}`))
176+
}))
177+
defer srv.Close()
178+
179+
c := NewClientWithBaseURL(testutil.FakeGitHubToken, srv.URL)
180+
numbers, hasLinks, err := c.GetOpenSubIssueNumbers(context.Background(), "o", "r", 10)
181+
if err != nil {
182+
t.Fatalf("GetOpenSubIssueNumbers: %v", err)
183+
}
184+
if hasLinks || numbers != nil {
185+
t.Errorf("got (%v, %v), want (nil, false)", numbers, hasLinks)
186+
}
187+
}

sdk/integrations/github/poller.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1603,6 +1603,13 @@ func ParseDependencies(body string) []int {
16031603
// parentRefRegex extracts parent issue references from issue body.
16041604
var parentRefRegex = regexp.MustCompile(`(?m)^Parent:\s*(?:GH-|#)(\d+)`)
16051605

1606+
// ParseParentIssueNumber extracts the parent issue number ("Parent: #N" /
1607+
// "Parent: GH-N") from an issue body. Returns 0 if no parent reference is
1608+
// found. Exported for hosts that apply parent-based gating outside the poller.
1609+
func ParseParentIssueNumber(body string) int {
1610+
return parseParentIssueNumber(body)
1611+
}
1612+
16061613
// parseParentIssueNumber extracts the parent issue number from an issue body.
16071614
// Returns 0 if no parent reference is found.
16081615
func parseParentIssueNumber(body string) int {

sdk/integrations/github/retry.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package github
22

33
import (
44
"context"
5+
"errors"
56
"regexp"
67
"strconv"
78
"strings"
@@ -97,6 +98,18 @@ func isRetryableError(err error) bool {
9798
return false
9899
}
99100

101+
// Typed fast path: doRequest classifies 429s and 403-rate-limits as
102+
// *RateLimitError (including 403s whose Error() string would not match
103+
// the status-code list below).
104+
var rlErr *RateLimitError
105+
if errors.As(err, &rlErr) {
106+
return true
107+
}
108+
var authErr *AuthError
109+
if errors.As(err, &authErr) {
110+
return false // dead token — retrying cannot help
111+
}
112+
100113
errStr := err.Error()
101114

102115
// Check for retryable HTTP status codes
@@ -149,6 +162,13 @@ func extractRetryAfter(err error) time.Duration {
149162
return 0
150163
}
151164

165+
// Typed fast path: doRequest parses Retry-After/X-RateLimit-Reset headers
166+
// into RateLimitError.RetryAfter — no string parsing needed.
167+
var rlErr *RateLimitError
168+
if errors.As(err, &rlErr) && rlErr.RetryAfter > 0 {
169+
return rlErr.RetryAfter
170+
}
171+
152172
errStr := err.Error()
153173

154174
// GitHub API sometimes includes retry-after info in error response

0 commit comments

Comments
 (0)