From 7855a4e087755e330f823ea21fc2e84cab7da104 Mon Sep 17 00:00:00 2001 From: Alassane <79718216+yattdev@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:36:11 -0400 Subject: [PATCH] fix(github): scope PR CI automation switches per linked PR (#2512) Co-authored-by: ayattara Co-authored-by: Carlos Florencio --- apps/backend/internal/github/controller.go | 16 + .../internal/github/controller_test.go | 115 ++++ apps/backend/internal/github/errors.go | 5 + apps/backend/internal/github/models.go | 87 ++- .../internal/github/service_ci_automation.go | 296 ++++++++-- .../github/service_ci_automation_test.go | 76 ++- .../github/service_cleanup_policy_test.go | 10 +- apps/backend/internal/github/store.go | 433 +++++++++++++- .../github/store_ci_automation_test.go | 337 ++++++++++- .../mcp/handlers/task_pr_automation.go | 8 + .../mcp/handlers/task_pr_automation_test.go | 90 +++ apps/backend/internal/mcp/server/server.go | 18 +- .../orchestrator/event_handlers_github.go | 32 +- .../event_handlers_github_ci_automation.go | 25 + .../event_handlers_github_ci_options_test.go | 107 ++++ .../github/pr-ci-automation-controls.tsx | 547 +----------------- .../github/pr-ci-automation-prompt-dialog.tsx | 124 ++++ .../github/pr-ci-automation-rows.tsx | 419 ++++++++++++++ .../github/pr-ci-popover.automation.test.tsx | 128 +++- .../pr-status-automation-badges.test.tsx | 29 +- .../github/pr-status-automation-badges.tsx | 65 ++- .../pr-status-chip.auto-fix-rounds.test.tsx | 22 +- .../github/pr-status-chip.test-fixtures.ts | 70 +++ .../components/github/pr-status-chip.test.tsx | 54 +- .../tests/pr/ci-automation-options.spec.ts | 94 ++- .../pr/mobile-ci-automation-options.spec.ts | 65 ++- apps/web/eslint.i18n.options.mjs | 2 + .../github/use-task-ci-options.test.tsx | 1 + apps/web/lib/github/ci-automation.test.ts | 63 +- apps/web/lib/github/ci-automation.ts | 37 +- .../state/slices/github/github-slice.test.ts | 1 + apps/web/lib/types/github.ts | 21 + apps/web/lib/ws/handlers/github.test.ts | 2 + apps/web/src/locales/en/github.json | 5 + apps/web/src/locales/pseudo/github.json | 5 + ...notifications-extend-task-pr-automation.md | 15 +- docs/public/feature-status.md | 2 +- docs/public/integrations.md | 2 +- docs/public/sessions-and-review.md | 2 + docs/specs/gitlab-integration/spec.md | 11 +- docs/specs/ui/ci-pr-automation.md | 152 ++++- 41 files changed, 2803 insertions(+), 790 deletions(-) create mode 100644 apps/backend/internal/orchestrator/event_handlers_github_ci_options_test.go create mode 100644 apps/web/components/github/pr-ci-automation-prompt-dialog.tsx create mode 100644 apps/web/components/github/pr-ci-automation-rows.tsx create mode 100644 apps/web/components/github/pr-status-chip.test-fixtures.ts diff --git a/apps/backend/internal/github/controller.go b/apps/backend/internal/github/controller.go index 2965c39b94..5cdec399f0 100644 --- a/apps/backend/internal/github/controller.go +++ b/apps/backend/internal/github/controller.go @@ -396,6 +396,10 @@ func (c *Controller) writeTaskCIOptionsError(ctx *gin.Context, err error, operat ctx.JSON(http.StatusNotFound, gin.H{"error": "task not found"}) return } + if errors.Is(err, ErrTaskPRNotLinked) { + ctx.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } if writeGitHubOperationalAuthError(ctx, err) { return } @@ -417,6 +421,18 @@ func parseTaskCIOptionsPatch(ctx *gin.Context) (TaskCIOptionsPatch, error) { var patch TaskCIOptionsPatch for key, value := range raw { switch key { + case "repository_id": + var repositoryID string + if err := json.Unmarshal(value, &repositoryID); err != nil { + return TaskCIOptionsPatch{}, err + } + patch.RepositoryID = &repositoryID + case "pr_number": + var prNumber int + if err := json.Unmarshal(value, &prNumber); err != nil { + return TaskCIOptionsPatch{}, err + } + patch.PRNumber = &prNumber case "auto_fix_enabled": var enabled bool if err := json.Unmarshal(value, &enabled); err != nil { diff --git a/apps/backend/internal/github/controller_test.go b/apps/backend/internal/github/controller_test.go index 8c7ac53ef9..0a648bbd2e 100644 --- a/apps/backend/internal/github/controller_test.go +++ b/apps/backend/internal/github/controller_test.go @@ -1202,6 +1202,121 @@ func TestHttpTaskCIOptions_DefaultAndPatch(t *testing.T) { } } +// 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. +func TestHttpPatchTaskCIOptions_TargetsOnePRWithoutAffectingSibling(t *testing.T) { + router, store := setupControllerStoreTest(t) + ctx := context.Background() + for _, prNumber := range []int{1, 2} { + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", + Owner: "acme", Repo: "widget", PRNumber: prNumber, + State: "open", CreatedAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("seed task pr #%d: %v", prNumber, err) + } + } + + body := bytes.NewBufferString(`{"repository_id":"repo-1","pr_number":1,"auto_fix_enabled":true}`) + req := httptest.NewRequest(http.MethodPatch, "/api/v1/github/tasks/task-1/ci-options", body) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + + opts1, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 1) + if err != nil { + t.Fatalf("get PR 1 options: %v", err) + } + if !opts1.AutoFixEnabled { + t.Fatalf("PR 1 auto_fix_enabled = false, want true") + } + opts2, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 2) + if err != nil { + t.Fatalf("get PR 2 options: %v", err) + } + if opts2.AutoFixEnabled { + t.Fatalf("PR 2 auto_fix_enabled = true, want unaffected false") + } +} + +// TestHttpPatchTaskCIOptions_UnlinkedPRReturns400AndWritesNothing covers AC8. +func TestHttpPatchTaskCIOptions_UnlinkedPRReturns400AndWritesNothing(t *testing.T) { + router, store := setupControllerStoreTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", + Owner: "acme", Repo: "widget", PRNumber: 1, + State: "open", CreatedAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("seed task pr: %v", err) + } + + body := bytes.NewBufferString(`{"repository_id":"repo-1","pr_number":999,"auto_fix_enabled":true,"auto_fix_prompt_override":"must-not-persist"}`) + req := httptest.NewRequest(http.MethodPatch, "/api/v1/github/tasks/task-1/ci-options", body) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String()) + } + + opts, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 999) + if err != nil { + t.Fatalf("get unlinked PR options: %v", err) + } + if opts.AutoFixEnabled { + t.Fatal("PATCH for an unlinked PR wrote a row instead of writing nothing") + } + taskOptions, err := store.GetTaskCIOptions(ctx, "task-1") + if err != nil { + t.Fatalf("get task options: %v", err) + } + if taskOptions.AutoFixPromptOverride != nil { + t.Fatalf("invalid PR identity partially updated task options: %+v", taskOptions) + } +} + +// TestHttpPatchTaskCIOptions_PartialPRIdentityRejected pins the client error +// for a patch that supplies only one side of the PR identity. +func TestHttpPatchTaskCIOptions_PartialPRIdentityRejected(t *testing.T) { + for name, body := range map[string]string{ + "repository only": `{"repository_id":"repo-1","auto_fix_enabled":true}`, + "PR number only": `{"pr_number":1,"auto_fix_enabled":true}`, + } { + t.Run(name, func(t *testing.T) { + router, store := setupControllerStoreTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", + Owner: "acme", Repo: "widget", PRNumber: 1, + State: "open", CreatedAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("seed task pr: %v", err) + } + + req := httptest.NewRequest(http.MethodPatch, "/api/v1/github/tasks/task-1/ci-options", bytes.NewBufferString(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String()) + } + + opts, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 1) + if err != nil { + t.Fatalf("get PR options: %v", err) + } + if opts.AutoFixEnabled { + t.Fatal("partial identity patch wrote a row instead of writing nothing") + } + }) + } +} + func TestHttpTaskCIOptions_DeniesForeignWorkspaceReadAndUpdate(t *testing.T) { store := newTestStore(t) log := newControllerTestLogger() diff --git a/apps/backend/internal/github/errors.go b/apps/backend/internal/github/errors.go index 6c128a8fd9..a6b0622831 100644 --- a/apps/backend/internal/github/errors.go +++ b/apps/backend/internal/github/errors.go @@ -51,6 +51,11 @@ var ErrInvalidToken = errors.New("invalid token") // transient upstream blip. var ErrRepoNotResolvable = errors.New("github: repository not resolvable") +// ErrTaskPRNotLinked is returned by UpdateTaskCIOptions when the patch names +// a repository_id/pr_number that is not currently linked to the task. HTTP +// callers translate this into a 400 rather than a generic 500. +var ErrTaskPRNotLinked = errors.New("PR is not linked to this task") + // errStoreUnavailable is returned from service methods when no Store is // wired (Provide can return a Service with store == nil when the SQLite // repos aren't configured). Returning a typed error instead of nil- diff --git a/apps/backend/internal/github/models.go b/apps/backend/internal/github/models.go index d51047fd8d..83ebb61df3 100644 --- a/apps/backend/internal/github/models.go +++ b/apps/backend/internal/github/models.go @@ -447,26 +447,46 @@ type TaskPR struct { } // TaskCIOptions stores task-level PR automation preferences. +// +// The five automation switches below (AutoFixEnabled, AutoMergeEnabled, +// PromptOnReviewRequested, PromptOnMerged, PromptOnClosed) are legacy: they +// are no longer written by UpdateTaskCIOptions and are read only by the +// one-time pr_scope_migrated_at fan-out migration +// (migrateTaskCIOptionsToPRScope). The per-PR source of truth is +// TaskPRAutomationOptions / github_task_pr_automation_options. Genuinely +// task-level fields (AutoFixPromptOverride, ReviewReviewerLogin) remain here. type TaskCIOptions struct { TaskID string `json:"task_id" db:"task_id"` - AutoFixEnabled bool `json:"auto_fix_enabled" db:"auto_fix_enabled"` - AutoMergeEnabled bool `json:"auto_merge_enabled" db:"auto_merge_enabled"` + AutoFixEnabled bool `json:"-" db:"auto_fix_enabled"` + AutoMergeEnabled bool `json:"-" db:"auto_merge_enabled"` AutoFixPromptOverride *string `json:"auto_fix_prompt_override,omitempty" db:"auto_fix_prompt_override"` - PromptOnReviewRequested bool `json:"prompt_on_review_requested" db:"prompt_on_review_requested"` - PromptOnMerged bool `json:"prompt_on_merged" db:"prompt_on_merged"` - PromptOnClosed bool `json:"prompt_on_closed" db:"prompt_on_closed"` + PromptOnReviewRequested bool `json:"-" db:"prompt_on_review_requested"` + PromptOnMerged bool `json:"-" db:"prompt_on_merged"` + PromptOnClosed bool `json:"-" db:"prompt_on_closed"` ReviewReviewerLogin string `json:"review_reviewer_login" db:"review_reviewer_login"` // Lifecycle override columns remain only to read and clear legacy rows during // the additive schema migration. They are not part of the update or API model. - ReviewPromptOverride *string `json:"-" db:"review_prompt_override"` - MergedPromptOverride *string `json:"-" db:"merged_prompt_override"` - ClosedPromptOverride *string `json:"-" db:"closed_prompt_override"` - CreatedAt time.Time `json:"created_at" db:"created_at"` - UpdatedAt time.Time `json:"updated_at" db:"updated_at"` + ReviewPromptOverride *string `json:"-" db:"review_prompt_override"` + MergedPromptOverride *string `json:"-" db:"merged_prompt_override"` + ClosedPromptOverride *string `json:"-" db:"closed_prompt_override"` + // PRScopeMigratedAt guards the one-time fan-out of the legacy booleans + // above onto github_task_pr_automation_options. Nil means "not yet + // migrated"; non-nil means the fan-out already ran for this task and + // must never re-run (it would silently re-enable a switch a user has + // since turned off for a specific PR). + PRScopeMigratedAt *time.Time `json:"-" db:"pr_scope_migrated_at"` + CreatedAt time.Time `json:"created_at" db:"created_at"` + UpdatedAt time.Time `json:"updated_at" db:"updated_at"` } // TaskCIOptionsPatch is a partial update for task CI automation options. +// RepositoryID/PRNumber optionally target one linked PR for the five +// automation switches; when both are nil the switches fan out to every PR +// currently linked to the task. AutoFixPromptOverride and ReviewReviewerLogin +// are always task-level regardless of PR identity. type TaskCIOptionsPatch struct { + RepositoryID *string + PRNumber *int AutoFixEnabled *bool AutoMergeEnabled *bool AutoFixPromptOverride *string @@ -483,7 +503,53 @@ func (p TaskCIOptionsPatch) HasAny() bool { p.ReviewReviewerLogin != nil } +// PRAutomationPatch extracts the per-PR automation switch fields. +func (p TaskCIOptionsPatch) PRAutomationPatch() TaskPRAutomationOptionsPatch { + return TaskPRAutomationOptionsPatch{ + AutoFixEnabled: p.AutoFixEnabled, + AutoMergeEnabled: p.AutoMergeEnabled, + PromptOnReviewRequested: p.PromptOnReviewRequested, + PromptOnMerged: p.PromptOnMerged, + PromptOnClosed: p.PromptOnClosed, + } +} + +// TaskPRAutomationOptions stores per-PR automation switches, keyed by +// (task_id, repository_id, pr_number). This is the source of truth for the +// five CI/lifecycle automation switches; TaskCIOptions keeps only the +// genuinely task-level fields (prompt override, reviewer login). +type TaskPRAutomationOptions struct { + TaskID string `json:"task_id" db:"task_id"` + RepositoryID string `json:"repository_id" db:"repository_id"` + PRNumber int `json:"pr_number" db:"pr_number"` + AutoFixEnabled bool `json:"auto_fix_enabled" db:"auto_fix_enabled"` + AutoMergeEnabled bool `json:"auto_merge_enabled" db:"auto_merge_enabled"` + PromptOnReviewRequested bool `json:"prompt_on_review_requested" db:"prompt_on_review_requested"` + PromptOnMerged bool `json:"prompt_on_merged" db:"prompt_on_merged"` + PromptOnClosed bool `json:"prompt_on_closed" db:"prompt_on_closed"` + CreatedAt time.Time `json:"created_at" db:"created_at"` + UpdatedAt time.Time `json:"updated_at" db:"updated_at"` +} + +// TaskPRAutomationOptionsPatch is a partial update for one PR's automation switches. +type TaskPRAutomationOptionsPatch struct { + AutoFixEnabled *bool + AutoMergeEnabled *bool + PromptOnReviewRequested *bool + PromptOnMerged *bool + PromptOnClosed *bool +} + +// HasAny reports whether the patch contains at least one requested field change. +func (p TaskPRAutomationOptionsPatch) HasAny() bool { + return p.AutoFixEnabled != nil || p.AutoMergeEnabled != nil || + p.PromptOnReviewRequested != nil || p.PromptOnMerged != nil || p.PromptOnClosed != nil +} + // TaskCIOptionsResponse is the HTTP shape for task CI automation options. +// The top-level automation booleans are an aggregate over PROptions ("every +// linked PR has this switch enabled, and at least one PR is linked") kept for +// MCP/API read compatibility; PROptions is the per-PR source of truth. type TaskCIOptionsResponse struct { TaskID string `json:"task_id"` AutoFixEnabled bool `json:"auto_fix_enabled"` @@ -501,6 +567,7 @@ type TaskCIOptionsResponse struct { EffectiveClosedPrompt string `json:"-"` UpdatedAt time.Time `json:"updated_at"` PRStates []*TaskCIPRAutomationState `json:"pr_states"` + PROptions []*TaskPRAutomationOptions `json:"pr_options"` // WorkspaceID is routing metadata for workspace-scoped WebSocket delivery. // It stays JSON-visible because NATS-backed event buses round-trip payloads. WorkspaceID string `json:"workspace_id,omitempty"` diff --git a/apps/backend/internal/github/service_ci_automation.go b/apps/backend/internal/github/service_ci_automation.go index ea494d9680..dc3715c1b3 100644 --- a/apps/backend/internal/github/service_ci_automation.go +++ b/apps/backend/internal/github/service_ci_automation.go @@ -8,6 +8,10 @@ import ( promptcfg "github.com/kandev/kandev/config/prompts" ) +// errTaskPRIdentityIncomplete signals a patch that set exactly one of +// repository_id/pr_number instead of both or neither. +var errTaskPRIdentityIncomplete = fmt.Errorf("%w: repository_id and pr_number must both be set", ErrTaskPRNotLinked) + // GetTaskCIOptionsResponse returns task CI automation options plus effective prompt text. func (s *Service) GetTaskCIOptionsResponse(ctx context.Context, taskID string) (*TaskCIOptionsResponse, error) { if s.store == nil { @@ -28,7 +32,12 @@ func (s *Service) GetTaskCIOptionsResponse(ctx context.Context, taskID string) ( return response, err } -// UpdateTaskCIOptions updates task CI automation options and returns the response shape. +// UpdateTaskCIOptions updates task CI automation options and returns the +// response shape. patch.RepositoryID/PRNumber optionally target one linked +// PR for the five automation switches; omitted identity fans the switches +// out to every PR currently linked to the task (unchanged behavior for +// existing MCP callers). AutoFixPromptOverride and ReviewReviewerLogin are +// always applied task-level regardless of PR identity. func (s *Service) UpdateTaskCIOptions(ctx context.Context, taskID string, patch TaskCIOptionsPatch) (*TaskCIOptionsResponse, error) { if s.store == nil { return nil, errStoreUnavailable @@ -37,10 +46,32 @@ func (s *Service) UpdateTaskCIOptions(ctx context.Context, taskID string, patch if err != nil { return nil, err } + if (patch.RepositoryID == nil) != (patch.PRNumber == nil) { + return nil, errTaskPRIdentityIncomplete + } + prPatch := patch.PRAutomationPatch() + var targets []*TaskPR + if patch.RepositoryID != nil || prPatch.HasAny() { + targets, err = s.resolveTaskPRAutomationTargets(ctx, taskID, patch.RepositoryID, patch.PRNumber) + if err != nil { + return nil, err + } + } if err := s.populateReviewReviewer(ctx, taskID, &patch); err != nil { return nil, err } - opts, err := s.store.UpdateTaskCIOptions(ctx, taskID, patch) + reviewerChanged, err := s.taskPRReviewerChanged(ctx, taskID, patch.ReviewReviewerLogin) + if err != nil { + return nil, err + } + var opts *TaskCIOptions + if prPatch.HasAny() { + opts, err = s.store.UpdateTaskCIOptionsWithPRAutomation( + ctx, taskID, patch, targets, prPatch, reviewerChanged, + ) + } else { + opts, err = s.store.UpdateTaskCIOptions(ctx, taskID, patch) + } if err != nil { return nil, err } @@ -80,6 +111,43 @@ func (s *Service) resolveAuthorizedTaskWorkspace(ctx context.Context, taskID str return workspaceID, nil } +// taskPRReviewerChanged compares the newly resolved task-level reviewer +// login (if the patch is setting one) against the value already stored, so +// callers can re-baseline review-request checkpoints precisely when the +// connected account actually changed. +func (s *Service) taskPRReviewerChanged(ctx context.Context, taskID string, newLogin *string) (bool, error) { + if newLogin == nil { + return false, nil + } + previous, err := s.store.GetTaskCIOptions(ctx, taskID) + if err != nil { + return false, err + } + return !strings.EqualFold(previous.ReviewReviewerLogin, strings.TrimSpace(*newLogin)), nil +} + +// resolveTaskPRAutomationTargets resolves which linked PR(s) a patch +// applies to. Nil identity fans out to every linked PR (AC7); a named +// identity that isn't currently linked is rejected with ErrTaskPRNotLinked +// (AC8) rather than silently creating an orphan automation row. +func (s *Service) resolveTaskPRAutomationTargets( + ctx context.Context, taskID string, repositoryID *string, prNumber *int, +) ([]*TaskPR, error) { + prs, err := s.store.ListTaskPRsByTask(ctx, taskID) + if err != nil { + return nil, err + } + if repositoryID == nil && prNumber == nil { + return prs, nil + } + for _, pr := range prs { + if pr.RepositoryID == *repositoryID && pr.PRNumber == *prNumber { + return []*TaskPR{pr}, nil + } + } + return nil, fmt.Errorf("%w: repository_id=%s pr_number=%d", ErrTaskPRNotLinked, *repositoryID, *prNumber) +} + func (s *Service) populateReviewReviewer( ctx context.Context, taskID string, @@ -88,23 +156,58 @@ func (s *Service) populateReviewReviewer( if patch.PromptOnReviewRequested == nil { return nil } - if !*patch.PromptOnReviewRequested { - empty := "" - patch.ReviewReviewerLogin = &empty + if *patch.PromptOnReviewRequested { + resolved, err := s.resolveTaskPRAutomationClient(ctx, taskID) + if err != nil { + return err + } + login, err := resolved.Client.GetAuthenticatedUser(ctx) + if err != nil { + return fmt.Errorf("resolve authenticated reviewer: %w", err) + } + patch.ReviewReviewerLogin = &login return nil } - resolved, err := s.resolveTaskPRAutomationClient(ctx, taskID) + // Disabling review-request on the target PR(s) only blanks the shared + // task-level reviewer login when no other linked PR still depends on + // it — otherwise this would silently break that PR's automation. + stillNeeded, err := s.anyOtherPRHasReviewRequestEnabled(ctx, taskID, patch.RepositoryID, patch.PRNumber) if err != nil { return err } - login, err := resolved.Client.GetAuthenticatedUser(ctx) - if err != nil { - return fmt.Errorf("resolve authenticated reviewer: %w", err) + if stillNeeded { + return nil } - patch.ReviewReviewerLogin = &login + empty := "" + patch.ReviewReviewerLogin = &empty return nil } +func (s *Service) anyOtherPRHasReviewRequestEnabled( + ctx context.Context, taskID string, excludeRepositoryID *string, excludePRNumber *int, +) (bool, error) { + fanOut := excludeRepositoryID == nil && excludePRNumber == nil + options, err := s.taskPRAutomationOptionsList(ctx, taskID) + if err != nil { + return false, err + } + for _, opt := range options { + if !opt.PromptOnReviewRequested { + continue + } + if fanOut { + // The fan-out disable targets every linked PR, so nothing + // survives with the switch on regardless of stored state. + continue + } + if opt.RepositoryID == *excludeRepositoryID && opt.PRNumber == *excludePRNumber { + continue + } + return true, nil + } + return false, nil +} + // GetTaskCIPRState returns per-PR CI automation state, or nil. func (s *Service) GetTaskCIPRState(ctx context.Context, taskID, repositoryID string, prNumber int) (*TaskCIPRAutomationState, error) { if s.store == nil { @@ -180,12 +283,20 @@ func (s *Service) IsReviewRequestedForLogin( return false, nil } +// HasEnabledTaskPRAgentPrompts reports whether any PR linked to the task has +// a lifecycle prompt switch enabled. Checks the per-PR table, not the legacy +// task row, so a task with mixed per-PR configuration is still detected. func (s *Service) HasEnabledTaskPRAgentPrompts(ctx context.Context, taskID string) (bool, error) { - opts, err := s.store.GetTaskCIOptions(ctx, taskID) + options, err := s.taskPRAutomationOptionsList(ctx, taskID) if err != nil { return false, err } - return opts.PromptOnReviewRequested || opts.PromptOnMerged || opts.PromptOnClosed, nil + for _, opt := range options { + if opt.PromptOnReviewRequested || opt.PromptOnMerged || opt.PromptOnClosed { + return true, nil + } + } + return false, nil } func (s *Service) SetTaskPRReviewRequestState( @@ -274,7 +385,7 @@ func (s *Service) RecordTaskPRLifecyclePrompt(ctx context.Context, prompt TaskPR func (s *Service) ShouldHoldTerminalPRWatch( ctx context.Context, taskID, repositoryID string, prNumber int, state string, ) (bool, error) { - opts, err := s.store.GetTaskCIOptions(ctx, taskID) + opts, err := s.store.GetTaskPRAutomationOptions(ctx, taskID, repositoryID, prNumber) if err != nil { return false, err } @@ -295,30 +406,145 @@ func (s *Service) buildTaskCIOptionsResponse(ctx context.Context, opts *TaskCIOp if err != nil { return nil, err } + prOptions, err := s.taskPRAutomationOptionsList(ctx, opts.TaskID) + if err != nil { + return nil, err + } effectivePrompt, usingDefault := s.effectiveCIAutoFixPrompt(ctx, opts) reviewPrompt := effectiveTaskPRPrompt("pr-review-requested") mergedPrompt := effectiveTaskPRPrompt("pr-merged-final") closedPrompt := effectiveTaskPRPrompt("pr-closed-final") + aggregate := aggregateTaskPRAutomationOptions(prOptions) return &TaskCIOptionsResponse{ TaskID: opts.TaskID, - AutoFixEnabled: opts.AutoFixEnabled, - AutoMergeEnabled: opts.AutoMergeEnabled, + AutoFixEnabled: aggregate.autoFixEnabled, + AutoMergeEnabled: aggregate.autoMergeEnabled, AutoFixPromptOverride: opts.AutoFixPromptOverride, AutoFixMaxRounds: TaskCIAutoFixMaxRounds, EffectiveAutoFixPrompt: effectivePrompt, UsingDefaultPrompt: usingDefault, - PromptOnReviewRequested: opts.PromptOnReviewRequested, - PromptOnMerged: opts.PromptOnMerged, - PromptOnClosed: opts.PromptOnClosed, + PromptOnReviewRequested: aggregate.promptOnReviewRequested, + PromptOnMerged: aggregate.promptOnMerged, + PromptOnClosed: aggregate.promptOnClosed, ReviewReviewerLogin: opts.ReviewReviewerLogin, EffectiveReviewPrompt: reviewPrompt, EffectiveMergedPrompt: mergedPrompt, EffectiveClosedPrompt: closedPrompt, UpdatedAt: opts.UpdatedAt, PRStates: prStates, + PROptions: prOptions, }, nil } +// taskPRAutomationOptionsList returns one entry per PR currently linked to +// the task (AC5), synthesizing all-off defaults for a linked PR that has no +// stored row yet — mirroring the placeholder pattern in taskCIPRStates. +func (s *Service) taskPRAutomationOptionsList(ctx context.Context, taskID string) ([]*TaskPRAutomationOptions, error) { + stored, err := s.store.ListTaskPRAutomationOptions(ctx, taskID) + if err != nil { + return nil, err + } + prs, err := s.store.ListTaskPRsByTask(ctx, taskID) + if err != nil { + return nil, err + } + allPRs, err := s.store.ListTaskPRsByTaskIncludingDetached(ctx, taskID) + if err != nil { + return nil, err + } + merged := mergeTaskPRRows(stored, prs, + func(opt *TaskPRAutomationOptions) string { return taskCIPRStateKey(opt.RepositoryID, opt.PRNumber) }, + func(pr *TaskPR) *TaskPRAutomationOptions { + return &TaskPRAutomationOptions{TaskID: taskID, RepositoryID: pr.RepositoryID, PRNumber: pr.PRNumber} + }, + ) + active := make(map[string]struct{}, len(prs)) + for _, pr := range prs { + active[taskCIPRStateKey(pr.RepositoryID, pr.PRNumber)] = struct{}{} + } + detached := make(map[string]struct{}) + for _, pr := range allPRs { + if pr.DetachedAt != nil { + detached[taskCIPRStateKey(pr.RepositoryID, pr.PRNumber)] = struct{}{} + } + } + filtered := make([]*TaskPRAutomationOptions, 0, len(prs)) + for _, option := range merged { + key := taskCIPRStateKey(option.RepositoryID, option.PRNumber) + if _, ok := detached[key]; ok { + continue + } + if _, ok := active[key]; ok { + filtered = append(filtered, option) + continue + } + // Keep legacy/single-repo option rows that predate a durable TaskPR + // association. Only a matching detached association is excluded. + if _, known := detached[key]; !known { + filtered = append(filtered, option) + } + } + return filtered, nil +} + +// mergeTaskPRRows overlays stored per-PR rows onto every PR currently linked +// to the task, synthesizing a default row (via makeDefault) for a linked PR +// with no stored row yet, and keeping any stored row for a PR no longer +// linked (e.g. detached) at the end. Task CI state keeps those historical rows +// for diagnostics, while task automation options filters them before exposing +// aggregates or lifecycle eligibility. Shared by taskCIPRStates and +// taskPRAutomationOptionsList, which differ only in row type. +func mergeTaskPRRows[T any](stored []T, prs []*TaskPR, keyOf func(T) string, makeDefault func(*TaskPR) T) []T { + byKey := make(map[string]T, len(stored)) + for _, row := range stored { + byKey[keyOf(row)] = row + } + out := make([]T, 0, max(len(prs), len(stored))) + seen := make(map[string]struct{}, len(prs)) + for _, pr := range prs { + key := taskCIPRStateKey(pr.RepositoryID, pr.PRNumber) + if row, ok := byKey[key]; ok { + out = append(out, row) + } else { + out = append(out, makeDefault(pr)) + } + seen[key] = struct{}{} + } + for _, row := range stored { + if _, ok := seen[keyOf(row)]; !ok { + out = append(out, row) + } + } + return out +} + +type taskPRAutomationAggregate struct { + autoFixEnabled, autoMergeEnabled bool + promptOnReviewRequested, promptOnMerged, promptOnClosed bool +} + +// aggregateTaskPRAutomationOptions reports a switch as "enabled" only when +// every linked PR has it on and at least one PR is linked — the top-level +// booleans answer "did my task-wide enable take", kept for MCP/API read +// compatibility (R3). Prefer PROptions for anything PR-specific. +func aggregateTaskPRAutomationOptions(options []*TaskPRAutomationOptions) taskPRAutomationAggregate { + if len(options) == 0 { + return taskPRAutomationAggregate{} + } + agg := taskPRAutomationAggregate{ + autoFixEnabled: true, autoMergeEnabled: true, + promptOnReviewRequested: true, promptOnMerged: true, promptOnClosed: true, + } + for _, opt := range options { + agg.autoFixEnabled = agg.autoFixEnabled && opt.AutoFixEnabled + agg.autoMergeEnabled = agg.autoMergeEnabled && opt.AutoMergeEnabled + agg.promptOnReviewRequested = agg.promptOnReviewRequested && opt.PromptOnReviewRequested + agg.promptOnMerged = agg.promptOnMerged && opt.PromptOnMerged + agg.promptOnClosed = agg.promptOnClosed && opt.PromptOnClosed + } + return agg +} + func effectiveTaskPRPrompt(name string) string { return promptcfg.Get(name) } @@ -342,36 +568,18 @@ func (s *Service) taskCIPRStates(ctx context.Context, taskID string) ([]*TaskCIP if err != nil { return nil, err } - byKey := make(map[string]*TaskCIPRAutomationState, len(stored)) - for _, state := range stored { - byKey[taskCIPRStateKey(state.RepositoryID, state.PRNumber)] = state - } prs, err := s.store.ListTaskPRsByTask(ctx, taskID) if err != nil { return nil, err } - out := make([]*TaskCIPRAutomationState, 0, max(len(prs), len(stored))) - seen := make(map[string]struct{}, len(prs)) - for _, pr := range prs { - key := taskCIPRStateKey(pr.RepositoryID, pr.PRNumber) - if state, ok := byKey[key]; ok { - out = append(out, state) - } else { - out = append(out, &TaskCIPRAutomationState{ - TaskID: taskID, - RepositoryID: pr.RepositoryID, - PRNumber: pr.PRNumber, - }) - } - seen[key] = struct{}{} - } - for _, state := range stored { - key := taskCIPRStateKey(state.RepositoryID, state.PRNumber) - if _, ok := seen[key]; !ok { - out = append(out, state) - } - } - return out, nil + return mergeTaskPRRows(stored, prs, + func(state *TaskCIPRAutomationState) string { + return taskCIPRStateKey(state.RepositoryID, state.PRNumber) + }, + func(pr *TaskPR) *TaskCIPRAutomationState { + return &TaskCIPRAutomationState{TaskID: taskID, RepositoryID: pr.RepositoryID, PRNumber: pr.PRNumber} + }, + ), nil } func taskCIPRStateKey(repositoryID string, prNumber int) string { diff --git a/apps/backend/internal/github/service_ci_automation_test.go b/apps/backend/internal/github/service_ci_automation_test.go index a2c1f13a07..20456728eb 100644 --- a/apps/backend/internal/github/service_ci_automation_test.go +++ b/apps/backend/internal/github/service_ci_automation_test.go @@ -232,6 +232,54 @@ func TestServiceTaskCIPRStatesMergesStoredAndCurrentPRs(t *testing.T) { } } +func TestServiceTaskCIOptionsExcludeDetachedPRAutomation(t *testing.T) { + store := newTestStore(t) + svc := NewService(&stubClient{}, "pat", nil, store, nil, testLogger(t)) + ctx := context.Background() + now := time.Now().UTC() + + active := &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "widget", + PRNumber: 1, State: "open", CreatedAt: now, + } + detached := &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "widget", + PRNumber: 2, State: "open", CreatedAt: now.Add(time.Second), + } + for _, pr := range []*TaskPR{active, detached} { + if err := store.CreateTaskPR(ctx, pr); err != nil { + t.Fatalf("create PR #%d: %v", pr.PRNumber, err) + } + } + if _, transitioned, err := store.DetachTaskPR(ctx, detached.ID); err != nil || !transitioned { + t.Fatalf("detach PR #%d: transitioned=%v err=%v", detached.PRNumber, transitioned, err) + } + + enabled := true + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-1", detached.PRNumber, + TaskPRAutomationOptionsPatch{PromptOnMerged: &enabled}, false); err != nil { + t.Fatalf("enable detached PR automation: %v", err) + } + + response, err := svc.GetTaskCIOptionsResponse(ctx, "task-1") + if err != nil { + t.Fatalf("get task CI options: %v", err) + } + if len(response.PROptions) != 1 || response.PROptions[0].PRNumber != active.PRNumber { + t.Fatalf("PR options = %+v, want only active PR #%d", response.PROptions, active.PRNumber) + } + if response.AutoFixEnabled || response.AutoMergeEnabled || response.PromptOnMerged { + t.Fatalf("detached automation affected task aggregate: %+v", response) + } + hasEnabled, err := svc.HasEnabledTaskPRAgentPrompts(ctx, "task-1") + if err != nil { + t.Fatalf("check enabled lifecycle prompts: %v", err) + } + if hasEnabled { + t.Fatal("detached lifecycle automation kept the task eligible for PR automation") + } +} + func TestServiceTaskPRAgentAutomationHelpers(t *testing.T) { store := newTestStore(t) client := NewMockClient() @@ -373,8 +421,15 @@ func TestServiceUpdateTaskCIOptionsResolvesReviewerForWorkspacelessTaskPR(t *tes if err != nil { t.Fatalf("get options: %v", err) } - if !options.PromptOnReviewRequested || options.ReviewReviewerLogin != "reviewer-a" { - t.Fatalf("options = %+v, want review prompt bound to reviewer-a", options) + if options.ReviewReviewerLogin != "reviewer-a" { + t.Fatalf("options = %+v, want reviewer login reviewer-a", options) + } + prOptions, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 42) + if err != nil { + t.Fatalf("get PR options: %v", err) + } + if !prOptions.PromptOnReviewRequested { + t.Fatalf("PR options = %+v, want prompt_on_review_requested=true", prOptions) } } @@ -509,10 +564,13 @@ func TestServiceTerminalWatchReenableKeepsOtherEventAccepted(t *testing.T) { ctx := context.Background() enabled := true disabled := false - if _, err := store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{ - PromptOnMerged: &enabled, PromptOnClosed: &enabled, - }); err != nil { - t.Fatalf("enable terminal prompts: %v", err) + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-merged", 1, + TaskPRAutomationOptionsPatch{PromptOnMerged: &enabled}, false); err != nil { + t.Fatalf("enable merged prompt: %v", err) + } + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-closed", 2, + TaskPRAutomationOptionsPatch{PromptOnClosed: &enabled}, false); err != nil { + t.Fatalf("enable closed prompt: %v", err) } for _, prompt := range []TaskPRLifecyclePrompt{ {TaskID: "task-1", RepositoryID: "repo-merged", PRNumber: 1, Event: "merged", ObservedState: prStateMerged}, @@ -522,10 +580,12 @@ func TestServiceTerminalWatchReenableKeepsOtherEventAccepted(t *testing.T) { t.Fatalf("record terminal prompt: %v", err) } } - if _, err := store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{PromptOnMerged: &disabled}); err != nil { + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-merged", 1, + TaskPRAutomationOptionsPatch{PromptOnMerged: &disabled}, false); err != nil { t.Fatalf("disable merged prompt: %v", err) } - if _, err := store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{PromptOnMerged: &enabled}); err != nil { + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-merged", 1, + TaskPRAutomationOptionsPatch{PromptOnMerged: &enabled}, false); err != nil { t.Fatalf("re-enable merged prompt: %v", err) } diff --git a/apps/backend/internal/github/service_cleanup_policy_test.go b/apps/backend/internal/github/service_cleanup_policy_test.go index b4ab4e3ff5..effc62222a 100644 --- a/apps/backend/internal/github/service_cleanup_policy_test.go +++ b/apps/backend/internal/github/service_cleanup_policy_test.go @@ -92,9 +92,8 @@ func TestShouldDeleteReviewTask_AutoPolicy_LifecyclePromptPreserves(t *testing.T Number: 99, State: prStateMerged, RepoOwner: "acme", RepoName: "widget", }) enabled := true - if _, err := store.UpdateTaskCIOptions(ctx, "task-99", TaskCIOptionsPatch{ - PromptOnMerged: &enabled, - }); err != nil { + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-99", "", 99, + TaskPRAutomationOptionsPatch{PromptOnMerged: &enabled}, false); err != nil { t.Fatalf("enable merged prompt: %v", err) } svc.taskSessionChecker = &recordingSessionChecker{} @@ -112,9 +111,8 @@ func TestShouldDeleteReviewTask_AutoPolicy_LifecyclePromptPreservesWithoutSessio Number: 99, State: prStateMerged, RepoOwner: "acme", RepoName: "widget", }) enabled := true - if _, err := store.UpdateTaskCIOptions(ctx, "task-99", TaskCIOptionsPatch{ - PromptOnMerged: &enabled, - }); err != nil { + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-99", "", 99, + TaskPRAutomationOptionsPatch{PromptOnMerged: &enabled}, false); err != nil { t.Fatalf("enable merged prompt: %v", err) } svc.taskSessionChecker = nil diff --git a/apps/backend/internal/github/store.go b/apps/backend/internal/github/store.go index 977a17fc35..4f641b8c03 100644 --- a/apps/backend/internal/github/store.go +++ b/apps/backend/internal/github/store.go @@ -367,10 +367,29 @@ const createTablesSQL = ` review_prompt_override TEXT, merged_prompt_override TEXT, closed_prompt_override TEXT, + pr_scope_migrated_at DATETIME, created_at DATETIME NOT NULL, updated_at DATETIME NOT NULL ); + -- Per-PR automation switches. Source of truth for the five automation + -- switches (AutoFixEnabled, AutoMergeEnabled, PromptOnReviewRequested, + -- PromptOnMerged, PromptOnClosed) formerly stored task-wide on + -- github_task_ci_options. See migrateTaskCIOptionsToPRScope. + CREATE TABLE IF NOT EXISTS github_task_pr_automation_options ( + task_id TEXT NOT NULL, + repository_id TEXT NOT NULL DEFAULT '', + pr_number INTEGER NOT NULL, + auto_fix_enabled BOOLEAN NOT NULL DEFAULT 0, + auto_merge_enabled BOOLEAN NOT NULL DEFAULT 0, + prompt_on_review_requested BOOLEAN NOT NULL DEFAULT 0, + prompt_on_merged BOOLEAN NOT NULL DEFAULT 0, + prompt_on_closed BOOLEAN NOT NULL DEFAULT 0, + created_at DATETIME NOT NULL, + updated_at DATETIME NOT NULL, + PRIMARY KEY (task_id, repository_id, pr_number) + ); + CREATE TABLE IF NOT EXISTS github_task_ci_pr_state ( task_id TEXT NOT NULL, repository_id TEXT NOT NULL DEFAULT '', @@ -526,6 +545,9 @@ func (s *Store) initSchemaUpgrades() error { if err := s.addTaskPRAgentAutomationColumns(); err != nil { return err } + if err := s.addPRScopeMigrationColumn(); err != nil { + return err + } if err := s.addGitHubAuthFlowExpectationColumns(); err != nil { return err } @@ -565,6 +587,9 @@ func (s *Store) initSchemaData(legacyUpgrade bool) error { if err := s.backfillPRWatchesRepositoryID(); err != nil { return fmt.Errorf("backfill github_pr_watches.repository_id: %w", err) } + if err := s.migrateTaskCIOptionsToPRScope(); err != nil { + return fmt.Errorf("migrate task CI options to PR scope: %w", err) + } if err := s.backfillGitHubWorkspaceOwnership(); err != nil { return err } @@ -576,6 +601,83 @@ func (s *Store) initSchemaData(legacyUpgrade bool) error { return nil } +// migrateTaskCIOptionsToPRScope seeds github_task_pr_automation_options rows +// from each pre-upgrade github_task_ci_options row's legacy booleans, fanning +// each task's values out onto every github_task_prs row currently linked to +// it. Guarded by pr_scope_migrated_at, stamped in the same transaction as the +// fan-out insert: without the marker, replaying this on every boot would +// re-enable a switch a user has since turned off for one PR (R2), and a PR +// linked to the task after migration would incorrectly inherit the legacy +// value instead of starting all-off (AC17) via ON CONFLICT DO NOTHING alone. +func (s *Store) migrateTaskCIOptionsToPRScope() error { + type legacyOptions struct { + taskID string + autoFix, autoMerge, promptReview, promptMerged, promptClosed bool + } + rows, err := s.db.Query(` + SELECT task_id, auto_fix_enabled, auto_merge_enabled, prompt_on_review_requested, + prompt_on_merged, prompt_on_closed + FROM github_task_ci_options + WHERE pr_scope_migrated_at IS NULL`) + if err != nil { + return fmt.Errorf("list unmigrated task CI options: %w", err) + } + var legacy []legacyOptions + for rows.Next() { + var row legacyOptions + if err := rows.Scan( + &row.taskID, &row.autoFix, &row.autoMerge, &row.promptReview, &row.promptMerged, &row.promptClosed, + ); err != nil { + _ = rows.Close() + return fmt.Errorf("scan unmigrated task CI options: %w", err) + } + legacy = append(legacy, row) + } + if err := rows.Err(); err != nil { + return fmt.Errorf("iterate unmigrated task CI options: %w", err) + } + if err := rows.Close(); err != nil { + return fmt.Errorf("close unmigrated task CI options rows: %w", err) + } + for _, row := range legacy { + if err := s.fanOutTaskCIOptionsToPRScope( + row.taskID, row.autoFix, row.autoMerge, row.promptReview, row.promptMerged, row.promptClosed, + ); err != nil { + return err + } + } + return nil +} + +func (s *Store) fanOutTaskCIOptionsToPRScope( + taskID string, autoFix, autoMerge, promptReview, promptMerged, promptClosed bool, +) error { + tx, err := s.db.Beginx() + if err != nil { + return err + } + defer func() { _ = tx.Rollback() }() + now := time.Now().UTC() + if _, err := tx.Exec(` + INSERT INTO github_task_pr_automation_options ( + task_id, repository_id, pr_number, auto_fix_enabled, auto_merge_enabled, + prompt_on_review_requested, prompt_on_merged, prompt_on_closed, created_at, updated_at + ) + SELECT task_id, repository_id, pr_number, ?, ?, ?, ?, ?, ?, ? + FROM github_task_prs + WHERE task_id = ? AND detached_at IS NULL + ON CONFLICT(task_id, repository_id, pr_number) DO NOTHING`, + autoFix, autoMerge, promptReview, promptMerged, promptClosed, now, now, taskID); err != nil { + return fmt.Errorf("fan out task CI options for %s: %w", taskID, err) + } + if _, err := tx.Exec( + `UPDATE github_task_ci_options SET pr_scope_migrated_at = ? WHERE task_id = ?`, now, taskID, + ); err != nil { + return fmt.Errorf("stamp pr_scope_migrated_at for %s: %w", taskID, err) + } + return tx.Commit() +} + func (s *Store) clearLifecyclePromptOverrides() error { _, err := s.db.Exec(` UPDATE github_task_ci_options @@ -1023,6 +1125,24 @@ func (s *Store) addTaskCIRoundColumns() error { return nil } +// addPRScopeMigrationColumn adds the marker column that guards the one-time +// fan-out of legacy task-level automation switches onto per-PR rows (see +// migrateTaskCIOptionsToPRScope). Column-precheck idiom, mirroring +// addTaskCIRoundColumns. +func (s *Store) addPRScopeMigrationColumn() error { + cols, err := s.tableColumns("github_task_ci_options") + if err != nil { + return fmt.Errorf("read github_task_ci_options columns: %w", err) + } + if _, ok := cols["pr_scope_migrated_at"]; ok { + return nil + } + if _, err := s.db.Exec(`ALTER TABLE github_task_ci_options ADD COLUMN pr_scope_migrated_at DATETIME`); err != nil { + return fmt.Errorf("add github_task_ci_options.pr_scope_migrated_at: %w", err) + } + return nil +} + // tableColumns returns the set of column names declared on `table`. Cheap // SQLite PRAGMA lookup; used by addWatchSelfHealColumns to skip ALTERs on a // fresh install whose createTablesSQL already includes the columns. Mirrors @@ -1586,6 +1706,23 @@ func (s *Store) ListTaskPRsByTask(ctx context.Context, taskID string) ([]*TaskPR return out, nil } +// ListTaskPRsByTaskIncludingDetached returns every PR association for a task, +// including detached tombstones. Callers that expose active task surfaces +// should use ListTaskPRsByTask; this variant is for cleanup of state keyed by +// an association that may have been detached. +func (s *Store) ListTaskPRsByTaskIncludingDetached(ctx context.Context, taskID string) ([]*TaskPR, error) { + var prs []TaskPR + if err := s.ro.SelectContext(ctx, &prs, + `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? ORDER BY created_at ASC`, taskID); err != nil { + return nil, err + } + out := make([]*TaskPR, 0, len(prs)) + for i := range prs { + out = append(out, &prs[i]) + } + return out, nil +} + // DetachTaskPR marks an association as removed from active task surfaces while // retaining the row so automatic PR discovery cannot silently resurrect it. // The bool return reports whether this call performed the transition. @@ -1871,20 +2008,69 @@ func (s *Store) mutateTaskCIPRState( // UpdateTaskCIOptions applies a partial update to task CI automation options. func (s *Store) UpdateTaskCIOptions(ctx context.Context, taskID string, patch TaskCIOptionsPatch) (*TaskCIOptions, error) { writeCtx := context.WithoutCancel(ctx) - now := time.Now().UTC() tx, err := s.db.BeginTxx(writeCtx, nil) if err != nil { return nil, err } defer func() { _ = tx.Rollback() }() - version, err := s.advanceTaskCIOptionsVersion(writeCtx, tx, taskID, now) + if err := s.updateTaskCIOptionsTx(writeCtx, tx, taskID, patch); err != nil { + return nil, err + } + if err := tx.Commit(); err != nil { + return nil, err + } + return s.GetTaskCIOptions(writeCtx, taskID) +} + +// UpdateTaskCIOptionsWithPRAutomation applies a task update and all selected +// per-PR switch updates in one transaction. The service resolves and +// validates targets before calling this method, so a rejected identity cannot +// leave task-level fields or a subset of fan-out rows persisted. +func (s *Store) UpdateTaskCIOptionsWithPRAutomation( + ctx context.Context, + taskID string, + patch TaskCIOptionsPatch, + targets []*TaskPR, + prPatch TaskPRAutomationOptionsPatch, + reviewerChanged bool, +) (*TaskCIOptions, error) { + writeCtx := context.WithoutCancel(ctx) + tx, err := s.db.BeginTxx(writeCtx, nil) if err != nil { return nil, err } - var previous TaskCIOptions - if err := tx.GetContext(writeCtx, &previous, `SELECT * FROM github_task_ci_options WHERE task_id = ?`, taskID); err != nil { + defer func() { _ = tx.Rollback() }() + if err := s.updateTaskCIOptionsTx(writeCtx, tx, taskID, patch); err != nil { return nil, err } + for _, target := range targets { + if target == nil { + continue + } + if err := s.updateTaskPRAutomationOptionsTx( + writeCtx, tx, taskID, target.RepositoryID, target.PRNumber, prPatch, reviewerChanged, + ); err != nil { + return nil, err + } + } + if err := tx.Commit(); err != nil { + return nil, err + } + return s.GetTaskCIOptions(writeCtx, taskID) +} + +func (s *Store) updateTaskCIOptionsTx( + ctx context.Context, tx *sqlx.Tx, taskID string, patch TaskCIOptionsPatch, +) error { + now := time.Now().UTC() + version, err := s.advanceTaskCIOptionsVersion(ctx, tx, taskID, now) + if err != nil { + return err + } + var previous TaskCIOptions + if err := tx.GetContext(ctx, &previous, `SELECT * FROM github_task_ci_options WHERE task_id = ?`, taskID); err != nil { + return err + } autoFixSet, autoFixValue := boolPatchValue(patch.AutoFixEnabled) autoMergeSet, autoMergeValue := boolPatchValue(patch.AutoMergeEnabled) reviewSet, reviewValue := boolPatchValue(patch.PromptOnReviewRequested) @@ -1896,7 +2082,7 @@ func (s *Store) UpdateTaskCIOptions(ctx context.Context, taskID string, patch Ta reviewerChanged := reviewerLoginSet && !strings.EqualFold( previous.ReviewReviewerLogin, normalizedString(patch.ReviewReviewerLogin), ) - if _, err := tx.ExecContext(writeCtx, ` + if _, err := tx.ExecContext(ctx, ` UPDATE github_task_ci_options SET auto_fix_enabled = CASE WHEN ? THEN ? ELSE auto_fix_enabled END, auto_merge_enabled = CASE WHEN ? THEN ? ELSE auto_merge_enabled END, @@ -1911,17 +2097,9 @@ func (s *Store) UpdateTaskCIOptions(ctx context.Context, taskID string, patch Ta reviewSet, reviewValue, mergedSet, mergedValue, closedSet, closedValue, reviewerLoginSet, normalizedString(patch.ReviewReviewerLogin), version, taskID); err != nil { - return nil, err + return err } - if err := applyTaskCIOptionResets( - writeCtx, tx, taskID, version, previous, patch, reviewerChanged, - ); err != nil { - return nil, err - } - if err := tx.Commit(); err != nil { - return nil, err - } - return s.GetTaskCIOptions(writeCtx, taskID) + return applyTaskCIOptionResets(ctx, tx, taskID, version, previous, patch, reviewerChanged) } func applyTaskCIOptionResets( @@ -1934,7 +2112,7 @@ func applyTaskCIOptionResets( reviewerChanged bool, ) error { if shouldResetAutoFix(patch.AutoFixEnabled, previous.AutoFixEnabled) { - if err := resetTaskCIAutoFixState(ctx, tx, taskID, now); err != nil { + if err := resetTaskCIAutoFixStateForTask(ctx, tx, taskID, now); err != nil { return err } } @@ -1949,29 +2127,17 @@ func applyTaskCIOptionResets( } } if shouldResetTerminalPrompt(patch.PromptOnMerged, previous.PromptOnMerged) { - if err := resetTaskCITerminalCheckpoint(ctx, tx, taskID, "merged", now); err != nil { + if err := resetTaskCITerminalCheckpointForTask(ctx, tx, taskID, "merged", now); err != nil { return err } } if shouldResetTerminalPrompt(patch.PromptOnClosed, previous.PromptOnClosed) { - return resetTaskCITerminalCheckpoint(ctx, tx, taskID, "closed", now) + return resetTaskCITerminalCheckpointForTask(ctx, tx, taskID, "closed", now) } return nil } -func shouldResetAutoFix(patchValue *bool, wasEnabled bool) bool { - return patchValue != nil && *patchValue && !wasEnabled -} - -func shouldResetReviewRequests(patchValue *bool, wasEnabled, reviewerChanged bool) bool { - return patchValue != nil && *patchValue && (!wasEnabled || reviewerChanged) -} - -func shouldResetTerminalPrompt(patchValue *bool, wasEnabled bool) bool { - return patchValue != nil && *patchValue && !wasEnabled -} - -func resetTaskCIAutoFixState( +func resetTaskCIAutoFixStateForTask( ctx context.Context, tx *sqlx.Tx, taskID string, @@ -1991,7 +2157,7 @@ func resetTaskCIAutoFixState( return err } -func resetTaskCITerminalCheckpoint( +func resetTaskCITerminalCheckpointForTask( ctx context.Context, tx *sqlx.Tx, taskID, state string, @@ -2009,6 +2175,209 @@ func resetTaskCITerminalCheckpoint( return err } +// GetTaskPRAutomationOptions returns one PR's automation switches, or disabled defaults. +func (s *Store) GetTaskPRAutomationOptions( + ctx context.Context, taskID, repositoryID string, prNumber int, +) (*TaskPRAutomationOptions, error) { + var opts TaskPRAutomationOptions + err := s.ro.GetContext(ctx, &opts, + `SELECT * FROM github_task_pr_automation_options WHERE task_id = ? AND repository_id = ? AND pr_number = ?`, + taskID, repositoryID, prNumber) + if errors.Is(err, sql.ErrNoRows) { + now := time.Now().UTC() + return &TaskPRAutomationOptions{ + TaskID: taskID, RepositoryID: repositoryID, PRNumber: prNumber, + CreatedAt: now, UpdatedAt: now, + }, nil + } + return &opts, err +} + +// ListTaskPRAutomationOptions returns every stored per-PR automation row for a task. +func (s *Store) ListTaskPRAutomationOptions(ctx context.Context, taskID string) ([]*TaskPRAutomationOptions, error) { + var rows []TaskPRAutomationOptions + if err := s.ro.SelectContext(ctx, &rows, + `SELECT * FROM github_task_pr_automation_options WHERE task_id = ? ORDER BY repository_id ASC, pr_number ASC`, + taskID); err != nil { + return nil, err + } + out := make([]*TaskPRAutomationOptions, 0, len(rows)) + for i := range rows { + out = append(out, &rows[i]) + } + return out, 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 +// patch's task-level ReviewReviewerLogin against the previously stored +// value) and passes it into every PR targeted by the same update, so a +// changed connected account re-baselines the review-request checkpoint even +// when the switch itself did not change value. +func (s *Store) UpdateTaskPRAutomationOptions( + ctx context.Context, taskID, repositoryID string, prNumber int, + patch TaskPRAutomationOptionsPatch, reviewerChanged bool, +) (*TaskPRAutomationOptions, error) { + writeCtx := context.WithoutCancel(ctx) + tx, err := s.db.BeginTxx(writeCtx, nil) + if err != nil { + return nil, err + } + defer func() { _ = tx.Rollback() }() + if err := s.updateTaskPRAutomationOptionsTx( + writeCtx, tx, taskID, repositoryID, prNumber, patch, reviewerChanged, + ); err != nil { + return nil, err + } + if err := tx.Commit(); err != nil { + return nil, err + } + return s.GetTaskPRAutomationOptions(writeCtx, taskID, repositoryID, prNumber) +} + +func (s *Store) updateTaskPRAutomationOptionsTx( + ctx context.Context, + tx *sqlx.Tx, + taskID, repositoryID string, + prNumber int, + patch TaskPRAutomationOptionsPatch, + reviewerChanged bool, +) error { + now := time.Now().UTC() + if _, err := tx.ExecContext(ctx, ` + INSERT INTO github_task_pr_automation_options ( + task_id, repository_id, pr_number, auto_fix_enabled, auto_merge_enabled, + prompt_on_review_requested, prompt_on_merged, prompt_on_closed, created_at, updated_at + ) VALUES (?, ?, ?, 0, 0, 0, 0, 0, ?, ?) + ON CONFLICT(task_id, repository_id, pr_number) DO NOTHING`, + taskID, repositoryID, prNumber, now, now); err != nil { + return err + } + var previous TaskPRAutomationOptions + if err := tx.GetContext(ctx, &previous, + `SELECT * FROM github_task_pr_automation_options WHERE task_id = ? AND repository_id = ? AND pr_number = ?`, + taskID, repositoryID, prNumber); err != nil { + return err + } + autoFixSet, autoFixValue := boolPatchValue(patch.AutoFixEnabled) + autoMergeSet, autoMergeValue := boolPatchValue(patch.AutoMergeEnabled) + reviewSet, reviewValue := boolPatchValue(patch.PromptOnReviewRequested) + mergedSet, mergedValue := boolPatchValue(patch.PromptOnMerged) + closedSet, closedValue := boolPatchValue(patch.PromptOnClosed) + if _, err := tx.ExecContext(ctx, ` + UPDATE github_task_pr_automation_options SET + auto_fix_enabled = CASE WHEN ? THEN ? ELSE auto_fix_enabled END, + auto_merge_enabled = CASE WHEN ? THEN ? ELSE auto_merge_enabled END, + prompt_on_review_requested = CASE WHEN ? THEN ? ELSE prompt_on_review_requested END, + prompt_on_merged = CASE WHEN ? THEN ? ELSE prompt_on_merged END, + prompt_on_closed = CASE WHEN ? THEN ? ELSE prompt_on_closed END, + updated_at = ? + WHERE task_id = ? AND repository_id = ? AND pr_number = ?`, + autoFixSet, autoFixValue, autoMergeSet, autoMergeValue, + reviewSet, reviewValue, mergedSet, mergedValue, closedSet, closedValue, + now, taskID, repositoryID, prNumber); err != nil { + return err + } + if err := applyTaskPRAutomationOptionResets( + ctx, tx, taskID, repositoryID, prNumber, now, previous, patch, reviewerChanged, + ); err != nil { + return err + } + return nil +} + +func applyTaskPRAutomationOptionResets( + ctx context.Context, + tx *sqlx.Tx, + taskID, repositoryID string, + prNumber int, + now time.Time, + previous TaskPRAutomationOptions, + patch TaskPRAutomationOptionsPatch, + reviewerChanged bool, +) error { + if shouldResetAutoFix(patch.AutoFixEnabled, previous.AutoFixEnabled) { + if err := resetTaskCIAutoFixState(ctx, tx, taskID, repositoryID, prNumber, now); err != nil { + return err + } + } + if shouldResetReviewRequests( + patch.PromptOnReviewRequested, previous.PromptOnReviewRequested, reviewerChanged, + ) { + if _, err := tx.ExecContext(ctx, ` + UPDATE github_task_ci_pr_state + SET review_request_initialized = 0, last_review_requested = 0, updated_at = ? + WHERE task_id = ? AND repository_id = ? AND pr_number = ?`, + now, taskID, repositoryID, prNumber); err != nil { + return err + } + } + if shouldResetTerminalPrompt(patch.PromptOnMerged, previous.PromptOnMerged) { + if err := resetTaskCITerminalCheckpoint(ctx, tx, taskID, repositoryID, prNumber, "merged", now); err != nil { + return err + } + } + if shouldResetTerminalPrompt(patch.PromptOnClosed, previous.PromptOnClosed) { + return resetTaskCITerminalCheckpoint(ctx, tx, taskID, repositoryID, prNumber, "closed", now) + } + return nil +} + +func shouldResetAutoFix(patchValue *bool, wasEnabled bool) bool { + return patchValue != nil && *patchValue && !wasEnabled +} + +func shouldResetReviewRequests(patchValue *bool, wasEnabled, reviewerChanged bool) bool { + return patchValue != nil && *patchValue && (!wasEnabled || reviewerChanged) +} + +func shouldResetTerminalPrompt(patchValue *bool, wasEnabled bool) bool { + return patchValue != nil && *patchValue && !wasEnabled +} + +func resetTaskCIAutoFixState( + ctx context.Context, + tx *sqlx.Tx, + taskID, repositoryID string, + prNumber int, + now time.Time, +) error { + _, err := tx.ExecContext(ctx, ` + UPDATE github_task_ci_pr_state + SET auto_fix_round_count = 0, + last_fix_signature = '', + last_fix_checkpoint_json = '', + 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, + auto_fix_exhausted_at = NULL, + updated_at = ? + WHERE task_id = ? AND repository_id = ? AND pr_number = ?`, now, taskID, repositoryID, prNumber) + return err +} + +func resetTaskCITerminalCheckpoint( + ctx context.Context, + tx *sqlx.Tx, + taskID, repositoryID string, + prNumber int, + state string, + now time.Time, +) error { + _, err := tx.ExecContext(ctx, ` + UPDATE github_task_ci_pr_state + SET last_observed_pr_state = '', + last_lifecycle_event = '', + last_lifecycle_prompt_at = NULL, + last_lifecycle_session_id = NULL, + updated_at = ? + WHERE task_id = ? AND repository_id = ? AND pr_number = ? + AND (last_observed_pr_state = ? OR last_lifecycle_event = ?)`, + now, taskID, repositoryID, prNumber, state, state) + return err +} + // RebindTaskPRReviewer atomically updates the task's authenticated reviewer // login and quietly resets only its review-request baselines when it changes. func (s *Store) RebindTaskPRReviewer(ctx context.Context, taskID, login string) (bool, error) { diff --git a/apps/backend/internal/github/store_ci_automation_test.go b/apps/backend/internal/github/store_ci_automation_test.go index 835e5a2c0b..68e3f1923e 100644 --- a/apps/backend/internal/github/store_ci_automation_test.go +++ b/apps/backend/internal/github/store_ci_automation_test.go @@ -18,6 +18,7 @@ func TestStoreTaskPRAgentAutomationSchema(t *testing.T) { "review_prompt_override", "merged_prompt_override", "closed_prompt_override", + "pr_scope_migrated_at", }, "github_task_ci_pr_state": { "review_request_initialized", @@ -27,6 +28,18 @@ func TestStoreTaskPRAgentAutomationSchema(t *testing.T) { "last_lifecycle_prompt_at", "last_lifecycle_session_id", }, + "github_task_pr_automation_options": { + "task_id", + "repository_id", + "pr_number", + "auto_fix_enabled", + "auto_merge_enabled", + "prompt_on_review_requested", + "prompt_on_merged", + "prompt_on_closed", + "created_at", + "updated_at", + }, } { got, err := store.tableColumns(table) if err != nil { @@ -40,6 +53,23 @@ func TestStoreTaskPRAgentAutomationSchema(t *testing.T) { } } +// 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. +func TestStoreTaskPRAutomationOptionsSchemaReplay(t *testing.T) { + store := newTestStore(t) + if err := store.initSchema(false); err != nil { + t.Fatalf("replay schema migration: %v", err) + } + got, err := store.tableColumns("github_task_pr_automation_options") + if err != nil { + t.Fatalf("tableColumns: %v", err) + } + if _, ok := got["auto_fix_enabled"]; !ok { + t.Fatal("github_task_pr_automation_options.auto_fix_enabled is missing after replay") + } +} + func TestStoreTaskPRAgentAutomationMigrationClearsLifecyclePromptOverrides(t *testing.T) { store := newTestStore(t) ctx := context.Background() @@ -197,15 +227,23 @@ func TestStoreRebindTaskPRReviewerQuietlyResetsReviewBaselines(t *testing.T) { } } -func TestStoreTaskCIOptionsReenableTerminalPromptResetsOnlyMatchingCheckpoint(t *testing.T) { +// TestStoreTaskPRAutomationOptionsReenableTerminalPromptResetsOnlyMatchingCheckpoint +// covers AC11/AC12-adjacent PR-scoping: re-enabling the merged-prompt switch +// on repo-merged/1 must reset only that PR's checkpoint, not repo-closed/2's +// — both because the reset predicate is keyed by observed state ("merged" +// vs "closed") and because it is now scoped to the specific PR. +func TestStoreTaskPRAutomationOptionsReenableTerminalPromptResetsOnlyMatchingCheckpoint(t *testing.T) { store := newTestStore(t) ctx := context.Background() enabled := true disabled := false - if _, err := store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{ - PromptOnMerged: &enabled, PromptOnClosed: &enabled, - }); err != nil { - t.Fatalf("enable terminal prompts: %v", err) + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-merged", 1, + TaskPRAutomationOptionsPatch{PromptOnMerged: &enabled}, false); err != nil { + t.Fatalf("enable merged prompt for repo-merged/1: %v", err) + } + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-closed", 2, + TaskPRAutomationOptionsPatch{PromptOnClosed: &enabled}, false); err != nil { + t.Fatalf("enable closed prompt for repo-closed/2: %v", err) } for _, prompt := range []TaskPRLifecyclePrompt{ {TaskID: "task-1", RepositoryID: "repo-merged", PRNumber: 1, Event: "merged", ObservedState: "merged"}, @@ -215,10 +253,12 @@ func TestStoreTaskCIOptionsReenableTerminalPromptResetsOnlyMatchingCheckpoint(t t.Fatalf("seed terminal checkpoint: %v", err) } } - if _, err := store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{PromptOnMerged: &disabled}); err != nil { + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-merged", 1, + TaskPRAutomationOptionsPatch{PromptOnMerged: &disabled}, false); err != nil { t.Fatalf("disable merged prompt: %v", err) } - if _, err := store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{PromptOnMerged: &enabled}); err != nil { + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-merged", 1, + TaskPRAutomationOptionsPatch{PromptOnMerged: &enabled}, false); err != nil { t.Fatalf("re-enable merged prompt: %v", err) } @@ -234,10 +274,14 @@ func TestStoreTaskCIOptionsReenableTerminalPromptResetsOnlyMatchingCheckpoint(t t.Fatalf("get closed state: %v", err) } if closed.LastObservedPRState != "closed" || closed.LastLifecycleEvent != "closed" { - t.Fatalf("closed checkpoint changed while re-enabling merged: %+v", closed) + t.Fatalf("closed checkpoint changed while re-enabling merged on a different PR: %+v", closed) } } +// TestStoreTaskCIOptions_DefaultAndUpdate covers the genuinely task-level +// fields still owned by github_task_ci_options: AutoFixPromptOverride. The +// five automation switches moved to per-PR scope — see +// TestStoreTaskPRAutomationOptions_DefaultAndUpdate. func TestStoreTaskCIOptions_DefaultAndUpdate(t *testing.T) { store := newTestStore(t) ctx := context.Background() @@ -249,37 +293,65 @@ func TestStoreTaskCIOptions_DefaultAndUpdate(t *testing.T) { if got.TaskID != "task-1" { t.Fatalf("TaskID=%q, want task-1", got.TaskID) } - if got.AutoFixEnabled || got.AutoMergeEnabled { - t.Fatalf("default options should be disabled, got %+v", got) - } if got.AutoFixPromptOverride != nil { t.Fatalf("default prompt override should be nil, got %q", *got.AutoFixPromptOverride) } override := "Fix only the new CI feedback." updated, err := store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{ - AutoFixEnabled: boolPtr(true), AutoFixPromptOverride: &override, }) if err != nil { t.Fatalf("update options: %v", err) } + if updated.AutoFixPromptOverride == nil || *updated.AutoFixPromptOverride != override { + t.Fatalf("override=%v, want %q", updated.AutoFixPromptOverride, override) + } + + clearOverride := "" + updated, err = store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{ + AutoFixPromptOverride: &clearOverride, + }) + if err != nil { + t.Fatalf("second update options: %v", err) + } + if updated.AutoFixPromptOverride != nil { + t.Fatalf("override should be cleared, got %q", *updated.AutoFixPromptOverride) + } +} + +// TestStoreTaskPRAutomationOptions_DefaultAndUpdate covers the per-PR +// automation switches, keyed by (task_id, repository_id, pr_number). +func TestStoreTaskPRAutomationOptions_DefaultAndUpdate(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + + got, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 1) + if err != nil { + t.Fatalf("get default options: %v", err) + } + if got.TaskID != "task-1" || got.RepositoryID != "repo-1" || got.PRNumber != 1 { + t.Fatalf("identity = %+v, want task-1/repo-1/1", got) + } + if got.AutoFixEnabled || got.AutoMergeEnabled { + t.Fatalf("default options should be disabled, got %+v", got) + } + + updated, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-1", 1, + TaskPRAutomationOptionsPatch{AutoFixEnabled: boolPtr(true)}, false) + if err != nil { + t.Fatalf("update options: %v", err) + } if !updated.AutoFixEnabled { t.Fatalf("AutoFixEnabled=false, want true") } if updated.AutoMergeEnabled { t.Fatalf("AutoMergeEnabled=true, want unchanged default false") } - if updated.AutoFixPromptOverride == nil || *updated.AutoFixPromptOverride != override { - t.Fatalf("override=%v, want %q", updated.AutoFixPromptOverride, override) - } enableMerge := true - clearOverride := "" - updated, err = store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{ - AutoMergeEnabled: &enableMerge, - AutoFixPromptOverride: &clearOverride, - }) + updated, err = store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-1", 1, + TaskPRAutomationOptionsPatch{AutoMergeEnabled: &enableMerge}, false) if err != nil { t.Fatalf("second update options: %v", err) } @@ -289,8 +361,23 @@ func TestStoreTaskCIOptions_DefaultAndUpdate(t *testing.T) { if !updated.AutoMergeEnabled { t.Fatalf("AutoMergeEnabled=false, want true") } - if updated.AutoFixPromptOverride != nil { - t.Fatalf("override should be cleared, got %q", *updated.AutoFixPromptOverride) + + // A different PR on the same task starts independently disabled (AC1/AC2). + other, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 2) + if err != nil { + t.Fatalf("get other PR options: %v", err) + } + if other.AutoFixEnabled || other.AutoMergeEnabled { + t.Fatalf("other PR should be independently disabled, got %+v", other) + } + + // Same PR number in a different repository is independent (AC4). + otherRepo, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-2", 1) + if err != nil { + t.Fatalf("get other repo options: %v", err) + } + if otherRepo.AutoFixEnabled || otherRepo.AutoMergeEnabled { + t.Fatalf("same PR number in a different repo should be independent, got %+v", otherRepo) } } @@ -408,11 +495,13 @@ func TestStoreTaskCIPRState_MarkExhaustedAndResetOnReenable(t *testing.T) { } disabled := false - if _, err := store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{AutoFixEnabled: &disabled}); err != nil { + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-1", 42, + TaskPRAutomationOptionsPatch{AutoFixEnabled: &disabled}, false); err != nil { t.Fatalf("disable auto-fix: %v", err) } enabled := true - if _, err := store.UpdateTaskCIOptions(ctx, "task-1", TaskCIOptionsPatch{AutoFixEnabled: &enabled}); err != nil { + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-1", 42, + TaskPRAutomationOptionsPatch{AutoFixEnabled: &enabled}, false); err != nil { t.Fatalf("re-enable auto-fix: %v", err) } state, err = store.GetTaskCIPRState(ctx, "task-1", "repo-1", 42) @@ -462,4 +551,204 @@ func TestStoreTaskCIPRState_RefreshCheckpointClearsPromptDispatchMetadata(t *tes } } +// seedLegacyTaskCIOptions inserts a pre-migration github_task_ci_options row +// directly, bypassing UpdateTaskCIOptions (which no longer writes the five +// legacy boolean columns), to simulate a database from before per-PR scoping. +func seedLegacyTaskCIOptions(t *testing.T, store *Store, taskID string, autoFix, autoMerge bool) { + t.Helper() + now := time.Now().UTC() + if _, err := store.db.Exec(` + INSERT INTO github_task_ci_options (task_id, auto_fix_enabled, auto_merge_enabled, created_at, updated_at) + VALUES (?, ?, ?, ?, ?)`, + taskID, autoFix, autoMerge, now, now); err != nil { + t.Fatalf("seed legacy task CI options: %v", err) + } +} + +// TestStoreMigrateTaskCIOptionsToPRScope_FansOutToLinkedPRs covers AC14: a +// pre-upgrade task row with two linked PRs yields, after one boot, two +// per-PR rows each matching the legacy booleans. +func TestStoreMigrateTaskCIOptionsToPRScope_FansOutToLinkedPRs(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + now := time.Now().UTC() + + seedLegacyTaskCIOptions(t, store, "task-1", true, false) + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 1, CreatedAt: now, + }); err != nil { + t.Fatalf("seed PR 1: %v", err) + } + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 2, CreatedAt: now, + }); err != nil { + t.Fatalf("seed PR 2: %v", err) + } + + if err := store.initSchema(false); err != nil { + t.Fatalf("replay schema migration: %v", err) + } + + for _, prNumber := range []int{1, 2} { + opts, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", prNumber) + if err != nil { + t.Fatalf("get PR %d options: %v", prNumber, err) + } + if !opts.AutoFixEnabled || opts.AutoMergeEnabled { + t.Fatalf("PR %d options = %+v, want auto_fix_enabled=true auto_merge_enabled=false", prNumber, opts) + } + } + migrated, err := store.GetTaskCIOptions(ctx, "task-1") + if err != nil { + t.Fatalf("get task options: %v", err) + } + if migrated.PRScopeMigratedAt == nil { + t.Fatal("pr_scope_migrated_at was not stamped") + } +} + +func TestStoreMigrateTaskCIOptionsToPRScope_SkipsDetachedPRs(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + now := time.Now().UTC() + + seedLegacyTaskCIOptions(t, store, "task-1", true, true) + active := &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 1, CreatedAt: now, + } + detached := &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 2, CreatedAt: now.Add(time.Second), + } + for _, pr := range []*TaskPR{active, detached} { + if err := store.CreateTaskPR(ctx, pr); err != nil { + t.Fatalf("seed PR #%d: %v", pr.PRNumber, err) + } + } + if _, transitioned, err := store.DetachTaskPR(ctx, detached.ID); err != nil || !transitioned { + t.Fatalf("detach PR #%d: transitioned=%v err=%v", detached.PRNumber, transitioned, err) + } + + if err := store.initSchema(false); err != nil { + t.Fatalf("replay schema migration: %v", err) + } + activeOptions, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", active.PRNumber) + if err != nil { + t.Fatalf("get active PR options: %v", err) + } + if !activeOptions.AutoFixEnabled || !activeOptions.AutoMergeEnabled { + t.Fatalf("active PR options = %+v, want legacy switches enabled", activeOptions) + } + detachedOptions, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", detached.PRNumber) + if err != nil { + t.Fatalf("get detached PR options: %v", err) + } + if detachedOptions.AutoFixEnabled || detachedOptions.AutoMergeEnabled { + t.Fatalf("detached PR inherited legacy switches: %+v", detachedOptions) + } +} + +// TestStoreMigrateTaskCIOptionsToPRScope_Idempotent covers AC15: replaying +// the migration on an already-migrated database changes no per-PR row. +func TestStoreMigrateTaskCIOptionsToPRScope_Idempotent(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + now := time.Now().UTC() + + seedLegacyTaskCIOptions(t, store, "task-1", true, false) + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 1, CreatedAt: now, + }); err != nil { + t.Fatalf("seed PR: %v", err) + } + if err := store.initSchema(false); err != nil { + t.Fatalf("first replay: %v", err) + } + before, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 1) + if err != nil { + t.Fatalf("get options after first replay: %v", err) + } + + if err := store.initSchema(false); err != nil { + t.Fatalf("second replay: %v", err) + } + after, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 1) + if err != nil { + t.Fatalf("get options after second replay: %v", err) + } + if !after.UpdatedAt.Equal(before.UpdatedAt) { + t.Fatalf("updated_at changed on replay: before=%v after=%v", before.UpdatedAt, after.UpdatedAt) + } +} + +// TestStoreMigrateTaskCIOptionsToPRScope_DoesNotReenableUserDisabled covers +// AC16: once migrated, a user's deliberate per-PR disable must survive a +// later boot even though the stale legacy row still says "enabled". +func TestStoreMigrateTaskCIOptionsToPRScope_DoesNotReenableUserDisabled(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + now := time.Now().UTC() + + seedLegacyTaskCIOptions(t, store, "task-1", true, false) + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 1, CreatedAt: now, + }); err != nil { + t.Fatalf("seed PR: %v", err) + } + if err := store.initSchema(false); err != nil { + t.Fatalf("first boot migration: %v", err) + } + + disabled := false + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-1", "repo-1", 1, + TaskPRAutomationOptionsPatch{AutoFixEnabled: &disabled}, false); err != nil { + t.Fatalf("user disables auto-fix: %v", err) + } + + if err := store.initSchema(false); err != nil { + t.Fatalf("second boot: %v", err) + } + + opts, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 1) + if err != nil { + t.Fatalf("get options: %v", err) + } + if opts.AutoFixEnabled { + t.Fatal("auto-fix was re-enabled by a replayed migration after the user turned it off") + } +} + +// TestStoreMigrateTaskCIOptionsToPRScope_NewlyLinkedPRStartsAllOff covers +// AC17: a PR linked to the task after migration has already run starts with +// all five switches off rather than inheriting the stale legacy value. +func TestStoreMigrateTaskCIOptionsToPRScope_NewlyLinkedPRStartsAllOff(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + now := time.Now().UTC() + + seedLegacyTaskCIOptions(t, store, "task-1", true, true) + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 1, CreatedAt: now, + }); err != nil { + t.Fatalf("seed PR 1: %v", err) + } + if err := store.initSchema(false); err != nil { + t.Fatalf("boot migration: %v", err) + } + + // PR 2 links to the task only after the fan-out migration already ran. + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 2, CreatedAt: now, + }); err != nil { + t.Fatalf("seed PR 2: %v", err) + } + + opts, err := store.GetTaskPRAutomationOptions(ctx, "task-1", "repo-1", 2) + if err != nil { + t.Fatalf("get options: %v", err) + } + if opts.AutoFixEnabled || opts.AutoMergeEnabled { + t.Fatalf("newly linked PR should start all-off, got %+v", opts) + } +} + func boolPtr(v bool) *bool { return &v } diff --git a/apps/backend/internal/mcp/handlers/task_pr_automation.go b/apps/backend/internal/mcp/handlers/task_pr_automation.go index 25f82462b7..a9ae1048e2 100644 --- a/apps/backend/internal/mcp/handlers/task_pr_automation.go +++ b/apps/backend/internal/mcp/handlers/task_pr_automation.go @@ -3,6 +3,7 @@ package handlers import ( "context" "encoding/json" + "errors" "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" @@ -53,6 +54,8 @@ func (h *Handlers) handleUpdateTaskPRAutomation(ctx context.Context, msg *ws.Mes } var req struct { TaskID string `json:"task_id"` + RepositoryID *string `json:"repository_id"` + PRNumber *int `json:"pr_number"` AutoFixEnabled *bool `json:"auto_fix_enabled"` AutoMergeEnabled *bool `json:"auto_merge_enabled"` AutoFixPromptOverride *string `json:"auto_fix_prompt_override"` @@ -67,6 +70,8 @@ func (h *Handlers) handleUpdateTaskPRAutomation(ctx context.Context, msg *ws.Mes return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "task_id is required", nil) } patch := github.TaskCIOptionsPatch{ + RepositoryID: req.RepositoryID, + PRNumber: req.PRNumber, AutoFixEnabled: req.AutoFixEnabled, AutoMergeEnabled: req.AutoMergeEnabled, AutoFixPromptOverride: req.AutoFixPromptOverride, @@ -82,6 +87,9 @@ func (h *Handlers) handleUpdateTaskPRAutomation(ctx context.Context, msg *ws.Mes } options, err := h.taskPRAutomation.UpdateTaskCIOptions(ctx, req.TaskID, patch) if err != nil { + if errors.Is(err, github.ErrTaskPRNotLinked) { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, err.Error(), nil) + } return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to update PR automation options: "+err.Error(), nil) } if h.eventBus != nil { diff --git a/apps/backend/internal/mcp/handlers/task_pr_automation_test.go b/apps/backend/internal/mcp/handlers/task_pr_automation_test.go index ce733a9d31..d0575a8270 100644 --- a/apps/backend/internal/mcp/handlers/task_pr_automation_test.go +++ b/apps/backend/internal/mcp/handlers/task_pr_automation_test.go @@ -27,6 +27,96 @@ func (s *recordingTaskPRAutomationService) UpdateTaskCIOptions( return &github.TaskCIOptionsResponse{}, nil } +// recordingTaskPRAutomationServiceWithErr optionally returns a fixed error +// from UpdateTaskCIOptions, for exercising error-code mapping. +type recordingTaskPRAutomationServiceWithErr struct { + recordingTaskPRAutomationService + err error +} + +func (s *recordingTaskPRAutomationServiceWithErr) UpdateTaskCIOptions( + ctx context.Context, taskID string, patch github.TaskCIOptionsPatch, +) (*github.TaskCIOptionsResponse, error) { + if s.err != nil { + s.calls++ + s.patch = patch + return nil, s.err + } + return s.recordingTaskPRAutomationService.UpdateTaskCIOptions(ctx, taskID, patch) +} + +// TestHandleUpdateTaskPRAutomationOmittedIdentityFansOut covers AC23: no +// repository_id/pr_number in the payload leaves both nil on the patch, which +// the service treats as a fan-out to every linked PR (unchanged behavior for +// agents written before per-PR scoping). +func TestHandleUpdateTaskPRAutomationOmittedIdentityFansOut(t *testing.T) { + automation := &recordingTaskPRAutomationService{} + h := &Handlers{taskPRAutomation: automation, logger: testLogger(t).WithFields()} + + msg := makeWSMessage(t, ws.ActionMCPUpdateTaskPRAutomation, map[string]any{ + "task_id": "task-current", + "auto_fix_enabled": true, + }) + response, err := h.handleUpdateTaskPRAutomation(context.Background(), msg) + + require.NoError(t, err) + assert.Equal(t, ws.MessageTypeResponse, response.Type) + assert.Equal(t, 1, automation.calls) + assert.Nil(t, automation.patch.RepositoryID) + assert.Nil(t, automation.patch.PRNumber) +} + +// TestHandleUpdateTaskPRAutomationWithIdentityTargetsOnePR covers AC24: a +// payload naming repository_id/pr_number reaches the service as PR identity +// on the patch, targeting one linked PR only. +func TestHandleUpdateTaskPRAutomationWithIdentityTargetsOnePR(t *testing.T) { + automation := &recordingTaskPRAutomationService{} + h := &Handlers{taskPRAutomation: automation, logger: testLogger(t).WithFields()} + + msg := makeWSMessage(t, ws.ActionMCPUpdateTaskPRAutomation, map[string]any{ + "task_id": "task-current", + "repository_id": "repo-1", + "pr_number": 42, + "auto_merge_enabled": true, + }) + response, err := h.handleUpdateTaskPRAutomation(context.Background(), msg) + + require.NoError(t, err) + assert.Equal(t, ws.MessageTypeResponse, response.Type) + require.NotNil(t, automation.patch.RepositoryID) + assert.Equal(t, "repo-1", *automation.patch.RepositoryID) + require.NotNil(t, automation.patch.PRNumber) + assert.Equal(t, 42, *automation.patch.PRNumber) +} + +// TestHandleUpdateTaskPRAutomationMapsUnlinkedPRToValidationError covers the +// MCP-side error mapping for ErrTaskPRNotLinked (HTTP's AC8 equivalent). +func TestHandleUpdateTaskPRAutomationMapsUnlinkedPRToValidationError(t *testing.T) { + for name, identity := range map[string]map[string]any{ + "repository only": {"repository_id": "repo-1"}, + "PR number only": {"pr_number": 999}, + } { + t.Run(name, func(t *testing.T) { + automation := &recordingTaskPRAutomationServiceWithErr{err: github.ErrTaskPRNotLinked} + h := &Handlers{taskPRAutomation: automation, logger: testLogger(t).WithFields()} + + payload := map[string]any{ + "task_id": "task-current", + "auto_fix_enabled": true, + } + for key, value := range identity { + payload[key] = value + } + msg := makeWSMessage(t, ws.ActionMCPUpdateTaskPRAutomation, payload) + response, err := h.handleUpdateTaskPRAutomation(context.Background(), msg) + + require.NoError(t, err) + assert.Equal(t, ws.MessageTypeError, response.Type) + assert.Contains(t, string(response.Payload), "PR is not linked to this task") + }) + } +} + func TestHandleUpdateTaskPRAutomationRejectsLifecyclePromptOverrides(t *testing.T) { automation := &recordingTaskPRAutomationService{} h := &Handlers{taskPRAutomation: automation, logger: testLogger(t).WithFields()} diff --git a/apps/backend/internal/mcp/server/server.go b/apps/backend/internal/mcp/server/server.go index 869444cc4a..a74bf4c34b 100644 --- a/apps/backend/internal/mcp/server/server.go +++ b/apps/backend/internal/mcp/server/server.go @@ -838,17 +838,29 @@ If the child has no live execution, the call succeeds idempotently with status=" func (s *Server) registerPRAutomationTools() { s.mcpServer.AddTool( mcp.NewToolWithRawSchema("get_task_pr_automation_kandev", - "Get the current task's GitHub PR automation settings, including lifecycle notification switches.", + "Get the current task's GitHub PR automation settings, including lifecycle notification switches. "+ + "The five automation switches are scoped per linked PR; pr_options carries one entry per PR "+ + "(repository_id, pr_number, and the five booleans). The top-level booleans are an aggregate "+ + "that reports true only when every linked PR has that switch on and at least one PR is linked "+ + "— use them to check whether a task-wide enable fully took, and use pr_options for anything "+ + "PR-specific.", json.RawMessage(`{"type":"object","properties":{}}`), ), s.wrapHandler("get_task_pr_automation_kandev", s.getTaskPRAutomationHandler()), ) s.mcpServer.AddTool( mcp.NewTool("update_task_pr_automation_kandev", - mcp.WithDescription("Update this task's PR automation options (auto-fix, auto-merge, and lifecycle notifications)."), + mcp.WithDescription( + "Update this task's PR automation options (auto-fix, auto-merge, and lifecycle notifications). "+ + "The five switches are scoped per linked PR: pass both repository_id and pr_number to target "+ + "one linked PR, or omit both to apply the change to every PR currently linked to the task "+ + "(unchanged default behavior). auto_fix_prompt_override applies task-wide regardless of PR identity.", + ), + mcp.WithString("repository_id", mcp.Description("Target one linked PR's repository_id; must be paired with pr_number. Omit both to apply to every linked PR.")), + mcp.WithNumber("pr_number", mcp.Description("Target one linked PR's number; must be paired with repository_id. Omit both to apply to every linked PR.")), mcp.WithBoolean("auto_fix_enabled", mcp.Description("Enable or disable auto-fix when CI checks fail")), mcp.WithBoolean("auto_merge_enabled", mcp.Description("Enable or disable auto-merge when PR passes all checks")), - mcp.WithString("auto_fix_prompt_override", mcp.Description("Custom prompt for auto-fix (empty string clears the override)")), + mcp.WithString("auto_fix_prompt_override", mcp.Description("Custom prompt for auto-fix (empty string clears the override). Task-wide; not affected by repository_id/pr_number.")), mcp.WithBoolean("prompt_on_review_requested", mcp.Description("Prompt this task's agent when a review is requested for the authenticated user")), mcp.WithBoolean("prompt_on_merged", mcp.Description("Prompt this task's agent once when the linked PR becomes merged")), mcp.WithBoolean("prompt_on_closed", mcp.Description("Prompt this task's agent once when the linked PR becomes closed without merge")), diff --git a/apps/backend/internal/orchestrator/event_handlers_github.go b/apps/backend/internal/orchestrator/event_handlers_github.go index 472cdf12fc..f5777b1f75 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github.go +++ b/apps/backend/internal/orchestrator/event_handlers_github.go @@ -201,9 +201,7 @@ func (s *Service) handleTaskPRUpdated(ctx context.Context, event *bus.Event) err func (s *Service) handleTaskCIOptionsUpdated(ctx context.Context, event *bus.Event) error { options, ok := event.Data.(*github.TaskCIOptionsResponse) if !ok || options == nil || event.Source == ciAutomationStateEventSource || - (!options.AutoFixEnabled && !options.AutoMergeEnabled && - !options.PromptOnReviewRequested && !options.PromptOnMerged && !options.PromptOnClosed) || - s.githubService == nil { + !anyTaskPRAutomationEnabled(options) || s.githubService == nil { return nil } detachedCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), ciAutomationDetachedTimeout) @@ -219,6 +217,34 @@ func (s *Service) handleTaskCIOptionsUpdated(ctx context.Context, event *bus.Eve return nil } +// anyTaskPRAutomationEnabled reports whether any linked PR has at least one +// automation switch on. Per-PR scoping means the task-wide aggregate on +// options can be false (mixed configuration across PRs) while individual +// PRs still need this update's PR sync — so this checks the per-PR array +// rather than the aggregated top-level booleans. Falls back to the top-level +// booleans when PROptions is empty (e.g. a caller that built the response by +// hand without per-PR data), matching resolvePRScopedCIOptions's same +// graceful-degradation pattern rather than silently swallowing the event. +func anyTaskPRAutomationEnabled(options *github.TaskCIOptionsResponse) bool { + if options == nil { + return false + } + if len(options.PROptions) == 0 { + return options.AutoFixEnabled || options.AutoMergeEnabled || + options.PromptOnReviewRequested || options.PromptOnMerged || options.PromptOnClosed + } + for _, opt := range options.PROptions { + if opt == nil { + continue + } + if opt.AutoFixEnabled || opt.AutoMergeEnabled || + opt.PromptOnReviewRequested || opt.PromptOnMerged || opt.PromptOnClosed { + return true + } + } + return false +} + func (s *Service) recordTaskCIOptionsSyncError(ctx context.Context, taskID string, synced []*github.TaskPR, syncErr error) { syncedKeys := make(map[ciAutomationTaskPRSyncKey]struct{}, len(synced)) for _, pr := range synced { 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 05c5e76fbb..a7a719594b 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go @@ -79,6 +79,7 @@ func (s *Service) handleTaskPRCIAutomationWithRefresh(ctx context.Context, pr *g s.logger.Debug("load CI automation options failed", zap.String("task_id", pr.TaskID), zap.Error(err)) return nil } + options = resolvePRScopedCIOptions(options, pr) freshlySynced := ciAutomationHasFreshPRStatus(pr) if refresh && (options.AutoFixEnabled || options.AutoMergeEnabled) { refreshed, synced, syncErr := s.refreshTaskPRForCIAutomation(ctx, pr) @@ -116,6 +117,30 @@ func (s *Service) handleTaskPRCIAutomationWithRefresh(ctx context.Context, pr *g return nil } +// resolvePRScopedCIOptions overlays this PR's own automation switches onto a +// copy of the task-scoped response, so the auto-fix, auto-merge, and +// lifecycle-prompt automation below acts on this PR's per-PR configuration +// instead of the task-wide aggregate. Task-level fields (reviewer login, +// effective prompts, prompt override) are left as returned. +func resolvePRScopedCIOptions(options *github.TaskCIOptionsResponse, pr *github.TaskPR) *github.TaskCIOptionsResponse { + if options == nil || pr == nil { + return options + } + scoped := *options + for _, opt := range options.PROptions { + if opt == nil || opt.RepositoryID != pr.RepositoryID || opt.PRNumber != pr.PRNumber { + continue + } + scoped.AutoFixEnabled = opt.AutoFixEnabled + scoped.AutoMergeEnabled = opt.AutoMergeEnabled + scoped.PromptOnReviewRequested = opt.PromptOnReviewRequested + scoped.PromptOnMerged = opt.PromptOnMerged + scoped.PromptOnClosed = opt.PromptOnClosed + break + } + return &scoped +} + func (s *Service) handleTaskPRLifecycleAutomation(ctx context.Context, pr *github.TaskPR, options *github.TaskCIOptionsResponse) { if !options.PromptOnReviewRequested && !options.PromptOnMerged && !options.PromptOnClosed { return diff --git a/apps/backend/internal/orchestrator/event_handlers_github_ci_options_test.go b/apps/backend/internal/orchestrator/event_handlers_github_ci_options_test.go new file mode 100644 index 0000000000..c183573d4e --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_github_ci_options_test.go @@ -0,0 +1,107 @@ +package orchestrator + +import ( + "testing" + + "github.com/kandev/kandev/internal/github" +) + +func TestResolvePRScopedCIOptions(t *testing.T) { + override := "task prompt" + options := &github.TaskCIOptionsResponse{ + TaskID: "task-1", + AutoFixEnabled: true, + AutoMergeEnabled: true, + PromptOnReviewRequested: true, + PromptOnMerged: true, + PromptOnClosed: true, + ReviewReviewerLogin: "reviewer", + AutoFixPromptOverride: &override, + EffectiveAutoFixPrompt: "effective prompt", + PROptions: []*github.TaskPRAutomationOptions{ + { + RepositoryID: "repo-1", PRNumber: 1, + AutoFixEnabled: false, AutoMergeEnabled: false, + PromptOnReviewRequested: false, PromptOnMerged: false, PromptOnClosed: false, + }, + { + RepositoryID: "repo-2", PRNumber: 2, + AutoFixEnabled: true, AutoMergeEnabled: false, + PromptOnReviewRequested: true, PromptOnMerged: false, PromptOnClosed: true, + }, + }, + } + pr := &github.TaskPR{TaskID: "task-1", RepositoryID: "repo-1", PRNumber: 1} + got := resolvePRScopedCIOptions(options, pr) + if got == options { + t.Fatal("resolver returned the input response instead of a scoped copy") + } + if got.AutoFixEnabled || got.AutoMergeEnabled || got.PromptOnReviewRequested || got.PromptOnMerged || got.PromptOnClosed { + t.Fatalf("scoped switches = %+v, want PR #1 values", got) + } + if got.ReviewReviewerLogin != options.ReviewReviewerLogin || + got.AutoFixPromptOverride != options.AutoFixPromptOverride || + got.EffectiveAutoFixPrompt != options.EffectiveAutoFixPrompt { + t.Fatalf("task-level fields changed during scoping: got=%+v want=%+v", got, options) + } + + missing := resolvePRScopedCIOptions(options, &github.TaskPR{TaskID: "task-1", RepositoryID: "repo-3", PRNumber: 3}) + if !missing.AutoFixEnabled || !missing.AutoMergeEnabled || !missing.PromptOnReviewRequested || !missing.PromptOnMerged || !missing.PromptOnClosed { + t.Fatal("missing PR row did not preserve the task aggregate fallback") + } + + withNil := *options + withNil.PROptions = append([]*github.TaskPRAutomationOptions{nil}, options.PROptions...) + if scoped := resolvePRScopedCIOptions(&withNil, &github.TaskPR{RepositoryID: "repo-2", PRNumber: 2}); !scoped.AutoFixEnabled { + t.Fatal("nil PR option entry prevented matching row resolution") + } + if resolvePRScopedCIOptions(nil, pr) != nil || resolvePRScopedCIOptions(options, nil) != options { + t.Fatal("nil input handling changed") + } +} + +func TestAnyTaskPRAutomationEnabled(t *testing.T) { + for _, test := range []struct { + name string + options *github.TaskCIOptionsResponse + want bool + }{ + { + name: "mixed per PR options", + options: &github.TaskCIOptionsResponse{PROptions: []*github.TaskPRAutomationOptions{ + {RepositoryID: "repo-1", PRNumber: 1}, + {RepositoryID: "repo-2", PRNumber: 2, AutoMergeEnabled: true}, + }}, + want: true, + }, + { + name: "all off", + options: &github.TaskCIOptionsResponse{PROptions: []*github.TaskPRAutomationOptions{ + {RepositoryID: "repo-1", PRNumber: 1}, + {RepositoryID: "repo-2", PRNumber: 2}, + }}, + want: false, + }, + { + name: "empty per PR options falls back to aggregate", + options: &github.TaskCIOptionsResponse{AutoFixEnabled: true}, + want: true, + }, + { + name: "nil entry is skipped", + options: &github.TaskCIOptionsResponse{PROptions: []*github.TaskPRAutomationOptions{nil}}, + want: false, + }, + { + name: "nil response", + options: nil, + want: false, + }, + } { + t.Run(test.name, func(t *testing.T) { + if got := anyTaskPRAutomationEnabled(test.options); got != test.want { + t.Fatalf("anyTaskPRAutomationEnabled() = %v, want %v", got, test.want) + } + }) + } +} diff --git a/apps/web/components/github/pr-ci-automation-controls.tsx b/apps/web/components/github/pr-ci-automation-controls.tsx index 2a8820bed9..260818545c 100644 --- a/apps/web/components/github/pr-ci-automation-controls.tsx +++ b/apps/web/components/github/pr-ci-automation-controls.tsx @@ -1,522 +1,19 @@ "use client"; -import { useCallback, useEffect, useState, type ReactNode } from "react"; -import { IconChevronDown, IconEdit, IconInfoCircle, IconRefresh } from "@tabler/icons-react"; -import { Button } from "@kandev/ui/button"; -import { Collapsible, CollapsibleContent, CollapsibleTrigger } from "@kandev/ui/collapsible"; -import { - Dialog, - DialogContent, - DialogDescription, - DialogFooter, - DialogHeader, - DialogTitle, -} from "@kandev/ui/dialog"; -import { Label } from "@kandev/ui/label"; -import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover"; -import { Switch } from "@kandev/ui/switch"; -import { Textarea } from "@kandev/ui/textarea"; -import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; +import { useCallback, useState } from "react"; import { useToast } from "@/components/toast-provider"; import { useTaskCIAutomationOptions } from "@/hooks/domains/github/use-task-ci-options"; -import { useResponsiveBreakpoint } from "@/hooks/use-responsive-breakpoint"; -import { autoFixRoundForState, findCIAutomationStateForPR } from "@/lib/github/ci-automation"; -import type { - TaskCIAutomationOptions, - TaskCIAutomationPatch, - TaskCIPRAutomationState, - TaskPR, -} from "@/lib/types/github"; -import { Trans, useTranslation } from "react-i18next"; - -const PR_FEEDBACK_PLACEHOLDER = "{{pr.feedback}}"; - -function CIAutomationInfoButton() { - const { t } = useTranslation(); - return ( - - - - - - {t("github:watchesThisTaskSLinkedPull")} - - - ); -} - -function insertPRFeedbackPlaceholder(prompt: string) { - if (prompt.includes(PR_FEEDBACK_PLACEHOLDER)) return prompt; - const trimmedEnd = prompt.trimEnd(); - if (!trimmedEnd) return PR_FEEDBACK_PLACEHOLDER; - return `${trimmedEnd}\n\n${PR_FEEDBACK_PLACEHOLDER}`; -} - -function CIAutomationPromptDialog({ - open, - prompt, - saving, - onPromptChange, - onClose, - onSave, - onReset, -}: { - open: boolean; - prompt: string; - saving: boolean; - onPromptChange: (value: string) => void; - onClose: () => void; - onSave: () => void; - onReset: () => void; -}) { - const { t } = useTranslation(); - const trimmed = prompt.trim(); - return ( - !next && onClose()}> - - - {t("github:autoFixPrompt")} - - - - - - -
-
- -
- - e.stopPropagation()} - > - {t("github:editDefaultPrompt")} - -
-
-
-

{t("github:thePlaceholderInsertsTheCurrentPr")}

-

{t("github:omitThePlaceholderIfYouWant")}

-
-