diff --git a/apps/backend/internal/backendapp/status_summary_adapter.go b/apps/backend/internal/backendapp/status_summary_adapter.go index 7a153ee606..076c6cb212 100644 --- a/apps/backend/internal/backendapp/status_summary_adapter.go +++ b/apps/backend/internal/backendapp/status_summary_adapter.go @@ -26,7 +26,18 @@ func (r *githubTaskStatusSummaryPRReader) ListTaskStatusSummaryPullRequests( if err != nil { return nil, err } + automationByTask, err := r.gh.ListTaskPRAutomationOptionsByTaskIDs(ctx, taskIDs) + if err != nil { + return nil, err + } for taskID, prs := range prsByTask { + optionsByKey := make(map[string]*github.TaskPRAutomationOptions, len(automationByTask[taskID])) + for _, options := range automationByTask[taskID] { + if options == nil { + continue + } + optionsByKey[fmt.Sprintf("%s#%d", options.RepositoryID, options.PRNumber)] = options + } for _, pr := range prs { if pr == nil { continue @@ -39,6 +50,7 @@ func (r *githubTaskStatusSummaryPRReader) ListTaskStatusSummaryPullRequests( if pr.RequiredReviews != nil { requiredReviews = *pr.RequiredReviews } + options := optionsByKey[key] result[taskID] = append(result[taskID], statussummary.PullRequestInput{ Key: key, State: pr.State, @@ -53,6 +65,8 @@ func (r *githubTaskStatusSummaryPRReader) ListTaskStatusSummaryPullRequests( RequiredReviews: requiredReviews, ChecksTotal: pr.ChecksTotal, ChecksPassing: pr.ChecksPassing, + AutoFixEnabled: options != nil && options.AutoFixEnabled, + AutoMergeEnabled: options != nil && options.AutoMergeEnabled, }) } } diff --git a/apps/backend/internal/backendapp/status_summary_adapter_test.go b/apps/backend/internal/backendapp/status_summary_adapter_test.go index 37dba3bf8f..89e42e973b 100644 --- a/apps/backend/internal/backendapp/status_summary_adapter_test.go +++ b/apps/backend/internal/backendapp/status_summary_adapter_test.go @@ -114,3 +114,38 @@ func TestGitHubTaskStatusSummaryPRReaderPreservesMergeQueueState(t *testing.T) { t.Fatalf("MergeQueueState = %q, want %q", inputs[0].MergeQueueState, queuePR.MergeQueueState) } } + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.10 +func TestGitHubTaskStatusSummaryPRReaderIncludesPerPRAutomationFlags(t *testing.T) { + ctx := context.Background() + store := newStatusSummaryTestStore(t) + queuePR := &github.TaskPR{ + TaskID: "task-automation-summary", RepositoryID: "repo-automation-summary", PRNumber: 42, + PRURL: "https://example.test/42", State: "open", CreatedAt: time.Now().UTC(), + } + if err := store.CreateTaskPR(ctx, queuePR); err != nil { + t.Fatalf("CreateTaskPR: %v", err) + } + autoFix := true + autoMerge := true + if _, err := store.UpdateTaskPRAutomationOptions(ctx, queuePR.TaskID, queuePR.RepositoryID, queuePR.PRNumber, + github.TaskPRAutomationOptionsPatch{AutoFixEnabled: &autoFix, AutoMergeEnabled: &autoMerge}, false, + ); err != nil { + t.Fatalf("UpdateTaskPRAutomationOptions: %v", err) + } + + reader := &githubTaskStatusSummaryPRReader{ + gh: github.NewService(nil, "", nil, store, nil, nil), + } + result, err := reader.ListTaskStatusSummaryPullRequests(ctx, []string{queuePR.TaskID}) + if err != nil { + t.Fatalf("ListTaskStatusSummaryPullRequests: %v", err) + } + inputs := result[queuePR.TaskID] + if len(inputs) != 1 { + t.Fatalf("summary inputs = %+v, want one input", inputs) + } + if !inputs[0].AutoFixEnabled || !inputs[0].AutoMergeEnabled { + t.Fatalf("automation flags = %+v, want both enabled", inputs[0]) + } +} diff --git a/apps/backend/internal/github/client.go b/apps/backend/internal/github/client.go index 94e0dddb36..21d0959f02 100644 --- a/apps/backend/internal/github/client.go +++ b/apps/backend/internal/github/client.go @@ -20,12 +20,43 @@ const ( MergeOutcomeMerged MergeOutcome = "merged" MergeOutcomeQueued MergeOutcome = "queued" mergeStatusFailed = "failed" + mergePollInterval = time.Second ) +// MergePRRequest defines the optional merge method and the pull-request head +// that GitHub must still expose when it accepts the merge. +type MergePRRequest struct { + MergeMethod string + ExpectedHeadSHA string +} + type mergeAsyncResponse struct { - Status string `json:"status"` - UUID string `json:"uuid"` - Message string `json:"message"` + Status string `json:"status"` + Details mergeAsyncResponseDetails `json:"details"` +} + +type mergeAsyncResponseDetails struct { + UUID string `json:"uuid"` + Message string `json:"message"` + ExpectedHeadSHA string `json:"expected_head_sha"` +} + +func waitForMergePoll(ctx context.Context, delay time.Duration) error { + timer := time.NewTimer(delay) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} + +func newMergeFailureError(details mergeAsyncResponseDetails) error { + if details.ExpectedHeadSHA != "" { + return fmt.Errorf("GitHub rejected merge for expected head %s: %s", details.ExpectedHeadSHA, details.Message) + } + return fmt.Errorf("GitHub rejected merge: %s", details.Message) } func normalizeMergeOutcome(status string) (MergeOutcome, error) { @@ -120,7 +151,7 @@ type Client interface { RequestReviewers(ctx context.Context, owner, repo string, number int, reviewers []string) error // MergePR merges a pull request immediately or queues it according to GitHub policy. - MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) + MergePR(ctx context.Context, owner, repo string, number int, request MergePRRequest) (MergeOutcome, error) // ListRepoBranches lists branches for a repository. ListRepoBranches(ctx context.Context, owner, repo string) ([]RepoBranch, error) diff --git a/apps/backend/internal/github/controller.go b/apps/backend/internal/github/controller.go index 1ce8f69862..6d253091ce 100644 --- a/apps/backend/internal/github/controller.go +++ b/apps/backend/internal/github/controller.go @@ -86,6 +86,7 @@ func (c *Controller) RegisterHTTPRoutes(router *gin.Engine) { api.DELETE("/tasks/:taskId/issue", c.httpUnlinkTaskIssue) api.GET("/tasks/:taskId/ci-options", c.httpGetTaskCIOptions) api.PATCH("/tasks/:taskId/ci-options", c.httpPatchTaskCIOptions) + api.POST("/tasks/:taskId/ci-automation/retry-merge", c.httpRetryTaskCIAutoMerge) api.GET("/prs/:owner/:repo/:number", c.httpGetPRFeedback) api.GET("/prs/:owner/:repo/:number/info", c.httpGetPRInfo) @@ -407,6 +408,48 @@ func (c *Controller) httpPatchTaskCIOptions(ctx *gin.Context) { ctx.JSON(http.StatusOK, resp) } +type retryTaskCIAutoMergeRequest struct { + RepositoryID string `json:"repository_id"` + PRNumber int `json:"pr_number"` +} + +func (c *Controller) httpRetryTaskCIAutoMerge(ctx *gin.Context) { + var request retryTaskCIAutoMergeRequest + if err := ctx.ShouldBindJSON(&request); err != nil || strings.TrimSpace(request.RepositoryID) == "" || request.PRNumber <= 0 { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "repository_id and pr_number are required"}) + return + } + pr, err := c.service.RetryTaskCIAutoMerge( + ctx.Request.Context(), ctx.Param("taskId"), strings.TrimSpace(request.RepositoryID), request.PRNumber, + ) + if err != nil { + if errors.Is(err, ErrTaskCIMergeRetryNotAllowed) { + ctx.JSON(http.StatusConflict, gin.H{"error": err.Error()}) + return + } + c.writeTaskCIOptionsError(ctx, err, "retry merge", "failed to retry automatic merge") + return + } + var publishErr error + if c.service == nil || c.service.eventBus == nil { + publishErr = errors.New("event bus unavailable") + } else { + event := bus.NewEvent(events.GitHubTaskPRUpdated, "github", pr) + publishErr = c.service.eventBus.Publish(ctx.Request.Context(), events.GitHubTaskPRUpdated, event) + } + if publishErr != nil { + if clearErr := c.service.ClearTaskCIMergeRetryAuthorization( + ctx.Request.Context(), pr.TaskID, pr.RepositoryID, pr.PRNumber, + ); clearErr != nil { + c.logger.Debug("clear automatic merge retry authorization failed", + zap.String("task_id", pr.TaskID), zap.Error(clearErr)) + } + c.writeTaskCIOptionsError(ctx, publishErr, "publish automatic merge retry", "failed to queue automatic merge retry") + return + } + ctx.JSON(http.StatusAccepted, gin.H{"accepted": true}) +} + func (c *Controller) writeTaskCIOptionsError(ctx *gin.Context, err error, operation, message string) { if errors.Is(err, ErrTaskNotFound) || errors.Is(err, repoerrors.ErrWorkspaceNotFound) || diff --git a/apps/backend/internal/github/controller_test.go b/apps/backend/internal/github/controller_test.go index a4258ab4f5..3673fd134e 100644 --- a/apps/backend/internal/github/controller_test.go +++ b/apps/backend/internal/github/controller_test.go @@ -132,9 +132,9 @@ func (s *stubClient) RequestReviewers(ctx context.Context, owner, repo string, n } return nil } -func (s *stubClient) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) { +func (s *stubClient) MergePR(ctx context.Context, owner, repo string, number int, request MergePRRequest) (MergeOutcome, error) { if s.mergePRFn != nil { - return s.mergePRFn(ctx, owner, repo, number, mergeMethod) + return s.mergePRFn(ctx, owner, repo, number, request.MergeMethod) } return MergeOutcomeMerged, nil } @@ -1250,6 +1250,161 @@ func TestHttpTaskCIOptions_DefaultAndPatch(t *testing.T) { } } +func TestHttpRetryMergeAcceptsLinkedFailedAttemptAndPublishesEvaluation(t *testing.T) { + svc, store := setupWatchServiceTest(t) + eventBus := &mockEventBus{} + svc.eventBus = eventBus + router := gin.New() + useControllerTestWorkspace(router) + NewController(svc, newControllerTestLogger()).RegisterHTTPRoutes(router) + ctx := context.Background() + pr := &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "widget", PRNumber: 42, + State: "open", CreatedAt: time.Now().UTC(), + } + if err := store.CreateTaskPR(ctx, pr); err != nil { + t.Fatalf("seed task PR: %v", err) + } + if err := store.RecordTaskCIMergeAttempt(ctx, TaskCIMergeAttempt{ + TaskID: pr.TaskID, RepositoryID: pr.RepositoryID, PRNumber: pr.PRNumber, + Signature: "ready-v1", AttemptedHeadSHA: "head-v1", + }); err != nil { + t.Fatalf("reserve attempt: %v", err) + } + if err := store.RecordTaskCIMergeAttemptResult( + ctx, pr.TaskID, pr.RepositoryID, pr.PRNumber, + "ready-v1", TaskCIMergeResultFailed, "merge PR: provider unavailable", + ); err != nil { + t.Fatalf("record failure: %v", err) + } + + body := bytes.NewBufferString(`{"repository_id":"repo-1","pr_number":42}`) + req := httptest.NewRequest(http.MethodPost, "/api/v1/github/tasks/task-1/ci-automation/retry-merge", body) + req.Header.Set("Content-Type", "application/json") + response := httptest.NewRecorder() + router.ServeHTTP(response, req) + + if response.Code != http.StatusAccepted { + t.Fatalf("status = %d, want 202: %s", response.Code, response.Body.String()) + } + state, err := store.GetTaskCIPRState(ctx, pr.TaskID, pr.RepositoryID, pr.PRNumber) + if err != nil { + t.Fatalf("get state: %v", err) + } + if state == nil || !state.MergeRetryPending { + t.Fatalf("retry authorization not persisted: %+v", state) + } + if eventBus.publishedCount() != 1 { + t.Fatalf("published events = %d, want 1", eventBus.publishedCount()) + } + event := eventBus.events[0] + if event.Type != "github.task_pr.updated" { + t.Fatalf("event type = %q, want github.task_pr.updated", event.Type) + } + published, ok := event.Data.(*TaskPR) + if !ok || published.RepositoryID != pr.RepositoryID || published.PRNumber != pr.PRNumber { + t.Fatalf("published PR = %#v, want exact linked PR", event.Data) + } +} + +func TestHttpRetryMergeClearsAuthorizationWhenPublishFails(t *testing.T) { + svc, store := setupWatchServiceTest(t) + svc.eventBus = &mockEventBus{err: errors.New("publish unavailable")} + router := gin.New() + useControllerTestWorkspace(router) + NewController(svc, newControllerTestLogger()).RegisterHTTPRoutes(router) + ctx := context.Background() + pr := &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "widget", PRNumber: 42, + State: "open", CreatedAt: time.Now().UTC(), + } + if err := store.CreateTaskPR(ctx, pr); err != nil { + t.Fatalf("seed task PR: %v", err) + } + if err := store.RecordTaskCIMergeAttempt(ctx, TaskCIMergeAttempt{ + TaskID: pr.TaskID, RepositoryID: pr.RepositoryID, PRNumber: pr.PRNumber, + Signature: "ready-v1", AttemptedHeadSHA: "head-v1", + }); err != nil { + t.Fatalf("reserve attempt: %v", err) + } + if err := store.RecordTaskCIMergeAttemptResult( + ctx, pr.TaskID, pr.RepositoryID, pr.PRNumber, + "ready-v1", TaskCIMergeResultFailed, "merge PR: provider unavailable", + ); err != nil { + t.Fatalf("record failure: %v", err) + } + + req := httptest.NewRequest( + http.MethodPost, + "/api/v1/github/tasks/task-1/ci-automation/retry-merge", + bytes.NewBufferString(`{"repository_id":"repo-1","pr_number":42}`), + ) + req.Header.Set("Content-Type", "application/json") + response := httptest.NewRecorder() + router.ServeHTTP(response, req) + + if response.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500: %s", response.Code, response.Body.String()) + } + state, err := store.GetTaskCIPRState(ctx, pr.TaskID, pr.RepositoryID, pr.PRNumber) + if err != nil { + t.Fatalf("get state: %v", err) + } + if state == nil || state.MergeRetryPending { + t.Fatalf("retry authorization remained pending after publish failure: %+v", state) + } +} + +func TestHttpRetryMergeRejectsUnlinkedAndDuplicateRequests(t *testing.T) { + svc, store := setupWatchServiceTest(t) + svc.eventBus = &mockEventBus{} + router := gin.New() + useControllerTestWorkspace(router) + NewController(svc, newControllerTestLogger()).RegisterHTTPRoutes(router) + ctx := context.Background() + pr := &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "widget", PRNumber: 42, + State: "open", CreatedAt: time.Now().UTC(), + } + if err := store.CreateTaskPR(ctx, pr); err != nil { + t.Fatalf("seed task PR: %v", err) + } + if err := store.RecordTaskCIMergeAttempt(ctx, TaskCIMergeAttempt{ + TaskID: pr.TaskID, RepositoryID: pr.RepositoryID, PRNumber: pr.PRNumber, + Signature: "ready-v1", AttemptedHeadSHA: "head-v1", + }); err != nil { + t.Fatalf("reserve attempt: %v", err) + } + if err := store.RecordTaskCIMergeAttemptResult( + ctx, pr.TaskID, pr.RepositoryID, pr.PRNumber, + "ready-v1", TaskCIMergeResultFailed, "merge PR: provider unavailable", + ); err != nil { + t.Fatalf("record failure: %v", err) + } + + request := func(body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/api/v1/github/tasks/task-1/ci-automation/retry-merge", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + response := httptest.NewRecorder() + router.ServeHTTP(response, req) + return response + } + + unlinked := request(`{"repository_id":"repo-1","pr_number":99}`) + if unlinked.Code != http.StatusBadRequest { + t.Fatalf("unlinked status = %d, want 400: %s", unlinked.Code, unlinked.Body.String()) + } + first := request(`{"repository_id":"repo-1","pr_number":42}`) + if first.Code != http.StatusAccepted { + t.Fatalf("first status = %d, want 202: %s", first.Code, first.Body.String()) + } + duplicate := request(`{"repository_id":"repo-1","pr_number":42}`) + if duplicate.Code != http.StatusConflict { + t.Fatalf("duplicate status = %d, want 409: %s", duplicate.Code, duplicate.Body.String()) + } +} + // TestHttpPatchTaskCIOptions_TargetsOnePRWithoutAffectingSibling covers AC6: // a PATCH naming one linked PR's repository_id/pr_number sets the switch for // that PR only; a second linked PR's row is unchanged. diff --git a/apps/backend/internal/github/gh_client.go b/apps/backend/internal/github/gh_client.go index 2a45fb78d8..c46f253528 100644 --- a/apps/backend/internal/github/gh_client.go +++ b/apps/backend/internal/github/gh_client.go @@ -32,7 +32,8 @@ const ghAccessibleReposPathFmt = "/user/repos?affiliation=%s&sort=pushed&per_pag // GHClient implements Client using the gh CLI. type GHClient struct { - rateTracker *RateTracker + rateTracker *RateTracker + mergePollWait func(context.Context, time.Duration) error } // NewGHClient creates a new gh CLI-based client. @@ -919,55 +920,86 @@ func (c *GHClient) RequestReviewers(ctx context.Context, owner, repo string, num return nil } -func (c *GHClient) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) { +func (c *GHClient) MergePR(ctx context.Context, owner, repo string, number int, request MergePRRequest) (MergeOutcome, error) { endpoint := fmt.Sprintf("repos/%s/%s/pulls/%d/merge-async", owner, repo, number) args := []string{"api", endpoint, "-X", "PUT", "-f", "merge_action=default"} - if mergeMethod != "" { - args = append(args, "-f", "merge_method="+mergeMethod) + if request.MergeMethod != "" { + args = append(args, "-f", "merge_method="+request.MergeMethod) + } + if request.ExpectedHeadSHA != "" { + args = append(args, "-f", "sha="+request.ExpectedHeadSHA) } out, err := c.run(ctx, args...) - conflictBody := false + response, err := decodeInitialMergeAsyncResponse(endpoint, number, out, err) if err != nil { + return "", err + } + response, err = c.pollMergeAsyncResponse(ctx, endpoint, number, response) + if err != nil { + return "", err + } + if response.Status == mergeStatusFailed { + return "", newMergeFailureError(response.Details) + } + return normalizeMergeOutcome(response.Status) +} + +func decodeInitialMergeAsyncResponse(endpoint string, number int, out string, runErr error) (mergeAsyncResponse, error) { + conflictBody := false + if runErr != nil { // Surface status-based errors as GitHubAPIError so httpMergePR can // translate merge rejections for gh CLI users, matching the PAT path. - if code, ok := ghMergeStatusCode(err); ok { + if code, ok := ghMergeStatusCode(runErr); ok { if code != http.StatusConflict { - return "", &GitHubAPIError{StatusCode: code, Endpoint: endpoint, Body: err.Error()} + return mergeAsyncResponse{}, &GitHubAPIError{StatusCode: code, Endpoint: endpoint, Body: runErr.Error()} } - out = err.Error() + out = runErr.Error() conflictBody = true } else { - return "", fmt.Errorf("merge PR #%d: %w", number, err) + return mergeAsyncResponse{}, fmt.Errorf("merge PR #%d: %w", number, runErr) } } - var response mergeAsyncResponse jsonBody := out if start := strings.Index(jsonBody, "{"); start >= 0 { if end := strings.LastIndex(jsonBody, "}"); end >= start { jsonBody = jsonBody[start : end+1] } } else if conflictBody { - return "", &GitHubAPIError{StatusCode: http.StatusConflict, Endpoint: endpoint, Body: out} + return mergeAsyncResponse{}, &GitHubAPIError{StatusCode: http.StatusConflict, Endpoint: endpoint, Body: out} } + var response mergeAsyncResponse if unmarshalErr := json.Unmarshal([]byte(jsonBody), &response); unmarshalErr != nil { - return "", fmt.Errorf("decode GitHub merge response: %w", unmarshalErr) + return mergeAsyncResponse{}, fmt.Errorf("decode GitHub merge response: %w", unmarshalErr) } + return response, nil +} + +func (c *GHClient) pollMergeAsyncResponse( + ctx context.Context, + endpoint string, + number int, + response mergeAsyncResponse, +) (mergeAsyncResponse, error) { for response.Status == "pending" { - if response.UUID == "" { - return "", fmt.Errorf("GitHub merge response is pending without a UUID") + if response.Details.UUID == "" { + return mergeAsyncResponse{}, fmt.Errorf("GitHub merge response is pending without a UUID") } - result, runErr := c.run(ctx, "api", endpoint+"/"+response.UUID) + wait := c.mergePollWait + if wait == nil { + wait = waitForMergePoll + } + if waitErr := wait(ctx, mergePollInterval); waitErr != nil { + return mergeAsyncResponse{}, fmt.Errorf("wait to poll merge PR #%d: %w", number, waitErr) + } + result, runErr := c.run(ctx, "api", endpoint+"/"+response.Details.UUID) if runErr != nil { - return "", fmt.Errorf("poll merge PR #%d: %w", number, runErr) + return mergeAsyncResponse{}, fmt.Errorf("poll merge PR #%d: %w", number, runErr) } if unmarshalErr := json.Unmarshal([]byte(result), &response); unmarshalErr != nil { - return "", fmt.Errorf("decode GitHub merge response: %w", unmarshalErr) + return mergeAsyncResponse{}, fmt.Errorf("decode GitHub merge response: %w", unmarshalErr) } } - if response.Status == mergeStatusFailed { - return "", fmt.Errorf("GitHub rejected merge: %s", response.Message) - } - return normalizeMergeOutcome(response.Status) + return response, nil } // ghMergeStatusCode extracts the HTTP status code from a gh CLI merge error. diff --git a/apps/backend/internal/github/gh_client_commands_test.go b/apps/backend/internal/github/gh_client_commands_test.go index 3119c8cc5e..8fa61e6049 100644 --- a/apps/backend/internal/github/gh_client_commands_test.go +++ b/apps/backend/internal/github/gh_client_commands_test.go @@ -531,7 +531,7 @@ func TestGHClient_SubmitReview(t *testing.T) { func TestGHClient_MergePR(t *testing.T) { t.Run("sends the merge method", func(t *testing.T) { calls := newFakeGH(t, ghResponse{Prefix: "api repos/", Stdout: `{"status":"merged"}`}) - if _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "squash"); err != nil { + if _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "squash"}); err != nil { t.Fatalf("MergePR: %v", err) } assertGHArgv(t, calls(t), 0, []string{ @@ -541,7 +541,7 @@ func TestGHClient_MergePR(t *testing.T) { }) t.Run("omits the merge method when unset", func(t *testing.T) { calls := newFakeGH(t, ghResponse{Prefix: "api repos/", Stdout: `{"status":"merged"}`}) - if _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, ""); err != nil { + if _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{}); err != nil { t.Fatalf("MergePR: %v", err) } assertGHArgv(t, calls(t), 0, []string{ @@ -551,6 +551,19 @@ func TestGHClient_MergePR(t *testing.T) { }) } +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.1 +func TestGHClient_MergePR_SendsExpectedHeadSHA(t *testing.T) { + calls := newFakeGH(t, ghResponse{Prefix: "api repos/", Stdout: `{"status":"merged"}`}) + request := MergePRRequest{MergeMethod: "squash", ExpectedHeadSHA: "abc123"} + if _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, request); err != nil { + t.Fatalf("MergePR: %v", err) + } + assertGHArgv(t, calls(t), 0, []string{ + "api", "repos/acme/widget/pulls/42/merge-async", "-X", "PUT", + "-f", "merge_action=default", "-f", "merge_method=squash", "-f", "sha=abc123", + }) +} + // The gh path must surface merge rejections as *GitHubAPIError so httpMergePR // can translate them the same way it does for the PAT client. func TestGHClient_MergePR_StatusIsRecoverable(t *testing.T) { @@ -566,7 +579,7 @@ func TestGHClient_MergePR_StatusIsRecoverable(t *testing.T) { for _, tc := range cases { t.Run(http.StatusText(tc.want), func(t *testing.T) { newFakeGH(t, ghResponse{Prefix: "api repos/", Stderr: tc.stderr, Exit: 1}) - _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "merge") + _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "merge"}) var apiErr *GitHubAPIError if !errors.As(err, &apiErr) { t.Fatalf("err = %v, want a *GitHubAPIError", err) @@ -583,18 +596,75 @@ func TestGHClient_MergePR_StatusIsRecoverable(t *testing.T) { func TestGHClient_MergePR_AlreadyQueuedIsIdempotent(t *testing.T) { newFakeGH(t, - ghResponse{Prefix: "api repos/acme/widget/pulls/42/merge-async/request-1", Stdout: `{"status":"enqueued","uuid":"request-1"}`}, - ghResponse{Prefix: "api repos/", Stderr: `{"status":"pending","uuid":"request-1"} gh: HTTP 409`, Exit: 1}, + ghResponse{Prefix: "api repos/acme/widget/pulls/42/merge-async/request-1", Stdout: `{"status":"enqueued","details":{"message":"Pull request was added to the merge queue."}}`}, + ghResponse{Prefix: "api repos/", Stderr: `{"status":"pending","details":{"message":"Merge request enqueued.","uuid":"request-1","merge_method":"squash","merge_action":"default","expected_head_sha":"abc123"}} gh: HTTP 409`, Exit: 1}, ) - outcome, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "squash") + outcome, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "squash"}) if err != nil || outcome != MergeOutcomeQueued { t.Fatalf("outcome = %q, err = %v, want queued", outcome, err) } } +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-001.3 +func TestGHClient_MergePR_PollsNestedPendingResponse(t *testing.T) { + calls := newFakeGH(t, + ghResponse{Prefix: "api repos/acme/widget/pulls/42/merge-async/request-1", Stdout: `{"status":"merged","details":{"sha":"abc123"}}`}, + ghResponse{Prefix: "api repos/", Stdout: `{"status":"pending","details":{"message":"Merge request enqueued.","uuid":"request-1","merge_method":"squash","merge_action":"default","expected_head_sha":"abc123"}}`}, + ) + + outcome, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "squash"}) + if err != nil || outcome != MergeOutcomeMerged { + t.Fatalf("outcome = %q, err = %v, want merged", outcome, err) + } + assertGHArgv(t, calls(t), 1, []string{ + "api", "repos/acme/widget/pulls/42/merge-async/request-1", + }) +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.7 +func TestGHClient_MergePR_PendingPollHonorsContextBeforeNextRequest(t *testing.T) { + calls := newFakeGH(t, ghResponse{ + Prefix: "api repos/", + Stdout: `{"status":"pending","details":{"uuid":"request-1"}}`, + }) + client := NewGHClient() + var gotDelay time.Duration + client.mergePollWait = func(_ context.Context, delay time.Duration) error { + gotDelay = delay + return context.DeadlineExceeded + } + + _, err := client.MergePR( + context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "squash"}, + ) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("MergePR error = %v, want context deadline", err) + } + if got := len(calls(t)); got != 1 { + t.Fatalf("request count = %d, want only the initial request before cancellation", got) + } + if gotDelay != time.Second { + t.Fatalf("poll delay = %s, want one second", gotDelay) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-001.13 +func TestGHClient_MergePR_ReportsNestedFailureMessage(t *testing.T) { + newFakeGH(t, ghResponse{ + Prefix: "api repos/", + Stdout: `{"status":"failed","details":{"message":"Branch protection blocked the merge.","expected_head_sha":"abc123"}}`, + }) + + _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "squash"}) + if err == nil || !strings.Contains(err.Error(), "Branch protection blocked the merge.") || + !strings.Contains(err.Error(), "abc123") { + t.Fatalf("err = %v, want the nested provider message and expected head", err) + } +} + func TestGHClient_MergePR_UnmappedStatusFallsBackToAPlainWrap(t *testing.T) { newFakeGH(t, ghResponse{Prefix: "api repos/", Stderr: "gh: HTTP 500: server error", Exit: 1}) - _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "merge") + _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "merge"}) if err == nil || !strings.Contains(err.Error(), "merge PR #42") { t.Fatalf("err = %v, want a plain wrap naming the PR", err) } diff --git a/apps/backend/internal/github/mock_client.go b/apps/backend/internal/github/mock_client.go index 4590bcd7a6..44950a7019 100644 --- a/apps/backend/internal/github/mock_client.go +++ b/apps/backend/internal/github/mock_client.go @@ -2,6 +2,7 @@ package github import ( "context" + "errors" "fmt" "sort" "strings" @@ -58,10 +59,11 @@ type requestedReviewers struct { // mergedPR records a MergePR call for test assertions. type mergedPR struct { - Owner string `json:"owner"` - Repo string `json:"repo"` - Number int `json:"number"` - MergeMethod string `json:"merge_method"` + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + MergeMethod string `json:"merge_method"` + ExpectedHeadSHA string `json:"expected_head_sha"` } // mockPRMergeQueueState is the provider-side queue snapshot used by the E2E @@ -134,6 +136,7 @@ type MockClient struct { requestedReviews []requestedReviewers mergedPRs []mergedPR mergeOutcomes map[prKey]MergeOutcome + mergeFailures map[prKey]string mergeMethods map[repoKey]RepoMergeMethods repositoryDetails map[repoKey]*GitHubRepository gists map[string]mockGist @@ -187,6 +190,7 @@ func NewMockClient() *MockClient { commitDetails: make(map[commitDetailKey]PRCommitDetail), mergeMethods: make(map[repoKey]RepoMergeMethods), mergeOutcomes: make(map[prKey]MergeOutcome), + mergeFailures: make(map[prKey]string), repositoryDetails: make(map[repoKey]*GitHubRepository), gists: make(map[string]mockGist), repoFiles: make(map[repoKey][]repoFileEntry), @@ -768,13 +772,27 @@ func (m *MockClient) SetRepoMergeMethods(owner, repo string, methods RepoMergeMe m.mergeMethods[repoKey{owner, repo}] = methods } -func (m *MockClient) MergePR(_ context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) { +func (m *MockClient) MergePR(_ context.Context, owner, repo string, number int, request MergePRRequest) (MergeOutcome, error) { m.mu.Lock() defer m.mu.Unlock() m.mergedPRs = append(m.mergedPRs, mergedPR{ - Owner: owner, Repo: repo, Number: number, MergeMethod: mergeMethod, + Owner: owner, Repo: repo, Number: number, MergeMethod: request.MergeMethod, + ExpectedHeadSHA: request.ExpectedHeadSHA, }) - outcome := m.mergeOutcomes[prKey{owner, repo, number}] + key := prKey{owner, repo, number} + if request.ExpectedHeadSHA != "" { + pr, ok := m.prs[key] + if !ok { + return "", fmt.Errorf("pull request not found for expected head %s", request.ExpectedHeadSHA) + } + if pr.HeadSHA != request.ExpectedHeadSHA { + return "", fmt.Errorf("pull request head changed: expected %s, current %s", request.ExpectedHeadSHA, pr.HeadSHA) + } + } + if message := m.mergeFailures[key]; message != "" { + return "", errors.New(message) + } + outcome := m.mergeOutcomes[key] if outcome == "" { outcome = MergeOutcomeMerged } @@ -793,7 +811,18 @@ func (m *MockClient) MergePR(_ context.Context, owner, repo string, number int, func (m *MockClient) SetMergeOutcome(owner, repo string, number int, outcome MergeOutcome) { m.mu.Lock() defer m.mu.Unlock() - m.mergeOutcomes[prKey{owner, repo, number}] = outcome + key := prKey{owner, repo, number} + m.mergeOutcomes[key] = outcome + delete(m.mergeFailures, key) +} + +// SetMergeFailure configures a provider diagnostic for one pull request. +func (m *MockClient) SetMergeFailure(owner, repo string, number int, message string) { + m.mu.Lock() + defer m.mu.Unlock() + key := prKey{owner, repo, number} + m.mergeFailures[key] = strings.TrimSpace(message) + delete(m.mergeOutcomes, key) } func (m *MockClient) CreateGist(_ context.Context, in CreateGistInput) (*GistResponse, error) { @@ -1104,6 +1133,7 @@ func (m *MockClient) Reset() { m.requestedReviews = nil m.mergedPRs = nil m.mergeOutcomes = make(map[prKey]MergeOutcome) + m.mergeFailures = make(map[prKey]string) m.mergeMethods = make(map[repoKey]RepoMergeMethods) m.gists = make(map[string]mockGist) m.deletedGists = nil diff --git a/apps/backend/internal/github/mock_client_merge_test.go b/apps/backend/internal/github/mock_client_merge_test.go new file mode 100644 index 0000000000..f9f6f64045 --- /dev/null +++ b/apps/backend/internal/github/mock_client_merge_test.go @@ -0,0 +1,36 @@ +package github + +import ( + "context" + "strings" + "testing" +) + +func TestMockClientMergePRRecordsExpectedHeadAndRejectsMismatch(t *testing.T) { + client := NewMockClient() + client.AddPR(&PR{RepoOwner: "acme", RepoName: "demo", Number: 42, HeadSHA: "head-current"}) + + _, err := client.MergePR(context.Background(), "acme", "demo", 42, MergePRRequest{ + MergeMethod: "squash", ExpectedHeadSHA: "head-reviewed", + }) + if err == nil || !strings.Contains(err.Error(), "head") { + t.Fatalf("head mismatch error = %v, want head diagnostic", err) + } + attempts := client.MergedPRs() + if len(attempts) != 1 || attempts[0].ExpectedHeadSHA != "head-reviewed" { + t.Fatalf("attempts = %+v, want reviewed head", attempts) + } +} + +func TestMockClientMergePRReturnsConfiguredFailure(t *testing.T) { + client := NewMockClient() + client.AddPR(&PR{RepoOwner: "acme", RepoName: "demo", Number: 42, HeadSHA: "head-current"}) + client.SetMergeFailure("acme", "demo", 42, "provider unavailable") + + _, err := client.MergePR(context.Background(), "acme", "demo", 42, MergePRRequest{ + MergeMethod: "squash", ExpectedHeadSHA: "head-current", + }) + if err == nil || !strings.Contains(err.Error(), "provider unavailable") { + t.Fatalf("configured failure = %v, want provider diagnostic", err) + } +} diff --git a/apps/backend/internal/github/mock_controller.go b/apps/backend/internal/github/mock_controller.go index eeb60d8fa2..3a25dc6372 100644 --- a/apps/backend/internal/github/mock_controller.go +++ b/apps/backend/internal/github/mock_controller.go @@ -523,21 +523,38 @@ func (c *MockController) setPRCommitsFailures(ctx *gin.Context) { func (c *MockController) setMergeOutcome(ctx *gin.Context) { var req struct { - Owner string `json:"owner"` - Repo string `json:"repo"` - Number int `json:"number"` - Outcome MergeOutcome `json:"outcome"` + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + Outcome string `json:"outcome"` } if err := ctx.ShouldBindJSON(&req); err != nil || strings.TrimSpace(req.Owner) == "" || - strings.TrimSpace(req.Repo) == "" || req.Number <= 0 || - (req.Outcome != MergeOutcomeMerged && req.Outcome != MergeOutcomeQueued) { + strings.TrimSpace(req.Repo) == "" || req.Number <= 0 || !validMockMergeOutcome(req.Outcome) { respondInvalidPayload(ctx) return } - c.mock.SetMergeOutcome(req.Owner, req.Repo, req.Number, req.Outcome) + switch req.Outcome { + case "failed": + c.mock.SetMergeFailure(req.Owner, req.Repo, req.Number, "mock merge provider unavailable") + case "pending": + c.mock.SetMergeFailure(req.Owner, req.Repo, req.Number, "mock merge request remained pending") + case "head_mismatch": + c.mock.SetMergeFailure(req.Owner, req.Repo, req.Number, "mock merge head mismatch") + default: + c.mock.SetMergeOutcome(req.Owner, req.Repo, req.Number, MergeOutcome(req.Outcome)) + } ctx.JSON(http.StatusOK, gin.H{"outcome": req.Outcome}) } +func validMockMergeOutcome(outcome string) bool { + switch outcome { + case string(MergeOutcomeMerged), string(MergeOutcomeQueued), "failed", "pending", "head_mismatch": + return true + default: + return false + } +} + type mockMergeQueueTransitionRequest struct { TaskID string `json:"task_id"` Owner string `json:"owner"` diff --git a/apps/backend/internal/github/models.go b/apps/backend/internal/github/models.go index 9378cba3a0..f05d717b8a 100644 --- a/apps/backend/internal/github/models.go +++ b/apps/backend/internal/github/models.go @@ -730,6 +730,8 @@ type TaskCIPRAutomationState struct { AutoFixExhaustedAt *time.Time `json:"auto_fix_exhausted_at" db:"auto_fix_exhausted_at"` LastMergeSignature string `json:"last_merge_signature" db:"last_merge_signature"` LastMergeAttemptAt *time.Time `json:"last_merge_attempt_at,omitempty" db:"last_merge_attempt_at"` + LastMergeResult string `json:"last_merge_result" db:"last_merge_result"` + MergeRetryPending bool `json:"-" db:"merge_retry_pending"` LastQueueAttemptHeadSHA string `json:"last_queue_attempt_head_sha" db:"last_queue_attempt_head_sha"` LastQueueFixEventID string `json:"last_queue_fix_event_id" db:"last_queue_fix_event_id"` LastQueueRemovalCause string `json:"last_queue_removal_cause" db:"last_queue_removal_cause"` @@ -740,10 +742,19 @@ type TaskCIPRAutomationState struct { LastLifecyclePromptAt *time.Time `json:"last_lifecycle_prompt_at,omitempty" db:"last_lifecycle_prompt_at"` LastLifecycleSessionID *string `json:"last_lifecycle_session_id,omitempty" db:"last_lifecycle_session_id"` LastError *string `json:"last_error,omitempty" db:"last_error"` + LastErrorKind string `json:"last_error_kind" db:"last_error_kind"` CreatedAt time.Time `json:"created_at" db:"created_at"` UpdatedAt time.Time `json:"updated_at" db:"updated_at"` } +const ( + TaskCIMergeResultInFlight = "in_flight" + TaskCIMergeResultFailed = "failed" + TaskCIMergeResultAccepted = "accepted" + TaskCIErrorKindAutoMerge = "auto_merge" + TaskCIErrorKindAutoFix = "auto_fix" +) + // TaskCIFixAttempt records an auto-fix prompt attempt for a task PR. type TaskCIFixAttempt struct { TaskID string @@ -779,6 +790,7 @@ type TaskCIMergeQueueObservation struct { RemovalEventID string RemovalCause string RemovalObservedHeadSHA string + Accepted bool } // TaskPRLifecyclePrompt records an accepted lifecycle prompt checkpoint. diff --git a/apps/backend/internal/github/noop_client.go b/apps/backend/internal/github/noop_client.go index 79bb8bea24..0b76c9b9ea 100644 --- a/apps/backend/internal/github/noop_client.go +++ b/apps/backend/internal/github/noop_client.go @@ -125,7 +125,7 @@ func (c *NoopClient) RequestReviewers(context.Context, string, string, int, []st return ErrNoClient } -func (c *NoopClient) MergePR(context.Context, string, string, int, string) (MergeOutcome, error) { +func (c *NoopClient) MergePR(context.Context, string, string, int, MergePRRequest) (MergeOutcome, error) { return "", ErrNoClient } diff --git a/apps/backend/internal/github/pat_client.go b/apps/backend/internal/github/pat_client.go index 82e5aab0ae..0a4b003325 100644 --- a/apps/backend/internal/github/pat_client.go +++ b/apps/backend/internal/github/pat_client.go @@ -728,11 +728,14 @@ func (c *PATClient) RequestReviewers(ctx context.Context, owner, repo string, nu return c.post(ctx, endpoint, jsonBody) } -func (c *PATClient) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) { +func (c *PATClient) MergePR(ctx context.Context, owner, repo string, number int, request MergePRRequest) (MergeOutcome, error) { endpoint := fmt.Sprintf("/repos/%s/%s/pulls/%d/merge-async", owner, repo, number) payload := map[string]string{"merge_action": "default"} - if mergeMethod != "" { - payload["merge_method"] = mergeMethod + if request.MergeMethod != "" { + payload["merge_method"] = request.MergeMethod + } + if request.ExpectedHeadSHA != "" { + payload["sha"] = request.ExpectedHeadSHA } jsonBody, err := json.Marshal(payload) if err != nil { @@ -742,20 +745,27 @@ func (c *PATClient) MergePR(ctx context.Context, owner, repo string, number int, if err := c.putJSON(ctx, endpoint, jsonBody, &response); err != nil { var apiErr *GitHubAPIError if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusConflict || - json.Unmarshal([]byte(apiErr.Body), &response) != nil || response.UUID == "" { + json.Unmarshal([]byte(apiErr.Body), &response) != nil || response.Details.UUID == "" { return "", err } } for response.Status == "pending" { - if response.UUID == "" { + if response.Details.UUID == "" { return "", fmt.Errorf("GitHub merge response is pending without a UUID") } - if err := c.get(ctx, endpoint+"/"+response.UUID, &response); err != nil { + wait := c.mergePollWait + if wait == nil { + wait = waitForMergePoll + } + if waitErr := wait(ctx, mergePollInterval); waitErr != nil { + return "", fmt.Errorf("wait to poll merge PR #%d: %w", number, waitErr) + } + if err := c.get(ctx, endpoint+"/"+response.Details.UUID, &response); err != nil { return "", err } } if response.Status == mergeStatusFailed { - return "", fmt.Errorf("GitHub rejected merge: %s", response.Message) + return "", newMergeFailureError(response.Details) } return normalizeMergeOutcome(response.Status) } diff --git a/apps/backend/internal/github/pat_client_writes_test.go b/apps/backend/internal/github/pat_client_writes_test.go index 1fc83e22f9..b9e73f79ec 100644 --- a/apps/backend/internal/github/pat_client_writes_test.go +++ b/apps/backend/internal/github/pat_client_writes_test.go @@ -75,7 +75,7 @@ func TestPATClient_MergePR(t *testing.T) { c, requests := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { return http.StatusOK, `{"status":"merged"}` }) - if _, err := c.MergePR(context.Background(), "acme", "widget", 42, "squash"); err != nil { + if _, err := c.MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "squash"}); err != nil { t.Fatalf("MergePR: %v", err) } got := (*requests)[0] @@ -93,7 +93,7 @@ func TestPATClient_MergePR(t *testing.T) { c, requests := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { return http.StatusOK, `{"status":"merged"}` }) - if _, err := c.MergePR(context.Background(), "acme", "widget", 42, ""); err != nil { + if _, err := c.MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{}); err != nil { t.Fatalf("MergePR: %v", err) } if (*requests)[0].Body != `{"merge_action":"default"}` { @@ -101,6 +101,18 @@ func TestPATClient_MergePR(t *testing.T) { (*requests)[0].Body) } }) + t.Run("sends the expected head SHA", func(t *testing.T) { + c, requests := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { + return http.StatusOK, `{"status":"merged"}` + }) + request := MergePRRequest{MergeMethod: "squash", ExpectedHeadSHA: "abc123"} + if _, err := c.MergePR(context.Background(), "acme", "widget", 42, request); err != nil { + t.Fatalf("MergePR: %v", err) + } + if got := (*requests)[0].Body; got != `{"merge_action":"default","merge_method":"squash","sha":"abc123"}` { + t.Errorf("body = %q, want expected head SHA", got) + } + }) } // A merge rejection must stay recoverable as *GitHubAPIError so the HTTP @@ -111,7 +123,7 @@ func TestPATClient_MergePR_StatusIsRecoverable(t *testing.T) { c, _ := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { return status, `{"message":"Pull Request is not mergeable"}` }) - _, err := c.MergePR(context.Background(), "acme", "widget", 42, "merge") + _, err := c.MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "merge"}) var apiErr *GitHubAPIError if !errors.As(err, &apiErr) { t.Fatalf("err = %v, want a *GitHubAPIError", err) @@ -132,16 +144,70 @@ func TestPATClient_MergePR_StatusIsRecoverable(t *testing.T) { func TestPATClient_MergePR_AlreadyQueuedIsIdempotent(t *testing.T) { c, _ := newRecordingPATServerFunc(t, func(r *http.Request) (int, string) { if r.Method == http.MethodGet { - return http.StatusOK, `{"status":"enqueued","uuid":"request-1"}` + return http.StatusOK, `{"status":"enqueued","details":{"message":"Pull request was added to the merge queue."}}` } - return http.StatusConflict, `{"status":"pending","uuid":"request-1"}` + return http.StatusConflict, `{"status":"pending","details":{"message":"Merge request enqueued.","uuid":"request-1","merge_method":"squash","merge_action":"default","expected_head_sha":"abc123"}}` }) - outcome, err := c.MergePR(context.Background(), "acme", "widget", 42, "squash") + outcome, err := c.MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "squash"}) if err != nil || outcome != MergeOutcomeQueued { t.Fatalf("outcome = %q, err = %v, want queued", outcome, err) } } +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-001.3 +func TestPATClient_MergePR_PollsNestedPendingResponse(t *testing.T) { + c, requests := newRecordingPATServerFunc(t, func(r *http.Request) (int, string) { + if r.Method == http.MethodGet { + return http.StatusOK, `{"status":"merged","details":{"sha":"abc123"}}` + } + return http.StatusAccepted, `{"status":"pending","details":{"message":"Merge request enqueued.","uuid":"request-1","merge_method":"squash","merge_action":"default","expected_head_sha":"abc123"}}` + }) + + outcome, err := c.MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "squash"}) + if err != nil || outcome != MergeOutcomeMerged { + t.Fatalf("outcome = %q, err = %v, want merged", outcome, err) + } + if len(*requests) != 2 || (*requests)[1].Path != "/repos/acme/widget/pulls/42/merge-async/request-1" { + t.Fatalf("requests = %#v, want a poll for request-1", *requests) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.7 +func TestPATClient_MergePR_PendingPollHonorsContextBeforeNextRequest(t *testing.T) { + c, requests := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { + return http.StatusAccepted, `{"status":"pending","details":{"uuid":"request-1"}}` + }) + var gotDelay time.Duration + c.mergePollWait = func(_ context.Context, delay time.Duration) error { + gotDelay = delay + return context.DeadlineExceeded + } + + _, err := c.MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "squash"}) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("MergePR error = %v, want context deadline", err) + } + if got := len(*requests); got != 1 { + t.Fatalf("request count = %d, want only the initial request before cancellation", got) + } + if gotDelay != time.Second { + t.Fatalf("poll delay = %s, want one second", gotDelay) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-001.13 +func TestPATClient_MergePR_ReportsNestedFailureMessage(t *testing.T) { + c, _ := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { + return http.StatusOK, `{"status":"failed","details":{"message":"Branch protection blocked the merge.","expected_head_sha":"abc123"}}` + }) + + _, err := c.MergePR(context.Background(), "acme", "widget", 42, MergePRRequest{MergeMethod: "squash"}) + if err == nil || !strings.Contains(err.Error(), "Branch protection blocked the merge.") || + !strings.Contains(err.Error(), "abc123") { + t.Fatalf("err = %v, want the nested provider message and expected head", err) + } +} + func TestPATClient_CreateGist(t *testing.T) { c, requests := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { return http.StatusCreated, `{"id":"abc123","html_url":"https://gist.github.com/abc123", diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index bbb210e891..dd987ac46a 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -333,6 +333,17 @@ func (s *Service) ListTaskPRsByTaskIDs(ctx context.Context, taskIDs []string) (m return s.store.ListTaskPRsByTaskIDs(ctx, taskIDs) } +// ListTaskPRAutomationOptionsByTaskIDs forwards bounded per-PR automation +// hydration for task-summary projection without exposing the GitHub store. +func (s *Service) ListTaskPRAutomationOptionsByTaskIDs( + ctx context.Context, taskIDs []string, +) (map[string][]*TaskPRAutomationOptions, error) { + if s.store == nil { + return map[string][]*TaskPRAutomationOptions{}, nil + } + return s.store.ListTaskPRAutomationOptionsByTaskIDs(ctx, taskIDs) +} + // TestEventBus returns the event bus for test/mock use only. func (s *Service) TestEventBus() bus.EventBus { return s.eventBus diff --git a/apps/backend/internal/github/service_automation_auth.go b/apps/backend/internal/github/service_automation_auth.go index a4f5ed4af2..3b6f5b311a 100644 --- a/apps/backend/internal/github/service_automation_auth.go +++ b/apps/backend/internal/github/service_automation_auth.go @@ -185,8 +185,11 @@ func (s *Service) MergePRForAutomation( ctx context.Context, workspaceID, owner, repo string, number int, - mergeMethod string, + mergeMethod, expectedHeadSHA string, ) error { + if strings.TrimSpace(expectedHeadSHA) == "" { + return fmt.Errorf("automatic merge requires an expected head SHA") + } if err := s.ensureRepositoryInWorkspaceScope(ctx, workspaceID, owner, repo); err != nil { return err } @@ -198,7 +201,9 @@ func (s *Service) MergePRForAutomation( return err } _, err = s.mergePRWithClient( - ctx, resolved.Client, resolved.CacheScope, owner, repo, number, mergeMethod, + ctx, resolved.Client, resolved.CacheScope, owner, repo, number, MergePRRequest{ + MergeMethod: mergeMethod, ExpectedHeadSHA: expectedHeadSHA, + }, ) return err } diff --git a/apps/backend/internal/github/service_automation_merge_test.go b/apps/backend/internal/github/service_automation_merge_test.go new file mode 100644 index 0000000000..3ecbdcd12a --- /dev/null +++ b/apps/backend/internal/github/service_automation_merge_test.go @@ -0,0 +1,18 @@ +package github + +import ( + "context" + "strings" + "testing" +) + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.1 +func TestMergePRForAutomationRequiresExpectedHeadSHA(t *testing.T) { + service := &Service{} + err := service.MergePRForAutomation( + context.Background(), "workspace-1", "acme", "widget", 42, "squash", "", + ) + if err == nil || !strings.Contains(err.Error(), "expected head SHA") { + t.Fatalf("error = %v, want expected-head validation", err) + } +} diff --git a/apps/backend/internal/github/service_ci_automation.go b/apps/backend/internal/github/service_ci_automation.go index f075a9326e..f7b594a1fa 100644 --- a/apps/backend/internal/github/service_ci_automation.go +++ b/apps/backend/internal/github/service_ci_automation.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "strings" + "time" promptcfg "github.com/kandev/kandev/config/prompts" ) @@ -82,6 +83,42 @@ func (s *Service) UpdateTaskCIOptions(ctx context.Context, taskID string, patch return response, err } +// RetryTaskCIAutoMerge authorizes one new evaluation of a failed automatic +// merge for the exact linked PR. The caller publishes the returned PR through +// the normal PR-updated event path; this method never calls GitHub. +func (s *Service) RetryTaskCIAutoMerge( + ctx context.Context, taskID, repositoryID string, prNumber int, +) (*TaskPR, error) { + if s.store == nil { + return nil, errStoreUnavailable + } + if _, err := s.resolveAuthorizedTaskWorkspace(ctx, taskID); err != nil { + return nil, err + } + targets, err := s.resolveTaskPRAutomationTargets(ctx, taskID, &repositoryID, &prNumber) + if err != nil { + return nil, err + } + if len(targets) != 1 { + return nil, ErrTaskPRNotLinked + } + if err := s.store.AuthorizeTaskCIMergeRetry(ctx, taskID, repositoryID, prNumber, time.Now().UTC()); err != nil { + return nil, err + } + return targets[0], nil +} + +// ClearTaskCIMergeRetryAuthorization removes a retry authorization that could +// not be delivered to the automation evaluator. +func (s *Service) ClearTaskCIMergeRetryAuthorization( + ctx context.Context, taskID, repositoryID string, prNumber int, +) error { + if s.store == nil { + return errStoreUnavailable + } + return s.store.ClearTaskCIMergeRetryAuthorization(ctx, taskID, repositoryID, prNumber) +} + // resolveAuthorizedTaskWorkspace resolves ownership through the task service // before any user-facing CI-option read or mutation. Unit tests and legacy // auth-disabled embeddings without a task store retain their unscoped path; @@ -240,6 +277,16 @@ func (s *Service) RecordTaskCIMergeAttempt(ctx context.Context, attempt TaskCIMe return s.store.RecordTaskCIMergeAttempt(ctx, attempt) } +// RecordTaskCIMergeAttemptResult completes a reserved auto-merge attempt. +func (s *Service) RecordTaskCIMergeAttemptResult( + ctx context.Context, taskID, repositoryID string, prNumber int, signature, result, message string, +) error { + if s.store == nil { + return errStoreUnavailable + } + return s.store.RecordTaskCIMergeAttemptResult(ctx, taskID, repositoryID, prNumber, signature, result, message) +} + // RecordTaskCIMergeQueueObservation persists active queue membership and the // latest removal cause so automation can recover safely after a restart. func (s *Service) RecordTaskCIMergeQueueObservation(ctx context.Context, observation TaskCIMergeQueueObservation) error { @@ -257,6 +304,14 @@ func (s *Service) RecordTaskCIError(ctx context.Context, taskID, repositoryID st return s.store.RecordTaskCIError(ctx, taskID, repositoryID, prNumber, message) } +// RecordTaskCIAutoMergeError records a typed automatic-merge error. +func (s *Service) RecordTaskCIAutoMergeError(ctx context.Context, taskID, repositoryID string, prNumber int, message string) error { + if s.store == nil { + return errStoreUnavailable + } + return s.store.RecordTaskCIAutoMergeError(ctx, taskID, repositoryID, prNumber, message) +} + // MarkTaskCIAutoFixExhausted records that auto-fix reached its per-PR round cap. func (s *Service) MarkTaskCIAutoFixExhausted(ctx context.Context, taskID, repositoryID string, prNumber int, message string) error { if s.store == nil { diff --git a/apps/backend/internal/github/service_pr.go b/apps/backend/internal/github/service_pr.go index 4cf5ecf94f..9ddec37360 100644 --- a/apps/backend/internal/github/service_pr.go +++ b/apps/backend/internal/github/service_pr.go @@ -128,7 +128,7 @@ func (s *Service) MergePR(ctx context.Context, owner, repo string, number int, m if s.client == nil { return "", ErrNoClient } - return s.mergePRWithClient(ctx, s.client, "legacy", owner, repo, number, mergeMethod) + return s.mergePRWithClient(ctx, s.client, "legacy", owner, repo, number, MergePRRequest{MergeMethod: mergeMethod}) } // MergePRForWorkspace performs a user-triggered merge using the workspace's @@ -153,7 +153,7 @@ func (s *Service) MergePRForWorkspace( return AuthPrincipal{}, "", err } outcome, err := s.mergePRWithClient( - ctx, resolved.Client, resolved.CacheScope, owner, repo, number, mergeMethod, + ctx, resolved.Client, resolved.CacheScope, owner, repo, number, MergePRRequest{MergeMethod: mergeMethod}, ) return resolved.Principal, outcome, err } @@ -165,9 +165,9 @@ func (s *Service) mergePRWithClient( owner string, repo string, number int, - mergeMethod string, + request MergePRRequest, ) (MergeOutcome, error) { - if mergeMethod == "" { + if request.MergeMethod == "" { // Resolve to an allowed method up-front so we don't rely on GitHub's // "default to merge" behavior, which 405s on repos that disallow // merge commits (squash-only / rebase-only). Best-effort: if the @@ -176,11 +176,11 @@ func (s *Service) mergePRWithClient( // blocking the merge attempt. if methods, err := s.getRepoMergeMethods(ctx, client, cacheScope, owner, repo); err == nil { if pick := pickDefaultMergeMethod(methods); pick != "" { - mergeMethod = pick + request.MergeMethod = pick } } } - return client.MergePR(ctx, owner, repo, number, mergeMethod) + return client.MergePR(ctx, owner, repo, number, request) } // GetRepoMergeMethods returns the merge methods a repo allows, cached for diff --git a/apps/backend/internal/github/service_test.go b/apps/backend/internal/github/service_test.go index 4e051d2dce..19d207b59c 100644 --- a/apps/backend/internal/github/service_test.go +++ b/apps/backend/internal/github/service_test.go @@ -762,9 +762,13 @@ type mockEventBus struct { mu sync.Mutex events []*bus.Event publishedCh chan struct{} + err error } func (m *mockEventBus) Publish(_ context.Context, _ string, event *bus.Event) error { + if m.err != nil { + return m.err + } m.mu.Lock() m.events = append(m.events, event) m.mu.Unlock() diff --git a/apps/backend/internal/github/store.go b/apps/backend/internal/github/store.go index 2b9eda9d09..f9284bb3cf 100644 --- a/apps/backend/internal/github/store.go +++ b/apps/backend/internal/github/store.go @@ -418,6 +418,8 @@ const createTablesSQL = ` auto_fix_exhausted_at DATETIME, last_merge_signature TEXT NOT NULL DEFAULT '', last_merge_attempt_at DATETIME, + last_merge_result TEXT NOT NULL DEFAULT '', + merge_retry_pending BOOLEAN NOT NULL DEFAULT 0, last_queue_attempt_head_sha TEXT NOT NULL DEFAULT '', last_queue_fix_event_id TEXT NOT NULL DEFAULT '', last_queue_removal_cause TEXT NOT NULL DEFAULT '', @@ -428,6 +430,7 @@ const createTablesSQL = ` last_lifecycle_prompt_at DATETIME, last_lifecycle_session_id TEXT, last_error TEXT, + last_error_kind TEXT NOT NULL DEFAULT '', created_at DATETIME NOT NULL, updated_at DATETIME NOT NULL, PRIMARY KEY (task_id, repository_id, pr_number) @@ -1072,6 +1075,9 @@ func (s *Store) addTaskPRAgentAutomationColumns() error { {"closed_prompt_override", "ALTER TABLE github_task_ci_options ADD COLUMN closed_prompt_override TEXT"}, }, "github_task_ci_pr_state": { + {"last_merge_result", "ALTER TABLE github_task_ci_pr_state ADD COLUMN last_merge_result TEXT NOT NULL DEFAULT ''"}, + {"merge_retry_pending", "ALTER TABLE github_task_ci_pr_state ADD COLUMN merge_retry_pending BOOLEAN NOT NULL DEFAULT 0"}, + {"last_error_kind", "ALTER TABLE github_task_ci_pr_state ADD COLUMN last_error_kind TEXT NOT NULL DEFAULT ''"}, {"last_queue_attempt_head_sha", "ALTER TABLE github_task_ci_pr_state ADD COLUMN last_queue_attempt_head_sha TEXT NOT NULL DEFAULT ''"}, {"last_queue_fix_event_id", "ALTER TABLE github_task_ci_pr_state ADD COLUMN last_queue_fix_event_id TEXT NOT NULL DEFAULT ''"}, {"last_queue_removal_cause", "ALTER TABLE github_task_ci_pr_state ADD COLUMN last_queue_removal_cause TEXT NOT NULL DEFAULT ''"}, @@ -1097,6 +1103,20 @@ func (s *Store) addTaskPRAgentAutomationColumns() error { } } } + return s.backfillTaskCIAutoMergeErrorKinds() +} + +func (s *Store) backfillTaskCIAutoMergeErrorKinds() error { + _, err := s.db.Exec(` + UPDATE github_task_ci_pr_state + SET last_error_kind = ? + WHERE last_error_kind = '' + AND (last_error LIKE 'merge PR:%' + OR last_error LIKE 'PR status is not freshly synced for auto-merge%')`, + TaskCIErrorKindAutoMerge) + if err != nil { + return fmt.Errorf("backfill CI auto-merge error kinds: %w", err) + } return nil } @@ -2501,6 +2521,7 @@ func resetTaskCIAutoFixStateForTask( last_fix_enqueued_at = NULL, last_fix_session_id = NULL, last_error = CASE WHEN auto_fix_exhausted_at IS NOT NULL THEN NULL ELSE last_error END, + last_error_kind = CASE WHEN auto_fix_exhausted_at IS NOT NULL THEN '' ELSE last_error_kind END, auto_fix_exhausted_at = NULL, updated_at = ? WHERE task_id = ?`, now, taskID) @@ -2558,6 +2579,34 @@ func (s *Store) ListTaskPRAutomationOptions(ctx context.Context, taskID string) return out, nil } +// ListTaskPRAutomationOptionsByTaskIDs returns stored per-PR automation rows +// grouped by task. It is used by bounded task-summary hydration so a task list +// does not issue one options query for each visible row. +func (s *Store) ListTaskPRAutomationOptionsByTaskIDs( + ctx context.Context, taskIDs []string, +) (map[string][]*TaskPRAutomationOptions, error) { + if len(taskIDs) == 0 { + return make(map[string][]*TaskPRAutomationOptions), nil + } + query, args, err := sqlx.In( + `SELECT * FROM github_task_pr_automation_options WHERE task_id IN (?) ORDER BY task_id ASC, repository_id ASC, pr_number ASC`, + taskIDs, + ) + if err != nil { + return nil, err + } + query = s.ro.Rebind(query) + var rows []TaskPRAutomationOptions + if err := s.ro.SelectContext(ctx, &rows, query, args...); err != nil { + return nil, err + } + result := make(map[string][]*TaskPRAutomationOptions) + for i := range rows { + result[rows[i].TaskID] = append(result[rows[i].TaskID], &rows[i]) + } + return result, nil +} + // UpdateTaskPRAutomationOptions applies a partial update to one PR's // automation switches, upserting the row if absent. reviewerChanged mirrors // the task-wide reviewer rebind: the caller resolves it once (comparing the @@ -2701,6 +2750,7 @@ func resetTaskCIAutoFixState( last_fix_enqueued_at = NULL, last_fix_session_id = NULL, last_error = CASE WHEN auto_fix_exhausted_at IS NOT NULL THEN NULL ELSE last_error END, + last_error_kind = CASE WHEN auto_fix_exhausted_at IS NOT NULL THEN '' ELSE last_error_kind END, auto_fix_exhausted_at = NULL, updated_at = ? WHERE task_id = ? AND repository_id = ? AND pr_number = ?`, now, taskID, repositoryID, prNumber) @@ -2824,6 +2874,7 @@ func (s *Store) RecordTaskCIFixAttempt(ctx context.Context, attempt TaskCIFixAtt WHEN excluded.last_queue_removal_cause <> '' THEN excluded.last_queue_removal_cause ELSE github_task_ci_pr_state.last_queue_removal_cause END, last_error = NULL, + last_error_kind = '', updated_at = excluded.updated_at`, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber, attempt.Signature, attempt.CheckpointJSON, when, nullableString(attempt.SessionID), roundCount, @@ -2845,35 +2896,162 @@ func (s *Store) RefreshTaskCIFixCheckpoint(ctx context.Context, taskID, reposito last_fix_enqueued_at = NULL, last_fix_session_id = NULL, last_error = NULL, + last_error_kind = '', updated_at = excluded.updated_at`, taskID, repositoryID, prNumber, signature, checkpointJSON, now, now) return err }) } -// RecordTaskCIMergeAttempt records an auto-merge attempt signature. +var ErrTaskCIMergeAttemptAlreadyReserved = errors.New("CI auto-merge attempt already reserved") +var ErrTaskCIMergeAttemptNotFound = errors.New("CI auto-merge attempt not found") +var ErrTaskCIMergeRetryNotAllowed = errors.New("CI auto-merge retry is not available") + +const taskCIMergeRetryInFlightTTL = 2 * time.Minute + +// AuthorizeTaskCIMergeRetry persists one single-use authorization to +// reevaluate a failed or expired automatic merge attempt. +func (s *Store) AuthorizeTaskCIMergeRetry( + ctx context.Context, taskID, repositoryID string, prNumber int, requestedAt time.Time, +) error { + if requestedAt.IsZero() { + requestedAt = time.Now().UTC() + } + expiredBefore := requestedAt.Add(-taskCIMergeRetryInFlightTTL) + return s.mutateTaskCIPRState(ctx, taskID, func(ctx context.Context, tx *sqlx.Tx, now time.Time) error { + result, err := tx.ExecContext(ctx, ` + UPDATE github_task_ci_pr_state SET + merge_retry_pending = 1, + updated_at = ? + WHERE task_id = ? AND repository_id = ? AND pr_number = ? + AND merge_retry_pending = 0 + AND ( + (last_merge_result = ? AND last_error_kind = ?) + OR (last_merge_result = ? AND last_merge_attempt_at <= ?) + )`, + now, taskID, repositoryID, prNumber, + TaskCIMergeResultFailed, TaskCIErrorKindAutoMerge, + TaskCIMergeResultInFlight, expiredBefore) + if err != nil { + return err + } + rows, err := result.RowsAffected() + if err != nil { + return err + } + if rows == 0 { + return ErrTaskCIMergeRetryNotAllowed + } + return nil + }) +} + +// ClearTaskCIMergeRetryAuthorization rolls back a one-shot retry authorization +// when its follow-up event could not be published. This keeps a failed publish +// from leaving the PR permanently blocked behind merge_retry_pending. +func (s *Store) ClearTaskCIMergeRetryAuthorization( + ctx context.Context, taskID, repositoryID string, prNumber int, +) error { + return s.mutateTaskCIPRState(ctx, taskID, func(ctx context.Context, tx *sqlx.Tx, now time.Time) error { + _, err := tx.ExecContext(ctx, ` + UPDATE github_task_ci_pr_state + SET merge_retry_pending = 0, updated_at = ? + WHERE task_id = ? AND repository_id = ? AND pr_number = ? + AND merge_retry_pending = 1`, + now, taskID, repositoryID, prNumber) + return err + }) +} + +// RecordTaskCIMergeAttempt reserves an auto-merge attempt signature before +// the provider side effect. An unchanged signature cannot replace an existing +// reservation or terminal result. func (s *Store) RecordTaskCIMergeAttempt(ctx context.Context, attempt TaskCIMergeAttempt) error { when := attempt.AttemptedAt if when.IsZero() { when = time.Now().UTC() } return s.mutateTaskCIPRState(ctx, attempt.TaskID, func(ctx context.Context, tx *sqlx.Tx, now time.Time) error { - _, err := tx.ExecContext(ctx, ` + result, err := tx.ExecContext(ctx, ` INSERT INTO github_task_ci_pr_state ( task_id, repository_id, pr_number, last_merge_signature, last_merge_attempt_at, - last_queue_attempt_head_sha, created_at, updated_at - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?) + last_merge_result, merge_retry_pending, last_queue_attempt_head_sha, created_at, updated_at + ) VALUES (?, ?, ?, ?, ?, ?, 0, ?, ?, ?) ON CONFLICT(task_id, repository_id, pr_number) DO UPDATE SET last_merge_signature = excluded.last_merge_signature, last_merge_attempt_at = excluded.last_merge_attempt_at, + last_merge_result = excluded.last_merge_result, + merge_retry_pending = 0, last_queue_attempt_head_sha = CASE WHEN excluded.last_queue_attempt_head_sha <> '' THEN excluded.last_queue_attempt_head_sha ELSE github_task_ci_pr_state.last_queue_attempt_head_sha END, - last_error = NULL, - updated_at = excluded.updated_at`, + last_error = CASE WHEN github_task_ci_pr_state.last_error_kind = ? + THEN NULL ELSE github_task_ci_pr_state.last_error END, + last_error_kind = CASE WHEN github_task_ci_pr_state.last_error_kind = ? + THEN '' ELSE github_task_ci_pr_state.last_error_kind END, + updated_at = excluded.updated_at + WHERE github_task_ci_pr_state.last_merge_signature <> excluded.last_merge_signature + OR github_task_ci_pr_state.merge_retry_pending = 1`, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber, attempt.Signature, when, - attempt.AttemptedHeadSHA, now, now) - return err + TaskCIMergeResultInFlight, attempt.AttemptedHeadSHA, now, now, + TaskCIErrorKindAutoMerge, TaskCIErrorKindAutoMerge) + if err != nil { + return err + } + rows, err := result.RowsAffected() + if err != nil { + return err + } + if rows == 0 { + return ErrTaskCIMergeAttemptAlreadyReserved + } + return nil + }) +} + +// RecordTaskCIMergeAttemptResult completes a reserved attempt and updates its +// typed error in the same transaction. +func (s *Store) RecordTaskCIMergeAttemptResult( + ctx context.Context, + taskID, repositoryID string, + prNumber int, + signature, result, message string, +) error { + if result != TaskCIMergeResultFailed && result != TaskCIMergeResultAccepted { + return fmt.Errorf("invalid CI auto-merge result %q", result) + } + return s.mutateTaskCIPRState(ctx, taskID, func(ctx context.Context, tx *sqlx.Tx, now time.Time) error { + outcome, err := tx.ExecContext(ctx, ` + UPDATE github_task_ci_pr_state SET + last_merge_result = ?, + last_error = CASE + WHEN ? = ? THEN ? + WHEN ? = ? AND last_error_kind = ? THEN NULL + ELSE last_error END, + last_error_kind = CASE + WHEN ? = ? THEN ? + WHEN ? = ? AND last_error_kind = ? THEN '' + ELSE last_error_kind END, + updated_at = ? + WHERE task_id = ? AND repository_id = ? AND pr_number = ? + AND last_merge_signature = ?`, + result, + result, TaskCIMergeResultFailed, strings.TrimSpace(message), + result, TaskCIMergeResultAccepted, TaskCIErrorKindAutoMerge, + result, TaskCIMergeResultFailed, TaskCIErrorKindAutoMerge, + result, TaskCIMergeResultAccepted, TaskCIErrorKindAutoMerge, + now, taskID, repositoryID, prNumber, signature) + if err != nil { + return err + } + rows, err := outcome.RowsAffected() + if err != nil { + return err + } + if rows == 0 { + return ErrTaskCIMergeAttemptNotFound + } + return nil }) } @@ -2885,18 +3063,28 @@ func (s *Store) RecordTaskCIMergeAttempt(ctx context.Context, attempt TaskCIMerg func (s *Store) RecordTaskCIMergeQueueObservation(ctx context.Context, observation TaskCIMergeQueueObservation) error { return s.mutateTaskCIPRState(ctx, observation.TaskID, func(ctx context.Context, tx *sqlx.Tx, now time.Time) error { baselineHead := "" + mergeResult := "" + if observation.ActiveQueueHeadSHA != "" || observation.Accepted { + mergeResult = TaskCIMergeResultAccepted + } if observation.ActiveQueueHeadSHA == "" { baselineHead = observation.RemovalObservedHeadSHA } _, err := tx.ExecContext(ctx, ` INSERT INTO github_task_ci_pr_state ( - task_id, repository_id, pr_number, last_merge_signature, + task_id, repository_id, pr_number, last_merge_signature, last_merge_result, last_queue_attempt_head_sha, last_queue_removal_cause, created_at, updated_at - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?) + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(task_id, repository_id, pr_number) DO UPDATE SET last_merge_signature = CASE WHEN excluded.last_merge_signature <> '' THEN excluded.last_merge_signature ELSE github_task_ci_pr_state.last_merge_signature END, + last_merge_result = CASE + WHEN excluded.last_merge_result <> '' THEN excluded.last_merge_result + ELSE github_task_ci_pr_state.last_merge_result END, + merge_retry_pending = CASE + WHEN excluded.last_merge_result = ? THEN 0 + ELSE github_task_ci_pr_state.merge_retry_pending END, last_queue_attempt_head_sha = CASE WHEN excluded.last_queue_attempt_head_sha <> '' THEN excluded.last_queue_attempt_head_sha @@ -2905,10 +3093,19 @@ func (s *Store) RecordTaskCIMergeQueueObservation(ctx context.Context, observati WHEN excluded.last_queue_removal_cause <> '' THEN excluded.last_queue_removal_cause ELSE github_task_ci_pr_state.last_queue_removal_cause END, + last_error = CASE + WHEN excluded.last_merge_result = ? AND github_task_ci_pr_state.last_error_kind = ? + THEN NULL ELSE github_task_ci_pr_state.last_error END, + last_error_kind = CASE + WHEN excluded.last_merge_result = ? AND github_task_ci_pr_state.last_error_kind = ? + THEN '' ELSE github_task_ci_pr_state.last_error_kind END, updated_at = excluded.updated_at`, observation.TaskID, observation.RepositoryID, observation.PRNumber, - observation.MergeSignature, - observation.ActiveQueueHeadSHA, observation.RemovalCause, now, now) + observation.MergeSignature, mergeResult, + observation.ActiveQueueHeadSHA, observation.RemovalCause, now, now, + TaskCIMergeResultAccepted, + TaskCIMergeResultAccepted, TaskCIErrorKindAutoMerge, + TaskCIMergeResultAccepted, TaskCIErrorKindAutoMerge) if err != nil { return err } @@ -2927,15 +3124,25 @@ func (s *Store) RecordTaskCIMergeQueueObservation(ctx context.Context, observati // RecordTaskCIError stores the latest user-visible CI automation error for a task PR. func (s *Store) RecordTaskCIError(ctx context.Context, taskID, repositoryID string, prNumber int, message string) error { + return s.recordTaskCIError(ctx, taskID, repositoryID, prNumber, "", message) +} + +// RecordTaskCIAutoMergeError stores an error produced by the automatic merge path. +func (s *Store) RecordTaskCIAutoMergeError(ctx context.Context, taskID, repositoryID string, prNumber int, message string) error { + return s.recordTaskCIError(ctx, taskID, repositoryID, prNumber, TaskCIErrorKindAutoMerge, message) +} + +func (s *Store) recordTaskCIError(ctx context.Context, taskID, repositoryID string, prNumber int, kind, message string) error { return s.mutateTaskCIPRState(ctx, taskID, func(ctx context.Context, tx *sqlx.Tx, now time.Time) error { _, err := tx.ExecContext(ctx, ` INSERT INTO github_task_ci_pr_state ( - task_id, repository_id, pr_number, last_error, created_at, updated_at - ) VALUES (?, ?, ?, ?, ?, ?) + task_id, repository_id, pr_number, last_error, last_error_kind, created_at, updated_at + ) VALUES (?, ?, ?, ?, ?, ?, ?) ON CONFLICT(task_id, repository_id, pr_number) DO UPDATE SET last_error = excluded.last_error, + last_error_kind = excluded.last_error_kind, updated_at = excluded.updated_at`, - taskID, repositoryID, prNumber, strings.TrimSpace(message), now, now) + taskID, repositoryID, prNumber, strings.TrimSpace(message), kind, now, now) return err }) } @@ -2945,13 +3152,14 @@ func (s *Store) MarkTaskCIAutoFixExhausted(ctx context.Context, taskID, reposito return s.mutateTaskCIPRState(ctx, taskID, func(ctx context.Context, tx *sqlx.Tx, now time.Time) error { _, err := tx.ExecContext(ctx, ` INSERT INTO github_task_ci_pr_state ( - task_id, repository_id, pr_number, auto_fix_exhausted_at, last_error, created_at, updated_at - ) VALUES (?, ?, ?, ?, ?, ?, ?) + task_id, repository_id, pr_number, auto_fix_exhausted_at, last_error, last_error_kind, created_at, updated_at + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(task_id, repository_id, pr_number) DO UPDATE SET auto_fix_exhausted_at = excluded.auto_fix_exhausted_at, last_error = excluded.last_error, + last_error_kind = excluded.last_error_kind, updated_at = excluded.updated_at`, - taskID, repositoryID, prNumber, now, strings.TrimSpace(message), now, now) + taskID, repositoryID, prNumber, now, strings.TrimSpace(message), TaskCIErrorKindAutoFix, now, now) return err }) } @@ -2960,7 +3168,7 @@ func (s *Store) MarkTaskCIAutoFixExhausted(ctx context.Context, taskID, reposito func (s *Store) ClearTaskCIError(ctx context.Context, taskID, repositoryID string, prNumber int) error { return s.mutateTaskCIPRState(ctx, taskID, func(ctx context.Context, tx *sqlx.Tx, now time.Time) error { _, err := tx.ExecContext(ctx, ` - UPDATE github_task_ci_pr_state SET last_error = NULL, updated_at = ? + UPDATE github_task_ci_pr_state SET last_error = NULL, last_error_kind = '', updated_at = ? WHERE task_id = ? AND repository_id = ? AND pr_number = ?`, now, taskID, repositoryID, prNumber) return err @@ -3034,6 +3242,7 @@ func (s *Store) RecordTaskPRLifecyclePrompt(ctx context.Context, prompt TaskPRLi last_lifecycle_prompt_at = excluded.last_lifecycle_prompt_at, last_lifecycle_session_id = excluded.last_lifecycle_session_id, last_error = NULL, + last_error_kind = '', updated_at = excluded.updated_at`, prompt.TaskID, prompt.RepositoryID, prompt.PRNumber, prompt.Event == "review_requested", prompt.ReviewRequested, diff --git a/apps/backend/internal/github/store_ci_automation_test.go b/apps/backend/internal/github/store_ci_automation_test.go index 0abec587d9..d7744ed5dc 100644 --- a/apps/backend/internal/github/store_ci_automation_test.go +++ b/apps/backend/internal/github/store_ci_automation_test.go @@ -2,6 +2,9 @@ package github import ( "context" + "errors" + "strings" + "sync" "testing" "time" ) @@ -21,6 +24,7 @@ func TestStoreTaskPRAgentAutomationSchema(t *testing.T) { "pr_scope_migrated_at", }, "github_task_ci_pr_state": { + "merge_retry_pending", "review_request_initialized", "last_review_requested", "last_observed_pr_state", @@ -53,6 +57,114 @@ func TestStoreTaskPRAgentAutomationSchema(t *testing.T) { } } +func TestStoreRetryMergeAuthorizesOneSameSignatureAttempt(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + attempt := TaskCIMergeAttempt{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + Signature: "ready-v1", AttemptedHeadSHA: "head-v1", AttemptedAt: time.Now().UTC(), + } + if err := store.RecordTaskCIMergeAttempt(ctx, attempt); err != nil { + t.Fatalf("reserve initial attempt: %v", err) + } + if err := store.RecordTaskCIMergeAttemptResult( + ctx, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber, + attempt.Signature, TaskCIMergeResultFailed, "merge PR: provider unavailable", + ); err != nil { + t.Fatalf("record failed attempt: %v", err) + } + + if err := store.AuthorizeTaskCIMergeRetry( + ctx, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber, time.Now().UTC(), + ); err != nil { + t.Fatalf("authorize retry: %v", err) + } + state, err := store.GetTaskCIPRState(ctx, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber) + if err != nil { + t.Fatalf("get authorized state: %v", err) + } + if state == nil || !state.MergeRetryPending { + t.Fatalf("retry authorization not persisted: %+v", state) + } + + if err := store.RecordTaskCIMergeAttempt(ctx, attempt); err != nil { + t.Fatalf("consume retry authorization: %v", err) + } + state, err = store.GetTaskCIPRState(ctx, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber) + if err != nil { + t.Fatalf("get consumed state: %v", err) + } + if state == nil || state.MergeRetryPending { + t.Fatalf("retry authorization was not consumed: %+v", state) + } + if err := store.RecordTaskCIMergeAttempt(ctx, attempt); !errors.Is(err, ErrTaskCIMergeAttemptAlreadyReserved) { + t.Fatalf("second same-signature retry error = %v, want %v", err, ErrTaskCIMergeAttemptAlreadyReserved) + } +} + +func TestStoreRetryMergeRejectsAcceptedAndDuplicateRequests(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + attempt := TaskCIMergeAttempt{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + Signature: "ready-v1", AttemptedHeadSHA: "head-v1", AttemptedAt: time.Now().UTC(), + } + if err := store.RecordTaskCIMergeAttempt(ctx, attempt); err != nil { + t.Fatalf("reserve attempt: %v", err) + } + if err := store.RecordTaskCIMergeAttemptResult( + ctx, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber, + attempt.Signature, TaskCIMergeResultFailed, "merge PR: provider unavailable", + ); err != nil { + t.Fatalf("record failure: %v", err) + } + if err := store.AuthorizeTaskCIMergeRetry( + ctx, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber, time.Now().UTC(), + ); err != nil { + t.Fatalf("authorize first retry: %v", err) + } + if err := store.AuthorizeTaskCIMergeRetry( + ctx, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber, time.Now().UTC(), + ); !errors.Is(err, ErrTaskCIMergeRetryNotAllowed) { + t.Fatalf("duplicate authorization error = %v, want %v", err, ErrTaskCIMergeRetryNotAllowed) + } + + if err := store.RecordTaskCIMergeAttempt(ctx, TaskCIMergeAttempt{ + TaskID: attempt.TaskID, RepositoryID: attempt.RepositoryID, PRNumber: 43, + Signature: "ready-v2", AttemptedHeadSHA: "head-v2", AttemptedAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("reserve accepted attempt: %v", err) + } + if err := store.RecordTaskCIMergeAttemptResult( + ctx, attempt.TaskID, attempt.RepositoryID, 43, + "ready-v2", TaskCIMergeResultAccepted, "", + ); err != nil { + t.Fatalf("record accepted attempt: %v", err) + } + if err := store.AuthorizeTaskCIMergeRetry( + ctx, attempt.TaskID, attempt.RepositoryID, 43, time.Now().UTC(), + ); !errors.Is(err, ErrTaskCIMergeRetryNotAllowed) { + t.Fatalf("accepted authorization error = %v, want %v", err, ErrTaskCIMergeRetryNotAllowed) + } +} + +func TestStoreRetryMergeAllowsExpiredInFlightAttempt(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + attempt := TaskCIMergeAttempt{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + Signature: "ready-v1", AttemptedHeadSHA: "head-v1", AttemptedAt: time.Now().UTC().Add(-3 * time.Minute), + } + if err := store.RecordTaskCIMergeAttempt(ctx, attempt); err != nil { + t.Fatalf("reserve stale attempt: %v", err) + } + if err := store.AuthorizeTaskCIMergeRetry( + ctx, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber, time.Now().UTC(), + ); err != nil { + t.Fatalf("authorize expired retry: %v", err) + } +} + // TestStoreTaskPRAutomationOptionsSchemaReplay confirms initSchema can run // twice against the same database without error — the fresh-DB CREATE TABLE // and the idempotent ADD COLUMN/fan-out migration must both tolerate replay. @@ -511,6 +623,244 @@ func TestStoreTaskCIMergeQueueRecoveryState(t *testing.T) { } } +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.2 +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.8 +func TestStoreTaskCIMergeAttemptJournalSchema(t *testing.T) { + store := newTestStore(t) + columns, err := store.tableColumns("github_task_ci_pr_state") + if err != nil { + t.Fatalf("read journal columns: %v", err) + } + for _, name := range []string{"last_merge_result", "last_error_kind"} { + if _, ok := columns[name]; !ok { + t.Errorf("missing journal column %q", name) + } + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.2 +func TestStoreTaskCIMergeAttemptReservationBlocksUnchangedAttempt(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + first := time.Date(2026, 8, 28, 12, 0, 0, 0, time.UTC) + attempt := TaskCIMergeAttempt{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + Signature: "ready-head-a", AttemptedHeadSHA: "head-a", AttemptedAt: first, + } + if err := store.RecordTaskCIMergeAttempt(ctx, attempt); err != nil { + t.Fatalf("reserve first merge attempt: %v", err) + } + attempt.AttemptedAt = first.Add(time.Minute) + if err := store.RecordTaskCIMergeAttempt(ctx, attempt); err == nil || + !strings.Contains(err.Error(), "already reserved") { + t.Fatalf("reserve unchanged attempt error = %v, want already reserved", err) + } + + state, err := store.GetTaskCIPRState(ctx, "task-1", "repo-1", 42) + if err != nil { + t.Fatalf("get merge attempt: %v", err) + } + if state.LastMergeResult != TaskCIMergeResultInFlight { + t.Fatalf("merge result = %q, want in_flight", state.LastMergeResult) + } + if state.LastMergeAttemptAt == nil || !state.LastMergeAttemptAt.Equal(first) { + t.Fatalf("attempt time = %v, want first reservation %v", state.LastMergeAttemptAt, first) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.5 +func TestStoreTaskCIMergeQueueObservationReconcilesOnlyAutoMergeError(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + now := time.Now().UTC() + if _, err := store.db.Exec(` + INSERT INTO github_task_ci_pr_state ( + task_id, repository_id, pr_number, last_merge_signature, last_merge_result, + last_error, last_error_kind, created_at, updated_at + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + "task-1", "repo-1", 42, "merge-a", TaskCIMergeResultFailed, + "merge PR: provider status was lost", TaskCIErrorKindAutoMerge, now, now); err != nil { + t.Fatalf("seed failed merge attempt: %v", err) + } + if err := store.RecordTaskCIMergeQueueObservation(ctx, TaskCIMergeQueueObservation{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + ActiveQueueHeadSHA: "head-a", MergeSignature: "merge-a", + }); err != nil { + t.Fatalf("record queue observation: %v", err) + } + state, err := store.GetTaskCIPRState(ctx, "task-1", "repo-1", 42) + if err != nil { + t.Fatalf("get reconciled attempt: %v", err) + } + if state.LastMergeResult != TaskCIMergeResultAccepted || state.LastError != nil || state.LastErrorKind != "" { + t.Fatalf("reconciled state = %+v, want accepted with cleared merge error", state) + } + + if _, err := store.db.Exec(` + UPDATE github_task_ci_pr_state + SET last_error = ?, last_error_kind = ? + WHERE task_id = ? AND repository_id = ? AND pr_number = ?`, + "auto-fix failed", "auto_fix", "task-1", "repo-1", 42); err != nil { + t.Fatalf("seed unrelated error: %v", err) + } + if err := store.RecordTaskCIMergeQueueObservation(ctx, TaskCIMergeQueueObservation{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + ActiveQueueHeadSHA: "head-a", MergeSignature: "merge-a", + }); err != nil { + t.Fatalf("record repeated queue observation: %v", err) + } + state, err = store.GetTaskCIPRState(ctx, "task-1", "repo-1", 42) + if err != nil { + t.Fatalf("get unrelated error state: %v", err) + } + if state.LastError == nil || *state.LastError != "auto-fix failed" || state.LastErrorKind != "auto_fix" { + t.Fatalf("unrelated error was cleared: %+v", state) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.2 +func TestStoreTaskCIMergeAttemptResultRecordsFailureAtomically(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + if err := store.RecordTaskCIMergeAttempt(ctx, TaskCIMergeAttempt{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + Signature: "ready-head-a", AttemptedHeadSHA: "head-a", + }); err != nil { + t.Fatalf("reserve merge attempt: %v", err) + } + message := "merge PR: GitHub response was lost" + if err := store.RecordTaskCIMergeAttemptResult( + ctx, "task-1", "repo-1", 42, "ready-head-a", TaskCIMergeResultFailed, message, + ); err != nil { + t.Fatalf("record failed merge attempt: %v", err) + } + state, err := store.GetTaskCIPRState(ctx, "task-1", "repo-1", 42) + if err != nil { + t.Fatalf("get failed merge attempt: %v", err) + } + if state.LastMergeResult != TaskCIMergeResultFailed || state.LastErrorKind != TaskCIErrorKindAutoMerge { + t.Fatalf("failed merge state = %+v", state) + } + if state.LastError == nil || *state.LastError != message { + t.Fatalf("last error = %v, want %q", state.LastError, message) + } + if err := store.MarkTaskCIAutoFixExhausted( + ctx, "task-1", "repo-1", 42, "CI auto-fix paused after the round limit", + ); err != nil { + t.Fatalf("record auto-fix exhaustion: %v", err) + } + state, err = store.GetTaskCIPRState(ctx, "task-1", "repo-1", 42) + if err != nil { + t.Fatalf("get auto-fix exhaustion state: %v", err) + } + if state.LastError == nil || *state.LastError != "CI auto-fix paused after the round limit" || + state.LastErrorKind != TaskCIErrorKindAutoFix { + t.Fatalf("auto-fix error did not replace merge error atomically: %+v", state) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.3 +func TestStoreTaskCIMergeAttemptResultIsSignatureBound(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + for _, signature := range []string{"ready-head-a", "ready-head-b"} { + if err := store.RecordTaskCIMergeAttempt(ctx, TaskCIMergeAttempt{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + Signature: signature, AttemptedHeadSHA: strings.TrimPrefix(signature, "ready-"), + }); err != nil { + t.Fatalf("reserve %s: %v", signature, err) + } + } + err := store.RecordTaskCIMergeAttemptResult( + ctx, "task-1", "repo-1", 42, "ready-head-a", TaskCIMergeResultAccepted, "", + ) + if !errors.Is(err, ErrTaskCIMergeAttemptNotFound) { + t.Fatalf("stale result error = %v, want attempt not found", err) + } + state, err := store.GetTaskCIPRState(ctx, "task-1", "repo-1", 42) + if err != nil { + t.Fatalf("get current attempt: %v", err) + } + if state.LastMergeSignature != "ready-head-b" || state.LastMergeResult != TaskCIMergeResultInFlight { + t.Fatalf("current attempt was overwritten: %+v", state) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.2 +func TestStoreTaskCIMergeAttemptReservationIsConcurrentSafe(t *testing.T) { + store := newTestStore(t) + start := make(chan struct{}) + results := make(chan error, 2) + attempt := TaskCIMergeAttempt{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + Signature: "ready-head-a", AttemptedHeadSHA: "head-a", + } + var wg sync.WaitGroup + for range 2 { + wg.Add(1) + go func() { + defer wg.Done() + <-start + results <- store.RecordTaskCIMergeAttempt(context.Background(), attempt) + }() + } + close(start) + wg.Wait() + close(results) + reserved, blocked := 0, 0 + for err := range results { + switch { + case err == nil: + reserved++ + case errors.Is(err, ErrTaskCIMergeAttemptAlreadyReserved): + blocked++ + default: + t.Fatalf("reserve merge attempt: %v", err) + } + } + if reserved != 1 || blocked != 1 { + t.Fatalf("reserved=%d blocked=%d, want one each", reserved, blocked) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.8 +func TestStoreTaskCIAutoMergeErrorKindBackfillIsNarrow(t *testing.T) { + store := newTestStore(t) + now := time.Now().UTC() + for _, row := range []struct { + repositoryID string + message string + }{ + {repositoryID: "known", message: "merge PR: GitHub unavailable"}, + {repositoryID: "unknown", message: "review prompt failed"}, + } { + if _, err := store.db.Exec(` + INSERT INTO github_task_ci_pr_state ( + task_id, repository_id, pr_number, last_error, created_at, updated_at + ) VALUES (?, ?, ?, ?, ?, ?)`, + "task-1", row.repositoryID, 42, row.message, now, now); err != nil { + t.Fatalf("seed %s error: %v", row.repositoryID, err) + } + } + if err := store.backfillTaskCIAutoMergeErrorKinds(); err != nil { + t.Fatalf("backfill error kinds: %v", err) + } + known, err := store.GetTaskCIPRState(context.Background(), "task-1", "known", 42) + if err != nil { + t.Fatalf("get known error: %v", err) + } + unknown, err := store.GetTaskCIPRState(context.Background(), "task-1", "unknown", 42) + if err != nil { + t.Fatalf("get unknown error: %v", err) + } + if known.LastErrorKind != TaskCIErrorKindAutoMerge { + t.Fatalf("known error kind = %q, want auto_merge", known.LastErrorKind) + } + if unknown.LastErrorKind != "" { + t.Fatalf("unknown error kind = %q, want empty", unknown.LastErrorKind) + } +} + func TestStoreTaskCIPRState_MarkExhaustedAndResetOnReenable(t *testing.T) { store := newTestStore(t) ctx := context.Background() diff --git a/apps/backend/internal/github/token_client.go b/apps/backend/internal/github/token_client.go index eac5495781..de26952cdb 100644 --- a/apps/backend/internal/github/token_client.go +++ b/apps/backend/internal/github/token_client.go @@ -1,6 +1,7 @@ package github import ( + "context" "fmt" "net/http" "time" @@ -32,11 +33,12 @@ type TokenPrincipal struct { // token source. The token itself remains private while principal metadata is // available for authorization, rate-limit, cache, and attribution keys. type TokenClient struct { - token string - httpClient *http.Client - username string // cached after first GetAuthenticatedUser call - rateTracker *RateTracker - principal TokenPrincipal + token string + httpClient *http.Client + username string // cached after first GetAuthenticatedUser call + rateTracker *RateTracker + principal TokenPrincipal + mergePollWait func(context.Context, time.Duration) error } // PATClient remains an alias so existing callers retain source compatibility. diff --git a/apps/backend/internal/orchestrator/event_handlers_github.go b/apps/backend/internal/orchestrator/event_handlers_github.go index 7eec5aaff3..c6f0e5994c 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github.go +++ b/apps/backend/internal/orchestrator/event_handlers_github.go @@ -6,6 +6,7 @@ import ( "fmt" "strconv" "strings" + "sync" "time" "go.uber.org/zap" @@ -61,12 +62,14 @@ type GitHubService interface { RecordTaskCIFixAttempt(ctx context.Context, attempt github.TaskCIFixAttempt) error RefreshTaskCIFixCheckpoint(ctx context.Context, taskID, repositoryID string, prNumber int, signature, checkpointJSON string) error RecordTaskCIMergeAttempt(ctx context.Context, attempt github.TaskCIMergeAttempt) error + RecordTaskCIMergeAttemptResult(ctx context.Context, taskID, repositoryID string, prNumber int, signature, result, message string) error RecordTaskCIMergeQueueObservation(ctx context.Context, observation github.TaskCIMergeQueueObservation) error RecordTaskCIError(ctx context.Context, taskID, repositoryID string, prNumber int, message string) error + RecordTaskCIAutoMergeError(ctx context.Context, taskID, repositoryID string, prNumber int, message string) error MarkTaskCIAutoFixExhausted(ctx context.Context, taskID, repositoryID string, prNumber int, message string) error ClearTaskCIError(ctx context.Context, taskID, repositoryID string, prNumber int) error GetPRFeedbackForAutomation(ctx context.Context, workspaceID, owner, repo string, number int) (*github.PRFeedback, error) - MergePRForAutomation(ctx context.Context, workspaceID, owner, repo string, number int, mergeMethod string) error + MergePRForAutomation(ctx context.Context, workspaceID, owner, repo string, number int, mergeMethod, expectedHeadSHA string) error ListActivePRWatches(ctx context.Context) ([]*github.PRWatch, error) ReserveReviewPRTask(ctx context.Context, watchID, repoOwner, repoName string, prNumber int, prURL string) (bool, error) AssignReviewPRTaskID(ctx context.Context, watchID, repoOwner, repoName string, prNumber int, taskID string) error @@ -299,24 +302,135 @@ func (s *Service) startTaskPRCIAutomationWithRefresh(ctx context.Context, pr *gi if pr == nil { return } + s.ciAutomationMu.Lock() + if s.ciAutomationStopped { + s.ciAutomationMu.Unlock() + return + } + if s.ciAutomationCtx == nil { + s.ciAutomationCtx, s.ciAutomationCancel = context.WithCancel(context.Background()) + } + workerCtx := s.ciAutomationCtx + s.ciAutomationWorkers.Add(1) + s.ciAutomationMu.Unlock() key := fmt.Sprintf("%s|%s|%d", pr.TaskID, pr.RepositoryID, pr.PRNumber) - if _, loaded := s.ciAutomationInFlight.LoadOrStore(key, struct{}{}); loaded { - s.logger.Debug("CI automation already in flight", + request := ciAutomationRequest{ctx: ctx, pr: pr, refresh: refresh} + if !s.ciAutomationInFlight.Enqueue(key, request) { + s.ciAutomationWorkers.Done() + s.logger.Debug("CI automation follow-up coalesced", zap.String("task_id", pr.TaskID), zap.String("repository_id", pr.RepositoryID), zap.Int("pr_number", pr.PRNumber)) return } go func() { - defer s.ciAutomationInFlight.Delete(key) - automationCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), ciAutomationDetachedTimeout) - defer cancel() - if err := s.handleTaskPRCIAutomationWithRefresh(automationCtx, pr, refresh); err != nil { - s.logger.Debug("CI automation handling failed", zap.String("task_id", pr.TaskID), zap.Error(err)) + defer s.ciAutomationWorkers.Done() + for { + if workerCtx.Err() != nil { + return + } + automationCtx, cancel := context.WithTimeout(workerCtx, ciAutomationDetachedTimeout) + err := s.handleTaskPRCIAutomationWithRefresh(automationCtx, request.pr, request.refresh) + cancel() + if err != nil { + s.logger.Debug("CI automation handling failed", zap.String("task_id", request.pr.TaskID), zap.Error(err)) + } + var ok bool + request, ok = s.ciAutomationInFlight.Next(key) + if !ok { + return + } } }() } +type ciAutomationRequest struct { + ctx context.Context + pr *github.TaskPR + refresh bool +} + +type ciAutomationWork struct { + pending *ciAutomationRequest +} + +// ciAutomationCoordinator owns the per-PR worker lifecycle. Enqueue returns +// true only to the caller that must start the worker. Later requests replace +// the single pending follow-up while preserving the strongest refresh need. +type ciAutomationCoordinator struct { + mu sync.Mutex + works map[string]*ciAutomationWork +} + +func (c *ciAutomationCoordinator) Enqueue(key string, request ciAutomationRequest) bool { + c.mu.Lock() + defer c.mu.Unlock() + if c.works == nil { + c.works = make(map[string]*ciAutomationWork) + } + if work, ok := c.works[key]; ok { + if work.pending != nil && work.pending.refresh { + request.refresh = true + } + work.pending = &request + return false + } + c.works[key] = &ciAutomationWork{} + return true +} + +func (c *ciAutomationCoordinator) Next(key string) (ciAutomationRequest, bool) { + c.mu.Lock() + defer c.mu.Unlock() + work, ok := c.works[key] + if !ok { + return ciAutomationRequest{}, false + } + if work.pending != nil { + request := *work.pending + work.pending = nil + return request, true + } + delete(c.works, key) + return ciAutomationRequest{}, false +} + +// Has reports whether a worker is currently active for key. It is intentionally +// read-only so tests can observe coalescing without exposing coordinator state. +func (c *ciAutomationCoordinator) Has(key string) bool { + c.mu.Lock() + defer c.mu.Unlock() + _, exists := c.works[key] + return exists +} + +func (s *Service) resetCIAutomationWorkers() { + s.ciAutomationMu.Lock() + defer s.ciAutomationMu.Unlock() + s.ciAutomationStopped = false + s.ciAutomationCtx, s.ciAutomationCancel = context.WithCancel(context.Background()) +} + +func (s *Service) stopCIAutomationWorkers() { + s.ciAutomationMu.Lock() + s.ciAutomationStopped = true + cancel := s.ciAutomationCancel + s.ciAutomationMu.Unlock() + if cancel != nil { + cancel() + } + done := make(chan struct{}) + go func() { + s.ciAutomationWorkers.Wait() + close(done) + }() + select { + case <-done: + case <-time.After(sendNowClaimRecoveryTimeout): + s.logger.Warn("timed out waiting for CI automation workers during shutdown") + } +} + // handleNewReviewPR creates a task for a new PR needing review. // Auto-start is determined by the workflow step's on_enter actions. func (s *Service) handleNewReviewPR(ctx context.Context, event *bus.Event) error { diff --git a/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go b/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go index d75d534ec6..a5e857a03e 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go @@ -120,10 +120,10 @@ func (s *Service) handleTaskPRCIAutomationWithRefresh(ctx context.Context, pr *g if options.AutoFixEnabled && ciAutomationCanAutoFixFromFeedback(pr) { autoFixBlockedMerge, autoFixError = s.handleTaskPRCIAutoFix(ctx, pr, options) } - ciError := "" + ciError := ciAutomationMergeOutcome{} if !autoFixBlockedMerge && options.AutoMergeEnabled && ciAutomationReadyToMerge(pr) { if !freshlySynced { - ciError = "PR status is not freshly synced for auto-merge" + ciError = ciAutomationMergeOutcome{message: "PR status is not freshly synced for auto-merge"} } else { ciError = s.handleTaskPRCIAutoMerge(ctx, pr) } @@ -133,8 +133,8 @@ func (s *Service) handleTaskPRCIAutomationWithRefresh(ctx context.Context, pr *g s.recordCIAutomationError(ctx, pr, autoFixError) s.publishTaskCIOptionsState(ctx, pr.TaskID) } - if ciError != "" { - s.recordCIAutomationError(ctx, pr, ciError) + if ciError.message != "" && !ciError.journaled { + s.recordCIAutoMergeError(ctx, pr, ciError.message) s.publishTaskCIOptionsState(ctx, pr.TaskID) } return nil @@ -390,20 +390,22 @@ func (s *Service) handleTaskPRCIAutoFixEmptyDelta(ctx context.Context, pr *githu return false } -func (s *Service) handleTaskPRCIAutoMerge(ctx context.Context, pr *github.TaskPR) string { +type ciAutomationMergeOutcome struct { + message string + journaled bool +} + +func (s *Service) handleTaskPRCIAutoMerge(ctx context.Context, pr *github.TaskPR) ciAutomationMergeOutcome { if ciAutomationHasActiveMergeQueueEntry(pr) { - return "" + return ciAutomationMergeOutcome{} } signature := ciAutomationMergeSignature(pr) state, err := s.githubService.GetTaskCIPRState(ctx, pr.TaskID, pr.RepositoryID, pr.PRNumber) if err != nil { - s.logger.Debug("load CI automation merge state failed; attempting merge without dedupe", zap.String("task_id", pr.TaskID), zap.Error(err)) + s.logger.Debug("load CI automation merge state failed; durable reservation will decide", zap.String("task_id", pr.TaskID), zap.Error(err)) } else if state != nil { - if state.LastMergeSignature == signature { - return "" - } - if pr.HeadSHA != "" && state.LastQueueAttemptHeadSHA == pr.HeadSHA { - return "" + if outcome, handled := s.ciAutomationExistingMergeOutcome(ctx, pr, signature, state); handled { + return outcome } } attempt := github.TaskCIMergeAttempt{ @@ -414,18 +416,81 @@ func (s *Service) handleTaskPRCIAutoMerge(ctx context.Context, pr *github.TaskPR AttemptedAt: time.Now().UTC(), AttemptedHeadSHA: pr.HeadSHA, } - if err := s.githubService.MergePRForAutomation( - ctx, pr.WorkspaceID, pr.Owner, pr.Repo, pr.PRNumber, "", - ); err != nil { - return fmt.Sprintf("merge PR: %v", err) + if err := s.githubService.RecordTaskCIMergeAttempt(context.WithoutCancel(ctx), attempt); err != nil { + if errors.Is(err, github.ErrTaskCIMergeAttemptAlreadyReserved) { + return ciAutomationMergeOutcome{} + } + return ciAutomationMergeOutcome{message: fmt.Sprintf("reserve merge PR attempt: %v", err)} } - _ = s.githubService.RecordTaskCIMergeAttempt(context.WithoutCancel(ctx), attempt) - _ = s.githubService.ClearTaskCIError(context.WithoutCancel(ctx), pr.TaskID, pr.RepositoryID, pr.PRNumber) - return "" + return s.executeCIAutoMerge(ctx, pr, signature) +} + +func (s *Service) ciAutomationExistingMergeOutcome( + ctx context.Context, + pr *github.TaskPR, + signature string, + state *github.TaskCIPRAutomationState, +) (ciAutomationMergeOutcome, bool) { + if state.LastMergeSignature != signature { + return ciAutomationQueueAttemptOutcome(pr, state) + } + if state.LastMergeResult == github.TaskCIMergeResultInFlight && + state.LastMergeAttemptAt != nil && time.Since(*state.LastMergeAttemptAt) > ciAutomationDetachedTimeout { + message := "merge PR: previous automatic merge attempt did not reach a terminal state" + if recordErr := s.githubService.RecordTaskCIMergeAttemptResult( + context.WithoutCancel(ctx), pr.TaskID, pr.RepositoryID, pr.PRNumber, + signature, github.TaskCIMergeResultFailed, message, + ); recordErr != nil { + return ciAutomationMergeOutcome{message: fmt.Sprintf("%s; record expired merge attempt: %v", message, recordErr)}, true + } + if !state.MergeRetryPending { + return ciAutomationMergeOutcome{message: message, journaled: true}, true + } + } + if !state.MergeRetryPending { + return ciAutomationMergeOutcome{}, true + } + return ciAutomationQueueAttemptOutcome(pr, state) +} + +func ciAutomationQueueAttemptOutcome( + pr *github.TaskPR, + state *github.TaskCIPRAutomationState, +) (ciAutomationMergeOutcome, bool) { + if pr.HeadSHA != "" && state.LastQueueAttemptHeadSHA == pr.HeadSHA && !state.MergeRetryPending { + return ciAutomationMergeOutcome{}, true + } + return ciAutomationMergeOutcome{}, false +} + +func (s *Service) executeCIAutoMerge( + ctx context.Context, + pr *github.TaskPR, + signature string, +) ciAutomationMergeOutcome { + if err := s.githubService.MergePRForAutomation( + ctx, pr.WorkspaceID, pr.Owner, pr.Repo, pr.PRNumber, "", pr.HeadSHA, + ); err != nil { + message := fmt.Sprintf("merge PR: %v", err) + if recordErr := s.githubService.RecordTaskCIMergeAttemptResult( + context.WithoutCancel(ctx), pr.TaskID, pr.RepositoryID, pr.PRNumber, + signature, github.TaskCIMergeResultFailed, message, + ); recordErr != nil { + return ciAutomationMergeOutcome{message: fmt.Sprintf("%s; record failed merge attempt: %v", message, recordErr)} + } + return ciAutomationMergeOutcome{message: message, journaled: true} + } + if err := s.githubService.RecordTaskCIMergeAttemptResult( + context.WithoutCancel(ctx), pr.TaskID, pr.RepositoryID, pr.PRNumber, + signature, github.TaskCIMergeResultAccepted, "", + ); err != nil { + return ciAutomationMergeOutcome{message: fmt.Sprintf("record accepted merge PR attempt: %v", err)} + } + return ciAutomationMergeOutcome{} } func (s *Service) recordTaskPRMergeQueueObservation(ctx context.Context, pr *github.TaskPR) { - if pr == nil || (pr.MergeQueueLastRemovalID == "" && !ciAutomationHasActiveMergeQueueEntry(pr)) { + if pr == nil || (pr.MergeQueueLastRemovalID == "" && !ciAutomationHasActiveMergeQueueEntry(pr) && pr.State != githubPRStateMerged) { return } removal, _ := ciAutomationQueueRemovalSnapshot(pr) @@ -435,6 +500,7 @@ func (s *Service) recordTaskPRMergeQueueObservation(ctx context.Context, pr *git PRNumber: pr.PRNumber, RemovalEventID: pr.MergeQueueLastRemovalID, RemovalObservedHeadSHA: pr.HeadSHA, + Accepted: pr.State == githubPRStateMerged, } if ciAutomationHasActiveMergeQueueEntry(pr) { observation.ActiveQueueHeadSHA = pr.MergeQueueEntryHeadSHA @@ -518,6 +584,19 @@ func (s *Service) recordCIAutomationError(ctx context.Context, pr *github.TaskPR } } +func (s *Service) recordCIAutoMergeError(ctx context.Context, pr *github.TaskPR, message string) { + s.logger.Warn("CI auto-merge error", + zap.String("task_id", pr.TaskID), + zap.String("repository_id", pr.RepositoryID), + zap.Int("pr_number", pr.PRNumber), + zap.String("error", message)) + if err := s.githubService.RecordTaskCIAutoMergeError( + context.WithoutCancel(ctx), pr.TaskID, pr.RepositoryID, pr.PRNumber, message, + ); err != nil { + s.logger.Debug("record CI auto-merge error failed", zap.String("task_id", pr.TaskID), zap.Error(err)) + } +} + func ciAutomationCanAutoFixFromFeedback(pr *github.TaskPR) bool { return pr != nil && pr.State != taskPRAgentEventClosed && pr.State != taskPRAgentEventMerged } @@ -909,7 +988,30 @@ func encodeCIAutomationCheckpoint(checkpoint ciAutomationCheckpoint) (string, st } func ciAutomationMergeSignature(pr *github.TaskPR) string { - payload := fmt.Sprintf("%s|%s|%d|%s|%s|%d|%d|%s|%s|%s|%d|%d", pr.TaskID, pr.RepositoryID, pr.PRNumber, pr.HeadSHA, pr.HeadBranch, pr.Additions, pr.Deletions, pr.ChecksState, pr.ReviewState, pr.MergeableState, pr.ReviewCount, pr.UnresolvedReviewThreads) - sum := sha256.Sum256([]byte(payload)) + requiredReviews := 0 + requiredReviewsObserved := pr.RequiredReviews != nil + if pr.RequiredReviews != nil { + requiredReviews = *pr.RequiredReviews + } + payload, _ := json.Marshal(struct { + TaskID, RepositoryID, HeadSHA, HeadBranch, State string + PRNumber, Additions, Deletions int + ChecksState, ReviewState, MergeableState string + ReviewCount, PendingReviewCount int + RequiredReviewsObserved bool + RequiredReviews, ChecksTotal, ChecksPassing int + UnresolvedReviewThreads int + }{ + TaskID: pr.TaskID, RepositoryID: pr.RepositoryID, PRNumber: pr.PRNumber, + HeadSHA: pr.HeadSHA, HeadBranch: pr.HeadBranch, + State: pr.State, + Additions: pr.Additions, Deletions: pr.Deletions, + ChecksState: pr.ChecksState, ReviewState: pr.ReviewState, MergeableState: pr.MergeableState, + ReviewCount: pr.ReviewCount, PendingReviewCount: pr.PendingReviewCount, + RequiredReviewsObserved: requiredReviewsObserved, RequiredReviews: requiredReviews, + ChecksTotal: pr.ChecksTotal, ChecksPassing: pr.ChecksPassing, + UnresolvedReviewThreads: pr.UnresolvedReviewThreads, + }) + sum := sha256.Sum256(payload) return hex.EncodeToString(sum[:]) } diff --git a/apps/backend/internal/orchestrator/event_handlers_github_ci_automation_test.go b/apps/backend/internal/orchestrator/event_handlers_github_ci_automation_test.go index aca9913d39..0ea4ba425b 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_ci_automation_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_ci_automation_test.go @@ -82,7 +82,7 @@ func (s *autoMergeFailureThenReviewRequestService) MergePRForAutomation( _ string, owner, repo string, number int, - method string, + method, _ string, ) error { return s.MergePR(ctx, owner, repo, number, method) } @@ -91,10 +91,22 @@ func (s *storeBackedLifecycleGitHubService) GetTaskCIPRState(ctx context.Context return s.store.GetTaskCIPRState(ctx, taskID, repositoryID, prNumber) } +func (s *storeBackedLifecycleGitHubService) RecordTaskCIMergeAttemptResult( + ctx context.Context, taskID, repositoryID string, prNumber int, signature, result, message string, +) error { + return s.store.RecordTaskCIMergeAttemptResult(ctx, taskID, repositoryID, prNumber, signature, result, message) +} + func (s *storeBackedLifecycleGitHubService) RecordTaskCIError(ctx context.Context, taskID, repositoryID string, prNumber int, message string) error { return s.store.RecordTaskCIError(ctx, taskID, repositoryID, prNumber, message) } +func (s *storeBackedLifecycleGitHubService) RecordTaskCIAutoMergeError( + ctx context.Context, taskID, repositoryID string, prNumber int, message string, +) error { + return s.store.RecordTaskCIAutoMergeError(ctx, taskID, repositoryID, prNumber, message) +} + func (s *storeBackedLifecycleGitHubService) ClearTaskCIError(ctx context.Context, taskID, repositoryID string, prNumber int) error { return s.store.ClearTaskCIError(ctx, taskID, repositoryID, prNumber) } @@ -1228,6 +1240,7 @@ func TestHandleTaskPRCIAutomationAutoMergeUsesFreshSync(t *testing.T) { fresh.ChecksState = "success" fresh.ReviewState = "approved" fresh.MergeableState = "clean" + fresh.HeadSHA = "reviewed-head" now := time.Now().UTC() fresh.LastSyncedAt = &now ghSvc := &mockGitHubService{ @@ -1248,6 +1261,9 @@ func TestHandleTaskPRCIAutomationAutoMergeUsesFreshSync(t *testing.T) { if ghSvc.mergeCalls != 1 { t.Fatalf("expected merge from fresh synced PR state, got %d", ghSvc.mergeCalls) } + if ghSvc.mergeExpectedHeadSHA != "reviewed-head" { + t.Fatalf("expected head SHA = %q, want reviewed-head", ghSvc.mergeExpectedHeadSHA) + } } func TestHandleTaskPRCIAutomationAutoMergeUsesPartialSyncMatch(t *testing.T) { @@ -1266,6 +1282,7 @@ func TestHandleTaskPRCIAutomationAutoMergeUsesPartialSyncMatch(t *testing.T) { ChecksState: "success", ReviewState: "approved", MergeableState: "clean", + HeadSHA: "head-a", LastSyncedAt: &now, } ghSvc := &mockGitHubService{ @@ -1323,6 +1340,9 @@ func TestHandleTaskPRCIAutomationAutoMergeRequiresFreshSync(t *testing.T) { if len(ghSvc.ciErrors) != 1 || ghSvc.ciErrors[0].LastError == nil || !strings.Contains(*ghSvc.ciErrors[0].LastError, "not freshly synced") { t.Fatalf("expected stale sync error to be recorded, got %+v", ghSvc.ciErrors) } + if ghSvc.ciErrors[0].LastErrorKind != github.TaskCIErrorKindAutoMerge { + t.Fatalf("error kind = %q, want auto_merge", ghSvc.ciErrors[0].LastErrorKind) + } } func TestCIAutomationHasFreshPRStatusAt(t *testing.T) { @@ -2496,6 +2516,36 @@ func TestCIAutomationMergeSignatureIgnoresVolatileUpdatedAt(t *testing.T) { } } +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.3 +func TestCIAutomationMergeSignatureIncludesEveryReadinessGate(t *testing.T) { + required := 1 + base := github.TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, HeadSHA: "head-a", + ChecksState: "success", ReviewState: "approved", MergeableState: "clean", + ReviewCount: 1, RequiredReviews: &required, ChecksTotal: 2, ChecksPassing: 2, + } + before := ciAutomationMergeSignature(&base) + tests := []struct { + name string + mutate func(*github.TaskPR) + }{ + {name: "PR lifecycle state", mutate: func(pr *github.TaskPR) { pr.State = "closed" }}, + {name: "pending reviews", mutate: func(pr *github.TaskPR) { pr.PendingReviewCount++ }}, + {name: "required review presence", mutate: func(pr *github.TaskPR) { pr.RequiredReviews = nil }}, + {name: "required review value", mutate: func(pr *github.TaskPR) { value := 2; pr.RequiredReviews = &value }}, + {name: "pending check count", mutate: func(pr *github.TaskPR) { pr.ChecksPassing-- }}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + changed := base + test.mutate(&changed) + if after := ciAutomationMergeSignature(&changed); after == before { + t.Fatalf("signature did not change for %s", test.name) + } + }) + } +} + func TestHandleTaskPRCIAutomationRecordsErrorWhenNoPromptableSession(t *testing.T) { ctx := context.Background() repo := setupTestRepo(t) @@ -2617,7 +2667,7 @@ func TestHandleTaskPRCIAutomationMergesWhenStateReadFails(t *testing.T) { } } -func TestHandleTaskPRCIAutomationRetriesMergeAfterTransientFailure(t *testing.T) { +func TestHandleTaskPRCIAutomationDoesNotRetryUnchangedMergeAfterFailure(t *testing.T) { ctx := context.Background() repo := setupTestRepo(t) seedTaskAndSession(t, repo, "task-1", "session-1", models.TaskSessionStateRunning) @@ -2640,6 +2690,7 @@ func TestHandleTaskPRCIAutomationRetriesMergeAfterTransientFailure(t *testing.T) ChecksState: "success", ReviewState: "approved", MergeableState: "clean", + HeadSHA: "head-a", } now := time.Now().UTC() pr.LastSyncedAt = &now @@ -2651,16 +2702,147 @@ func TestHandleTaskPRCIAutomationRetriesMergeAfterTransientFailure(t *testing.T) if ghSvc.mergeCalls != 1 { t.Fatalf("expected one merge call, got %d", ghSvc.mergeCalls) } - if len(ghSvc.mergeAttempts) != 0 { - t.Fatalf("expected failed merge not to record dedupe signature, got %+v", ghSvc.mergeAttempts) + if len(ghSvc.mergeAttempts) != 1 { + t.Fatalf("expected failed merge to keep one reservation, got %+v", ghSvc.mergeAttempts) } ghSvc.mergeErr = nil if err := svc.handleTaskPRCIAutomation(ctx, pr); err != nil { t.Fatalf("retry auto-merge: %v", err) } - if ghSvc.mergeCalls != 2 || len(ghSvc.mergeAttempts) != 1 { - t.Fatalf("expected retry to merge and record one attempt, calls=%d attempts=%d", ghSvc.mergeCalls, len(ghSvc.mergeAttempts)) + if ghSvc.mergeCalls != 1 || len(ghSvc.mergeAttempts) != 1 { + t.Fatalf("expected unchanged failure to remain blocked, calls=%d attempts=%d", ghSvc.mergeCalls, len(ghSvc.mergeAttempts)) + } + + pr.HeadSHA = "head-b" + ghSvc.triggerPRSyncAllPRs = []*github.TaskPR{pr} + if err := svc.handleTaskPRCIAutomation(ctx, pr); err != nil { + t.Fatalf("merge changed head: %v", err) + } + if ghSvc.mergeCalls != 2 || len(ghSvc.mergeAttempts) != 2 { + t.Fatalf("expected changed head to rearm once, calls=%d attempts=%d", ghSvc.mergeCalls, len(ghSvc.mergeAttempts)) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.5 +func TestRecordTaskPRMergeQueueObservationReconcilesMergedPR(t *testing.T) { + svc := &Service{} + message := "merge PR: provider status was lost" + ghSvc := &mockGitHubService{ciPRState: &github.TaskCIPRAutomationState{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + LastMergeResult: github.TaskCIMergeResultFailed, + LastError: &message, LastErrorKind: github.TaskCIErrorKindAutoMerge, + }} + svc.SetGitHubService(ghSvc) + svc.recordTaskPRMergeQueueObservation(context.Background(), &github.TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + State: githubPRStateMerged, HeadSHA: "head-a", + }) + + if ghSvc.ciPRState.LastMergeResult != github.TaskCIMergeResultAccepted || + ghSvc.ciPRState.LastError != nil || ghSvc.ciPRState.LastErrorKind != "" { + t.Fatalf("merged reconciliation state = %+v", ghSvc.ciPRState) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.4 +func TestHandleTaskPRCIAutomationConsumesExplicitMergeRetryAuthorization(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task-1", "session-1", models.TaskSessionStateRunning) + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + now := time.Now().UTC() + pr := &github.TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "widget", PRNumber: 42, + State: "open", ChecksState: "success", ReviewState: "approved", MergeableState: "clean", + HeadSHA: "head-a", LastSyncedAt: &now, + } + ghSvc := &mockGitHubService{ + ciOptionsResp: &github.TaskCIOptionsResponse{TaskID: "task-1", AutoMergeEnabled: true}, + triggerPRSyncAllPRs: []*github.TaskPR{pr}, + ciPRState: &github.TaskCIPRAutomationState{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + LastMergeSignature: ciAutomationMergeSignature(pr), + LastMergeResult: github.TaskCIMergeResultFailed, + LastQueueAttemptHeadSHA: pr.HeadSHA, + MergeRetryPending: true, + }, + } + svc.SetGitHubService(ghSvc) + + if err := svc.handleTaskPRCIAutomation(ctx, pr); err != nil { + t.Fatalf("handle explicit merge retry: %v", err) + } + if ghSvc.mergeCalls != 1 || len(ghSvc.mergeAttempts) != 1 { + t.Fatalf("merge calls = %d attempts = %d, want one", ghSvc.mergeCalls, len(ghSvc.mergeAttempts)) + } + if ghSvc.ciPRState.MergeRetryPending { + t.Fatal("explicit retry authorization was not consumed") + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.2 +func TestHandleTaskPRCIAutomationExpiresStaleInFlightMergeWithoutResubmission(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task-1", "session-1", models.TaskSessionStateRunning) + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + now := time.Now().UTC() + pr := &github.TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "widget", PRNumber: 42, + State: "open", ChecksState: "success", ReviewState: "approved", MergeableState: "clean", + HeadSHA: "head-a", LastSyncedAt: &now, + } + staleAttempt := now.Add(-ciAutomationDetachedTimeout - time.Second) + ghSvc := &mockGitHubService{ + ciOptionsResp: &github.TaskCIOptionsResponse{TaskID: "task-1", AutoMergeEnabled: true}, + triggerPRSyncAllPRs: []*github.TaskPR{pr}, + ciPRState: &github.TaskCIPRAutomationState{ + TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 42, + LastMergeSignature: ciAutomationMergeSignature(pr), + LastMergeResult: github.TaskCIMergeResultInFlight, LastMergeAttemptAt: &staleAttempt, + }, + } + svc.SetGitHubService(ghSvc) + + if err := svc.handleTaskPRCIAutomation(ctx, pr); err != nil { + t.Fatalf("handle stale in-flight merge: %v", err) + } + if ghSvc.mergeCalls != 0 { + t.Fatalf("merge calls = %d, want no resubmission", ghSvc.mergeCalls) + } + if ghSvc.ciPRState.LastMergeResult != github.TaskCIMergeResultFailed { + t.Fatalf("merge result = %q, want failed", ghSvc.ciPRState.LastMergeResult) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.6 +func TestHandleTaskPRCIAutomationReservationFailurePreventsProviderCall(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task-1", "session-1", models.TaskSessionStateRunning) + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + now := time.Now().UTC() + pr := &github.TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "widget", PRNumber: 42, + State: "open", ChecksState: "success", ReviewState: "approved", MergeableState: "clean", + HeadSHA: "head-a", LastSyncedAt: &now, + } + ghSvc := &mockGitHubService{ + ciOptionsResp: &github.TaskCIOptionsResponse{TaskID: "task-1", AutoMergeEnabled: true}, + triggerPRSyncAllPRs: []*github.TaskPR{pr}, + mergeAttemptErr: errors.New("state store unavailable"), + } + svc.SetGitHubService(ghSvc) + + if err := svc.handleTaskPRCIAutomation(ctx, pr); err != nil { + t.Fatalf("handle reservation failure: %v", err) + } + if ghSvc.mergeCalls != 0 { + t.Fatalf("merge calls = %d, want zero", ghSvc.mergeCalls) + } + if len(ghSvc.ciErrors) != 1 || ghSvc.ciErrors[0].LastErrorKind != github.TaskCIErrorKindAutoMerge { + t.Fatalf("typed retryable error = %+v", ghSvc.ciErrors) } } @@ -2698,10 +2880,10 @@ func TestHandlePRFeedbackStartsAutomationForMatchingPR(t *testing.T) { if ghSvc.lastExactPRLookup.Owner != "acme" || ghSvc.lastExactPRLookup.Repo != "back" || ghSvc.lastExactPRLookup.PRNumber != 2 { t.Fatalf("unexpected exact lookup: %+v", ghSvc.lastExactPRLookup) } - if _, loaded := svc.ciAutomationInFlight.Load("task-1|repo-back|2"); !loaded { + if !svc.ciAutomationInFlight.Has("task-1|repo-back|2") { t.Fatal("expected automation to run for matching repo-back PR") } - if _, loaded := svc.ciAutomationInFlight.Load("task-1|repo-front|1"); loaded { + if svc.ciAutomationInFlight.Has("task-1|repo-front|1") { t.Fatal("unexpected automation for non-matching repo-front PR") } close(block) @@ -2734,10 +2916,10 @@ func TestHandleTaskCIOptionsUpdatedStartsAutomationForTaskPRs(t *testing.T) { t.Fatalf("handle CI options updated: %v", err) } <-started - if _, loaded := svc.ciAutomationInFlight.Load("task-1|repo-front|1"); !loaded { + if !svc.ciAutomationInFlight.Has("task-1|repo-front|1") { t.Fatal("expected automation to run for repo-front PR") } - if _, loaded := svc.ciAutomationInFlight.Load("task-1|repo-back|2"); !loaded { + if !svc.ciAutomationInFlight.Has("task-1|repo-back|2") { t.Fatal("expected automation to run for repo-back PR") } close(block) @@ -2839,7 +3021,7 @@ func TestHandleTaskCIOptionsUpdatedStartsAutomationForPartialSyncResults(t *test case <-time.After(200 * time.Millisecond): t.Fatal("timed out waiting for CI automation to start") } - if _, loaded := svc.ciAutomationInFlight.Load("task-1|repo-1|42"); !loaded { + if !svc.ciAutomationInFlight.Has("task-1|repo-1|42") { t.Fatal("expected automation to run for partial sync result") } if len(ghSvc.ciErrors) != 0 { @@ -2948,7 +3130,7 @@ func waitForCIAutomationIdle(t *testing.T, svc *Service, key string, timeout tim ticker := time.NewTicker(time.Millisecond) defer ticker.Stop() for { - if _, loaded := svc.ciAutomationInFlight.Load(key); !loaded { + if !svc.ciAutomationInFlight.Has(key) { return } select { diff --git a/apps/backend/internal/orchestrator/event_handlers_github_test.go b/apps/backend/internal/orchestrator/event_handlers_github_test.go index ca37d518fa..2449cad535 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_test.go @@ -108,8 +108,10 @@ type mockGitHubService struct { fixAttempts []github.TaskCIFixAttempt fixCheckpointRefresh []github.TaskCIFixAttempt mergeAttempts []github.TaskCIMergeAttempt + mergeAttemptErr error mergeCalls int mergeErr error + mergeExpectedHeadSHA string ciErrors []github.TaskCIPRAutomationState ciExhausted []github.TaskCIPRAutomationState lifecyclePrompts []github.TaskPRLifecyclePrompt @@ -217,7 +219,40 @@ func (m *mockGitHubService) RefreshTaskCIFixCheckpoint(_ context.Context, taskID return nil } func (m *mockGitHubService) RecordTaskCIMergeAttempt(_ context.Context, attempt github.TaskCIMergeAttempt) error { + if m.mergeAttemptErr != nil { + return m.mergeAttemptErr + } + if m.ciPRState != nil && m.ciPRState.LastMergeSignature == attempt.Signature && !m.ciPRState.MergeRetryPending { + return github.ErrTaskCIMergeAttemptAlreadyReserved + } m.mergeAttempts = append(m.mergeAttempts, attempt) + if m.ciPRState == nil { + m.ciPRState = &github.TaskCIPRAutomationState{ + TaskID: attempt.TaskID, RepositoryID: attempt.RepositoryID, PRNumber: attempt.PRNumber, + } + } + m.ciPRState.LastMergeSignature = attempt.Signature + m.ciPRState.LastMergeResult = github.TaskCIMergeResultInFlight + m.ciPRState.MergeRetryPending = false + m.ciPRState.LastMergeAttemptAt = &attempt.AttemptedAt + return nil +} +func (m *mockGitHubService) RecordTaskCIMergeAttemptResult( + _ context.Context, taskID, repositoryID string, prNumber int, signature, result, message string, +) error { + if m.ciPRState == nil { + m.ciPRState = &github.TaskCIPRAutomationState{ + TaskID: taskID, RepositoryID: repositoryID, PRNumber: prNumber, + } + } + if m.ciPRState.LastMergeSignature != "" && m.ciPRState.LastMergeSignature != signature { + return github.ErrTaskCIMergeAttemptNotFound + } + m.ciPRState.LastMergeResult = result + if result == github.TaskCIMergeResultFailed { + m.ciPRState.LastError = &message + m.ciPRState.LastErrorKind = github.TaskCIErrorKindAutoMerge + } return nil } func (m *mockGitHubService) RecordTaskCIMergeQueueObservation(_ context.Context, observation github.TaskCIMergeQueueObservation) error { @@ -229,6 +264,13 @@ func (m *mockGitHubService) RecordTaskCIMergeQueueObservation(_ context.Context, if observation.ActiveQueueHeadSHA != "" { m.ciPRState.LastQueueAttemptHeadSHA = observation.ActiveQueueHeadSHA } + if observation.ActiveQueueHeadSHA != "" || observation.Accepted { + m.ciPRState.LastMergeResult = github.TaskCIMergeResultAccepted + if m.ciPRState.LastErrorKind == github.TaskCIErrorKindAutoMerge { + m.ciPRState.LastError = nil + m.ciPRState.LastErrorKind = "" + } + } if observation.RemovalCause != "" { m.ciPRState.LastQueueRemovalCause = observation.RemovalCause } @@ -243,6 +285,13 @@ func (m *mockGitHubService) RecordTaskCIError(_ context.Context, taskID, reposit }) return nil } +func (m *mockGitHubService) RecordTaskCIAutoMergeError(_ context.Context, taskID, repositoryID string, prNumber int, message string) error { + m.ciErrors = append(m.ciErrors, github.TaskCIPRAutomationState{ + TaskID: taskID, RepositoryID: repositoryID, PRNumber: prNumber, + LastError: &message, LastErrorKind: github.TaskCIErrorKindAutoMerge, + }) + return nil +} func (m *mockGitHubService) MarkTaskCIAutoFixExhausted(_ context.Context, taskID, repositoryID string, prNumber int, message string) error { now := time.Now().UTC() m.ciExhausted = append(m.ciExhausted, github.TaskCIPRAutomationState{ @@ -251,6 +300,7 @@ func (m *mockGitHubService) MarkTaskCIAutoFixExhausted(_ context.Context, taskID PRNumber: prNumber, AutoFixExhaustedAt: &now, LastError: &message, + LastErrorKind: github.TaskCIErrorKindAutoFix, }) return nil } @@ -315,8 +365,9 @@ func (m *mockGitHubService) MergePR(context.Context, string, string, int, string } func (m *mockGitHubService) MergePRForAutomation( - ctx context.Context, _, owner, repo string, number int, method string, + ctx context.Context, _, owner, repo string, number int, method, expectedHeadSHA string, ) error { + m.mergeExpectedHeadSHA = expectedHeadSHA return m.MergePR(ctx, owner, repo, number, method) } func (m *mockGitHubService) EnsurePRWatch(_ context.Context, _, _, _, _, _, branch string) (*github.PRWatch, error) { diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index c46e01a0d0..a39f1a50d7 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/apps/backend/internal/orchestrator/service.go @@ -665,9 +665,9 @@ type Service struct { // GitHub service for PR auto-detection on push githubService GitHubService - // ciAutomationInFlight prevents PR feedback and task-PR update events from - // racing duplicate auto-fix prompts or merge calls for the same PR. - ciAutomationInFlight sync.Map + // ciAutomationInFlight serializes each PR's evaluation and coalesces one + // follow-up request instead of dropping an event that arrives mid-run. + ciAutomationInFlight ciAutomationCoordinator // GitLab MR lifecycle notification automation. Nil-safe: without it, // gitlab.task_mr.updated events are observed but no lifecycle prompt is @@ -1051,6 +1051,15 @@ type Service struct { sendNowCancel context.CancelFunc sendNowStopped bool sendNowWorkers sync.WaitGroup + + // ciAutomationWorkers owns the asynchronous per-PR automation loops. The + // service-owned context lets Stop cancel in-flight evaluations and prevents + // workers from outliving the orchestrator across a restart. + ciAutomationMu sync.Mutex + ciAutomationCtx context.Context + ciAutomationCancel context.CancelFunc + ciAutomationStopped bool + ciAutomationWorkers sync.WaitGroup } type RouteAction string @@ -1219,6 +1228,7 @@ func NewService( // Create the service (watcher will be created after we have handlers) sendNowCtx, sendNowCancel := context.WithCancel(context.Background()) + ciAutomationCtx, ciAutomationCancel := context.WithCancel(context.Background()) s := &Service{ config: cfg, logger: svcLogger, @@ -1238,6 +1248,8 @@ func NewService( reservedPromptCallbacks: newReservedPromptCallbackOwner(), sendNowCtx: sendNowCtx, sendNowCancel: sendNowCancel, + ciAutomationCtx: ciAutomationCtx, + ciAutomationCancel: ciAutomationCancel, idleReaper: newIdleSessionReaper(), } // Always publish queue-status after a task-scoped queue purge so the @@ -2369,6 +2381,7 @@ func (s *Service) Start(ctx context.Context) error { s.logger.Info("starting orchestrator service") s.resetReservedPromptCallbacks() s.resetSendNowWorkers() + s.resetCIAutomationWorkers() // Reconcile session state from persisted runtime state on startup. // This does NOT launch any agent processes — sessions are recovered lazily @@ -2502,6 +2515,7 @@ func (s *Service) Stop() error { s.cancelAllClarificationWatchdogs() s.cancelAllTransientRetries() s.stopSendNowWorkers() + s.stopCIAutomationWorkers() if len(errs) > 0 { return errs[0] diff --git a/apps/backend/internal/task/statussummary/model.go b/apps/backend/internal/task/statussummary/model.go index 581990ca01..4bec99c84e 100644 --- a/apps/backend/internal/task/statussummary/model.go +++ b/apps/backend/internal/task/statussummary/model.go @@ -78,13 +78,15 @@ type GitSummary struct { // PullRequestSummary is intentionally an aggregate plus one representative // identity. It is not a list of PR records. type PullRequestSummary struct { - Count int `json:"count,omitempty"` - OpenCount int `json:"open_count,omitempty"` - Attention bool `json:"attention,omitempty"` - AggregateState string `json:"aggregate_state,omitempty"` - State string `json:"state,omitempty"` - Number int `json:"number,omitempty"` - URL string `json:"url,omitempty"` + Count int `json:"count,omitempty"` + OpenCount int `json:"open_count,omitempty"` + Attention bool `json:"attention,omitempty"` + AutoFixEnabled bool `json:"auto_fix_enabled,omitempty"` + AutoMergeEnabled bool `json:"auto_merge_enabled,omitempty"` + AggregateState string `json:"aggregate_state,omitempty"` + State string `json:"state,omitempty"` + Number int `json:"number,omitempty"` + URL string `json:"url,omitempty"` } // StoredTaskStatusSummary is the persistence boundary for one task. The diff --git a/apps/backend/internal/task/statussummary/projector.go b/apps/backend/internal/task/statussummary/projector.go index c81e2262af..5f8b03f7d8 100644 --- a/apps/backend/internal/task/statussummary/projector.go +++ b/apps/backend/internal/task/statussummary/projector.go @@ -189,6 +189,8 @@ type pullRequestObservation struct { requiredReviews int checksTotal int checksPassing int + autoFixEnabled bool + autoMergeEnabled bool } func NewProjector(cfg ProjectorConfig) *Projector { @@ -244,6 +246,7 @@ func (p *Projector) Start(ctx context.Context) error { events.BuildPermissionRequestWildcardSubject(), events.BuildGitEventWildcardSubject(), events.GitHubTaskPRUpdated, + events.GitHubTaskCIOptionsUpdated, events.MessageQueueStatusChanged, } for _, pattern := range patterns { @@ -331,6 +334,14 @@ func (p *Projector) handleEvent(ctx context.Context, event *bus.Event) error { } return fmt.Errorf("task status summary %q has no workspace", taskID) } + pullRequestChanged := false + if event.Type == events.GitHubTaskCIOptionsUpdated && p.loadPullRequests != nil { + before := derivePullRequestSummary(state) + if err := p.restorePullRequestObservations(ctx, taskID, state); err != nil { + return err + } + pullRequestChanged = !equalPullRequestSummary(before, derivePullRequestSummary(state)) + } taskErrorChanged := false if p.loadTaskLaunchError != nil && isTaskErrorRefreshEvent(event.Type) { taskErrorChanged, err = p.refreshTaskLaunchError(ctx, taskID, state) @@ -359,11 +370,11 @@ func (p *Projector) handleEvent(ctx context.Context, event *bus.Event) error { if refreshErr != nil { return refreshErr } - changed := p.applySourceEventLocked(state, event.Type, data) || pendingChanged || taskErrorChanged + changed := p.applySourceEventLocked(state, event.Type, data) || pendingChanged || taskErrorChanged || pullRequestChanged return p.persistPendingRefreshLocked(ctx, taskID, state, changed, event.Type, data) } - changed := p.applySourceEventLocked(state, event.Type, data) || taskErrorChanged + changed := p.applySourceEventLocked(state, event.Type, data) || taskErrorChanged || pullRequestChanged if !changed { return nil } diff --git a/apps/backend/internal/task/statussummary/projector_events.go b/apps/backend/internal/task/statussummary/projector_events.go index 595b1ddf5e..36b9604bcf 100644 --- a/apps/backend/internal/task/statussummary/projector_events.go +++ b/apps/backend/internal/task/statussummary/projector_events.go @@ -777,6 +777,21 @@ func (p *Projector) applyPREventLocked(state *projectionState, data map[string]i checksTotal: intValueOrZero(data["checks_total"]), checksPassing: intValueOrZero(data["checks_passing"]), } + // PR refresh events do not carry the per-PR automation switches. Preserve + // the last authoritative values from the CI-options projection instead of + // treating an omitted field as an explicit disable. A future producer may + // include either field; in that case its bool is authoritative, including + // false. + if existing, ok := state.prs[key]; ok { + observation.autoFixEnabled = existing.autoFixEnabled + observation.autoMergeEnabled = existing.autoMergeEnabled + } + if value, ok := data["auto_fix_enabled"].(bool); ok { + observation.autoFixEnabled = value + } + if value, ok := data["auto_merge_enabled"].(bool); ok { + observation.autoMergeEnabled = value + } if value, ok := intValue(data["required_reviews"]); ok { observation.requiredReviews = maxInt(value, 0) } diff --git a/apps/backend/internal/task/statussummary/projector_pr.go b/apps/backend/internal/task/statussummary/projector_pr.go index a5f9bea140..aefa3f3182 100644 --- a/apps/backend/internal/task/statussummary/projector_pr.go +++ b/apps/backend/internal/task/statussummary/projector_pr.go @@ -31,6 +31,8 @@ func applyPullRequestInputs(state *projectionState, inputs []PullRequestInput) { requiredReviews: maxInt(input.RequiredReviews, 0), checksTotal: maxInt(input.ChecksTotal, 0), checksPassing: maxInt(input.ChecksPassing, 0), + autoFixEnabled: input.AutoFixEnabled, + autoMergeEnabled: input.AutoMergeEnabled, } } } @@ -52,6 +54,12 @@ func derivePullRequestSummary(state *projectionState) *PullRequestSummary { summary.Count++ if strings.EqualFold(observation.state, prStateOpen) { summary.OpenCount++ + if observation.autoFixEnabled { + summary.AutoFixEnabled = true + } + if observation.autoMergeEnabled { + summary.AutoMergeEnabled = true + } } if pullRequestNeedsAttention(observation) { summary.Attention = true @@ -71,6 +79,13 @@ func derivePullRequestSummary(state *projectionState) *PullRequestSummary { return &summary } +func equalPullRequestSummary(left, right *PullRequestSummary) bool { + if left == nil || right == nil { + return left == right + } + return *left == *right +} + func pullRequestNeedsAttention(pr pullRequestObservation) bool { if strings.EqualFold(pr.reviewState, prStateChanges) || strings.EqualFold(pr.checksState, prStateFailure) || diff --git a/apps/backend/internal/task/statussummary/projector_restart_rehydration_test.go b/apps/backend/internal/task/statussummary/projector_restart_rehydration_test.go index 7f42d64e0e..129f63aaf6 100644 --- a/apps/backend/internal/task/statussummary/projector_restart_rehydration_test.go +++ b/apps/backend/internal/task/statussummary/projector_restart_rehydration_test.go @@ -102,6 +102,96 @@ func TestProjectorRehydratesPullRequestObservationsAfterRestartWithoutAggregate( } } +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.10 +func TestProjectorRefreshesAutomationFlagsAfterCIOptionsUpdate(t *testing.T) { + store := newProjectorTestStore() + store.rows["task-pr-options-refresh"] = &StoredTaskStatusSummary{ + TaskID: "task-pr-options-refresh", + WorkspaceID: "workspace-1", + Summary: TaskStatusSummary{ + Revision: 1, + PullRequest: &PullRequestSummary{ + Count: 1, OpenCount: 1, State: prStateOpen, Number: 41, + AutoFixEnabled: true, + }, + }, + } + loaderCalls := 0 + projector := NewProjector(ProjectorConfig{ + Store: store, + ResolveWorkspace: func(context.Context, string) (string, error) { + return "workspace-1", nil + }, + LoadPullRequests: func(context.Context, string) ([]PullRequestInput, error) { + loaderCalls++ + return []PullRequestInput{{ + Key: "repo-a#41", State: prStateOpen, Number: 41, + AutoFixEnabled: loaderCalls == 1, + }}, nil + }, + Now: func() time.Time { return time.Date(2026, 8, 16, 0, 30, 0, 0, time.UTC) }, + }) + + if err := projector.HandleEvent(context.Background(), bus.NewEvent(events.GitHubTaskCIOptionsUpdated, "test", map[string]interface{}{ + "task_id": "task-pr-options-refresh", "workspace_id": "workspace-1", + })); err != nil { + t.Fatalf("replay CI options event: %v", err) + } + updated := store.summary("task-pr-options-refresh") + if updated == nil || updated.PullRequest == nil || updated.PullRequest.AutoFixEnabled { + t.Fatalf("updated summary = %+v, want auto-fix disabled", updated) + } + if loaderCalls != 2 { + t.Fatalf("PR loader calls = %d, want initial load plus options refresh", loaderCalls) + } +} + +func TestProjectorPreservesAutomationFlagsAcrossPRRefresh(t *testing.T) { + store := newProjectorTestStore() + loaderCalls := 0 + projector := NewProjector(ProjectorConfig{ + Store: store, + ResolveWorkspace: func(context.Context, string) (string, error) { + return "workspace-1", nil + }, + LoadPullRequests: func(context.Context, string) ([]PullRequestInput, error) { + loaderCalls++ + return []PullRequestInput{{ + Key: "repo-a#41", State: prStateOpen, Number: 41, + AutoFixEnabled: loaderCalls > 1, + }}, nil + }, + Now: func() time.Time { return time.Date(2026, 8, 16, 0, 30, 0, 0, time.UTC) }, + }) + + prEvent := func() *bus.Event { + return bus.NewEvent(events.GitHubTaskPRUpdated, "test", map[string]interface{}{ + "task_id": "task-pr-options-preserve", "workspace_id": "workspace-1", + "repository_id": "repo-a", "state": prStateOpen, "pr_number": 41, + "pr_url": "https://example.test/41", + }) + } + if err := projector.HandleEvent(context.Background(), prEvent()); err != nil { + t.Fatalf("initial PR refresh: %v", err) + } + if err := projector.HandleEvent(context.Background(), bus.NewEvent(events.GitHubTaskCIOptionsUpdated, "test", map[string]interface{}{ + "task_id": "task-pr-options-preserve", "workspace_id": "workspace-1", + })); err != nil { + t.Fatalf("CI options refresh: %v", err) + } + if err := projector.HandleEvent(context.Background(), prEvent()); err != nil { + t.Fatalf("subsequent PR refresh: %v", err) + } + + got := store.summary("task-pr-options-preserve") + if got == nil || got.PullRequest == nil || !got.PullRequest.AutoFixEnabled { + t.Fatalf("PR summary after refresh = %+v, want auto-fix preserved", got) + } + if loaderCalls != 2 { + t.Fatalf("PR loader calls = %d, want initial load plus options refresh", loaderCalls) + } +} + func TestProjectorRehydratesSourceObservationsWithoutPersistedSummary(t *testing.T) { store := newProjectorTestStore() sessionLoaderCalls := 0 diff --git a/apps/backend/internal/task/statussummary/rebuild.go b/apps/backend/internal/task/statussummary/rebuild.go index 57914dfe84..59a880e141 100644 --- a/apps/backend/internal/task/statussummary/rebuild.go +++ b/apps/backend/internal/task/statussummary/rebuild.go @@ -41,6 +41,8 @@ type PullRequestInput struct { RequiredReviews int ChecksTotal int ChecksPassing int + AutoFixEnabled bool + AutoMergeEnabled bool } // RebuildInput contains the authoritative bounded facts available from diff --git a/apps/backend/internal/task/statussummary/rebuild_test.go b/apps/backend/internal/task/statussummary/rebuild_test.go index 64c74e79ff..2a8482ba1f 100644 --- a/apps/backend/internal/task/statussummary/rebuild_test.go +++ b/apps/backend/internal/task/statussummary/rebuild_test.go @@ -86,6 +86,44 @@ func TestBuildFromAuthoritativeAggregatesDurableSources(t *testing.T) { } } +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.10 +func TestBuildFromAuthoritativeAggregatesAutomationOnlyForOpenPullRequests(t *testing.T) { + got := BuildFromAuthoritative(RebuildInput{ + PRObserved: true, + PullRequests: []PullRequestInput{ + {Key: "repo-a#1", State: prStateOpen, Number: 1, AutoFixEnabled: true}, + {Key: "repo-a#2", State: prStateOpen, Number: 2, AutoMergeEnabled: true}, + {Key: "repo-a#3", State: prStateMerged, Number: 3, AutoFixEnabled: true, AutoMergeEnabled: true}, + {Key: "repo-a#4", State: prStateClosed, Number: 4, AutoFixEnabled: true, AutoMergeEnabled: true}, + }, + }) + + if got.PullRequest == nil { + t.Fatal("pull request summary is nil") + } + if !got.PullRequest.AutoFixEnabled || !got.PullRequest.AutoMergeEnabled { + t.Fatalf("automation flags = %+v, want both active flags", got.PullRequest) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-MERGE-QUEUE-002.10 +func TestBuildFromAuthoritativeOmitsAutomationForTerminalPullRequests(t *testing.T) { + got := BuildFromAuthoritative(RebuildInput{ + PRObserved: true, + PullRequests: []PullRequestInput{ + {Key: "repo-a#3", State: prStateMerged, Number: 3, AutoFixEnabled: true}, + {Key: "repo-a#4", State: prStateClosed, Number: 4, AutoMergeEnabled: true}, + }, + }) + + if got.PullRequest == nil { + t.Fatal("pull request summary is nil") + } + if got.PullRequest.AutoFixEnabled || got.PullRequest.AutoMergeEnabled { + t.Fatalf("terminal automation flags = %+v, want both false", got.PullRequest) + } +} + func TestDeriveForegroundActivityPrefersGeneratingOverBackground(t *testing.T) { got := deriveForegroundActivity([]string{activityBackground, activityGenerating}) if got != activityGenerating { diff --git a/apps/web/components/github/pr-ci-automation-controls.test.tsx b/apps/web/components/github/pr-ci-automation-controls.test.tsx new file mode 100644 index 0000000000..c0da62980a --- /dev/null +++ b/apps/web/components/github/pr-ci-automation-controls.test.tsx @@ -0,0 +1,113 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import type { TaskCIAutomationOptions, TaskPR } from "@/lib/types/github"; + +const hookMocks = vi.hoisted(() => ({ + error: null as string | null, + options: null as TaskCIAutomationOptions | null, + refresh: vi.fn(), + retryMerge: vi.fn(), +})); + +vi.mock("@/hooks/domains/github/use-task-ci-options", () => ({ + useTaskCIAutomationOptions: () => ({ + options: hookMocks.options, + loading: false, + saving: false, + error: hookMocks.error, + refresh: hookMocks.refresh, + retryMerge: hookMocks.retryMerge, + update: vi.fn(), + resetPrompt: vi.fn(), + }), +})); + +vi.mock("@/components/toast-provider", () => ({ + useToast: () => ({ toast: vi.fn() }), +})); + +vi.mock("@/components/github/pr-ci-automation-prompt-dialog", () => ({ + CIAutomationPromptDialog: () => null, +})); + +vi.mock("@/components/github/pr-ci-automation-rows", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + CIAutomationHeader: () => null, + CIAutomationOptionRows: () => null, + }; +}); + +import { PRCIAutomationControls } from "./pr-ci-automation-controls"; + +const pr = { + task_id: "task-1", + repository_id: "repo-1", + pr_number: 42, +} as TaskPR; + +function makeOptions(errorKind: string): TaskCIAutomationOptions { + return { + task_id: "task-1", + auto_fix_enabled: false, + auto_merge_enabled: true, + auto_fix_prompt_override: null, + effective_auto_fix_prompt: "Default prompt", + using_default_prompt: true, + updated_at: "2026-08-28T10:00:00Z", + pr_options: [], + pr_states: [ + { + task_id: "task-1", + repository_id: "repo-1", + pr_number: 42, + last_error: "merge PR: provider unavailable", + last_error_kind: errorKind, + } as TaskCIAutomationOptions["pr_states"][number], + ], + }; +} + +beforeEach(() => { + hookMocks.error = null; + hookMocks.options = makeOptions("auto_merge"); + hookMocks.refresh.mockReset().mockResolvedValue(null); + hookMocks.retryMerge.mockReset().mockResolvedValue({ accepted: true }); +}); + +afterEach(cleanup); + +describe("PRCIAutomationControls retry actions", () => { + it("rearms only a typed auto-merge error for the exact linked PR", () => { + render(); + + const action = screen.getByRole("button", { name: "Retry" }); + expect(action.className).toContain("h-11"); + fireEvent.click(action); + + expect(hookMocks.retryMerge).toHaveBeenCalledWith("repo-1", 42); + expect(hookMocks.refresh).not.toHaveBeenCalled(); + }); + + it("refreshes state for a stored error that is not an auto-merge failure", () => { + hookMocks.options = makeOptions("auto_fix"); + render(); + + fireEvent.click(screen.getByRole("button", { name: "Refresh" })); + + expect(hookMocks.refresh).toHaveBeenCalledTimes(1); + expect(hookMocks.retryMerge).not.toHaveBeenCalled(); + }); + + it("refreshes state after a load error and never rearms merge", () => { + hookMocks.options = null; + hookMocks.error = "backend unavailable"; + render(); + + fireEvent.click(screen.getByRole("button", { name: "Refresh" })); + + expect(hookMocks.refresh).toHaveBeenCalledTimes(1); + expect(hookMocks.retryMerge).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/components/github/pr-ci-automation-controls.tsx b/apps/web/components/github/pr-ci-automation-controls.tsx index e30842148c..a264164637 100644 --- a/apps/web/components/github/pr-ci-automation-controls.tsx +++ b/apps/web/components/github/pr-ci-automation-controls.tsx @@ -1,6 +1,7 @@ "use client"; import { useCallback, useState } from "react"; +import { useTranslation } from "react-i18next"; import { useToast } from "@/components/toast-provider"; import { useTaskCIAutomationOptions } from "@/hooks/domains/github/use-task-ci-options"; import { @@ -17,9 +18,10 @@ import { } from "@/components/github/pr-ci-automation-rows"; export function PRCIAutomationControls({ pr }: { pr: TaskPR }) { - const { options, loading, saving, error, refresh, update, resetPrompt } = + const { options, loading, saving, error, refresh, update, resetPrompt, retryMerge } = useTaskCIAutomationOptions(pr.task_id); const { toast } = useToast(); + const { t } = useTranslation(); const [promptOpen, setPromptOpen] = useState(false); const [promptDraft, setPromptDraft] = useState(""); const automationState = findCIAutomationStateForPR(options?.pr_states, pr); @@ -48,9 +50,11 @@ export function PRCIAutomationControls({ pr }: { pr: TaskPR }) { repository_id: pr.repository_id ?? "", pr_number: pr.pr_number, }; - Promise.resolve(update(scoped)).catch(() => reportError("Failed to update CI automation.")); + Promise.resolve(update(scoped)).catch(() => + reportError(t("github:failedToUpdateCiAutomation")), + ); }, - [pr, reportError, update], + [pr, reportError, t, update], ); // The auto-fix prompt override stays task-level (applies to every linked @@ -60,18 +64,26 @@ export function PRCIAutomationControls({ pr }: { pr: TaskPR }) { if (!value) return; Promise.resolve(update({ auto_fix_prompt_override: value })) .then(() => setPromptOpen(false)) - .catch(() => reportError("Failed to save auto-fix prompt.")); - }, [promptDraft, reportError, update]); + .catch(() => reportError(t("github:failedToSaveAutoFixPrompt"))); + }, [promptDraft, reportError, t, update]); const useDefaultPrompt = useCallback(() => { Promise.resolve(resetPrompt()) .then(() => setPromptOpen(false)) - .catch(() => reportError("Failed to reset auto-fix prompt.")); - }, [reportError, resetPrompt]); + .catch(() => reportError(t("github:failedToResetAutoFixPrompt"))); + }, [reportError, resetPrompt, t]); const retryLoad = useCallback(() => { - Promise.resolve(refresh()).catch(() => reportError("Failed to load CI automation.")); - }, [refresh, reportError]); + Promise.resolve(refresh()).catch(() => + reportError(t("github:failedToLoadCiAutomationOptions")), + ); + }, [refresh, reportError, t]); + + const retryAutoMerge = useCallback(() => { + Promise.resolve(retryMerge(pr.repository_id ?? "", pr.pr_number)) + .then(() => toast({ description: t("github:autoMergeRetryAccepted") })) + .catch(() => reportError(t("github:failedToRetryAutoMerge"))); + }, [pr.pr_number, pr.repository_id, reportError, retryMerge, t, toast]); const disabled = loading || saving || !options; return ( @@ -93,14 +105,17 @@ export function PRCIAutomationControls({ pr }: { pr: TaskPR }) { patchOption={patchOption} automationState={automationState} /> - {automationState?.last_error && ( - - )} - {error && } + ); } + +function PRCIAutomationErrors({ + storedError, + storedErrorKind, + loadError, + loading, + saving, + retryLabel, + refreshLabel, + onRetryMerge, + onRefresh, +}: { + storedError?: string | null; + storedErrorKind?: string; + loadError?: string | null; + loading: boolean; + saving: boolean; + retryLabel: string; + refreshLabel: string; + onRetryMerge: () => void; + onRefresh: () => void; +}) { + const mergeError = storedErrorKind === "auto_merge"; + return ( + <> + {storedError && ( + + )} + {loadError && ( + + )} + + ); +} diff --git a/apps/web/components/github/pr-ci-automation-rows.tsx b/apps/web/components/github/pr-ci-automation-rows.tsx index 61581a79fd..ebb165160f 100644 --- a/apps/web/components/github/pr-ci-automation-rows.tsx +++ b/apps/web/components/github/pr-ci-automation-rows.tsx @@ -104,13 +104,14 @@ export function CIAutomationRow({ export function CIAutomationErrorRow({ error, loading, - onRetry, + actionLabel, + onAction, }: { error: string; loading: boolean; - onRetry: () => void; + actionLabel: string; + onAction: () => void; }) { - const { t } = useTranslation(); return (
- {t("github:retry")} + {actionLabel}
); diff --git a/apps/web/components/github/pr-ci-popover.automation.test.tsx b/apps/web/components/github/pr-ci-popover.automation.test.tsx index 6f7f97e5d0..b2107a0c9b 100644 --- a/apps/web/components/github/pr-ci-popover.automation.test.tsx +++ b/apps/web/components/github/pr-ci-popover.automation.test.tsx @@ -290,7 +290,9 @@ describe("PRCIPopover automation status", () => { auto_fix_exhausted_at: null, last_merge_signature: "", last_merge_attempt_at: null, + last_merge_result: "", last_error: "No promptable task session is available.", + last_error_kind: "", created_at: "", updated_at: "", }, @@ -338,10 +340,12 @@ describe("PRCIPopover automation status", () => { auto_fix_exhausted_at: null, last_merge_signature: "", last_merge_attempt_at: null, + last_merge_result: "", last_queue_attempt_head_sha: "head-a", last_queue_fix_event_id: "", last_queue_removal_cause: "provider_changed", last_error: null, + last_error_kind: "", created_at: "", updated_at: "", }, @@ -599,12 +603,12 @@ describe("PRCIPopover task prompts", () => { expect(hookMocks.updateMock).not.toHaveBeenCalled(); }); - it("offers retry after CI automation options fail to load", () => { + it("offers refresh after CI automation options fail to load", () => { hookMocks.error = BACKEND_UNAVAILABLE; renderPopover(); expect(screen.getByText(BACKEND_UNAVAILABLE)).not.toBeNull(); - fireEvent.click(screen.getByRole("button", { name: "Retry" })); + fireEvent.click(screen.getByRole("button", { name: "Refresh" })); expect(hookMocks.refreshMock).toHaveBeenCalledTimes(1); }); diff --git a/apps/web/components/github/pr-status-chip.auto-fix-rounds.test.tsx b/apps/web/components/github/pr-status-chip.auto-fix-rounds.test.tsx index da16f67264..9998314cb9 100644 --- a/apps/web/components/github/pr-status-chip.auto-fix-rounds.test.tsx +++ b/apps/web/components/github/pr-status-chip.auto-fix-rounds.test.tsx @@ -143,7 +143,9 @@ function makeCIPrState(roundCount: number, exhausted = false) { auto_fix_exhausted_at: exhausted ? "2026-06-18T11:00:00Z" : null, last_merge_signature: "", last_merge_attempt_at: null, + last_merge_result: "" as const, last_error: exhausted ? "CI auto-fix paused after 10 rounds for this PR" : null, + last_error_kind: "", created_at: "2026-06-18T10:00:00Z", updated_at: "2026-06-18T10:00:00Z", }; diff --git a/apps/web/components/github/pr-task-automation.ts b/apps/web/components/github/pr-task-automation.ts new file mode 100644 index 0000000000..dd6f58aeea --- /dev/null +++ b/apps/web/components/github/pr-task-automation.ts @@ -0,0 +1,51 @@ +import type { TaskCIAutomationOptions, TaskPR } from "@/lib/types/github"; +import { automationForPR } from "./pr-status-automation-badges"; +import type { TaskPRInfo } from "@/components/task/task-pr-info"; + +export type { TaskPRInfo } from "@/components/task/task-pr-info"; + +export type TaskPRAutomationDetail = { + number: number; + repository?: string; + autoFixEnabled: boolean; + autoMergeEnabled: boolean; +}; + +export type TaskPRAutomationSummary = { + autoFixEnabled: boolean; + autoMergeEnabled: boolean; + details: TaskPRAutomationDetail[]; +}; + +export function getTaskPRAutomationSummary( + prs: TaskPR[], + prInfo?: TaskPRInfo, + options?: TaskCIAutomationOptions | null, +): TaskPRAutomationSummary { + const activePRs = prs.filter((pr) => pr.state.trim().toLowerCase() === "open"); + // The bounded task-row projection is the source of truth until the full PR + // list is hydrated. Keep its flags visible even when a previously hydrated + // options response is present but the scoped PR records are not. + if (!options || prs.length === 0) { + return { + autoFixEnabled: prInfo?.autoFixEnabled === true, + autoMergeEnabled: prInfo?.autoMergeEnabled === true, + details: [], + }; + } + const details = activePRs.map((pr) => { + const automation = automationForPR(options, pr); + const repository = [pr.owner, pr.repo].filter(Boolean).join("/") || undefined; + return { + number: pr.pr_number, + ...(repository ? { repository } : {}), + autoFixEnabled: automation.autoFix, + autoMergeEnabled: automation.autoMerge, + }; + }); + return { + autoFixEnabled: details.some((detail) => detail.autoFixEnabled), + autoMergeEnabled: details.some((detail) => detail.autoMergeEnabled), + details, + }; +} diff --git a/apps/web/components/github/pr-task-icon-disclosure.tsx b/apps/web/components/github/pr-task-icon-disclosure.tsx new file mode 100644 index 0000000000..27b94f2433 --- /dev/null +++ b/apps/web/components/github/pr-task-icon-disclosure.tsx @@ -0,0 +1,312 @@ +"use client"; + +import { IconGitPullRequest } from "@tabler/icons-react"; +import { + forwardRef, + type FocusEventHandler, + type MouseEventHandler, + type PointerEventHandler, + type ReactNode, + type Ref, +} from "react"; +import { + Drawer, + DrawerContent, + DrawerDescription, + DrawerHeader, + DrawerTitle, + DrawerTrigger, +} from "@kandev/ui/drawer"; +import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; +import { useTranslation } from "react-i18next"; +import { cn } from "@/lib/utils"; +import type { TaskPR } from "@/lib/types/github"; +import type { TaskPRTooltipHydrationStatus } from "@/hooks/domains/github/use-task-pr-tooltip-hydration"; +import { useChangeRequestTaskTooltipState } from "@/components/integrations/use-change-request-task-tooltip-state"; +import type { TaskPRAutomationSummary, TaskPRInfo } from "./pr-task-automation"; + +export type PRTaskIconDisclosureProps = { + taskId: string; + prInfo?: TaskPRInfo; + prs: TaskPR[]; + hasFullData: boolean; + singlePR: TaskPR | null; + readyToMerge: boolean; + allReadyToMerge: boolean; + displayState: string | undefined; + displayCount: number; + iconColor: string; + ariaLabel: string; + icon: ReactNode; + content: ReactNode; +}; + +export function AutomationIndicatorDots({ + autoFixEnabled, + autoMergeEnabled, +}: Pick) { + return ( + <> + {autoFixEnabled && ( +