From aa81e3d818fb62b182225bc255644d9289b19125 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Jan 2026 21:16:46 +0000 Subject: [PATCH 1/9] Initial plan From fd50a3eba68d02a2eb7703031244342e344223c5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Jan 2026 21:23:49 +0000 Subject: [PATCH 2/9] Add retry logic with jitter for sub_issue_write to handle parallel calls Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com> --- pkg/github/issues.go | 81 ++++++++++++++++----- pkg/github/issues_test.go | 143 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 18 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 62e1a0bac..4e1df5411 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "io" + "math/rand/v2" "net/http" "strings" "time" @@ -800,37 +801,81 @@ Options are: } func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, subIssueID int, replaceParent bool) (*mcp.CallToolResult, error) { + const maxRetries = 3 subIssueRequest := github.SubIssueRequest{ SubIssueID: int64(subIssueID), ReplaceParent: github.Ptr(replaceParent), } - subIssue, resp, err := client.SubIssue.Add(ctx, owner, repo, int64(issueNumber), subIssueRequest) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to add sub-issue", - resp, - err, - ), nil - } + var lastResp *github.Response + var lastErr error - defer func() { _ = resp.Body.Close() }() + for attempt := 0; attempt < maxRetries; attempt++ { + if attempt > 0 { + // Random jitter: 50-200ms to desynchronize parallel retries + // #nosec G404 -- Using non-cryptographic random for jitter is acceptable + jitter := time.Duration(50+rand.IntN(151)) * time.Millisecond + time.Sleep(jitter) + } - if resp.StatusCode != http.StatusCreated { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) + subIssue, resp, err := client.SubIssue.Add(ctx, owner, repo, int64(issueNumber), subIssueRequest) + lastResp = resp + lastErr = err + + // Success case + if err == nil && resp.StatusCode == http.StatusCreated { + defer func() { _ = resp.Body.Close() }() + r, err := json.Marshal(subIssue) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + return utils.NewToolResultText(string(r)), nil + } + + // Check if this is a retryable priority conflict error + shouldRetry := false + if resp != nil && resp.StatusCode == http.StatusUnprocessableEntity { + // Read the body to check for priority conflict + if resp.Body != nil { + body, readErr := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if readErr == nil { + bodyStr := string(body) + if strings.Contains(bodyStr, "Priority has already been taken") || + (err != nil && strings.Contains(err.Error(), "Priority has already been taken")) { + shouldRetry = true + } + } + } + } + + // If not a retryable error or last attempt, break and return error + if !shouldRetry || attempt == maxRetries-1 { + break } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", resp, body), nil } - r, err := json.Marshal(subIssue) - if err != nil { - return nil, fmt.Errorf("failed to marshal response: %w", err) + // Handle final error after all retries exhausted + if lastErr != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to add sub-issue", + lastResp, + lastErr, + ), nil } - return utils.NewToolResultText(string(r)), nil + if lastResp != nil { + defer func() { _ = lastResp.Body.Close() }() + if lastResp.StatusCode != http.StatusCreated { + body, err := io.ReadAll(lastResp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", lastResp, body), nil + } + } + return nil, fmt.Errorf("failed to add sub-issue after %d retries", maxRetries) } func RemoveSubIssue(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, subIssueID int) (*mcp.CallToolResult, error) { diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index a338efcba..2c10c798c 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -3020,6 +3020,149 @@ func Test_AddSubIssue(t *testing.T) { } } +func Test_AddSubIssue_RetryLogic(t *testing.T) { + // Setup mock issue for success case + mockIssue := &github.Issue{ + Number: github.Ptr(42), + Title: github.Ptr("Parent Issue"), + Body: github.Ptr("This is the parent issue with a sub-issue"), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/42"), + User: &github.User{ + Login: github.Ptr("testuser"), + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]interface{} + expectSuccess bool + expectedErrMsg string + }{ + { + name: "successful retry after priority conflict", + mockedClient: func() *http.Client { + // Create a handler that fails twice with priority conflict, then succeeds + callCount := 0 + handler := func(w http.ResponseWriter, _ *http.Request) { + callCount++ + if callCount <= 2 { + // First two calls fail with priority conflict + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message": "An error occurred while adding the sub-issue to the parent issue. Priority has already been taken"}`)) + } else { + // Third call succeeds + w.WriteHeader(http.StatusCreated) + responseJSON, _ := json.Marshal(mockIssue) + _, _ = w.Write(responseJSON) + } + } + return MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber: handler, + }) + }(), + requestArgs: map[string]interface{}{ + "method": "add", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "sub_issue_id": float64(123), + }, + expectSuccess: true, + }, + { + name: "exhausted retries with priority conflict", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, _ *http.Request) { + // Always fail with priority conflict + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message": "An error occurred while adding the sub-issue to the parent issue. Priority has already been taken"}`)) + }, + }), + requestArgs: map[string]interface{}{ + "method": "add", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "sub_issue_id": float64(123), + }, + expectSuccess: false, + expectedErrMsg: "Priority has already been taken", + }, + { + name: "non-retryable 422 error - sub-issue cannot be parent of itself", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, _ *http.Request) { + // Non-retryable 422 error + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message": "Validation failed", "errors": [{"message": "Sub-issue cannot be a parent of itself"}]}`)) + }, + }), + requestArgs: map[string]interface{}{ + "method": "add", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "sub_issue_id": float64(42), + }, + expectSuccess: false, + expectedErrMsg: "failed to add sub-issue", + }, + { + name: "first attempt succeeds - no retry needed", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusCreated, mockIssue), + }), + requestArgs: map[string]interface{}{ + "method": "add", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "sub_issue_id": float64(123), + }, + expectSuccess: true, + }, + } + + serverTool := SubIssueWrite(translations.NullTranslationHelper) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Setup client with mock + client := github.NewClient(tc.mockedClient) + deps := BaseDeps{ + Client: client, + } + handler := serverTool.Handler(deps) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectSuccess { + require.NoError(t, err) + require.NotNil(t, result) + + // Parse and verify the result + textContent := getTextResult(t, result) + var returnedIssue github.Issue + err = json.Unmarshal([]byte(textContent.Text), &returnedIssue) + require.NoError(t, err) + assert.Equal(t, *mockIssue.Number, *returnedIssue.Number) + assert.Equal(t, *mockIssue.Title, *returnedIssue.Title) + } else { + // Expect error in result content + require.NotNil(t, result) + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, tc.expectedErrMsg) + } + }) + } +} + func Test_GetSubIssues(t *testing.T) { // Verify tool definition once serverTool := IssueRead(translations.NullTranslationHelper) From b8f0b139ec15b82793ba73942d9b7784083ea397 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Jan 2026 21:26:03 +0000 Subject: [PATCH 3/9] Address code review feedback: simplify error checking and fix logic Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com> --- pkg/github/issues.go | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 4e1df5411..badb22094 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -834,18 +834,12 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo // Check if this is a retryable priority conflict error shouldRetry := false - if resp != nil && resp.StatusCode == http.StatusUnprocessableEntity { + if resp != nil && resp.StatusCode == http.StatusUnprocessableEntity && resp.Body != nil { // Read the body to check for priority conflict - if resp.Body != nil { - body, readErr := io.ReadAll(resp.Body) - _ = resp.Body.Close() - if readErr == nil { - bodyStr := string(body) - if strings.Contains(bodyStr, "Priority has already been taken") || - (err != nil && strings.Contains(err.Error(), "Priority has already been taken")) { - shouldRetry = true - } - } + body, readErr := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if readErr == nil && strings.Contains(string(body), "Priority has already been taken") { + shouldRetry = true } } @@ -864,15 +858,14 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo ), nil } - if lastResp != nil { + // Handle non-201 status codes after retries exhausted + if lastResp != nil && lastResp.StatusCode != http.StatusCreated { defer func() { _ = lastResp.Body.Close() }() - if lastResp.StatusCode != http.StatusCreated { - body, err := io.ReadAll(lastResp.Body) - if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) - } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", lastResp, body), nil + body, err := io.ReadAll(lastResp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", lastResp, body), nil } return nil, fmt.Errorf("failed to add sub-issue after %d retries", maxRetries) From 3f87488365f252432951a1b535eee69bbd04503f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Jan 2026 21:27:43 +0000 Subject: [PATCH 4/9] Fix response body handling and remove unreachable code Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com> --- pkg/github/issues.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index badb22094..2a5d15e6b 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -824,7 +824,6 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo // Success case if err == nil && resp.StatusCode == http.StatusCreated { - defer func() { _ = resp.Body.Close() }() r, err := json.Marshal(subIssue) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) @@ -860,15 +859,16 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo // Handle non-201 status codes after retries exhausted if lastResp != nil && lastResp.StatusCode != http.StatusCreated { - defer func() { _ = lastResp.Body.Close() }() body, err := io.ReadAll(lastResp.Body) + defer func() { _ = lastResp.Body.Close() }() if err != nil { return nil, fmt.Errorf("failed to read response body: %w", err) } return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", lastResp, body), nil } - return nil, fmt.Errorf("failed to add sub-issue after %d retries", maxRetries) + // This should not be reached in normal operation + return nil, fmt.Errorf("unexpected error: failed to add sub-issue") } func RemoveSubIssue(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, subIssueID int) (*mcp.CallToolResult, error) { From 6e0e72a5bc300b604832f0fd4ef02ef92f7b07c1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Jan 2026 21:29:30 +0000 Subject: [PATCH 5/9] Fix defer placement for response body closing Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com> --- pkg/github/issues.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 2a5d15e6b..0190efb91 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -859,8 +859,8 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo // Handle non-201 status codes after retries exhausted if lastResp != nil && lastResp.StatusCode != http.StatusCreated { - body, err := io.ReadAll(lastResp.Body) defer func() { _ = lastResp.Body.Close() }() + body, err := io.ReadAll(lastResp.Body) if err != nil { return nil, fmt.Errorf("failed to read response body: %w", err) } From 7e0380c53d75be58090da08bfc94e98e6e1a952c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Jan 2026 21:31:35 +0000 Subject: [PATCH 6/9] Fix response body handling and improve thread safety in tests Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com> --- pkg/github/issues.go | 25 ++++++++++++++++++------- pkg/github/issues_test.go | 5 +++-- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 0190efb91..e2bda6b6a 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -809,6 +809,7 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo var lastResp *github.Response var lastErr error + var lastBody []byte for attempt := 0; attempt < maxRetries; attempt++ { if attempt > 0 { @@ -837,8 +838,11 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo // Read the body to check for priority conflict body, readErr := io.ReadAll(resp.Body) _ = resp.Body.Close() - if readErr == nil && strings.Contains(string(body), "Priority has already been taken") { - shouldRetry = true + if readErr == nil { + lastBody = body + if strings.Contains(string(body), "Priority has already been taken") { + shouldRetry = true + } } } @@ -859,12 +863,19 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo // Handle non-201 status codes after retries exhausted if lastResp != nil && lastResp.StatusCode != http.StatusCreated { - defer func() { _ = lastResp.Body.Close() }() - body, err := io.ReadAll(lastResp.Body) - if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) + // Use the body we already read during retry attempts if available + if len(lastBody) > 0 { + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", lastResp, lastBody), nil + } + // Otherwise try to read the body if it's still available + if lastResp.Body != nil { + body, err := io.ReadAll(lastResp.Body) + _ = lastResp.Body.Close() + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", lastResp, body), nil } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", lastResp, body), nil } // This should not be reached in normal operation diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 2c10c798c..c7069313a 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -3044,10 +3044,11 @@ func Test_AddSubIssue_RetryLogic(t *testing.T) { name: "successful retry after priority conflict", mockedClient: func() *http.Client { // Create a handler that fails twice with priority conflict, then succeeds - callCount := 0 + var callCount int32 handler := func(w http.ResponseWriter, _ *http.Request) { + count := callCount callCount++ - if callCount <= 2 { + if count < 2 { // First two calls fail with priority conflict w.WriteHeader(http.StatusUnprocessableEntity) _, _ = w.Write([]byte(`{"message": "An error occurred while adding the sub-issue to the parent issue. Priority has already been taken"}`)) From d3cf18d8a8ee2ffe24bdace3fa6ad13c4ad5c316 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Jan 2026 21:33:46 +0000 Subject: [PATCH 7/9] Use atomic operations for thread-safe test counter Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com> --- pkg/github/issues_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index c7069313a..ffea00b7f 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "strings" + "sync/atomic" "testing" "time" @@ -3044,11 +3045,10 @@ func Test_AddSubIssue_RetryLogic(t *testing.T) { name: "successful retry after priority conflict", mockedClient: func() *http.Client { // Create a handler that fails twice with priority conflict, then succeeds - var callCount int32 + var callCount atomic.Int32 handler := func(w http.ResponseWriter, _ *http.Request) { - count := callCount - callCount++ - if count < 2 { + count := callCount.Add(1) + if count <= 2 { // First two calls fail with priority conflict w.WriteHeader(http.StatusUnprocessableEntity) _, _ = w.Write([]byte(`{"message": "An error occurred while adding the sub-issue to the parent issue. Priority has already been taken"}`)) From 9469022f402333c18d0d0106921ced8876d4f1c6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Jan 2026 21:35:19 +0000 Subject: [PATCH 8/9] Add named constants for jitter and improve error diagnostics Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com> --- pkg/github/issues.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index e2bda6b6a..6bf96aca1 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -801,7 +801,13 @@ Options are: } func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, subIssueID int, replaceParent bool) (*mcp.CallToolResult, error) { - const maxRetries = 3 + const ( + maxRetries = 3 + minJitterMs = 50 + maxJitterMs = 200 + jitterRangeMs = maxJitterMs - minJitterMs + 1 + ) + subIssueRequest := github.SubIssueRequest{ SubIssueID: int64(subIssueID), ReplaceParent: github.Ptr(replaceParent), @@ -815,7 +821,7 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo if attempt > 0 { // Random jitter: 50-200ms to desynchronize parallel retries // #nosec G404 -- Using non-cryptographic random for jitter is acceptable - jitter := time.Duration(50+rand.IntN(151)) * time.Millisecond + jitter := time.Duration(minJitterMs+rand.IntN(jitterRangeMs)) * time.Millisecond time.Sleep(jitter) } @@ -878,8 +884,12 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo } } - // This should not be reached in normal operation - return nil, fmt.Errorf("unexpected error: failed to add sub-issue") + // This should not be reached in normal operation - indicates a logic error + statusCode := 0 + if lastResp != nil { + statusCode = lastResp.StatusCode + } + return nil, fmt.Errorf("unexpected error: failed to add sub-issue after %d attempts (last status: %d)", maxRetries, statusCode) } func RemoveSubIssue(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, subIssueID int) (*mcp.CallToolResult, error) { From 8e0a81fb76f7d8854f70c98f009f7745b1fc8f07 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 20 Jan 2026 09:39:09 +0000 Subject: [PATCH 9/9] Fix resource leak: close response body in success path Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com> --- pkg/github/issues.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 6bf96aca1..f2a8b16cd 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -831,6 +831,7 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo // Success case if err == nil && resp.StatusCode == http.StatusCreated { + _ = resp.Body.Close() r, err := json.Marshal(subIssue) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err)