diff --git a/.gitignore b/.gitignore index 9c3d438e0e..f7f941b8de 100644 --- a/.gitignore +++ b/.gitignore @@ -155,3 +155,6 @@ apps/web/e2e/.auth-screenshots/ # Restart-action screenshot artifacts (generated by feature-toggles-restart.spec.ts) apps/web/e2e/.restart-screenshots/ + +# Manual dev-environment scratch repos (apps/web/e2e/manual-seed-*.ts) +apps/backend/.manual-env/ diff --git a/apps/backend/internal/gitlab/controller_mr_automation.go b/apps/backend/internal/gitlab/controller_mr_automation.go index b85bfa5e2f..60ed11b6a9 100644 --- a/apps/backend/internal/gitlab/controller_mr_automation.go +++ b/apps/backend/internal/gitlab/controller_mr_automation.go @@ -39,6 +39,8 @@ var errUnknownMRAutomationField = errors.New("unknown MR automation field") // as a bad request. var errNullMRAutomationSwitch = errors.New("MR automation switch must be a boolean, not null") +var errNullMRAutomationIdentity = errors.New("MR automation identity fields must not be null") + // RegisterMRAutomationHTTPRoutes registers the GET/PATCH MR automation // endpoints on an existing /api/v1/gitlab router group. func (c *Controller) RegisterMRAutomationHTTPRoutes(api *gin.RouterGroup) { @@ -74,6 +76,12 @@ func (c *Controller) httpPatchTaskMRAutomation(ctx *gin.Context) { if writeMRAutomationTaskNotFound(ctx, err) { return } + // A patch naming an MR that isn't linked (or naming one only + // partially) is a caller mistake, not a server fault. + if errors.Is(err, ErrTaskMRNotLinked) { + ctx.JSON(http.StatusBadRequest, gin.H{responseErrorKey: err.Error()}) + return + } c.logger.Error("update task MR automation failed", zap.String("task_id", ctx.Param("taskID")), zap.Error(err)) ctx.JSON(http.StatusInternalServerError, gin.H{responseErrorKey: "failed to update MR automation options"}) return @@ -150,6 +158,12 @@ func applyMRAutomationPatchField(patch *TaskMRAutomationPatch, key string, value return decodeMRAutomationSwitch(value, &patch.PromptOnMerged) case "prompt_on_closed": return decodeMRAutomationSwitch(value, &patch.PromptOnClosed) + case "repository_id": + return decodeMRAutomationIdentityString(value, &patch.RepositoryID) + case "project_path": + return decodeMRAutomationIdentityString(value, &patch.ProjectPath) + case "mr_iid": + return decodeMRAutomationIdentityInteger(value, &patch.MRIID) case "review_prompt_override", "merged_prompt_override", "closed_prompt_override": return errLifecyclePromptOverridesUnsupported default: @@ -187,6 +201,20 @@ func decodeMRAutomationSwitch(value json.RawMessage, dst **bool) error { return json.Unmarshal(value, dst) } +func decodeMRAutomationIdentityString(value json.RawMessage, dst **string) error { + if string(value) == "null" { + return errNullMRAutomationIdentity + } + return json.Unmarshal(value, dst) +} + +func decodeMRAutomationIdentityInteger(value json.RawMessage, dst **int) error { + if string(value) == "null" { + return errNullMRAutomationIdentity + } + return json.Unmarshal(value, dst) +} + func (c *Controller) publishTaskMRAutomationUpdated(ctx context.Context, resp *TaskMRAutomationResponse) { if c.service == nil || resp == nil { return diff --git a/apps/backend/internal/gitlab/controller_mr_automation_test.go b/apps/backend/internal/gitlab/controller_mr_automation_test.go index 36c706db0b..05227da475 100644 --- a/apps/backend/internal/gitlab/controller_mr_automation_test.go +++ b/apps/backend/internal/gitlab/controller_mr_automation_test.go @@ -20,6 +20,11 @@ func newMRAutomationControllerFixture(t *testing.T) (*gin.Engine, *Service) { store := newTestStore(t) seedWorkspace(t, store, "ws-1") seedTask(t, store, "task-1", "ws-1") + // The switches are per-MR, so a PATCH only has somewhere to land once the + // task has at least one linked MR. + if err := store.UpsertTaskMR(context.Background(), newTestMR("task-1", "", "group/a", 1)); err != nil { + t.Fatalf("seed linked MR: %v", err) + } if err := store.SaveConfigForWorkspace(context.Background(), "ws-1", &GitLabConfig{ Host: "https://gitlab.example.com", AuthMethod: AuthMethodPAT, }); err != nil { @@ -379,3 +384,80 @@ func TestControllerPatchTaskMRAutomation_PublishesEvent(t *testing.T) { t.Fatal("expected GitLabTaskMROptionsUpdated event to be published") } } + +// TestControllerPatchTaskMRAutomation_ScopesToTheNamedMR covers the per-MR +// PATCH contract: a body carrying repository_id/project_path/mr_iid applies +// the switch to that MR alone. +func TestControllerPatchTaskMRAutomation_ScopesToTheNamedMR(t *testing.T) { + router, svc := newMRAutomationControllerFixture(t) + if err := svc.store.UpsertTaskMR(context.Background(), newTestMR("task-1", "", "group/b", 2)); err != nil { + t.Fatalf("seed second MR: %v", err) + } + + body := `{"repository_id":"","project_path":"group/a","mr_iid":1,"auto_merge_enabled":true}` + req := httptest.NewRequest(http.MethodPatch, "/api/v1/gitlab/tasks/task-1/mr-automation", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + if resp.Code != http.StatusOK { + t.Fatalf("PATCH status = %d, body = %s", resp.Code, resp.Body.String()) + } + var got TaskMRAutomationResponse + if err := json.Unmarshal(resp.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if len(got.MROptions) != 2 { + t.Fatalf("expected one mr_options entry per linked MR, got %+v", got.MROptions) + } + for _, opt := range got.MROptions { + wantEnabled := opt.ProjectPath == "group/a" + if opt.AutoMergeEnabled != wantEnabled { + t.Errorf("MR %s auto_merge_enabled = %v, want %v", opt.ProjectPath, opt.AutoMergeEnabled, wantEnabled) + } + } +} + +// TestControllerPatchTaskMRAutomation_RejectsBadMRIdentity keeps a caller +// mistake a 400 rather than a 500 — and, for a partial identity, keeps it +// from being silently reinterpreted as "apply to every linked MR". +func TestControllerPatchTaskMRAutomation_RejectsBadMRIdentity(t *testing.T) { + router, _ := newMRAutomationControllerFixture(t) + for name, body := range map[string]string{ + "unlinked MR": `{"repository_id":"","project_path":"group/nope","mr_iid":9,"auto_fix_enabled":true}`, + "partial identity": `{"project_path":"group/a","auto_fix_enabled":true}`, + "null identity": `{"repository_id":null,"project_path":null,"mr_iid":null,"auto_fix_enabled":true}`, + "identity only": `{"repository_id":"","project_path":"group/a","mr_iid":1}`, + } { + req := httptest.NewRequest(http.MethodPatch, "/api/v1/gitlab/tasks/task-1/mr-automation", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + if resp.Code != http.StatusBadRequest { + t.Errorf("%s: status = %d, want 400 (body = %s)", name, resp.Code, resp.Body.String()) + } + } +} + +// TestControllerPatchTaskMRAutomation_RejectsSwitchesWithNoLinkedMRs is the +// HTTP half of the zero-target rule: because the switches only exist per MR, +// a task with none has nowhere to store them, and answering 200 would report +// a write that never happened. The task-level prompt override stays accepted. +func TestControllerPatchTaskMRAutomation_RejectsSwitchesWithNoLinkedMRs(t *testing.T) { + router, svc := newMRAutomationControllerFixture(t) + seedTask(t, svc.store, "task-2", "ws-1") + + patch := func(body string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodPatch, "/api/v1/gitlab/tasks/task-2/mr-automation", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + return resp + } + + if resp := patch(`{"auto_merge_enabled":true}`); resp.Code != http.StatusBadRequest { + t.Errorf("switch patch status = %d, want 400 (body = %s)", resp.Code, resp.Body.String()) + } + if resp := patch(`{"auto_fix_prompt_override":"custom"}`); resp.Code != http.StatusOK { + t.Errorf("prompt override status = %d, want 200 (body = %s)", resp.Code, resp.Body.String()) + } +} diff --git a/apps/backend/internal/gitlab/models_mr_automation.go b/apps/backend/internal/gitlab/models_mr_automation.go index c9c68ea0de..0cbd554c8f 100644 --- a/apps/backend/internal/gitlab/models_mr_automation.go +++ b/apps/backend/internal/gitlab/models_mr_automation.go @@ -18,26 +18,87 @@ const ( // Mirrors github.TaskCIAutoFixMaxRounds. const TaskMRAutoFixMaxRounds = 10 -// TaskMRAutomationOptions stores task-level MR automation preferences: -// the three lifecycle notification switches from #2125, plus auto-fix CI -// and auto-merge (this task). Parallel to github.TaskCIOptions. +// MRIdentity names one merge request linked to a task. RepositoryID may be +// empty for single-repo tasks, matching gitlab_task_mrs' own key. +type MRIdentity struct { + RepositoryID string + ProjectPath string + MRIID int +} + +// TaskMRAutomationOptions stores the genuinely task-level MR automation +// preferences: the auto-fix prompt override and the server-resolved reviewer +// username. Parallel to github.TaskCIOptions. +// +// The five switch fields below are legacy: they are no longer written by +// UpdateTaskMRAutomationOptions and are read only by the one-time +// mr_scope_migrated_at fan-out migration (migrateTaskMROptionsToMRScope). +// The per-MR source of truth is TaskMRAutomationOptionsForMR / +// gitlab_task_mr_automation_options. type TaskMRAutomationOptions struct { TaskID string `json:"task_id" db:"task_id"` + AutomationRevision int64 `json:"-" db:"automation_revision"` + 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:"-" db:"prompt_on_review_requested"` + PromptOnMerged bool `json:"-" db:"prompt_on_merged"` + PromptOnClosed bool `json:"-" db:"prompt_on_closed"` + ReviewReviewerUsername string `json:"review_reviewer_username" db:"review_reviewer_username"` + CreatedAt time.Time `json:"created_at" db:"created_at"` + UpdatedAt time.Time `json:"updated_at" db:"updated_at"` +} + +// TaskMRAutomationOptionsForMR stores the five automation switches for one +// linked merge request. This is the per-MR source of truth; the aggregated +// booleans on TaskMRAutomationResponse only report "every linked MR has this +// switch on". Parallel to github.TaskPRAutomationOptions. +type TaskMRAutomationOptionsForMR struct { + TaskID string `json:"task_id" db:"task_id"` + RepositoryID string `json:"repository_id" db:"repository_id"` + ProjectPath string `json:"project_path" db:"project_path"` + MRIID int `json:"mr_iid" db:"mr_iid"` AutoFixEnabled bool `json:"auto_fix_enabled" db:"auto_fix_enabled"` AutoMergeEnabled bool `json:"auto_merge_enabled" 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"` - ReviewReviewerUsername string `json:"review_reviewer_username" db:"review_reviewer_username"` CreatedAt time.Time `json:"created_at" db:"created_at"` UpdatedAt time.Time `json:"updated_at" db:"updated_at"` } +// Identity returns the MR this options row belongs to. +func (o *TaskMRAutomationOptionsForMR) Identity() MRIdentity { + return MRIdentity{RepositoryID: o.RepositoryID, ProjectPath: o.ProjectPath, MRIID: o.MRIID} +} + +// TaskMRAutomationSwitchPatch is a partial update for one MR's automation +// switches. +type TaskMRAutomationSwitchPatch 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 TaskMRAutomationSwitchPatch) HasAny() bool { + return p.AutoFixEnabled != nil || p.AutoMergeEnabled != nil || + p.PromptOnReviewRequested != nil || p.PromptOnMerged != nil || p.PromptOnClosed != nil +} + // TaskMRAutomationPatch is a partial update for task MR automation options. -// ReviewReviewerUsername is intentionally absent — it is server-resolved from -// the workspace's authenticated GitLab user, never client-supplied. +// RepositoryID/ProjectPath/MRIID optionally target one linked MR for the five +// automation switches; when all three are nil the switches fan out to every +// MR currently linked to the task. AutoFixPromptOverride is always +// task-level. ReviewReviewerUsername is intentionally absent — it is +// server-resolved from the workspace's authenticated GitLab user, never +// client-supplied. type TaskMRAutomationPatch struct { + RepositoryID *string + ProjectPath *string + MRIID *int AutoFixEnabled *bool AutoMergeEnabled *bool AutoFixPromptOverride *string @@ -46,16 +107,71 @@ type TaskMRAutomationPatch struct { PromptOnClosed *bool } -// HasAny reports whether the patch contains at least one requested field change. +// HasAny reports whether the patch contains at least one requested field +// change. MR identity alone is not a change — it only says which MR the +// (absent) switch changes would have applied to. func (p TaskMRAutomationPatch) HasAny() bool { - return p.AutoFixEnabled != nil || p.AutoMergeEnabled != nil || p.AutoFixPromptOverride != nil || - p.PromptOnReviewRequested != nil || p.PromptOnMerged != nil || p.PromptOnClosed != nil + return p.AutoFixPromptOverride != nil || p.SwitchPatch().HasAny() +} + +// SwitchPatch extracts the per-MR automation switch fields. +func (p TaskMRAutomationPatch) SwitchPatch() TaskMRAutomationSwitchPatch { + return TaskMRAutomationSwitchPatch{ + AutoFixEnabled: p.AutoFixEnabled, + AutoMergeEnabled: p.AutoMergeEnabled, + PromptOnReviewRequested: p.PromptOnReviewRequested, + PromptOnMerged: p.PromptOnMerged, + PromptOnClosed: p.PromptOnClosed, + } +} + +// HasMRIdentity reports whether the patch names a specific MR. +func (p TaskMRAutomationPatch) HasMRIdentity() bool { + return p.RepositoryID != nil && p.ProjectPath != nil && p.MRIID != nil +} + +// HasPartialMRIdentity reports whether the patch names some but not all of +// the three MR identity fields — a caller mistake that must be rejected +// rather than silently fanned out to every linked MR. +func (p TaskMRAutomationPatch) HasPartialMRIdentity() bool { + set := 0 + if p.RepositoryID != nil { + set++ + } + if p.ProjectPath != nil { + set++ + } + if p.MRIID != nil { + set++ + } + return set != 0 && set != 3 +} + +// MRIdentity returns the MR named by the patch. Only meaningful when +// HasMRIdentity reports true. +func (p TaskMRAutomationPatch) MRIdentity() MRIdentity { + id := MRIdentity{} + if p.RepositoryID != nil { + id.RepositoryID = *p.RepositoryID + } + if p.ProjectPath != nil { + id.ProjectPath = *p.ProjectPath + } + if p.MRIID != nil { + id.MRIID = *p.MRIID + } + return id } // TaskMRAutomationResponse is the HTTP/MCP shape for task MR automation -// options, including the per-MR lifecycle checkpoints for observability. +// options, including the per-MR switches, and the per-MR lifecycle +// checkpoints for observability. The five top-level switch booleans are an +// aggregate over MROptions ("every linked MR has this switch on, and at +// least one MR is linked") kept for MCP/API read compatibility; MROptions is +// the per-MR source of truth. type TaskMRAutomationResponse struct { TaskID string `json:"task_id"` + AutomationRevision int64 `json:"automation_revision"` AutoFixEnabled bool `json:"auto_fix_enabled"` AutoMergeEnabled bool `json:"auto_merge_enabled"` AutoFixPromptOverride *string `json:"auto_fix_prompt_override"` @@ -68,6 +184,9 @@ type TaskMRAutomationResponse struct { ReviewReviewerUsername string `json:"review_reviewer_username"` UpdatedAt time.Time `json:"updated_at"` MRStates []*TaskMRLifecycleState `json:"mr_states"` + // MROptions carries one entry per MR currently linked to the task, so + // the UI can render each MR's own switches instead of the aggregate. + MROptions []*TaskMRAutomationOptionsForMR `json:"mr_options"` // WorkspaceID is internal routing metadata (best-effort resolved, may be // empty) that lets the websocket broadcaster scope the // gitlab.task_mr_options.updated event to the owning workspace instead of diff --git a/apps/backend/internal/gitlab/poller_lifecycle_test.go b/apps/backend/internal/gitlab/poller_lifecycle_test.go index a72de32a43..333ae29a43 100644 --- a/apps/backend/internal/gitlab/poller_lifecycle_test.go +++ b/apps/backend/internal/gitlab/poller_lifecycle_test.go @@ -51,11 +51,9 @@ func TestPoller_RunMRLifecycleSync_SyncsSubscribedRowsAndPublishes(t *testing.T) if err := store.UpsertTaskMR(ctx, unsubscribed); err != nil { t.Fatalf("seed unsubscribed MR: %v", err) } - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + setMRSwitches(t, store, "task-1", mrIdentity("group/subscribed", 1), TaskMRAutomationSwitchPatch{ PromptOnMerged: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable switch: %v", err) - } + }) memBus := bus.NewMemoryEventBus(newTestLogger(t)) svc.SetEventBus(memBus) @@ -128,13 +126,12 @@ func TestPoller_RunMRLifecycleSync_ErrorOnOneRowDoesNotAbortOthers(t *testing.T) if err := store.UpsertTaskMR(ctx, ok); err != nil { t.Fatalf("seed ok MR: %v", err) } - for _, taskID := range []string{"task-1", "task-2"} { - if _, err := store.UpdateTaskMRAutomationOptions(ctx, taskID, TaskMRAutomationPatch{ - PromptOnMerged: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable switch for %s: %v", taskID, err) - } - } + setMRSwitches(t, store, "task-1", mrIdentity("group/broken", 1), TaskMRAutomationSwitchPatch{ + PromptOnMerged: boolPtr(true), + }) + setMRSwitches(t, store, "task-2", mrIdentity("group/ok", 2), TaskMRAutomationSwitchPatch{ + PromptOnMerged: boolPtr(true), + }) memBus := bus.NewMemoryEventBus(newTestLogger(t)) svc.SetEventBus(memBus) @@ -194,11 +191,9 @@ func TestPoller_RunMRLifecycleSync_UsesStrictClient(t *testing.T) { if err := store.UpsertTaskMR(ctx, newTestMR("task-1", "", "group/subscribed", 1)); err != nil { t.Fatalf("seed subscribed MR: %v", err) } - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + setMRSwitches(t, store, "task-1", mrIdentity("group/subscribed", 1), TaskMRAutomationSwitchPatch{ PromptOnMerged: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable switch: %v", err) - } + }) memBus := bus.NewMemoryEventBus(newTestLogger(t)) svc.SetEventBus(memBus) @@ -260,11 +255,9 @@ func TestPoller_RunMRLifecycleSync_RejectsHostChangeSinceLink(t *testing.T) { if err := store.UpsertTaskMR(ctx, linked); err != nil { t.Fatalf("seed linked MR: %v", err) } - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + setMRSwitches(t, store, "task-1", mrIdentity("group/subscribed", 1), TaskMRAutomationSwitchPatch{ PromptOnMerged: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable switch: %v", err) - } + }) memBus := bus.NewMemoryEventBus(newTestLogger(t)) svc.SetEventBus(memBus) @@ -323,11 +316,9 @@ func TestPoller_RunMRLifecycleSync_ClearsRecoveredError(t *testing.T) { if err := store.UpsertTaskMR(ctx, subscribed); err != nil { t.Fatalf("seed MR: %v", err) } - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + setMRSwitches(t, store, "task-1", mrIdentity("group/subscribed", 1), TaskMRAutomationSwitchPatch{ PromptOnMerged: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable switch: %v", err) - } + }) if err := store.RecordTaskMRSyncError(ctx, "task-1", "", "group/subscribed", 1, "prior failure"); err != nil { t.Fatalf("seed prior error: %v", err) } diff --git a/apps/backend/internal/gitlab/service_cleanup_mr_automation_test.go b/apps/backend/internal/gitlab/service_cleanup_mr_automation_test.go index a6133567f2..29da5f93a1 100644 --- a/apps/backend/internal/gitlab/service_cleanup_mr_automation_test.go +++ b/apps/backend/internal/gitlab/service_cleanup_mr_automation_test.go @@ -18,11 +18,9 @@ func TestDeleteReviewMRTaskIfTerminal_RetainsWhenLifecyclePromptsEnabled(t *test mock.SeedMR(project, &MR{IID: 7, State: gitlabStateMerged}) seedTask(t, svc.store, "task-subscribed", "") - if _, err := svc.store.UpdateTaskMRAutomationOptions(ctx, "task-subscribed", TaskMRAutomationPatch{ + setMRSwitches(t, svc.store, "task-subscribed", mrIdentity(project, 7), TaskMRAutomationSwitchPatch{ PromptOnMerged: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable switch: %v", err) - } + }) rec := &recordingReasonDeleter{} task := &ReviewMRTask{ID: "rmt-1", ProjectPath: project, MRIID: 7, TaskID: "task-subscribed"} @@ -47,11 +45,9 @@ func TestDeleteReviewMRTaskIfTerminal_AlwaysPolicyIgnoresLifecyclePrompts(t *tes mock.SeedMR(project, &MR{IID: 7, State: gitlabStateMerged}) seedTask(t, svc.store, "task-subscribed", "") - if _, err := svc.store.UpdateTaskMRAutomationOptions(ctx, "task-subscribed", TaskMRAutomationPatch{ + setMRSwitches(t, svc.store, "task-subscribed", mrIdentity(project, 7), TaskMRAutomationSwitchPatch{ PromptOnMerged: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable switch: %v", err) - } + }) rec := &recordingReasonDeleter{} task := &ReviewMRTask{ID: "rmt-1", ProjectPath: project, MRIID: 7, TaskID: "task-subscribed"} diff --git a/apps/backend/internal/gitlab/service_mr_automation.go b/apps/backend/internal/gitlab/service_mr_automation.go index 0a84caa9ff..7045bf1681 100644 --- a/apps/backend/internal/gitlab/service_mr_automation.go +++ b/apps/backend/internal/gitlab/service_mr_automation.go @@ -3,6 +3,7 @@ package gitlab import ( "context" "errors" + "fmt" "strings" promptcfg "github.com/kandev/kandev/config/prompts" @@ -13,6 +14,19 @@ import ( // defaultCIAutoFixPromptName. const defaultMRAutoFixPromptName = "mr-auto-fix" +// ErrTaskMRNotLinked reports a patch naming an MR that is not linked to the +// task. Exported so the HTTP controller and MCP handler can map it to a +// client error instead of a 500. +var ErrTaskMRNotLinked = errors.New("gitlab: merge request is not linked to this task") + +// ErrTaskMRIdentityIncomplete reports a patch that set some but not all of +// repository_id/project_path/mr_iid. Sending a partial identity is a caller +// mistake, and treating it as "no identity" would silently fan the switch +// change out to every linked MR. +var ErrTaskMRIdentityIncomplete = fmt.Errorf( + "%w: repository_id, project_path and mr_iid must all be set", ErrTaskMRNotLinked, +) + // GetTaskMRAutomationResponse returns a task's MR automation options plus // its per-MR lifecycle checkpoints (AC1). func (s *Service) GetTaskMRAutomationResponse(ctx context.Context, taskID string) (*TaskMRAutomationResponse, error) { @@ -35,11 +49,52 @@ func (s *Service) GetTaskMRAutomationResponse(ctx context.Context, taskID string if err != nil { return nil, err } + mrOptions, err := s.taskMRAutomationOptionsList(ctx, taskID) + if err != nil { + return nil, err + } states, err := store.ListTaskMRLifecycleStates(ctx, taskID) if err != nil { return nil, err } - return s.taskMRAutomationResponseFromOptions(ctx, opts, states, workspaceID), nil + return s.taskMRAutomationResponseFromOptions(ctx, opts, mrOptions, states, workspaceID), nil +} + +// taskMRAutomationOptionsList returns one entry per MR currently linked to +// the task, synthesizing all-off defaults for a linked MR that has no stored +// row yet. A stored row whose MR is no longer linked is dropped: it can no +// longer be configured or evaluated, and surfacing it would let a detached +// MR's leftover switch drag the aggregate down. +func (s *Service) taskMRAutomationOptionsList(ctx context.Context, taskID string) ([]*TaskMRAutomationOptionsForMR, error) { + store := s.requireStore() + if store == nil { + return nil, errStoreUnavailable + } + stored, err := store.ListTaskMRAutomationOptions(ctx, taskID) + if err != nil { + return nil, err + } + mrs, err := store.ListTaskMRsByTask(ctx, taskID) + if err != nil { + return nil, err + } + byIdentity := make(map[MRIdentity]*TaskMRAutomationOptionsForMR, len(stored)) + for _, row := range stored { + byIdentity[row.Identity()] = row + } + out := make([]*TaskMRAutomationOptionsForMR, 0, len(mrs)) + for _, mr := range mrs { + id := MRIdentity{RepositoryID: mr.RepositoryID, ProjectPath: mr.ProjectPath, MRIID: mr.MRIID} + if row, ok := byIdentity[id]; ok { + out = append(out, row) + continue + } + out = append(out, &TaskMRAutomationOptionsForMR{ + TaskID: taskID, RepositoryID: id.RepositoryID, + ProjectPath: id.ProjectPath, MRIID: id.MRIID, + }) + } + return out, nil } // GetTaskMRAutomationEvaluation returns the narrow snapshot needed for one @@ -79,6 +134,13 @@ func (s *Service) GetTaskMRAutomationEvaluation( if err != nil { return nil, err } + id := MRIdentity{RepositoryID: repositoryID, ProjectPath: projectPath, MRIID: mrIID} + // Only this MR's own switches drive this evaluation — a sibling MR's + // configuration must never enable automation here. + mrOpts, err := store.GetTaskMRAutomationOptionsForMR(ctx, taskID, id) + if err != nil { + return nil, err + } checkpoint, err := store.GetTaskMRLifecycleState(ctx, taskID, repositoryID, projectPath, mrIID) if err != nil { return nil, err @@ -89,7 +151,9 @@ func (s *Service) GetTaskMRAutomationEvaluation( evaluationOpts := *opts evaluationOpts.ReviewReviewerUsername = reviewerUsername return &TaskMRAutomationEvaluation{ - Options: s.taskMRAutomationResponseFromOptions(ctx, &evaluationOpts, nil, workspaceID), + Options: s.taskMRAutomationResponseFromOptions( + ctx, &evaluationOpts, []*TaskMRAutomationOptionsForMR{mrOpts}, nil, workspaceID, + ), Checkpoint: checkpoint, }, nil } @@ -103,27 +167,54 @@ func (s *Service) rebindTaskMRReviewerFromConfig(ctx context.Context, taskID, us } func (s *Service) taskMRAutomationResponseFromOptions( - ctx context.Context, opts *TaskMRAutomationOptions, states []*TaskMRLifecycleState, workspaceID string, + ctx context.Context, opts *TaskMRAutomationOptions, mrOptions []*TaskMRAutomationOptionsForMR, + states []*TaskMRLifecycleState, workspaceID string, ) *TaskMRAutomationResponse { effectivePrompt, usingDefault := s.effectiveMRAutoFixPrompt(ctx, opts) + aggregate := aggregateMRAutomationOptions(mrOptions) return &TaskMRAutomationResponse{ TaskID: opts.TaskID, - AutoFixEnabled: opts.AutoFixEnabled, - AutoMergeEnabled: opts.AutoMergeEnabled, + AutomationRevision: opts.AutomationRevision, + AutoFixEnabled: aggregate.AutoFixEnabled, + AutoMergeEnabled: aggregate.AutoMergeEnabled, AutoFixPromptOverride: opts.AutoFixPromptOverride, AutoFixMaxRounds: TaskMRAutoFixMaxRounds, EffectiveAutoFixPrompt: effectivePrompt, UsingDefaultPrompt: usingDefault, - PromptOnReviewRequested: opts.PromptOnReviewRequested, - PromptOnMerged: opts.PromptOnMerged, - PromptOnClosed: opts.PromptOnClosed, + PromptOnReviewRequested: aggregate.PromptOnReviewRequested, + PromptOnMerged: aggregate.PromptOnMerged, + PromptOnClosed: aggregate.PromptOnClosed, ReviewReviewerUsername: opts.ReviewReviewerUsername, UpdatedAt: opts.UpdatedAt, MRStates: states, + MROptions: mrOptions, WorkspaceID: workspaceID, } } +// aggregateMRAutomationOptions collapses per-MR switches into the response's +// top-level booleans: a switch reads as on only when every linked MR has it +// on and at least one MR is linked. "Every" rather than "any" keeps the +// aggregate a safe answer to "is this on for the MRs I'd act on" for an MCP +// caller that cannot name one MR. +func aggregateMRAutomationOptions(mrOptions []*TaskMRAutomationOptionsForMR) TaskMRAutomationOptionsForMR { + if len(mrOptions) == 0 { + return TaskMRAutomationOptionsForMR{} + } + aggregate := TaskMRAutomationOptionsForMR{ + AutoFixEnabled: true, AutoMergeEnabled: true, PromptOnReviewRequested: true, + PromptOnMerged: true, PromptOnClosed: true, + } + for _, opt := range mrOptions { + aggregate.AutoFixEnabled = aggregate.AutoFixEnabled && opt.AutoFixEnabled + aggregate.AutoMergeEnabled = aggregate.AutoMergeEnabled && opt.AutoMergeEnabled + aggregate.PromptOnReviewRequested = aggregate.PromptOnReviewRequested && opt.PromptOnReviewRequested + aggregate.PromptOnMerged = aggregate.PromptOnMerged && opt.PromptOnMerged + aggregate.PromptOnClosed = aggregate.PromptOnClosed && opt.PromptOnClosed + } + return aggregate +} + // effectiveMRAutoFixPrompt resolves the prompt text that will actually be // sent on the next auto-fix dispatch: a non-empty per-task override wins; // otherwise the default template, itself resolved through the editable @@ -144,11 +235,18 @@ func (s *Service) effectiveMRAutoFixPrompt(ctx context.Context, opts *TaskMRAuto return resolver.ResolvePromptContent(ctx, defaultMRAutoFixPromptName, fallback), true } -// UpdateTaskMRAutomationOptions applies a partial update. When the patch -// turns prompt_on_review_requested on, the workspace's authenticated GitLab -// username is resolved and persisted (AC5); turning it off clears the -// stored username. Resolution always goes through the strict, non-ambient -// workspace client (AC32). +// UpdateTaskMRAutomationOptions applies a partial update. The five automation +// switches are applied to the single MR named by +// patch.RepositoryID/ProjectPath/MRIID, or — when no MR is named — fanned out +// to every MR currently linked to the task, which preserves the behavior of +// MCP callers that have no MR identity to send. The auto-fix prompt override +// stays task-level either way. +// +// When the patch turns prompt_on_review_requested on, the workspace's +// authenticated GitLab username is resolved and persisted (AC5); turning it +// off clears the stored username unless another linked MR still needs it. +// Resolution always goes through the strict, non-ambient workspace client +// (AC32). func (s *Service) UpdateTaskMRAutomationOptions(ctx context.Context, taskID string, patch TaskMRAutomationPatch) (*TaskMRAutomationResponse, error) { if err := s.authorizeTaskMRAccess(ctx, taskID); err != nil { return nil, err @@ -157,17 +255,42 @@ func (s *Service) UpdateTaskMRAutomationOptions(ctx context.Context, taskID stri if store == nil { return nil, errStoreUnavailable } + if patch.HasPartialMRIdentity() { + return nil, ErrTaskMRIdentityIncomplete + } // See the identical check in GetTaskMRAutomationResponse: rejects an // unknown task ID before it can create an orphan options row. workspaceID, err := store.WorkspaceIDForTask(ctx, taskID) if err != nil { return nil, err } - reviewerUsername, err := s.resolveReviewerUsernameForPatch(ctx, taskID, patch) + targets, err := s.resolveTaskMRAutomationTargets(ctx, taskID, patch) if err != nil { return nil, err } - opts, err := store.UpdateTaskMRAutomationOptions(ctx, taskID, patch, reviewerUsername) + // The switches are per-MR, so with nothing linked there is no row to write + // them to. Returning 200 here would report success for a write that stored + // nothing, and the caller would only discover it much later when the + // automation it thought it had enabled never fired. (Before the switches + // became per-MR this persisted on the task row and a later-linked MR + // inherited it.) The prompt override is task-level and still allowed. + if patch.SwitchPatch().HasAny() && len(targets) == 0 { + return nil, fmt.Errorf("%w: the task has no linked merge requests", ErrTaskMRNotLinked) + } + reviewerUsername, err := s.resolveReviewerUsernameForPatch(ctx, taskID, patch, targets) + if err != nil { + return nil, err + } + // One transaction covering the task-level fields and every target: a + // failure must not leave the reviewer or prompt override committed while + // the per-MR switches roll back. + opts, err := store.UpdateTaskMRAutomationOptionsWithMRs( + ctx, taskID, patch, reviewerUsername, targets, patch.SwitchPatch(), + ) + if err != nil { + return nil, err + } + mrOptions, err := s.taskMRAutomationOptionsList(ctx, taskID) if err != nil { return nil, err } @@ -175,22 +298,97 @@ func (s *Service) UpdateTaskMRAutomationOptions(ctx context.Context, taskID stri if err != nil { return nil, err } - return s.taskMRAutomationResponseFromOptions(ctx, opts, states, workspaceID), nil + return s.taskMRAutomationResponseFromOptions(ctx, opts, mrOptions, states, workspaceID), nil +} + +// resolveTaskMRAutomationTargets resolves which linked MR(s) a patch applies +// to. No identity fans out to every linked MR; a named identity that is not +// currently linked is rejected rather than silently creating an orphan +// automation row. +func (s *Service) resolveTaskMRAutomationTargets( + ctx context.Context, taskID string, patch TaskMRAutomationPatch, +) ([]MRIdentity, error) { + store := s.requireStore() + if store == nil { + return nil, errStoreUnavailable + } + mrs, err := store.ListTaskMRsByTask(ctx, taskID) + if err != nil { + return nil, err + } + linked := make([]MRIdentity, 0, len(mrs)) + for _, mr := range mrs { + linked = append(linked, MRIdentity{ + RepositoryID: mr.RepositoryID, ProjectPath: mr.ProjectPath, MRIID: mr.MRIID, + }) + } + if !patch.HasMRIdentity() { + return linked, nil + } + wanted := patch.MRIdentity() + for _, id := range linked { + if id == wanted { + return []MRIdentity{id}, nil + } + } + return nil, fmt.Errorf("%w: project_path=%s mr_iid=%d", ErrTaskMRNotLinked, wanted.ProjectPath, wanted.MRIID) } -func (s *Service) resolveReviewerUsernameForPatch(ctx context.Context, taskID string, patch TaskMRAutomationPatch) (*string, error) { +// resolveReviewerUsernameForPatch resolves the task-level reviewer username +// implied by a review-request switch change. Disabling only blanks the shared +// username when no *other* linked MR still has the switch on — otherwise this +// would silently break that MR's automation. +func (s *Service) resolveReviewerUsernameForPatch( + ctx context.Context, taskID string, patch TaskMRAutomationPatch, targets []MRIdentity, +) (*string, error) { if patch.PromptOnReviewRequested == nil { return nil, nil } - if !*patch.PromptOnReviewRequested { - empty := "" - return &empty, nil + if *patch.PromptOnReviewRequested { + username, err := s.resolveAuthenticatedUsernameStrict(ctx, taskID) + if err != nil { + return nil, err + } + return &username, nil } - username, err := s.resolveAuthenticatedUsernameStrict(ctx, taskID) + stillNeeded, err := s.anyOtherMRHasReviewRequestEnabled(ctx, taskID, targets) if err != nil { return nil, err } - return &username, nil + if stillNeeded { + return nil, nil + } + empty := "" + return &empty, nil +} + +// anyOtherMRHasReviewRequestEnabled reports whether a linked MR outside +// `targets` still has prompt_on_review_requested on. +func (s *Service) anyOtherMRHasReviewRequestEnabled( + ctx context.Context, taskID string, targets []MRIdentity, +) (bool, error) { + store := s.requireStore() + if store == nil { + return false, errStoreUnavailable + } + stored, err := store.ListTaskMRAutomationOptions(ctx, taskID) + if err != nil { + return false, err + } + targeted := make(map[MRIdentity]struct{}, len(targets)) + for _, id := range targets { + targeted[id] = struct{}{} + } + for _, opt := range stored { + if !opt.PromptOnReviewRequested { + continue + } + if _, ok := targeted[opt.Identity()]; ok { + continue + } + return true, nil + } + return false, nil } func (s *Service) resolveAuthenticatedUsernameStrict(ctx context.Context, taskID string) (string, error) { @@ -208,18 +406,25 @@ func (s *Service) resolveAuthenticatedUsernameStrict(ctx context.Context, taskID return username, nil } -// HasEnabledTaskMRAgentPrompts reports whether any lifecycle switch is on, -// used by cleanup retention (AC24) and by the poll/evaluation gate (AC16). +// HasEnabledTaskMRAgentPrompts reports whether any MR linked to the task has +// a lifecycle switch on, used by cleanup retention (AC24) and by the +// poll/evaluation gate (AC16). Reads the per-MR table, so a task whose MRs +// are configured differently is still detected. func (s *Service) HasEnabledTaskMRAgentPrompts(ctx context.Context, taskID string) (bool, error) { store := s.requireStore() if store == nil { return false, errStoreUnavailable } - opts, err := store.GetTaskMRAutomationOptions(ctx, taskID) + stored, err := store.ListTaskMRAutomationOptions(ctx, taskID) if err != nil { return false, err } - return opts.PromptOnReviewRequested || opts.PromptOnMerged || opts.PromptOnClosed, nil + for _, opt := range stored { + if opt.PromptOnReviewRequested || opt.PromptOnMerged || opt.PromptOnClosed { + return true, nil + } + } + return false, nil } // RebindTaskMRReviewer re-resolves the workspace's authenticated GitLab diff --git a/apps/backend/internal/gitlab/service_mr_automation_test.go b/apps/backend/internal/gitlab/service_mr_automation_test.go index c15a51302f..c671726198 100644 --- a/apps/backend/internal/gitlab/service_mr_automation_test.go +++ b/apps/backend/internal/gitlab/service_mr_automation_test.go @@ -143,11 +143,12 @@ func TestHasEnabledTaskMRAgentPrompts(t *testing.T) { t.Fatalf("expected disabled by default: enabled=%v err=%v", enabled, err) } - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + // One linked MR with a lifecycle switch on is enough — the gate reads the + // per-MR table, so a task whose MRs are configured differently still + // counts as subscribed. + setMRSwitches(t, store, "task-1", mrIdentity("group/a", 1), TaskMRAutomationSwitchPatch{ PromptOnClosed: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable switch: %v", err) - } + }) enabled, err = svc.HasEnabledTaskMRAgentPrompts(ctx, "task-1") if err != nil || !enabled { t.Fatalf("expected enabled after patch: enabled=%v err=%v", enabled, err) @@ -171,6 +172,9 @@ func TestUpdateTaskMRAutomationOptions_ResolvesAndClearsReviewer(t *testing.T) { return mock, nil } ctx := context.Background() + if err := store.UpsertTaskMR(ctx, newTestMR("task-1", "", "group/a", 1)); err != nil { + t.Fatalf("upsert MR: %v", err) + } resp, err := svc.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ PromptOnReviewRequested: boolPtr(true), @@ -193,6 +197,209 @@ func TestUpdateTaskMRAutomationOptions_ResolvesAndClearsReviewer(t *testing.T) { } } +// newMRAutomationServiceFixture builds a workspace-configured service whose +// strict client resolves to `username`, the shape every targeting test below +// needs. +func newMRAutomationServiceFixture(t *testing.T, username string) (*Service, *Store) { + t.Helper() + store := newTestStore(t) + seedWorkspace(t, store, "ws-1") + seedTask(t, store, "task-1", "ws-1") + if err := store.SaveConfigForWorkspace(context.Background(), "ws-1", &GitLabConfig{ + Host: "https://gitlab.example.com", AuthMethod: AuthMethodPAT, + }); err != nil { + t.Fatalf("save config: %v", err) + } + secrets := &configTestSecrets{values: map[string]string{SecretKeyForWorkspace("ws-1"): "token"}} + svc := newWorkspaceConfigService(t, store, secrets) + mock := NewMockClient("https://gitlab.example.com") + mock.SetUser(username) + svc.workspaceClientFn = func(_ context.Context, _ *GitLabConfig, _ string) (Client, error) { + return mock, nil + } + return svc, store +} + +// TestUpdateTaskMRAutomationOptions_TargetsOnlyTheNamedMR is the regression +// this change exists for at the service layer: a patch carrying one MR's +// identity must not touch the task's other linked MRs. +func TestUpdateTaskMRAutomationOptions_TargetsOnlyTheNamedMR(t *testing.T) { + svc, store := newMRAutomationServiceFixture(t, "alice") + ctx := context.Background() + for _, mr := range []*TaskMR{ + newTestMR("task-1", "", "group/a", 1), + newTestMR("task-1", "", "group/b", 2), + } { + if err := store.UpsertTaskMR(ctx, mr); err != nil { + t.Fatalf("upsert MR %s: %v", mr.ProjectPath, err) + } + } + + resp, err := svc.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + RepositoryID: stringPtr(""), ProjectPath: stringPtr("group/a"), MRIID: intPtr(1), + AutoMergeEnabled: boolPtr(true), + }) + if err != nil { + t.Fatalf("UpdateTaskMRAutomationOptions: %v", err) + } + if len(resp.MROptions) != 2 { + t.Fatalf("expected one entry per linked MR, got %+v", resp.MROptions) + } + byProject := map[string]*TaskMRAutomationOptionsForMR{} + for _, opt := range resp.MROptions { + byProject[opt.ProjectPath] = opt + } + if !byProject["group/a"].AutoMergeEnabled { + t.Errorf("targeted MR did not get the switch: %+v", byProject["group/a"]) + } + if byProject["group/b"].AutoMergeEnabled { + t.Errorf("untargeted MR inherited the switch: %+v", byProject["group/b"]) + } + // The aggregate is "every linked MR", so a partially configured task + // must not report the switch as on. + if resp.AutoMergeEnabled { + t.Errorf("aggregate reported auto-merge on while only one of two MRs has it") + } +} + +// TestUpdateTaskMRAutomationOptions_FansOutWhenNoMRIsNamed preserves the +// behavior of MCP callers that have no MR identity to send. +func TestUpdateTaskMRAutomationOptions_FansOutWhenNoMRIsNamed(t *testing.T) { + svc, store := newMRAutomationServiceFixture(t, "alice") + ctx := context.Background() + for _, mr := range []*TaskMR{ + newTestMR("task-1", "", "group/a", 1), + newTestMR("task-1", "", "group/b", 2), + } { + if err := store.UpsertTaskMR(ctx, mr); err != nil { + t.Fatalf("upsert MR %s: %v", mr.ProjectPath, err) + } + } + + resp, err := svc.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + PromptOnMerged: boolPtr(true), + }) + if err != nil { + t.Fatalf("UpdateTaskMRAutomationOptions: %v", err) + } + for _, opt := range resp.MROptions { + if !opt.PromptOnMerged { + t.Errorf("MR %s missed the fan-out: %+v", opt.ProjectPath, opt) + } + } + if !resp.PromptOnMerged { + t.Errorf("aggregate should report on once every linked MR has it: %+v", resp) + } +} + +// TestUpdateTaskMRAutomationOptions_RollsBackMixedPatch keeps the task-level +// prompt override and per-MR switch write in one transaction. A failed switch +// write must not leave the prompt override committed while returning an error. +func TestUpdateTaskMRAutomationOptions_RollsBackMixedPatch(t *testing.T) { + svc, store := newMRAutomationServiceFixture(t, "alice") + ctx := context.Background() + if err := store.UpsertTaskMR(ctx, newTestMR("task-1", "", "group/a", 1)); err != nil { + t.Fatalf("upsert MR: %v", err) + } + if _, err := store.db.Exec(` + CREATE TRIGGER fail_mr_switch BEFORE UPDATE ON gitlab_task_mr_automation_options + BEGIN SELECT RAISE(ABORT, 'injected failure'); END`); err != nil { + t.Fatalf("create failure trigger: %v", err) + } + + _, err := svc.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + AutoFixPromptOverride: stringPtr("must rollback"), + AutoMergeEnabled: boolPtr(true), + }) + if err == nil { + t.Fatal("expected mixed patch to fail") + } + options, err := store.GetTaskMRAutomationOptions(ctx, "task-1") + if err != nil { + t.Fatalf("get task options: %v", err) + } + if options.AutoFixPromptOverride != nil { + t.Fatalf("failed mixed patch persisted prompt override: %+v", options) + } +} + +// TestUpdateTaskMRAutomationOptions_RejectsUnlinkedOrPartialIdentity keeps a +// caller mistake from silently creating an orphan automation row, or from +// being reinterpreted as a fan-out over every MR. +func TestUpdateTaskMRAutomationOptions_RejectsUnlinkedOrPartialIdentity(t *testing.T) { + svc, store := newMRAutomationServiceFixture(t, "alice") + ctx := context.Background() + if err := store.UpsertTaskMR(ctx, newTestMR("task-1", "", "group/a", 1)); err != nil { + t.Fatalf("upsert MR: %v", err) + } + + _, err := svc.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + RepositoryID: stringPtr(""), ProjectPath: stringPtr("group/nope"), MRIID: intPtr(42), + AutoFixEnabled: boolPtr(true), + }) + if !errors.Is(err, ErrTaskMRNotLinked) { + t.Fatalf("expected ErrTaskMRNotLinked for an unlinked MR, got %v", err) + } + + _, err = svc.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + ProjectPath: stringPtr("group/a"), AutoFixEnabled: boolPtr(true), + }) + if !errors.Is(err, ErrTaskMRIdentityIncomplete) { + t.Fatalf("expected ErrTaskMRIdentityIncomplete for a partial identity, got %v", err) + } + + stored, err := store.ListTaskMRAutomationOptions(ctx, "task-1") + if err != nil { + t.Fatalf("ListTaskMRAutomationOptions: %v", err) + } + if len(stored) != 0 { + t.Fatalf("a rejected patch still wrote rows: %+v", stored) + } +} + +// TestUpdateTaskMRAutomationOptions_KeepsReviewerWhileAnotherMRNeedsIt covers +// the one field the switches share: disabling review-request on one MR must +// not blank the task-level reviewer another MR's automation still depends on. +func TestUpdateTaskMRAutomationOptions_KeepsReviewerWhileAnotherMRNeedsIt(t *testing.T) { + svc, store := newMRAutomationServiceFixture(t, "alice") + ctx := context.Background() + for _, mr := range []*TaskMR{ + newTestMR("task-1", "", "group/a", 1), + newTestMR("task-1", "", "group/b", 2), + } { + if err := store.UpsertTaskMR(ctx, mr); err != nil { + t.Fatalf("upsert MR %s: %v", mr.ProjectPath, err) + } + } + if _, err := svc.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + PromptOnReviewRequested: boolPtr(true), + }); err != nil { + t.Fatalf("enable on both MRs: %v", err) + } + + resp, err := svc.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + RepositoryID: stringPtr(""), ProjectPath: stringPtr("group/a"), MRIID: intPtr(1), + PromptOnReviewRequested: boolPtr(false), + }) + if err != nil { + t.Fatalf("disable on one MR: %v", err) + } + if resp.ReviewReviewerUsername != "alice" { + t.Fatalf("reviewer cleared while another MR still needs it: %+v", resp) + } + + resp, err = svc.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + RepositoryID: stringPtr(""), ProjectPath: stringPtr("group/b"), MRIID: intPtr(2), + PromptOnReviewRequested: boolPtr(false), + }) + if err != nil { + t.Fatalf("disable on the last MR: %v", err) + } + if resp.ReviewReviewerUsername != "" { + t.Fatalf("expected reviewer cleared once no MR needs it, got %+v", resp) + } +} + func TestGetTaskMRAutomationEvaluation_UsesConfigUsernameAndExactCheckpoint(t *testing.T) { store := newTestStore(t) seedWorkspace(t, store, "ws-1") @@ -255,3 +462,73 @@ func TestGetTaskMRAutomationEvaluation_UsesConfigUsernameAndExactCheckpoint(t *t t.Fatalf("evaluation loaded task-wide MR states: %+v", evaluation.Options.MRStates) } } + +// TestGetTaskMRAutomationEvaluation_UsesOnlyTheTargetMRsSwitches is the +// orchestrator-facing half of the per-MR scoping: an evaluation for one MR +// must not see a sibling MR's enabled automation. +func TestGetTaskMRAutomationEvaluation_UsesOnlyTheTargetMRsSwitches(t *testing.T) { + svc, store := newMRAutomationServiceFixture(t, "alice") + ctx := context.Background() + for _, mr := range []*TaskMR{ + newTestMR("task-1", "", "group/a", 1), + newTestMR("task-1", "", "group/b", 2), + } { + if err := store.UpsertTaskMR(ctx, mr); err != nil { + t.Fatalf("upsert MR %s: %v", mr.ProjectPath, err) + } + } + setMRSwitches(t, store, "task-1", mrIdentity("group/a", 1), TaskMRAutomationSwitchPatch{ + AutoFixEnabled: boolPtr(true), PromptOnMerged: boolPtr(true), + }) + + configured, err := svc.GetTaskMRAutomationEvaluation(ctx, "task-1", "", "group/a", 1) + if err != nil { + t.Fatalf("evaluate configured MR: %v", err) + } + if !configured.Options.AutoFixEnabled || !configured.Options.PromptOnMerged { + t.Fatalf("configured MR lost its own switches: %+v", configured.Options) + } + + sibling, err := svc.GetTaskMRAutomationEvaluation(ctx, "task-1", "", "group/b", 2) + if err != nil { + t.Fatalf("evaluate sibling MR: %v", err) + } + if sibling.Options.AutoFixEnabled || sibling.Options.PromptOnMerged || + sibling.Options.AutoMergeEnabled || sibling.Options.PromptOnReviewRequested || + sibling.Options.PromptOnClosed { + t.Fatalf("sibling MR inherited another MR's automation: %+v", sibling.Options) + } +} + +// TestUpdateTaskMRAutomationOptions_RejectsSwitchesWithNoLinkedMRs covers the +// zero-target case. The switches live per-MR, so with nothing linked there is +// no row to write them to; returning success would report a write that stored +// nothing, and the caller would only find out when the automation it believed +// it had enabled never fired. +func TestUpdateTaskMRAutomationOptions_RejectsSwitchesWithNoLinkedMRs(t *testing.T) { + svc, _ := newMRAutomationServiceFixture(t, "alice") + + _, err := svc.UpdateTaskMRAutomationOptions(context.Background(), "task-1", TaskMRAutomationPatch{ + PromptOnMerged: boolPtr(true), + }) + if !errors.Is(err, ErrTaskMRNotLinked) { + t.Fatalf("expected ErrTaskMRNotLinked for a task with no linked MRs, got %v", err) + } +} + +// TestUpdateTaskMRAutomationOptions_AllowsPromptOverrideWithNoLinkedMRs is the +// other half of the same rule: the auto-fix prompt override is task-level, so +// it stays settable before any MR is linked. +func TestUpdateTaskMRAutomationOptions_AllowsPromptOverrideWithNoLinkedMRs(t *testing.T) { + svc, _ := newMRAutomationServiceFixture(t, "alice") + + resp, err := svc.UpdateTaskMRAutomationOptions(context.Background(), "task-1", TaskMRAutomationPatch{ + AutoFixPromptOverride: stringPtr("fix it please"), + }) + if err != nil { + t.Fatalf("prompt override with no linked MRs should succeed, got %v", err) + } + if resp.AutoFixPromptOverride == nil || *resp.AutoFixPromptOverride != "fix it please" { + t.Errorf("override not persisted: %+v", resp.AutoFixPromptOverride) + } +} diff --git a/apps/backend/internal/gitlab/store.go b/apps/backend/internal/gitlab/store.go index 4c90a8d75b..c71973a300 100644 --- a/apps/backend/internal/gitlab/store.go +++ b/apps/backend/internal/gitlab/store.go @@ -223,6 +223,11 @@ func (s *Store) createTables() error { if err := s.migrateMRAutomationAutomationColumns(); err != nil { return err } + // Must follow migrateMRAutomationAutomationColumns, which adds the + // mr_scope_migrated_at column this migration is guarded by. + if err := s.migrateTaskMROptionsToMRScope(); err != nil { + return err + } if err := s.migrateConfigRevision(); err != nil { return err } @@ -624,12 +629,15 @@ func (s *Store) ListTaskMRsByWorkspaceID(ctx context.Context, workspaceID string } // DeleteTaskMR removes a single task↔MR row, cascading to that MR's -// lifecycle checkpoint (gitlab_task_mr_state). Without this, re-linking the +// lifecycle checkpoint (gitlab_task_mr_state) and its per-MR automation +// switches (gitlab_task_mr_automation_options). Without this, re-linking the // same MR later would inherit the old checkpoint and could suppress its next -// lifecycle prompt — gitlab_task_mrs has no FK relationship to -// gitlab_task_mr_state (it's keyed by (task_id, repository_id, project_path, -// mr_iid), not by gitlab_task_mrs.id) for the database to cascade this -// automatically. +// lifecycle prompt, or silently re-arm a switch — including auto-merge — that +// no surface displays but the evaluator still reads. gitlab_task_mrs has no FK +// relationship to either table (both are keyed by (task_id, repository_id, +// project_path, mr_iid), not by gitlab_task_mrs.id) for the database to +// cascade this automatically. Keep in step with +// DeleteTaskMRForWorkspace, which performs the same three-table cleanup. func (s *Store) DeleteTaskMR(ctx context.Context, id string) error { tx, err := s.db.BeginTxx(ctx, nil) if err != nil { @@ -656,6 +664,12 @@ func (s *Store) DeleteTaskMR(ctx context.Context, id string) error { mr.TaskID, mr.RepositoryID, mr.ProjectPath, mr.MRIID); err != nil { return err } + if _, err := tx.ExecContext(ctx, + `DELETE FROM gitlab_task_mr_automation_options + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ?`, + mr.TaskID, mr.RepositoryID, mr.ProjectPath, mr.MRIID); err != nil { + return err + } if _, err := tx.ExecContext(ctx, `DELETE FROM gitlab_task_mrs WHERE id = ?`, id); err != nil { return err } diff --git a/apps/backend/internal/gitlab/store_e2e_reset_mr_automation_test.go b/apps/backend/internal/gitlab/store_e2e_reset_mr_automation_test.go index aa4e75fc57..eda1e0ed9d 100644 --- a/apps/backend/internal/gitlab/store_e2e_reset_mr_automation_test.go +++ b/apps/backend/internal/gitlab/store_e2e_reset_mr_automation_test.go @@ -3,19 +3,21 @@ package gitlab import "testing" // TestStoreResetWorkspaceE2E_ClearsMRAutomationState covers AC38-adjacent -// discipline for the E2E reset invariant (apps/backend/AGENTS.md): the two -// new MR automation tables must be wiped for the reset workspace and left -// untouched for other workspaces, ahead of the task rows they reference. +// discipline for the E2E reset invariant (apps/backend/AGENTS.md): every MR +// automation table must be wiped for the reset workspace and left untouched +// for other workspaces, ahead of the task rows they reference. func TestStoreResetWorkspaceE2E_ClearsMRAutomationState(t *testing.T) { store := newTestStore(t) for _, workspaceID := range []string{"ws-a", "ws-b"} { seedWorkspace(t, store, workspaceID) seedTask(t, store, "task-"+workspaceID, workspaceID) if _, err := store.UpdateTaskMRAutomationOptions(t.Context(), "task-"+workspaceID, TaskMRAutomationPatch{ - PromptOnMerged: boolPtr(true), + AutoFixPromptOverride: stringPtr("custom"), }, nil); err != nil { t.Fatalf("seed options %s: %v", workspaceID, err) } + setMRSwitches(t, store, "task-"+workspaceID, mrIdentity("group/project", 7), + TaskMRAutomationSwitchPatch{PromptOnMerged: boolPtr(true)}) if err := store.SetTaskMRObservedState(t.Context(), "task-"+workspaceID, "", "group/project", 7, "open"); err != nil { t.Fatalf("seed state %s: %v", workspaceID, err) } @@ -33,6 +35,7 @@ func assertMRAutomationRowCount(t *testing.T, store *Store, taskID string, want t.Helper() for _, query := range []string{ `SELECT COUNT(*) FROM gitlab_task_mr_options WHERE task_id = ?`, + `SELECT COUNT(*) FROM gitlab_task_mr_automation_options WHERE task_id = ?`, `SELECT COUNT(*) FROM gitlab_task_mr_state WHERE task_id = ?`, } { var got int diff --git a/apps/backend/internal/gitlab/store_mr_automation.go b/apps/backend/internal/gitlab/store_mr_automation.go index 6ffedd4e4a..6770492245 100644 --- a/apps/backend/internal/gitlab/store_mr_automation.go +++ b/apps/backend/internal/gitlab/store_mr_automation.go @@ -7,6 +7,8 @@ import ( "fmt" "strings" "time" + + "github.com/jmoiron/sqlx" ) const createMRAutomationTablesSQL = ` @@ -19,11 +21,35 @@ const createMRAutomationTablesSQL = ` prompt_on_merged BOOLEAN NOT NULL DEFAULT 0, prompt_on_closed BOOLEAN NOT NULL DEFAULT 0, review_reviewer_username TEXT NOT NULL DEFAULT '', + automation_revision INTEGER NOT NULL DEFAULT 0, + mr_scope_migrated_at DATETIME, created_at DATETIME NOT NULL, updated_at DATETIME NOT NULL, FOREIGN KEY (task_id) REFERENCES tasks(id) ON DELETE CASCADE ); + -- Per-MR automation switches. Source of truth for the five switches + -- (auto-fix, auto-merge, and the three lifecycle notifications) that + -- gitlab_task_mr_options used to hold task-wide. Keyed by the same + -- four-part MR identity as gitlab_task_mr_state, so a task with two + -- linked MRs configures each independently. See + -- migrateTaskMROptionsToMRScope. + CREATE TABLE IF NOT EXISTS gitlab_task_mr_automation_options ( + task_id TEXT NOT NULL, + repository_id TEXT NOT NULL DEFAULT '', + project_path TEXT NOT NULL, + mr_iid 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, project_path, mr_iid), + FOREIGN KEY (task_id) REFERENCES tasks(id) ON DELETE CASCADE + ); + CREATE TABLE IF NOT EXISTS gitlab_task_mr_state ( task_id TEXT NOT NULL, repository_id TEXT NOT NULL DEFAULT '', @@ -72,6 +98,10 @@ func (s *Store) migrateMRAutomationAutomationColumns() error { {"auto_fix_enabled", sqlBooleanDefaultFalse}, {"auto_merge_enabled", sqlBooleanDefaultFalse}, {"auto_fix_prompt_override", "TEXT"}, + {"automation_revision", sqlIntegerDefaultZero}, + // Guards the one-time fan-out of the legacy task-wide switches onto + // per-MR rows — see migrateTaskMROptionsToMRScope. + {"mr_scope_migrated_at", "DATETIME"}, } if err := addMissingColumns(s, "gitlab_task_mr_options", optionsColumns); err != nil { return err @@ -89,6 +119,89 @@ func (s *Store) migrateMRAutomationAutomationColumns() error { return addMissingColumns(s, "gitlab_task_mr_state", stateColumns) } +// legacyMROptionsRow is one pre-scope-migration gitlab_task_mr_options row, +// read only so its task-wide switch values can be fanned out onto the task's +// linked MRs exactly once. +type legacyMROptionsRow struct { + taskID string + autoFix, autoMerge, promptReview, promptMerged, promptClosed bool +} + +// migrateTaskMROptionsToMRScope seeds gitlab_task_mr_automation_options rows +// from each pre-upgrade gitlab_task_mr_options row's legacy switches, fanning +// each task's values out onto every gitlab_task_mrs row currently linked to +// it. Guarded by mr_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 the user has since turned off for one MR, and an MR +// linked to the task after migration would inherit the legacy value instead +// of starting all-off (which ON CONFLICT DO NOTHING alone cannot prevent). +// Mirrors github.Store.migrateTaskCIOptionsToPRScope. +func (s *Store) migrateTaskMROptionsToMRScope() error { + legacy, err := s.unmigratedMROptionRows() + if err != nil { + return err + } + for _, row := range legacy { + if err := s.fanOutMROptionsToMRScope(row); err != nil { + return err + } + } + return nil +} + +func (s *Store) unmigratedMROptionRows() ([]legacyMROptionsRow, error) { + rows, err := s.ro.Query(` + SELECT task_id, auto_fix_enabled, auto_merge_enabled, prompt_on_review_requested, + prompt_on_merged, prompt_on_closed + FROM gitlab_task_mr_options + WHERE mr_scope_migrated_at IS NULL`) + if err != nil { + return nil, fmt.Errorf("list unmigrated task MR options: %w", err) + } + defer func() { _ = rows.Close() }() + var legacy []legacyMROptionsRow + for rows.Next() { + var row legacyMROptionsRow + if err := rows.Scan(&row.taskID, &row.autoFix, &row.autoMerge, + &row.promptReview, &row.promptMerged, &row.promptClosed); err != nil { + return nil, fmt.Errorf("scan unmigrated task MR options: %w", err) + } + legacy = append(legacy, row) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("iterate unmigrated task MR options: %w", err) + } + return legacy, nil +} + +func (s *Store) fanOutMROptionsToMRScope(row legacyMROptionsRow) 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 gitlab_task_mr_automation_options ( + task_id, repository_id, project_path, mr_iid, 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, project_path, mr_iid, ?, ?, ?, ?, ?, ?, ? + FROM gitlab_task_mrs + WHERE task_id = ? + ON CONFLICT(task_id, repository_id, project_path, mr_iid) DO NOTHING`, + row.autoFix, row.autoMerge, row.promptReview, row.promptMerged, row.promptClosed, + now, now, row.taskID); err != nil { + return fmt.Errorf("fan out task MR options for %s: %w", row.taskID, err) + } + if _, err := tx.Exec( + `UPDATE gitlab_task_mr_options SET mr_scope_migrated_at = ? WHERE task_id = ?`, now, row.taskID, + ); err != nil { + return fmt.Errorf("stamp mr_scope_migrated_at for %s: %w", row.taskID, err) + } + return tx.Commit() +} + func addMissingColumns(s *Store, table string, columns []struct{ name, ddl string }) error { existing, err := s.tableColumns(table) if err != nil { @@ -112,7 +225,7 @@ type execContext interface { ExecContext(ctx context.Context, query string, args ...interface{}) (sql.Result, error) } -const mrAutomationOptionsSelectCols = `task_id, auto_fix_enabled, auto_merge_enabled, auto_fix_prompt_override, +const mrAutomationOptionsSelectCols = `task_id, automation_revision, auto_fix_enabled, auto_merge_enabled, auto_fix_prompt_override, prompt_on_review_requested, prompt_on_merged, prompt_on_closed, review_reviewer_username, created_at, updated_at` @@ -142,20 +255,23 @@ func boolPatchValue(value *bool) (bool, bool) { return true, *value } -// UpdateTaskMRAutomationOptions applies a partial update. Every column write -// is a single atomic UPDATE ... CASE WHEN THEN ELSE END statement — there is no read-modify-write step for -// the values themselves, so two concurrent PATCHes touching different fields -// cannot lose one side's change to the other's stale snapshot (this does NOT -// hold under a bare transaction + full-row read-then-upsert on PostgreSQL, -// which only guarantees isolation between statements a transaction actually -// executes, not implicit serialization of concurrent read-then-write pairs). +// UpdateTaskMRAutomationOptions applies a partial update to the genuinely +// task-level MR automation fields: the auto-fix prompt override and the +// server-resolved reviewer username. The five automation switches are +// per-MR — see UpdateTaskMRAutomationOptionsForMR. +// +// Every column write is a single atomic UPDATE ... CASE WHEN THEN ELSE END statement — there is no +// read-modify-write step for the values themselves, so two concurrent PATCHes +// touching different fields cannot lose one side's change to the other's +// stale snapshot (this does NOT hold under a bare transaction + full-row +// read-then-upsert on PostgreSQL, which only guarantees isolation between +// statements a transaction actually executes, not implicit serialization of +// concurrent read-then-write pairs). // // The one read this still performs (of the pre-patch row) exists solely to -// decide whether the review-request baseline and per-event terminal -// checkpoints need resetting — a stale read there only risks a redundant or -// skipped reset on a genuinely concurrent flip of the exact same field, -// never a lost field write. +// detect a reviewer-identity change, which invalidates every linked MR's +// review-request baseline at once. func (s *Store) UpdateTaskMRAutomationOptions( ctx context.Context, taskID string, patch TaskMRAutomationPatch, reviewerUsername *string, ) (*TaskMRAutomationOptions, error) { @@ -164,82 +280,279 @@ func (s *Store) UpdateTaskMRAutomationOptions( return nil, err } defer func() { _ = tx.Rollback() }() + if err := updateTaskMRAutomationOptionsTx(ctx, tx, taskID, patch, reviewerUsername); err != nil { + return nil, err + } + if err := tx.Commit(); err != nil { + return nil, err + } + return s.GetTaskMRAutomationOptions(ctx, taskID) +} + +// UpdateTaskMRAutomationOptionsWithMRs applies task-level fields and one +// per-MR switch patch in the same transaction. A mixed PATCH must either +// commit both kinds of settings or leave neither behind. +func (s *Store) UpdateTaskMRAutomationOptionsWithMRs( + ctx context.Context, taskID string, patch TaskMRAutomationPatch, reviewerUsername *string, + ids []MRIdentity, switches TaskMRAutomationSwitchPatch, +) (*TaskMRAutomationOptions, error) { + tx, err := s.db.BeginTxx(ctx, nil) + if err != nil { + return nil, err + } + defer func() { _ = tx.Rollback() }() + if err := updateTaskMRAutomationOptionsTx(ctx, tx, taskID, patch, reviewerUsername); err != nil { + return nil, err + } + if err := applyMRSwitchPatchBatchTx(ctx, tx, taskID, ids, switches); err != nil { + return nil, err + } + if err := tx.Commit(); err != nil { + return nil, err + } + return s.GetTaskMRAutomationOptions(ctx, taskID) +} +// updateTaskMRAutomationOptionsTx applies the task-level half of an +// automation patch in its caller's transaction. +func updateTaskMRAutomationOptionsTx( + ctx context.Context, tx *sqlx.Tx, taskID string, patch TaskMRAutomationPatch, reviewerUsername *string, +) error { now := time.Now().UTC() if _, err := tx.ExecContext(ctx, ` INSERT INTO gitlab_task_mr_options (task_id, created_at, updated_at) VALUES (?, ?, ?) ON CONFLICT(task_id) DO NOTHING`, taskID, now, now); err != nil { - return nil, err + return err } var previous TaskMRAutomationOptions if err := tx.GetContext(ctx, &previous, `SELECT `+mrAutomationOptionsSelectCols+` FROM gitlab_task_mr_options WHERE task_id = ?`, taskID); err != nil { - return nil, err + return err } - fields := mrAutomationPatchFields(patch, reviewerUsername) + promptSet := patch.AutoFixPromptOverride != nil + promptValue := normalizedMRPromptOverride(patch.AutoFixPromptOverride) + reviewerSet := reviewerUsername != nil + reviewerValue := "" + if reviewerSet { + reviewerValue = *reviewerUsername + } if _, err := tx.ExecContext(ctx, ` UPDATE gitlab_task_mr_options SET + auto_fix_prompt_override = CASE WHEN ? THEN ? ELSE auto_fix_prompt_override END, + review_reviewer_username = CASE WHEN ? THEN ? ELSE review_reviewer_username END, + automation_revision = automation_revision + 1, + updated_at = ? + WHERE task_id = ?`, + promptSet, promptValue, reviewerSet, reviewerValue, now, taskID); err != nil { + return err + } + // A changed connected GitLab account invalidates every linked MR's + // review-request baseline, not just one MR's: a baseline recorded against + // the old identity would otherwise survive and could suppress or misfire + // the next prompt evaluated against the new one. + if reviewerSet && previous.ReviewReviewerUsername != reviewerValue { + if err := resetReviewBaselinesForTask(ctx, tx, taskID); err != nil { + return err + } + } + return nil +} + +const mrAutomationSwitchSelectCols = `task_id, repository_id, project_path, mr_iid, + auto_fix_enabled, auto_merge_enabled, prompt_on_review_requested, + prompt_on_merged, prompt_on_closed, created_at, updated_at` + +// GetTaskMRAutomationOptionsForMR returns one linked MR's automation +// switches, or all-off defaults when nothing has been persisted for it yet. +func (s *Store) GetTaskMRAutomationOptionsForMR( + ctx context.Context, taskID string, id MRIdentity, +) (*TaskMRAutomationOptionsForMR, error) { + var row TaskMRAutomationOptionsForMR + err := s.ro.GetContext(ctx, &row, ` + SELECT `+mrAutomationSwitchSelectCols+` + FROM gitlab_task_mr_automation_options + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ?`, + taskID, id.RepositoryID, id.ProjectPath, id.MRIID) + if errors.Is(err, sql.ErrNoRows) { + return &TaskMRAutomationOptionsForMR{ + TaskID: taskID, RepositoryID: id.RepositoryID, + ProjectPath: id.ProjectPath, MRIID: id.MRIID, + }, nil + } + if err != nil { + return nil, err + } + return &row, nil +} + +// ListTaskMRAutomationOptions returns every stored per-MR switch row for a task. +func (s *Store) ListTaskMRAutomationOptions(ctx context.Context, taskID string) ([]*TaskMRAutomationOptionsForMR, error) { + var rows []TaskMRAutomationOptionsForMR + if err := s.ro.SelectContext(ctx, &rows, ` + SELECT `+mrAutomationSwitchSelectCols+` + FROM gitlab_task_mr_automation_options + WHERE task_id = ? ORDER BY project_path ASC, mr_iid ASC`, taskID); err != nil { + return nil, err + } + out := make([]*TaskMRAutomationOptionsForMR, 0, len(rows)) + for i := range rows { + out = append(out, &rows[i]) + } + return out, nil +} + +// UpdateTaskMRAutomationOptionsForMR applies a partial update to one linked +// MR's five automation switches, upserting the row when absent. It carries +// the same atomic CASE-WHEN write and single pre-patch read as +// UpdateTaskMRAutomationOptions; here the read decides which of that MR's +// checkpoints need resetting. +func (s *Store) UpdateTaskMRAutomationOptionsForMR( + ctx context.Context, taskID string, id MRIdentity, patch TaskMRAutomationSwitchPatch, +) (*TaskMRAutomationOptionsForMR, error) { + if err := s.UpdateTaskMRAutomationOptionsForMRs(ctx, taskID, []MRIdentity{id}, patch); err != nil { + return nil, err + } + return s.GetTaskMRAutomationOptionsForMR(ctx, taskID, id) +} + +// UpdateTaskMRAutomationOptionsForMRs applies one switch patch to every +// identity in ids inside a SINGLE transaction. The fan-out case (a PATCH or +// MCP call that names no MR, so the change applies to every linked MR) must +// not be a loop of independent transactions: a failure partway through would +// leave the switch armed on the MRs already committed while returning an +// error to the caller, and auto-merge would then act on those MRs even though +// the operation reported failure. All-or-nothing instead. +func (s *Store) UpdateTaskMRAutomationOptionsForMRs( + ctx context.Context, taskID string, ids []MRIdentity, patch TaskMRAutomationSwitchPatch, +) error { + if len(ids) == 0 { + return nil + } + tx, err := s.db.BeginTxx(ctx, nil) + if err != nil { + return err + } + defer func() { _ = tx.Rollback() }() + if err := applyMRSwitchPatchBatchTx(ctx, tx, taskID, ids, patch); err != nil { + return err + } + return tx.Commit() +} + +// applyMRSwitchPatchBatchTx applies a switch patch to every identity inside +// an existing transaction so task-level and per-MR changes can commit together. +func applyMRSwitchPatchBatchTx( + ctx context.Context, tx *sqlx.Tx, taskID string, ids []MRIdentity, patch TaskMRAutomationSwitchPatch, +) error { + if !patch.HasAny() { + return nil + } + now := time.Now().UTC() + fields := mrAutomationSwitchFields(patch) + for _, id := range ids { + if err := applyMRSwitchPatchTx(ctx, tx, taskID, id, now, fields); err != nil { + return err + } + } + return nil +} + +// applyMRSwitchPatchTx upserts one MR's switch row, applies the patch, and +// runs that MR's checkpoint resets — all against the caller's transaction, so +// several MRs can be updated atomically. Split out of +// UpdateTaskMRAutomationOptionsForMRs to keep it under the function-length +// limit. +func applyMRSwitchPatchTx( + ctx context.Context, tx *sqlx.Tx, taskID string, id MRIdentity, + now time.Time, fields mrAutomationSwitchPatchFields, +) error { + if err := ensureTaskMRLinkTx(ctx, tx, taskID, id); err != nil { + return err + } + if _, err := tx.ExecContext(ctx, ` + INSERT INTO gitlab_task_mr_automation_options ( + task_id, repository_id, project_path, mr_iid, created_at, updated_at + ) VALUES (?, ?, ?, ?, ?, ?) + ON CONFLICT(task_id, repository_id, project_path, mr_iid) DO NOTHING`, + taskID, id.RepositoryID, id.ProjectPath, id.MRIID, now, now); err != nil { + return err + } + var previous TaskMRAutomationOptionsForMR + if err := tx.GetContext(ctx, &previous, ` + SELECT `+mrAutomationSwitchSelectCols+` + FROM gitlab_task_mr_automation_options + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ?`, + taskID, id.RepositoryID, id.ProjectPath, id.MRIID); err != nil { + return err + } + if _, err := tx.ExecContext(ctx, ` + UPDATE gitlab_task_mr_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, - auto_fix_prompt_override = CASE WHEN ? THEN ? ELSE auto_fix_prompt_override 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, - review_reviewer_username = CASE WHEN ? THEN ? ELSE review_reviewer_username END, updated_at = ? - WHERE task_id = ?`, + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ?`, fields.autoFixSet, fields.autoFixValue, fields.autoMergeSet, fields.autoMergeValue, - fields.promptSet, fields.promptValue, fields.reviewSet, fields.reviewValue, fields.mergedSet, fields.mergedValue, - fields.closedSet, fields.closedValue, fields.reviewerSet, fields.reviewerValue, - now, taskID); err != nil { - return nil, err + fields.closedSet, fields.closedValue, + now, taskID, id.RepositoryID, id.ProjectPath, id.MRIID); err != nil { + return err } - if err := applyMRAutomationOptionResets(ctx, tx, taskID, now, previous, fields); err != nil { - return nil, err + return applyMRAutomationOptionResets(ctx, tx, taskID, id, now, previous, fields) +} + +func ensureTaskMRLinkTx(ctx context.Context, tx *sqlx.Tx, taskID string, id MRIdentity) error { + // A plain SELECT is not enough here. PostgreSQL's READ COMMITTED isolation + // allows an unlink to delete the association after the read and before the + // options INSERT, which leaves an orphan row that can re-arm on relink. + // The no-op UPDATE takes a write lock on the association row and is portable + // across PostgreSQL and SQLite. The unlink transaction must wait until this + // transaction commits, then removes the association and its options. + result, err := tx.ExecContext(ctx, ` + UPDATE gitlab_task_mrs SET updated_at = updated_at + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ?`, + taskID, id.RepositoryID, id.ProjectPath, id.MRIID) + if err != nil { + return err } - if err := tx.Commit(); err != nil { - return nil, err + linked, err := result.RowsAffected() + if err != nil { + return err } - return s.GetTaskMRAutomationOptions(ctx, taskID) + if linked == 0 { + return fmt.Errorf("%w: project_path=%s mr_iid=%d", ErrTaskMRNotLinked, id.ProjectPath, id.MRIID) + } + return nil } -// mrAutomationOptionsPatchFields flattens an options patch (plus the -// server-resolved reviewer username) into the "was this field present, what -// value" pairs both the atomic UPDATE and the reset-decision logic need. -type mrAutomationOptionsPatchFields struct { +// mrAutomationSwitchPatchFields flattens a switch patch into the "was this +// field present, what value" pairs both the atomic UPDATE and the +// reset-decision logic need. +type mrAutomationSwitchPatchFields struct { autoFixSet, autoFixValue bool autoMergeSet, autoMergeValue bool - promptSet bool - promptValue *string reviewSet, reviewValue bool mergedSet, mergedValue bool closedSet, closedValue bool - reviewerSet bool - reviewerValue string } -func mrAutomationPatchFields(patch TaskMRAutomationPatch, reviewerUsername *string) mrAutomationOptionsPatchFields { +func mrAutomationSwitchFields(patch TaskMRAutomationSwitchPatch) mrAutomationSwitchPatchFields { 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) - reviewerValue := "" - if reviewerUsername != nil { - reviewerValue = *reviewerUsername - } - return mrAutomationOptionsPatchFields{ + return mrAutomationSwitchPatchFields{ autoFixSet: autoFixSet, autoFixValue: autoFixValue, autoMergeSet: autoMergeSet, autoMergeValue: autoMergeValue, - promptSet: patch.AutoFixPromptOverride != nil, promptValue: normalizedMRPromptOverride(patch.AutoFixPromptOverride), reviewSet: reviewSet, reviewValue: reviewValue, mergedSet: mergedSet, mergedValue: mergedValue, closedSet: closedSet, closedValue: closedValue, - reviewerSet: reviewerUsername != nil, reviewerValue: reviewerValue, } } @@ -253,36 +566,28 @@ func normalizedMRPromptOverride(override *string) *string { return override } -// applyMRAutomationOptionResets resets the review-request baseline, a -// terminal event's checkpoint, or the auto-fix round-cap state when the -// corresponding switch actually changed value against the pre-patch row. -// Mirrors GitHub's applyTaskCIOptionResets. +// applyMRAutomationOptionResets resets the targeted MR's review-request +// baseline, a terminal event's checkpoint, or the auto-fix round-cap state +// when the corresponding switch actually changed value against the pre-patch +// row. Every reset is scoped to the one MR being patched, so reconfiguring +// one linked MR never re-arms another's checkpoints. Mirrors GitHub's +// applyTaskPRAutomationOptionResets. func applyMRAutomationOptionResets( - ctx context.Context, tx execContext, taskID string, now time.Time, - previous TaskMRAutomationOptions, fields mrAutomationOptionsPatchFields, + ctx context.Context, tx execContext, taskID string, id MRIdentity, now time.Time, + previous TaskMRAutomationOptionsForMR, fields mrAutomationSwitchPatchFields, ) error { - // Reset on either a boolean flip or a reviewer-identity change: a patch - // that resends prompt_on_review_requested=true while it was already true - // still re-resolves the authenticated username - // (resolveReviewerUsernameForPatch), which can differ from the stored one - // after the workspace's connected GitLab account changes. Without the - // second condition, a baseline recorded against the old identity would - // survive and could suppress or misfire the next prompt evaluated - // against the new one. - reviewChanged := (fields.reviewSet && previous.PromptOnReviewRequested != fields.reviewValue) || - (fields.reviewerSet && previous.ReviewReviewerUsername != fields.reviewerValue) - if reviewChanged { - if err := resetReviewBaselinesForTask(ctx, tx, taskID); err != nil { + if fields.reviewSet && previous.PromptOnReviewRequested != fields.reviewValue { + if err := resetReviewBaselineForMR(ctx, tx, taskID, id); err != nil { return err } } - if err := resetMRTerminalCheckpointsOnReenable(ctx, tx, taskID, now, previous, fields); err != nil { + if err := resetMRTerminalCheckpointsOnReenable(ctx, tx, taskID, id, now, previous, fields); err != nil { return err } // Re-enabling auto-fix after it was off must clear the round cap and // exhaustion so a fresh evaluation pass can dispatch again (AC11). if fields.autoFixSet && fields.autoFixValue && !previous.AutoFixEnabled { - if err := resetMRAutoFixState(ctx, tx, taskID, now); err != nil { + if err := resetMRAutoFixState(ctx, tx, taskID, id, now); err != nil { return err } } @@ -296,27 +601,27 @@ func applyMRAutomationOptionResets( // applyMRAutomationOptionResets to keep that function under the cyclomatic // complexity limit. func resetMRTerminalCheckpointsOnReenable( - ctx context.Context, tx execContext, taskID string, now time.Time, - previous TaskMRAutomationOptions, fields mrAutomationOptionsPatchFields, + ctx context.Context, tx execContext, taskID string, id MRIdentity, now time.Time, + previous TaskMRAutomationOptionsForMR, fields mrAutomationSwitchPatchFields, ) error { if fields.mergedSet && fields.mergedValue && !previous.PromptOnMerged { - if err := resetMRTerminalCheckpoint(ctx, tx, taskID, gitlabStateMerged, now); err != nil { + if err := resetMRTerminalCheckpoint(ctx, tx, taskID, id, gitlabStateMerged, now); err != nil { return err } } if fields.closedSet && fields.closedValue && !previous.PromptOnClosed { - if err := resetMRTerminalCheckpoint(ctx, tx, taskID, gitlabStateClosed, now); err != nil { + if err := resetMRTerminalCheckpoint(ctx, tx, taskID, id, gitlabStateClosed, now); err != nil { return err } } return nil } -// resetMRAutoFixState clears every MR auto-fix round-cap/checkpoint column -// for a task, mirroring GitHub's resetTaskCIAutoFixState. A previously -// recorded exhaustion error is cleared along with it; any other kind of -// error (sync, merge) is left untouched. -func resetMRAutoFixState(ctx context.Context, exec execContext, taskID string, now time.Time) error { +// resetMRAutoFixState clears one MR's auto-fix round-cap/checkpoint columns, +// mirroring GitHub's resetTaskCIAutoFixState. A previously recorded +// exhaustion error is cleared along with it; any other kind of error (sync, +// merge) is left untouched. +func resetMRAutoFixState(ctx context.Context, exec execContext, taskID string, id MRIdentity, now time.Time) error { _, err := exec.ExecContext(ctx, ` UPDATE gitlab_task_mr_state SET auto_fix_round_count = 0, @@ -327,7 +632,8 @@ func resetMRAutoFixState(ctx context.Context, exec execContext, taskID string, n 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 = ?`, now, taskID) + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ?`, + now, taskID, id.RepositoryID, id.ProjectPath, id.MRIID) return err } @@ -553,6 +859,10 @@ func (s *Store) RebindTaskMRReviewer(ctx context.Context, taskID, username strin return true, nil } +// resetReviewBaselinesForTask clears every linked MR's review-request +// baseline. Reserved for a change of the task-level reviewer identity, which +// invalidates all of them at once; a single MR's switch flip goes through +// resetReviewBaselineForMR instead. func resetReviewBaselinesForTask(ctx context.Context, exec execContext, taskID string) error { _, err := exec.ExecContext(ctx, ` UPDATE gitlab_task_mr_state @@ -561,12 +871,24 @@ func resetReviewBaselinesForTask(ctx context.Context, exec execContext, taskID s return err } -// resetMRTerminalCheckpoint clears the terminal checkpoint for a task's MR -// rows currently observed in (or last recorded as) the given terminal state, -// so a switch re-enabled after being off can re-evaluate and re-fire for an -// MR that reached that state while the switch was disabled. Matches GitHub's -// resetTaskCITerminalCheckpoint. -func resetMRTerminalCheckpoint(ctx context.Context, exec execContext, taskID, state string, now time.Time) error { +// resetReviewBaselineForMR clears one linked MR's review-request baseline. +func resetReviewBaselineForMR(ctx context.Context, exec execContext, taskID string, id MRIdentity) error { + _, err := exec.ExecContext(ctx, ` + UPDATE gitlab_task_mr_state + SET review_request_initialized = 0, last_review_requested = 0, updated_at = ? + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ?`, + time.Now().UTC(), taskID, id.RepositoryID, id.ProjectPath, id.MRIID) + return err +} + +// resetMRTerminalCheckpoint clears the terminal checkpoint for one MR when it +// is currently observed in (or was last recorded as) the given terminal +// state, so a switch re-enabled after being off can re-evaluate and re-fire +// for an MR that reached that state while the switch was disabled. Matches +// GitHub's resetTaskCITerminalCheckpoint. +func resetMRTerminalCheckpoint( + ctx context.Context, exec execContext, taskID string, id MRIdentity, state string, now time.Time, +) error { _, err := exec.ExecContext(ctx, ` UPDATE gitlab_task_mr_state SET last_observed_state = '', @@ -574,21 +896,28 @@ func resetMRTerminalCheckpoint(ctx context.Context, exec execContext, taskID, st last_lifecycle_prompt_at = NULL, last_lifecycle_session_id = NULL, updated_at = ? - WHERE task_id = ? AND (last_observed_state = ? OR last_lifecycle_event = ?)`, - now, taskID, state, state) + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ? + AND (last_observed_state = ? OR last_lifecycle_event = ?)`, + now, taskID, id.RepositoryID, id.ProjectPath, id.MRIID, state, state) return err } // ListAutomationSubscribedTaskMRs returns every linked MR (gitlab_task_mrs -// row) whose task has at least one lifecycle switch OR auto-fix OR -// auto-merge enabled. Drives the poller's sync pass (AC22); widened from -// the #2125 lifecycle-only ListLifecycleSubscribedTaskMRs so auto-fix and -// auto-merge get evaluated on the same poll without a second query. +// row) that has at least one lifecycle switch OR auto-fix OR auto-merge +// enabled. Drives the poller's sync pass (AC22); widened from the #2125 +// lifecycle-only ListLifecycleSubscribedTaskMRs so auto-fix and auto-merge +// get evaluated on the same poll without a second query. The join is on the +// full MR identity, not just task_id, so a task's unconfigured MRs are not +// polled just because a sibling MR has a switch on. func (s *Store) ListAutomationSubscribedTaskMRs(ctx context.Context) ([]*TaskMR, error) { var mrs []TaskMR if err := s.ro.SelectContext(ctx, &mrs, ` SELECT `+taskMRSelectColsQualified+` FROM gitlab_task_mrs gtm - INNER JOIN gitlab_task_mr_options o ON o.task_id = gtm.task_id + INNER JOIN gitlab_task_mr_automation_options o + ON o.task_id = gtm.task_id + AND o.repository_id = gtm.repository_id + AND o.project_path = gtm.project_path + AND o.mr_iid = gtm.mr_iid WHERE o.prompt_on_review_requested = 1 OR o.prompt_on_merged = 1 OR o.prompt_on_closed = 1 OR o.auto_fix_enabled = 1 OR o.auto_merge_enabled = 1 ORDER BY gtm.created_at ASC`); err != nil { @@ -636,7 +965,9 @@ func (s *Store) RecordTaskMRFixAttempt(ctx context.Context, attempt TaskMRFixAtt } // RefreshTaskMRFixCheckpoint updates the current feedback checkpoint -// without recording a new prompt dispatch (the empty-delta path — AC7). +// without recording a new prompt dispatch (the empty-delta path — AC7). It +// is called only at the start of a fresh auto-fix cycle, so replacing a +// conflicting row intentionally clears its stale enqueue/session markers. // Mirrors GitHub's RefreshTaskCIFixCheckpoint. func (s *Store) RefreshTaskMRFixCheckpoint(ctx context.Context, taskID, repositoryID, projectPath string, mrIID int, signature, checkpointJSON string) error { ctx = context.WithoutCancel(ctx) @@ -717,6 +1048,11 @@ func (s *Store) deleteMRAutomationForWorkspace(ctx context.Context, tx execConte (SELECT id FROM tasks WHERE workspace_id = ?)`, workspaceID); err != nil { return fmt.Errorf("delete gitlab_task_mr_state: %w", err) } + if _, err := tx.ExecContext(ctx, ` + DELETE FROM gitlab_task_mr_automation_options WHERE task_id IN + (SELECT id FROM tasks WHERE workspace_id = ?)`, workspaceID); err != nil { + return fmt.Errorf("delete gitlab_task_mr_automation_options: %w", err) + } if _, err := tx.ExecContext(ctx, ` DELETE FROM gitlab_task_mr_options WHERE task_id IN (SELECT id FROM tasks WHERE workspace_id = ?)`, workspaceID); err != nil { diff --git a/apps/backend/internal/gitlab/store_mr_automation_link_test.go b/apps/backend/internal/gitlab/store_mr_automation_link_test.go new file mode 100644 index 0000000000..215fe42f2d --- /dev/null +++ b/apps/backend/internal/gitlab/store_mr_automation_link_test.go @@ -0,0 +1,42 @@ +package gitlab + +import ( + "context" + "strings" + "testing" +) + +func TestStore_UpdateTaskMRAutomationOptionsForMR_LocksLinkBeforeWritingOptions(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedTask(t, store, "task-1", "") + id := mrIdentity("group/linked", 7) + if err := store.UpsertTaskMR(ctx, newTestMR("task-1", "", id.ProjectPath, id.MRIID)); err != nil { + t.Fatalf("link MR: %v", err) + } + + // A concurrent unlink must be serialized behind the link validation write. + // This trigger stands in for a database-side conflict: the old read-only + // validation never touches the trigger and would incorrectly create an + // options row, while the locking validation surfaces the conflict before + // it can write one. + if _, err := store.db.Exec(` + CREATE TRIGGER reject_link_update BEFORE UPDATE ON gitlab_task_mrs + BEGIN SELECT RAISE(ABORT, 'simulated concurrent unlink'); END`); err != nil { + t.Fatalf("create link conflict trigger: %v", err) + } + + _, err := store.UpdateTaskMRAutomationOptionsForMR( + ctx, "task-1", id, TaskMRAutomationSwitchPatch{AutoMergeEnabled: boolPtr(true)}, + ) + if err == nil || !strings.Contains(err.Error(), "simulated concurrent unlink") { + t.Fatalf("expected link conflict before options write, got %v", err) + } + options, err := store.ListTaskMRAutomationOptions(ctx, "task-1") + if err != nil { + t.Fatalf("list options: %v", err) + } + if len(options) != 0 { + t.Fatalf("link conflict created automation options: %+v", options) + } +} diff --git a/apps/backend/internal/gitlab/store_mr_automation_test.go b/apps/backend/internal/gitlab/store_mr_automation_test.go index 602b08e7eb..af1041ec1f 100644 --- a/apps/backend/internal/gitlab/store_mr_automation_test.go +++ b/apps/backend/internal/gitlab/store_mr_automation_test.go @@ -2,6 +2,7 @@ package gitlab import ( "context" + "errors" "path/filepath" "testing" "time" @@ -13,36 +14,140 @@ import ( func boolPtr(b bool) *bool { return &b } func stringPtr(s string) *string { return &s } +func intPtr(i int) *int { return &i } -// TestStore_UpdateTaskMRAutomationOptions_AutoMergeAndPromptOverrideRoundTrip -// closes a coverage gap: auto_fix_enabled is exercised indirectly by -// TestStore_UpdateTaskMRAutomationOptions_ReenablingAutoFixResetsRoundCap, -// but nothing wrote auto_merge_enabled or auto_fix_prompt_override and read -// them back, including the empty-string-normalizes-to-NULL path -// (normalizedMRPromptOverride). -func TestStore_UpdateTaskMRAutomationOptions_AutoMergeAndPromptOverrideRoundTrip(t *testing.T) { +// mrIdentity builds the single-repo MR identity most of these tests use. +func mrIdentity(projectPath string, iid int) MRIdentity { + return MRIdentity{ProjectPath: projectPath, MRIID: iid} +} + +// setMRSwitches is the per-MR switch write these tests exercise, kept short +// so the assertions stay the focus. +func setMRSwitches( + t *testing.T, store *Store, taskID string, id MRIdentity, patch TaskMRAutomationSwitchPatch, +) *TaskMRAutomationOptionsForMR { + t.Helper() + mrs, err := store.ListTaskMRsByTask(context.Background(), taskID) + if err != nil { + t.Fatalf("list linked MRs: %v", err) + } + linked := false + for _, mr := range mrs { + if (MRIdentity{RepositoryID: mr.RepositoryID, ProjectPath: mr.ProjectPath, MRIID: mr.MRIID}) == id { + linked = true + break + } + } + if !linked { + if err := store.UpsertTaskMR(context.Background(), newTestMR(taskID, id.RepositoryID, id.ProjectPath, id.MRIID)); err != nil { + t.Fatalf("link MR for switch write: %v", err) + } + } + got, err := store.UpdateTaskMRAutomationOptionsForMR(context.Background(), taskID, id, patch) + if err != nil { + t.Fatalf("UpdateTaskMRAutomationOptionsForMR(%s, %+v): %v", taskID, id, err) + } + return got +} + +func TestStore_UpdateTaskMRAutomationOptionsForMR_RejectsUnlinkedMR(t *testing.T) { store := newTestStore(t) ctx := context.Background() seedTask(t, store, "task-1", "") + id := mrIdentity("group/not-linked", 99) - updated, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ - AutoMergeEnabled: boolPtr(true), - }, nil) + _, err := store.UpdateTaskMRAutomationOptionsForMR( + ctx, "task-1", id, TaskMRAutomationSwitchPatch{AutoMergeEnabled: boolPtr(true)}, + ) + if !errors.Is(err, ErrTaskMRNotLinked) { + t.Fatalf("expected ErrTaskMRNotLinked, got %v", err) + } + options, err := store.ListTaskMRAutomationOptions(ctx, "task-1") if err != nil { - t.Fatalf("enable auto-merge: %v", err) + t.Fatalf("list options: %v", err) } + if len(options) != 0 { + t.Fatalf("unlinked write created automation options: %+v", options) + } +} + +// TestStore_UpdateTaskMRAutomationOptionsForMR_AutoMergeRoundTrip closes a +// coverage gap: auto_fix_enabled is exercised indirectly by +// TestStore_UpdateTaskMRAutomationOptionsForMR_ReenablingAutoFixResetsRoundCap, +// but nothing wrote auto_merge_enabled and read it back. +func TestStore_UpdateTaskMRAutomationOptionsForMR_AutoMergeRoundTrip(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedTask(t, store, "task-1", "") + id := mrIdentity("group/a", 1) + + updated := setMRSwitches(t, store, "task-1", id, TaskMRAutomationSwitchPatch{ + AutoMergeEnabled: boolPtr(true), + }) if !updated.AutoMergeEnabled { t.Fatalf("AutoMergeEnabled = false immediately after patch, want true") } - got, err := store.GetTaskMRAutomationOptions(ctx, "task-1") + got, err := store.GetTaskMRAutomationOptionsForMR(ctx, "task-1", id) if err != nil { - t.Fatalf("GetTaskMRAutomationOptions: %v", err) + t.Fatalf("GetTaskMRAutomationOptionsForMR: %v", err) } if !got.AutoMergeEnabled { t.Fatalf("AutoMergeEnabled = false after persisted read-back, want true") } - updated, err = store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + // An unrelated per-MR patch must not disturb it. + setMRSwitches(t, store, "task-1", id, TaskMRAutomationSwitchPatch{PromptOnClosed: boolPtr(true)}) + got, err = store.GetTaskMRAutomationOptionsForMR(ctx, "task-1", id) + if err != nil { + t.Fatalf("GetTaskMRAutomationOptionsForMR: %v", err) + } + if !got.AutoMergeEnabled || !got.PromptOnClosed { + t.Fatalf("expected both switches on after independent patches, got %+v", got) + } +} + +// TestStore_UpdateTaskMRAutomationOptionsForMR_ScopesSwitchesPerMR is the +// regression this change exists for: configuring one linked MR must leave +// every other MR on the same task untouched. +func TestStore_UpdateTaskMRAutomationOptionsForMR_ScopesSwitchesPerMR(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedTask(t, store, "task-1", "") + first := mrIdentity("group/a", 1) + second := mrIdentity("group/b", 2) + + setMRSwitches(t, store, "task-1", first, TaskMRAutomationSwitchPatch{ + AutoFixEnabled: boolPtr(true), AutoMergeEnabled: boolPtr(true), + PromptOnMerged: boolPtr(true), + }) + + other, err := store.GetTaskMRAutomationOptionsForMR(ctx, "task-1", second) + if err != nil { + t.Fatalf("GetTaskMRAutomationOptionsForMR(second): %v", err) + } + if other.AutoFixEnabled || other.AutoMergeEnabled || other.PromptOnMerged || + other.PromptOnReviewRequested || other.PromptOnClosed { + t.Fatalf("second MR inherited the first MR's switches: %+v", other) + } + + stored, err := store.ListTaskMRAutomationOptions(ctx, "task-1") + if err != nil { + t.Fatalf("ListTaskMRAutomationOptions: %v", err) + } + if len(stored) != 1 || stored[0].Identity() != first { + t.Fatalf("expected exactly the configured MR to be stored, got %+v", stored) + } +} + +// TestStore_UpdateTaskMRAutomationOptions_PromptOverrideRoundTrip covers the +// one field that is still genuinely task-level, including the +// empty-string-normalizes-to-NULL path (normalizedMRPromptOverride). +func TestStore_UpdateTaskMRAutomationOptions_PromptOverrideRoundTrip(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedTask(t, store, "task-1", "") + + updated, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ AutoFixPromptOverride: stringPtr("custom prompt text"), }, nil) if err != nil { @@ -51,16 +156,14 @@ func TestStore_UpdateTaskMRAutomationOptions_AutoMergeAndPromptOverrideRoundTrip if updated.AutoFixPromptOverride == nil || *updated.AutoFixPromptOverride != "custom prompt text" { t.Fatalf("AutoFixPromptOverride = %v immediately after patch, want \"custom prompt text\"", updated.AutoFixPromptOverride) } - got, err = store.GetTaskMRAutomationOptions(ctx, "task-1") + firstRevision := updated.AutomationRevision + got, err := store.GetTaskMRAutomationOptions(ctx, "task-1") if err != nil { t.Fatalf("GetTaskMRAutomationOptions: %v", err) } if got.AutoFixPromptOverride == nil || *got.AutoFixPromptOverride != "custom prompt text" { t.Fatalf("AutoFixPromptOverride = %v after persisted read-back, want \"custom prompt text\"", got.AutoFixPromptOverride) } - if !got.AutoMergeEnabled { - t.Fatalf("AutoMergeEnabled reverted to false after an unrelated patch, want still true") - } // Clearing via an empty string must normalize to NULL, not persist "". updated, err = store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ @@ -72,6 +175,9 @@ func TestStore_UpdateTaskMRAutomationOptions_AutoMergeAndPromptOverrideRoundTrip if updated.AutoFixPromptOverride != nil { t.Fatalf("AutoFixPromptOverride = %v immediately after clearing, want nil", updated.AutoFixPromptOverride) } + if updated.AutomationRevision <= firstRevision { + t.Fatalf("automation revision = %d after update, want > %d", updated.AutomationRevision, firstRevision) + } got, err = store.GetTaskMRAutomationOptions(ctx, "task-1") if err != nil { t.Fatalf("GetTaskMRAutomationOptions: %v", err) @@ -94,37 +200,42 @@ func TestStore_GetTaskMRAutomationOptions_ImplicitDefault(t *testing.T) { } } -func TestStore_UpdateTaskMRAutomationOptions_RoundTrip(t *testing.T) { +func TestStore_UpdateTaskMRAutomationOptionsForMR_RoundTrip(t *testing.T) { store := newTestStore(t) ctx := context.Background() seedTask(t, store, "task-1", "") - updated, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + id := mrIdentity("group/a", 1) + + updated := setMRSwitches(t, store, "task-1", id, TaskMRAutomationSwitchPatch{ PromptOnMerged: boolPtr(true), - }, nil) - if err != nil { - t.Fatalf("UpdateTaskMRAutomationOptions: %v", err) - } + }) if !updated.PromptOnMerged || updated.PromptOnReviewRequested || updated.PromptOnClosed { t.Fatalf("unexpected options after first patch: %+v", updated) } - username := "alice" - updated, err = store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + updated = setMRSwitches(t, store, "task-1", id, TaskMRAutomationSwitchPatch{ PromptOnReviewRequested: boolPtr(true), - }, &username) - if err != nil { - t.Fatalf("UpdateTaskMRAutomationOptions second patch: %v", err) + }) + if !updated.PromptOnMerged || !updated.PromptOnReviewRequested { + t.Fatalf("expected merged switch preserved alongside the new one, got %+v", updated) } - if !updated.PromptOnMerged || !updated.PromptOnReviewRequested || updated.ReviewReviewerUsername != "alice" { - t.Fatalf("expected merged switch preserved and reviewer set, got %+v", updated) + + // The reviewer username stays task-level. + username := "alice" + if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{}, &username); err != nil { + t.Fatalf("persist reviewer: %v", err) } - got, err := store.GetTaskMRAutomationOptions(ctx, "task-1") + got, err := store.GetTaskMRAutomationOptionsForMR(ctx, "task-1", id) if err != nil { - t.Fatalf("GetTaskMRAutomationOptions: %v", err) + t.Fatalf("GetTaskMRAutomationOptionsForMR: %v", err) } - if !got.PromptOnMerged || !got.PromptOnReviewRequested || got.ReviewReviewerUsername != "alice" { - t.Fatalf("persisted options mismatch: %+v", got) + if !got.PromptOnMerged || !got.PromptOnReviewRequested { + t.Fatalf("persisted switches mismatch: %+v", got) + } + taskOpts, err := store.GetTaskMRAutomationOptions(ctx, "task-1") + if err != nil || taskOpts.ReviewReviewerUsername != "alice" { + t.Fatalf("persisted reviewer mismatch: %+v err=%v", taskOpts, err) } } @@ -394,38 +505,33 @@ func TestStore_UpdateTaskMRAutomationOptions_ResendingSameSwitchResetsBaselineOn } } -// TestStore_UpdateTaskMRAutomationOptions_ReenablingMergedSwitchResetsCheckpoint +// TestStore_UpdateTaskMRAutomationOptionsForMR_ReenablingMergedSwitchResetsCheckpoint // is the P1 finding: an MR that reached the merged state while the switch was // off must not stay permanently suppressed once the switch is re-enabled. -func TestStore_UpdateTaskMRAutomationOptions_ReenablingMergedSwitchResetsCheckpoint(t *testing.T) { +// The sibling MR proves the reset is scoped to the MR being reconfigured. +func TestStore_UpdateTaskMRAutomationOptionsForMR_ReenablingMergedSwitchResetsCheckpoint(t *testing.T) { store := newTestStore(t) ctx := context.Background() seedTask(t, store, "task-1", "") - - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ - PromptOnMerged: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable switch: %v", err) - } - if err := store.RecordTaskMRLifecyclePrompt(ctx, TaskMRLifecyclePrompt{ - TaskID: "task-1", ProjectPath: "group/a", MRIID: 1, - Event: gitlabStateMerged, PromptedAt: time.Now().UTC(), ObservedState: gitlabStateMerged, - }); err != nil { - t.Fatalf("record merged prompt: %v", err) + id := mrIdentity("group/a", 1) + + setMRSwitches(t, store, "task-1", id, TaskMRAutomationSwitchPatch{PromptOnMerged: boolPtr(true)}) + for _, mr := range []struct { + project string + iid int + }{{"group/a", 1}, {"group/b", 2}} { + if err := store.RecordTaskMRLifecyclePrompt(ctx, TaskMRLifecyclePrompt{ + TaskID: "task-1", ProjectPath: mr.project, MRIID: mr.iid, + Event: gitlabStateMerged, PromptedAt: time.Now().UTC(), ObservedState: gitlabStateMerged, + }); err != nil { + t.Fatalf("record merged prompt for %s: %v", mr.project, err) + } } // Disable, then re-enable — the checkpoint from the still-merged MR must // not survive, or the re-enabled switch would never fire for it again. - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ - PromptOnMerged: boolPtr(false), - }, nil); err != nil { - t.Fatalf("disable switch: %v", err) - } - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ - PromptOnMerged: boolPtr(true), - }, nil); err != nil { - t.Fatalf("re-enable switch: %v", err) - } + setMRSwitches(t, store, "task-1", id, TaskMRAutomationSwitchPatch{PromptOnMerged: boolPtr(false)}) + setMRSwitches(t, store, "task-1", id, TaskMRAutomationSwitchPatch{PromptOnMerged: boolPtr(true)}) got, err := store.GetTaskMRLifecycleState(ctx, "task-1", "", "group/a", 1) if err != nil || got == nil { @@ -434,22 +540,26 @@ func TestStore_UpdateTaskMRAutomationOptions_ReenablingMergedSwitchResetsCheckpo if got.LastObservedState != "" || got.LastLifecycleEvent != "" { t.Fatalf("expected terminal checkpoint reset after re-enabling the switch, got %+v", got) } + sibling, err := store.GetTaskMRLifecycleState(ctx, "task-1", "", "group/b", 2) + if err != nil || sibling == nil { + t.Fatalf("GetTaskMRLifecycleState(sibling): %+v err=%v", sibling, err) + } + if sibling.LastLifecycleEvent != gitlabStateMerged { + t.Fatalf("sibling MR's checkpoint was cleared by another MR's switch flip: %+v", sibling) + } } -// TestStore_UpdateTaskMRAutomationOptions_ReenablingAutoFixResetsRoundCap +// TestStore_UpdateTaskMRAutomationOptionsForMR_ReenablingAutoFixResetsRoundCap // covers AC11: toggling auto-fix off and on again must clear // auto_fix_exhausted_at and auto_fix_round_count so a subsequent failing // pipeline can dispatch again. -func TestStore_UpdateTaskMRAutomationOptions_ReenablingAutoFixResetsRoundCap(t *testing.T) { +func TestStore_UpdateTaskMRAutomationOptionsForMR_ReenablingAutoFixResetsRoundCap(t *testing.T) { store := newTestStore(t) ctx := context.Background() seedTask(t, store, "task-1", "") + id := mrIdentity("group/a", 1) - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ - AutoFixEnabled: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable auto-fix: %v", err) - } + setMRSwitches(t, store, "task-1", id, TaskMRAutomationSwitchPatch{AutoFixEnabled: boolPtr(true)}) if err := store.RecordTaskMRFixAttempt(ctx, TaskMRFixAttempt{ TaskID: "task-1", ProjectPath: "group/a", MRIID: 1, Signature: "sig-1", CheckpointJSON: "{}", SessionID: "sess-1", @@ -470,16 +580,8 @@ func TestStore_UpdateTaskMRAutomationOptions_ReenablingAutoFixResetsRoundCap(t * } // Disable, then re-enable — the round cap and exhaustion must clear. - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ - AutoFixEnabled: boolPtr(false), - }, nil); err != nil { - t.Fatalf("disable auto-fix: %v", err) - } - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ - AutoFixEnabled: boolPtr(true), - }, nil); err != nil { - t.Fatalf("re-enable auto-fix: %v", err) - } + setMRSwitches(t, store, "task-1", id, TaskMRAutomationSwitchPatch{AutoFixEnabled: boolPtr(false)}) + setMRSwitches(t, store, "task-1", id, TaskMRAutomationSwitchPatch{AutoFixEnabled: boolPtr(true)}) got, err = store.GetTaskMRLifecycleState(ctx, "task-1", "", "group/a", 1) if err != nil || got == nil { @@ -564,10 +666,13 @@ func TestStore_TaskDeleteCascadesMRAutomationRows(t *testing.T) { seedTask(t, store, "task-1", "") if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ - PromptOnMerged: boolPtr(true), + AutoFixPromptOverride: stringPtr("custom"), }, nil); err != nil { t.Fatalf("seed options: %v", err) } + setMRSwitches(t, store, "task-1", mrIdentity("group/a", 1), TaskMRAutomationSwitchPatch{ + PromptOnMerged: boolPtr(true), + }) if err := store.SetTaskMRObservedState(ctx, "task-1", "", "group/a", 1, "opened"); err != nil { t.Fatalf("seed checkpoint: %v", err) } @@ -580,9 +685,16 @@ func TestStore_TaskDeleteCascadesMRAutomationRows(t *testing.T) { if err != nil { t.Fatalf("GetTaskMRAutomationOptions: %v", err) } - if opts.PromptOnMerged { + if opts.AutoFixPromptOverride != nil { t.Fatalf("expected options row cascaded away, got %+v", opts) } + switches, err := store.ListTaskMRAutomationOptions(ctx, "task-1") + if err != nil { + t.Fatalf("ListTaskMRAutomationOptions: %v", err) + } + if len(switches) != 0 { + t.Fatalf("expected per-MR switch rows cascaded away, got %+v", switches) + } state, err := store.GetTaskMRLifecycleState(ctx, "task-1", "", "group/a", 1) if err != nil { t.Fatalf("GetTaskMRLifecycleState: %v", err) @@ -612,19 +724,15 @@ func TestStore_ListAutomationSubscribedTaskMRs(t *testing.T) { if err := store.UpsertTaskMR(ctx, autoFixOnly); err != nil { t.Fatalf("upsert auto-fix-only MR: %v", err) } - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-1", TaskMRAutomationPatch{ + setMRSwitches(t, store, "task-1", mrIdentity("group/subscribed", 1), TaskMRAutomationSwitchPatch{ PromptOnMerged: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable switch for task-1: %v", err) - } + }) // task-3 has no lifecycle switch on, only auto-fix — must still be // returned since the query was widened to OR in auto_fix_enabled / // auto_merge_enabled (this task's change). - if _, err := store.UpdateTaskMRAutomationOptions(ctx, "task-3", TaskMRAutomationPatch{ + setMRSwitches(t, store, "task-3", mrIdentity("group/autofix", 3), TaskMRAutomationSwitchPatch{ AutoFixEnabled: boolPtr(true), - }, nil); err != nil { - t.Fatalf("enable auto-fix for task-3: %v", err) - } + }) rows, err := store.ListAutomationSubscribedTaskMRs(ctx) if err != nil { @@ -706,3 +814,155 @@ func assertMRAutomationTablesExist(t *testing.T, sqlxDB *sqlx.DB) { } } } + +// TestStore_DeleteTaskMRForWorkspace_DropsPerMRAutomationOptions covers the +// unlink half of the per-MR switch lifecycle. Leaving the switch row behind +// meant re-linking the same MR — by hand, or through push-detection auto-link +// — silently re-armed whatever was configured before the unlink, including +// auto-merge, with no surface showing it (taskMRAutomationOptionsList hides +// rows whose MR is not linked) but the evaluator still reading it. +func TestStore_DeleteTaskMRForWorkspace_DropsPerMRAutomationOptions(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws-1") + seedTask(t, store, "task-1", "ws-1") + + mr := newTestMR("task-1", "", "group/a", 1) + if err := store.UpsertTaskMR(ctx, mr); err != nil { + t.Fatalf("upsert MR: %v", err) + } + id := MRIdentity{RepositoryID: "", ProjectPath: "group/a", MRIID: 1} + if _, err := store.UpdateTaskMRAutomationOptionsForMR( + ctx, "task-1", id, TaskMRAutomationSwitchPatch{AutoMergeEnabled: boolPtr(true)}, + ); err != nil { + t.Fatalf("enable auto-merge: %v", err) + } + if err := store.SetTaskMRObservedState(ctx, "task-1", "", "group/a", 1, "merged"); err != nil { + t.Fatalf("seed lifecycle state: %v", err) + } + + if err := store.DeleteTaskMRForWorkspace(ctx, "ws-1", mr.ID); err != nil { + t.Fatalf("DeleteTaskMRForWorkspace: %v", err) + } + + stored, err := store.ListTaskMRAutomationOptions(ctx, "task-1") + if err != nil { + t.Fatalf("ListTaskMRAutomationOptions: %v", err) + } + if len(stored) != 0 { + t.Fatalf("unlink left automation rows behind: %+v", stored) + } + state, err := store.GetTaskMRLifecycleState(ctx, "task-1", "", "group/a", 1) + if err != nil { + t.Fatalf("get lifecycle state: %v", err) + } + if state != nil { + t.Fatalf("unlink left lifecycle state behind: %+v", state) + } + + // Re-linking the same MR must start from all-off, not resurrect the + // pre-unlink configuration. + relinked := newTestMR("task-1", "", "group/a", 1) + if err := store.UpsertTaskMR(ctx, relinked); err != nil { + t.Fatalf("re-link MR: %v", err) + } + opts, err := store.GetTaskMRAutomationOptionsForMR(ctx, "task-1", id) + if err != nil { + t.Fatalf("GetTaskMRAutomationOptionsForMR: %v", err) + } + if opts.AutoMergeEnabled { + t.Errorf("re-linked MR silently re-armed auto-merge: %+v", opts) + } +} + +// TestStore_DeleteTaskMR_DropsPerMRAutomationOptions pins the same cleanup on +// the association-ID delete path, which cascades to gitlab_task_mr_state +// independently of DeleteTaskMRForWorkspace. The two must stay in step: a +// caller routed through this one would otherwise leave an enabled auto-merge +// switch behind for the next link of the same MR to inherit. +func TestStore_DeleteTaskMR_DropsPerMRAutomationOptions(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws-1") + seedTask(t, store, "task-1", "ws-1") + + mr := newTestMR("task-1", "", "group/a", 1) + if err := store.UpsertTaskMR(ctx, mr); err != nil { + t.Fatalf("upsert MR: %v", err) + } + id := MRIdentity{RepositoryID: "", ProjectPath: "group/a", MRIID: 1} + if _, err := store.UpdateTaskMRAutomationOptionsForMR( + ctx, "task-1", id, TaskMRAutomationSwitchPatch{AutoMergeEnabled: boolPtr(true)}, + ); err != nil { + t.Fatalf("enable auto-merge: %v", err) + } + + if err := store.DeleteTaskMR(ctx, mr.ID); err != nil { + t.Fatalf("DeleteTaskMR: %v", err) + } + + stored, err := store.ListTaskMRAutomationOptions(ctx, "task-1") + if err != nil { + t.Fatalf("ListTaskMRAutomationOptions: %v", err) + } + if len(stored) != 0 { + t.Fatalf("delete left automation rows behind: %+v", stored) + } +} + +// TestStore_UpdateTaskMRAutomationOptionsForMRs_IsAllOrNothing pins the +// fan-out atomicity: a failure partway through must not leave the switch +// armed on the MRs processed before it while the caller is told the operation +// failed. A trigger supplies the deterministic mid-batch failure — the second +// identity's UPDATE aborts, and the first identity's already-applied write +// must roll back with it. +func TestStore_UpdateTaskMRAutomationOptionsForMRs_IsAllOrNothing(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws-1") + seedTask(t, store, "task-1", "ws-1") + + first := MRIdentity{RepositoryID: "", ProjectPath: "group/a", MRIID: 1} + second := MRIdentity{RepositoryID: "", ProjectPath: "group/b", MRIID: 2} + for _, id := range []MRIdentity{first, second} { + if err := store.UpsertTaskMR(ctx, newTestMR("task-1", id.RepositoryID, id.ProjectPath, id.MRIID)); err != nil { + t.Fatalf("link MR %s: %v", id.ProjectPath, err) + } + } + if _, err := store.db.Exec(` + CREATE TRIGGER fail_second_mr BEFORE UPDATE ON gitlab_task_mr_automation_options + WHEN NEW.project_path = 'group/b' + BEGIN SELECT RAISE(ABORT, 'injected failure'); END`); err != nil { + t.Fatalf("create failure trigger: %v", err) + } + + err := store.UpdateTaskMRAutomationOptionsForMRs( + ctx, "task-1", []MRIdentity{first, second}, + TaskMRAutomationSwitchPatch{AutoMergeEnabled: boolPtr(true)}, + ) + if err == nil { + t.Fatal("expected the batch to fail on the second identity") + } + + got, err := store.GetTaskMRAutomationOptionsForMR(ctx, "task-1", first) + if err != nil { + t.Fatalf("GetTaskMRAutomationOptionsForMR: %v", err) + } + if got.AutoMergeEnabled { + t.Error("first MR kept auto-merge after the batch failed — fan-out was not atomic") + } +} + +// TestStore_UpdateTaskMRAutomationOptionsForMRs_EmptyIsNoop guards the +// zero-target call so it cannot open an empty transaction per request. +func TestStore_UpdateTaskMRAutomationOptionsForMRs_EmptyIsNoop(t *testing.T) { + store := newTestStore(t) + seedWorkspace(t, store, "ws-1") + seedTask(t, store, "task-1", "ws-1") + if err := store.UpdateTaskMRAutomationOptionsForMRs( + context.Background(), "task-1", nil, + TaskMRAutomationSwitchPatch{AutoMergeEnabled: boolPtr(true)}, + ); err != nil { + t.Fatalf("empty batch should be a no-op, got %v", err) + } +} diff --git a/apps/backend/internal/gitlab/store_mr_scope_migration_test.go b/apps/backend/internal/gitlab/store_mr_scope_migration_test.go new file mode 100644 index 0000000000..8698a28a56 --- /dev/null +++ b/apps/backend/internal/gitlab/store_mr_scope_migration_test.go @@ -0,0 +1,246 @@ +package gitlab + +import ( + "context" + "path/filepath" + "testing" + + "github.com/jmoiron/sqlx" + + "github.com/kandev/kandev/internal/db" +) + +// legacySwitches names the five task-wide booleans as they were stored +// before this change, so each test reads as the configuration it is +// simulating rather than as five positional bools. +type legacySwitches struct { + autoFix, autoMerge, review, merged, closed bool +} + +// writeLegacyTaskMROptions puts a task back into the pre-scope-migration +// shape: task-wide switch values on gitlab_task_mr_options with the +// mr_scope_migrated_at marker cleared, exactly what an installation upgrading +// across this change starts from. +func writeLegacyTaskMROptions(t *testing.T, store *Store, taskID string, s legacySwitches) { + t.Helper() + if _, err := store.db.Exec(` + INSERT INTO gitlab_task_mr_options ( + task_id, auto_fix_enabled, auto_merge_enabled, prompt_on_review_requested, + prompt_on_merged, prompt_on_closed, mr_scope_migrated_at, created_at, updated_at + ) VALUES (?, ?, ?, ?, ?, ?, NULL, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) + ON CONFLICT(task_id) DO UPDATE SET + auto_fix_enabled = excluded.auto_fix_enabled, + auto_merge_enabled = excluded.auto_merge_enabled, + prompt_on_review_requested = excluded.prompt_on_review_requested, + prompt_on_merged = excluded.prompt_on_merged, + prompt_on_closed = excluded.prompt_on_closed, + mr_scope_migrated_at = NULL`, + taskID, s.autoFix, s.autoMerge, s.review, s.merged, s.closed); err != nil { + t.Fatalf("write legacy options for %s: %v", taskID, err) + } +} + +// TestStore_MigrateTaskMROptionsToMRScope_FansOutToEveryLinkedMR covers the +// upgrade path: a task that had the switches on task-wide keeps them on for +// every MR it had linked at the time, so nobody's configured automation +// silently stops working across the upgrade. +func TestStore_MigrateTaskMROptionsToMRScope_FansOutToEveryLinkedMR(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws-1") + seedTask(t, store, "task-1", "ws-1") + + for _, mr := range []*TaskMR{ + newTestMR("task-1", "", "group/a", 1), + newTestMR("task-1", "", "group/b", 2), + } { + if err := store.UpsertTaskMR(ctx, mr); err != nil { + t.Fatalf("upsert MR %s: %v", mr.ProjectPath, err) + } + } + writeLegacyTaskMROptions(t, store, "task-1", legacySwitches{autoFix: true, merged: true}) + + if err := store.migrateTaskMROptionsToMRScope(); err != nil { + t.Fatalf("migrateTaskMROptionsToMRScope: %v", err) + } + + stored, err := store.ListTaskMRAutomationOptions(ctx, "task-1") + if err != nil { + t.Fatalf("ListTaskMRAutomationOptions: %v", err) + } + if len(stored) != 2 { + t.Fatalf("expected one row per linked MR, got %+v", stored) + } + for _, row := range stored { + if !row.AutoFixEnabled || !row.PromptOnMerged { + t.Errorf("MR %s did not inherit the legacy switches: %+v", row.ProjectPath, row) + } + if row.AutoMergeEnabled || row.PromptOnReviewRequested || row.PromptOnClosed { + t.Errorf("MR %s gained a switch the task never had on: %+v", row.ProjectPath, row) + } + } +} + +// TestStore_MigrateTaskMROptionsToMRScope_DoesNotReplayOverADeliberateDisable +// is what the mr_scope_migrated_at marker exists for: once the fan-out has +// run, a later boot must not re-enable a switch the user has since turned off +// for one specific MR. +func TestStore_MigrateTaskMROptionsToMRScope_DoesNotReplayOverADeliberateDisable(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws-1") + seedTask(t, store, "task-1", "ws-1") + first := mrIdentity("group/a", 1) + + if err := store.UpsertTaskMR(ctx, newTestMR("task-1", "", "group/a", 1)); err != nil { + t.Fatalf("upsert MR: %v", err) + } + writeLegacyTaskMROptions(t, store, "task-1", legacySwitches{merged: true}) + if err := store.migrateTaskMROptionsToMRScope(); err != nil { + t.Fatalf("first migration: %v", err) + } + setMRSwitches(t, store, "task-1", first, TaskMRAutomationSwitchPatch{PromptOnMerged: boolPtr(false)}) + + // Simulates the next process start. + if err := store.migrateTaskMROptionsToMRScope(); err != nil { + t.Fatalf("replayed migration: %v", err) + } + + got, err := store.GetTaskMRAutomationOptionsForMR(ctx, "task-1", first) + if err != nil { + t.Fatalf("GetTaskMRAutomationOptionsForMR: %v", err) + } + if got.PromptOnMerged { + t.Fatalf("replayed migration re-enabled a deliberately disabled switch: %+v", got) + } +} + +// TestStore_MigrateTaskMROptionsToMRScope_LaterLinkedMRStartsAllOff pins the +// other half of the marker's job: an MR linked *after* the fan-out must start +// with every switch off rather than inheriting the task's legacy values. +func TestStore_MigrateTaskMROptionsToMRScope_LaterLinkedMRStartsAllOff(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws-1") + seedTask(t, store, "task-1", "ws-1") + + if err := store.UpsertTaskMR(ctx, newTestMR("task-1", "", "group/a", 1)); err != nil { + t.Fatalf("upsert first MR: %v", err) + } + writeLegacyTaskMROptions(t, store, "task-1", legacySwitches{autoFix: true, autoMerge: true, merged: true, closed: true}) + if err := store.migrateTaskMROptionsToMRScope(); err != nil { + t.Fatalf("first migration: %v", err) + } + + if err := store.UpsertTaskMR(ctx, newTestMR("task-1", "", "group/late", 9)); err != nil { + t.Fatalf("upsert later MR: %v", err) + } + if err := store.migrateTaskMROptionsToMRScope(); err != nil { + t.Fatalf("replayed migration: %v", err) + } + + got, err := store.GetTaskMRAutomationOptionsForMR(ctx, "task-1", mrIdentity("group/late", 9)) + if err != nil { + t.Fatalf("GetTaskMRAutomationOptionsForMR: %v", err) + } + if got.AutoFixEnabled || got.AutoMergeEnabled || got.PromptOnMerged || got.PromptOnClosed { + t.Fatalf("an MR linked after the migration inherited legacy switches: %+v", got) + } +} + +// TestStore_MigrateTaskMROptionsToMRScope_UpgradesADBWithoutTheMarkerColumn +// runs the whole boot path against a database whose gitlab_task_mr_options +// predates mr_scope_migrated_at: the ALTER must add the column and the +// fan-out must then run against the rows already there. +func TestStore_MigrateTaskMROptionsToMRScope_UpgradesADBWithoutTheMarkerColumn(t *testing.T) { + tmp := t.TempDir() + dbConn, err := db.OpenSQLite(filepath.Join(tmp, "gitlab-legacy-scope.db")) + if err != nil { + t.Fatalf("open db: %v", err) + } + t.Cleanup(func() { _ = dbConn.Close() }) + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { _ = sqlxDB.Close() }) + + if _, err := sqlxDB.Exec(` + CREATE TABLE workspaces (id TEXT PRIMARY KEY); + CREATE TABLE tasks (id TEXT PRIMARY KEY, workspace_id TEXT NOT NULL DEFAULT '', archived_at DATETIME); + INSERT INTO tasks (id, workspace_id) VALUES ('task-1', 'ws-1'); + CREATE TABLE gitlab_task_mr_options ( + task_id TEXT PRIMARY KEY, + auto_fix_enabled BOOLEAN NOT NULL DEFAULT 0, + auto_merge_enabled BOOLEAN NOT NULL DEFAULT 0, + auto_fix_prompt_override TEXT, + 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, + review_reviewer_username TEXT NOT NULL DEFAULT '', + created_at DATETIME NOT NULL, + updated_at DATETIME NOT NULL + ); + INSERT INTO gitlab_task_mr_options ( + task_id, auto_merge_enabled, prompt_on_closed, created_at, updated_at + ) VALUES ('task-1', 1, 1, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP);`); err != nil { + t.Fatalf("create legacy schema: %v", err) + } + + store, err := NewStore(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("NewStore against legacy DB: %v", err) + } + ctx := context.Background() + // The MR is linked before the *second* boot, so the first boot's fan-out + // found nothing to seed — the marker is already stamped, which is exactly + // the "starts all-off" contract. + if err := store.UpsertTaskMR(ctx, newTestMR("task-1", "", "group/a", 1)); err != nil { + t.Fatalf("upsert MR: %v", err) + } + if _, err := NewStore(sqlxDB, sqlxDB); err != nil { + t.Fatalf("second boot: %v", err) + } + + columns, err := store.tableColumns("gitlab_task_mr_options") + if err != nil { + t.Fatalf("tableColumns: %v", err) + } + if _, ok := columns["mr_scope_migrated_at"]; !ok { + t.Fatalf("mr_scope_migrated_at missing after upgrading a legacy DB") + } + got, err := store.GetTaskMRAutomationOptionsForMR(ctx, "task-1", mrIdentity("group/a", 1)) + if err != nil { + t.Fatalf("GetTaskMRAutomationOptionsForMR: %v", err) + } + if got.AutoMergeEnabled || got.PromptOnClosed { + t.Fatalf("an MR linked after the marker was stamped inherited legacy switches: %+v", got) + } +} + +// TestStore_MigrateTaskMROptionsToMRScope_FansOutOnFirstBootOfALegacyDB is the +// companion to the test above: when the MR was already linked, the very first +// boot after the upgrade must carry the task's switches onto it. +func TestStore_MigrateTaskMROptionsToMRScope_FansOutOnFirstBootOfALegacyDB(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws-1") + seedTask(t, store, "task-1", "ws-1") + if err := store.UpsertTaskMR(ctx, newTestMR("task-1", "repo-1", "group/a", 1)); err != nil { + t.Fatalf("upsert MR: %v", err) + } + writeLegacyTaskMROptions(t, store, "task-1", legacySwitches{autoMerge: true, review: true}) + + if err := store.migrateTaskMROptionsToMRScope(); err != nil { + t.Fatalf("migrateTaskMROptionsToMRScope: %v", err) + } + + got, err := store.GetTaskMRAutomationOptionsForMR(ctx, "task-1", + MRIdentity{RepositoryID: "repo-1", ProjectPath: "group/a", MRIID: 1}) + if err != nil { + t.Fatalf("GetTaskMRAutomationOptionsForMR: %v", err) + } + if !got.AutoMergeEnabled || !got.PromptOnReviewRequested { + t.Fatalf("legacy switches did not reach the linked MR: %+v", got) + } + if got.RepositoryID != "repo-1" { + t.Fatalf("fan-out lost the MR's repository identity: %+v", got) + } +} diff --git a/apps/backend/internal/gitlab/store_task_mr_link.go b/apps/backend/internal/gitlab/store_task_mr_link.go index 340c8e3e28..18668182ae 100644 --- a/apps/backend/internal/gitlab/store_task_mr_link.go +++ b/apps/backend/internal/gitlab/store_task_mr_link.go @@ -394,6 +394,27 @@ func (s *Store) DeleteTaskMRForWorkspace(ctx context.Context, workspaceID, assoc ); err != nil { return fmt.Errorf("delete task MR refresh watch: %w", err) } + if _, err = tx.ExecContext(ctx, ` + DELETE FROM gitlab_task_mr_state + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ?`, + association.TaskID, association.RepositoryID, association.ProjectPath, association.MRIID, + ); err != nil { + return fmt.Errorf("delete task MR lifecycle state: %w", err) + } + // Drop this MR's automation switches with the association. Leaving the row + // behind means re-linking the same MR later — by hand, or through + // push-detection auto-link — silently re-arms whatever was configured + // before it was unlinked, including auto-merge. Unlinking is the natural + // "stop automating this" action, so it must not leave a latent enabled + // switch that no surface displays (taskMRAutomationOptionsList hides rows + // whose MR is not linked) but the evaluator still reads. + if _, err = tx.ExecContext(ctx, ` + DELETE FROM gitlab_task_mr_automation_options + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ?`, + association.TaskID, association.RepositoryID, association.ProjectPath, association.MRIID, + ); err != nil { + return fmt.Errorf("delete task MR automation options: %w", err) + } if _, err = tx.ExecContext(ctx, `DELETE FROM gitlab_task_mrs WHERE id = ?`, association.ID); err != nil { return fmt.Errorf("delete task MR association: %w", err) } diff --git a/apps/backend/internal/mcp/handlers/task_mr_automation.go b/apps/backend/internal/mcp/handlers/task_mr_automation.go index 22fe6b8768..25b3014195 100644 --- a/apps/backend/internal/mcp/handlers/task_mr_automation.go +++ b/apps/backend/internal/mcp/handlers/task_mr_automation.go @@ -67,6 +67,12 @@ func (h *Handlers) mrAutomationErrorResponse(msg *ws.Message, taskID, logMsg str if errors.Is(err, repoerrors.ErrTaskNotFound) || errors.Is(err, repoerrors.ErrWorkspaceNotFound) { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, "task not found", nil) } + // Naming an MR that isn't linked to this task (or naming one only + // partially) is a caller mistake — report it as such instead of burying + // it in a generic internal error. + if errors.Is(err, gitlab.ErrTaskMRNotLinked) { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, err.Error(), nil) + } h.logger.Error(logMsg, zap.String("task_id", taskID), zap.Error(err)) return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "failed to process MR automation request", nil) } @@ -83,7 +89,13 @@ func (h *Handlers) handleUpdateTaskMRAutomation(ctx context.Context, msg *ws.Mes if errResp != nil || err != nil { return errResp, err } + // repository_id/project_path/mr_iid optionally scope the switch changes + // to one linked MR; omitting all three applies them to every linked MR, + // which is what an agent with no MR identity to send gets. var req struct { + RepositoryID *string `json:"repository_id"` + ProjectPath *string `json:"project_path"` + MRIID *int `json:"mr_iid"` AutoFixEnabled *bool `json:"auto_fix_enabled"` AutoMergeEnabled *bool `json:"auto_merge_enabled"` AutoFixPromptOverride *string `json:"auto_fix_prompt_override"` @@ -95,6 +107,9 @@ func (h *Handlers) handleUpdateTaskMRAutomation(ctx context.Context, msg *ws.Mes return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) } patch := gitlab.TaskMRAutomationPatch{ + RepositoryID: req.RepositoryID, + ProjectPath: req.ProjectPath, + MRIID: req.MRIID, AutoFixEnabled: req.AutoFixEnabled, AutoMergeEnabled: req.AutoMergeEnabled, AutoFixPromptOverride: req.AutoFixPromptOverride, diff --git a/apps/backend/internal/mcp/handlers/task_mr_automation_test.go b/apps/backend/internal/mcp/handlers/task_mr_automation_test.go index a906ba73c3..13c84a4843 100644 --- a/apps/backend/internal/mcp/handlers/task_mr_automation_test.go +++ b/apps/backend/internal/mcp/handlers/task_mr_automation_test.go @@ -91,3 +91,47 @@ func TestHandleGetTaskMRAutomation(t *testing.T) { require.NoError(t, err) assert.Equal(t, ws.MessageTypeResponse, response.Type) } + +// TestHandleUpdateTaskMRAutomationForwardsMRIdentity covers the per-MR MCP +// contract: an agent that knows which MR it means can say so, and the +// identity reaches the service instead of being dropped into a fan-out. +func TestHandleUpdateTaskMRAutomationForwardsMRIdentity(t *testing.T) { + automation := &recordingTaskMRAutomationService{} + h := &Handlers{taskMRAutomation: automation, logger: testLogger(t).WithFields()} + + msg := makeWSMessage(t, ws.ActionMCPUpdateTaskMRAutomation, map[string]any{ + "task_id": "task-current", + "repository_id": "repo-1", + "project_path": "group/a", + "mr_iid": 7, + "auto_merge_enabled": true, + }) + response, err := h.handleUpdateTaskMRAutomation(context.Background(), msg) + + require.NoError(t, err) + assert.Equal(t, ws.MessageTypeResponse, response.Type) + require.Equal(t, 1, automation.calls) + require.True(t, automation.patch.HasMRIdentity()) + assert.Equal(t, gitlab.MRIdentity{RepositoryID: "repo-1", ProjectPath: "group/a", MRIID: 7}, + automation.patch.MRIdentity()) +} + +// TestHandleUpdateTaskMRAutomationRejectsIdentityOnlyPayload keeps MR +// identity from counting as a requested change on its own — it only says +// which MR the (absent) switch changes would have applied to. +func TestHandleUpdateTaskMRAutomationRejectsIdentityOnlyPayload(t *testing.T) { + automation := &recordingTaskMRAutomationService{} + h := &Handlers{taskMRAutomation: automation, logger: testLogger(t).WithFields()} + + msg := makeWSMessage(t, ws.ActionMCPUpdateTaskMRAutomation, map[string]any{ + "task_id": "task-current", + "repository_id": "repo-1", + "project_path": "group/a", + "mr_iid": 7, + }) + response, err := h.handleUpdateTaskMRAutomation(context.Background(), msg) + + require.NoError(t, err) + assert.Equal(t, ws.MessageTypeError, response.Type) + assert.Zero(t, automation.calls) +} diff --git a/apps/backend/internal/mcp/server/handlers.go b/apps/backend/internal/mcp/server/handlers.go index 900599064b..b23ddc52bb 100644 --- a/apps/backend/internal/mcp/server/handlers.go +++ b/apps/backend/internal/mcp/server/handlers.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "math" "strings" "time" @@ -366,6 +367,33 @@ func (s *Server) getTaskMRAutomationHandler() server.ToolHandlerFunc { } } +// copyMRIdentityArgs copies the optional repository_id/project_path/mr_iid +// triple that scopes a patch to one linked MR. repository_id is frequently +// an empty string on self-managed hosts without a numeric project ID (R6), +// so presence in args — not non-emptiness — is what marks it as sent; +// copyOptionalStringArg's "empty means absent" rule would silently turn a +// complete-but-empty identity into a partial one and get it rejected. +func copyMRIdentityArgs(payload, args map[string]interface{}) error { + for _, key := range []string{"repository_id", "project_path"} { + if value, ok := args[key]; ok { + if s, ok := value.(string); ok { + payload[key] = s + } + } + } + if value, ok := args["mr_iid"].(float64); ok { + if !isValidMRIID(value) { + return fmt.Errorf("mr_iid must be a positive integer") + } + payload["mr_iid"] = int(value) + } + return nil +} + +func isValidMRIID(value float64) bool { + return !math.IsNaN(value) && !math.IsInf(value, 0) && value > 0 && math.Trunc(value) == value +} + func (s *Server) updateTaskMRAutomationHandler() server.ToolHandlerFunc { return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { payload := map[string]interface{}{"task_id": s.taskID} @@ -373,12 +401,24 @@ func (s *Server) updateTaskMRAutomationHandler() server.ToolHandlerFunc { if hasLifecyclePromptOverrideArgument(args) { return mcp.NewToolResultError("lifecycle prompt overrides are not supported"), nil } - for _, key := range []string{"prompt_on_review_requested", "prompt_on_merged", "prompt_on_closed"} { + if err := copyMRIdentityArgs(payload, args); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + fieldCount := 0 + for _, key := range []string{ + "auto_fix_enabled", "auto_merge_enabled", + "prompt_on_review_requested", "prompt_on_merged", "prompt_on_closed", + } { if value, ok := args[key].(bool); ok { payload[key] = value + fieldCount++ } } - if len(payload) == 1 { + if value, ok := args["auto_fix_prompt_override"].(string); ok { + payload["auto_fix_prompt_override"] = value + fieldCount++ + } + if fieldCount == 0 { return mcp.NewToolResultError("at least one MR automation option is required"), nil } var result map[string]interface{} diff --git a/apps/backend/internal/mcp/server/handlers_test.go b/apps/backend/internal/mcp/server/handlers_test.go index dc1aeb39ea..b02f9effb4 100644 --- a/apps/backend/internal/mcp/server/handlers_test.go +++ b/apps/backend/internal/mcp/server/handlers_test.go @@ -979,6 +979,69 @@ func TestTaskMRAutomationToolsNoTaskIDArgument(t *testing.T) { assert.NotContains(t, properties, "task_id") } +// TestUpdateTaskMRAutomationToolForwardsMRIdentityAndAutoFixFields covers +// AC31: repository_id/project_path/mr_iid must reach the backend payload +// unchanged (including an explicit empty repository_id, R6) so the WS +// handler can scope the patch to one linked MR, and auto_fix_enabled / +// auto_merge_enabled / auto_fix_prompt_override must also be forwarded — +// the tool's input schema declares all six, but until this test regressed +// them, the handler only ever copied the three lifecycle booleans. +func TestUpdateTaskMRAutomationToolForwardsMRIdentityAndAutoFixFields(t *testing.T) { + backend := &testBackend{response: map[string]interface{}{"task_id": "task-current"}} + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "update_task_mr_automation_kandev", map[string]interface{}{ + "repository_id": "", + "project_path": "group/project", + "mr_iid": float64(7), + "auto_fix_enabled": true, + "auto_merge_enabled": false, + "auto_fix_prompt_override": "custom prompt", + }) + assert.False(t, result.IsError) + assert.Equal(t, ws.ActionMCPUpdateTaskMRAutomation, backend.lastAction) + payload, ok := backend.lastPayload.(map[string]interface{}) + require.True(t, ok) + assert.Equal(t, "", payload["repository_id"]) + assert.Equal(t, "group/project", payload["project_path"]) + assert.Equal(t, 7, payload["mr_iid"]) + assert.Equal(t, true, payload["auto_fix_enabled"]) + assert.Equal(t, false, payload["auto_merge_enabled"]) + assert.Equal(t, "custom prompt", payload["auto_fix_prompt_override"]) +} + +// TestUpdateTaskMRAutomationToolRejectsIdentityAloneWithNoSwitch ensures MR +// identity by itself (no actual option change) is still rejected — mirrors +// TaskMRAutomationPatch.HasAny() treating identity as "which MR", not "a +// change". +func TestUpdateTaskMRAutomationToolRejectsIdentityAloneWithNoSwitch(t *testing.T) { + backend := &testBackend{} + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "update_task_mr_automation_kandev", map[string]interface{}{ + "repository_id": "", + "project_path": "group/project", + "mr_iid": float64(7), + }) + assert.True(t, result.IsError) + assert.Empty(t, backend.lastAction, "identity-only calls must not reach the backend") +} + +func TestUpdateTaskMRAutomationToolRejectsFractionalMRIID(t *testing.T) { + backend := &testBackend{} + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "update_task_mr_automation_kandev", map[string]interface{}{ + "repository_id": "", + "project_path": "group/project", + "mr_iid": float64(7.5), + "auto_merge_enabled": true, + }) + + assert.True(t, result.IsError) + assert.Empty(t, backend.lastAction, "invalid MR IID must not reach the backend") +} + func TestTaskMRAutomationToolsDoNotExposeLifecyclePromptOverrides(t *testing.T) { backend := &testBackend{} s := newTaskModeServer(t, backend, "task-current") diff --git a/apps/backend/internal/mcp/server/server.go b/apps/backend/internal/mcp/server/server.go index 476cfac9fb..2978a22271 100644 --- a/apps/backend/internal/mcp/server/server.go +++ b/apps/backend/internal/mcp/server/server.go @@ -1135,7 +1135,17 @@ func (s *Server) registerMRAutomationTools() { ) s.mcpServer.AddTool( mcp.NewTool("update_task_mr_automation_kandev", - mcp.WithDescription("Update this task's GitLab merge request lifecycle notification switches."), + mcp.WithDescription("Update this task's GitLab merge request automation options (auto-fix, auto-merge, "+ + "and lifecycle notifications). The five switches are per merge request: pass repository_id, "+ + "project_path and mr_iid together to target one linked MR, or omit all three to apply them to "+ + "every MR linked to this task. auto_fix_prompt_override applies to every linked MR regardless "+ + "of MR identity."), + mcp.WithString("repository_id", mcp.Description("Repository ID of the linked MR to target (omit to target every linked MR)")), + mcp.WithString("project_path", mcp.Description("Project path of the linked MR to target, e.g. group/project")), + mcp.WithNumber("mr_iid", mcp.Description("IID of the linked MR to target")), + mcp.WithBoolean("auto_fix_enabled", mcp.Description("Enable or disable auto-fix when the linked MR's pipeline fails")), + mcp.WithBoolean("auto_merge_enabled", mcp.Description("Enable or disable auto-merge when the linked MR is ready")), + mcp.WithString("auto_fix_prompt_override", mcp.Description("Task-level custom prompt for auto-fix; valid without linked MRs and not scoped by MR identity (empty string clears the override)")), 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 MR becomes merged")), mcp.WithBoolean("prompt_on_closed", mcp.Description("Prompt this task's agent once when the linked MR becomes closed without merge")), diff --git a/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_automation_test.go b/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_automation_test.go index d5921c257a..284f4576fc 100644 --- a/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_automation_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_automation_test.go @@ -21,7 +21,11 @@ import ( type mockGitLabMRAutomationService struct { mu sync.Mutex - options *gitlab.TaskMRAutomationResponse + options *gitlab.TaskMRAutomationResponse + // optionsByMRIID, when set, returns per-MR evaluation options keyed by MR + // IID — the shape the real service produces now that the five switches + // are per linked MR. See optionsForMR. + optionsByMRIID map[int]*gitlab.TaskMRAutomationResponse optionsErr error checkpoint *gitlab.TaskMRLifecycleState checkpointErr error @@ -97,7 +101,7 @@ func (m *mockGitLabMRAutomationService) GetTaskMRAutomationResponse(ctx context. } func (m *mockGitLabMRAutomationService) GetTaskMRAutomationEvaluation( - ctx context.Context, _ string, _ string, _ string, _ int, + ctx context.Context, _ string, _ string, _ string, mrIID int, ) (*gitlab.TaskMRAutomationEvaluation, error) { m.evaluationCalls.Add(1) if m.optionsErr != nil { @@ -112,13 +116,27 @@ func (m *mockGitLabMRAutomationService) GetTaskMRAutomationEvaluation( if m.checkpointErr != nil { return nil, m.checkpointErr } - options := m.options - if options == nil { - options = &gitlab.TaskMRAutomationResponse{} - } + options := m.optionsForMR(mrIID) return &gitlab.TaskMRAutomationEvaluation{Options: options, Checkpoint: m.checkpoint}, nil } +// optionsForMR models the real service's per-MR resolution: the switches live +// per linked MR, so the evaluation snapshot for one MR carries that MR's own +// values. optionsByMRIID left nil keeps the single-options behaviour every +// other test in this package relies on. +func (m *mockGitLabMRAutomationService) optionsForMR(mrIID int) *gitlab.TaskMRAutomationResponse { + if m.optionsByMRIID != nil { + if options, ok := m.optionsByMRIID[mrIID]; ok { + return options + } + return &gitlab.TaskMRAutomationResponse{} + } + if m.options == nil { + return &gitlab.TaskMRAutomationResponse{} + } + return m.options +} + func (m *mockGitLabMRAutomationService) GetTaskMRLifecycleState(context.Context, string, string, string, int) (*gitlab.TaskMRLifecycleState, error) { if m.checkpointCalls != nil { select { diff --git a/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_ci_automation_test.go b/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_ci_automation_test.go index b4f2835656..8648f4b038 100644 --- a/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_ci_automation_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_ci_automation_test.go @@ -648,3 +648,62 @@ func TestHandleTaskMRCIAutomation_ExhaustedAutoFixStillBlocksUnreadyMerge(t *tes t.Fatalf("MergeMRForAutomation calls = %d, want 0 — a failing pipeline must still block auto-merge", fake.mergeCalls.Load()) } } + +// TestHandleTaskMRLifecycleAutomation_AutoMergeOnOneMRDoesNotMergeAnother is +// the orchestrator-level statement of the whole per-MR change: auto-merge +// enabled on one linked MR must not merge a sibling MR on the same task. +// +// It deliberately enters through handleTaskMRLifecycleAutomation rather than +// calling handleTaskMRCIAutomation with hand-built options, because the +// per-MR resolution being verified happens in GetTaskMRAutomationEvaluation — +// options passed in directly would assume away the thing under test. +func TestHandleTaskMRLifecycleAutomation_AutoMergeOnOneMRDoesNotMergeAnother(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task-1", "session-1", models.TaskSessionStateRunning) + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + + mergeableSnapshot := func(iid int) *gitlab.MRAutomationSnapshot { + return &gitlab.MRAutomationSnapshot{ + MR: &gitlab.MR{ + State: gitlabMRStateOpen, IID: iid, ProjectPath: "group/widget", + MergeStatus: "can_be_merged", DetailedMergeStatus: "mergeable", + }, + PipelineStatus: "success", + UnresolvedDiscussions: 0, + } + } + // Auto-merge is on for MR !1 only; !2 is left all-off. + fake := &mockGitLabMRAutomationService{ + optionsByMRIID: map[int]*gitlab.TaskMRAutomationResponse{ + 1: {TaskID: "task-1", AutoMergeEnabled: true, WorkspaceID: "ws-1"}, + 2: {TaskID: "task-1", WorkspaceID: "ws-1"}, + }, + snapshot: mergeableSnapshot(2), + } + svc.SetGitLabMRAutomationService(fake) + + mrTwo := &gitlab.TaskMR{ + TaskID: "task-1", Host: "https://gitlab.example.com", + ProjectPath: "group/widget", MRIID: 2, State: gitlabMRStateOpen, + } + if err := svc.handleTaskMRLifecycleAutomation(ctx, mrTwo); err != nil { + t.Fatalf("evaluate MR !2: %v", err) + } + if got := fake.mergeCalls.Load(); got != 0 { + t.Fatalf("MergeMRForAutomation calls for MR !2 = %d, want 0 — a sibling MR's auto-merge must not merge this one", got) + } + + // Same task, same fully-mergeable state, but this MR owns the switch. + fake.snapshot = mergeableSnapshot(1) + mrOne := &gitlab.TaskMR{ + TaskID: "task-1", Host: "https://gitlab.example.com", + ProjectPath: "group/widget", MRIID: 1, State: gitlabMRStateOpen, + } + if err := svc.handleTaskMRLifecycleAutomation(ctx, mrOne); err != nil { + t.Fatalf("evaluate MR !1: %v", err) + } + if got := fake.mergeCalls.Load(); got != 1 { + t.Fatalf("MergeMRForAutomation calls after MR !1 = %d, want 1 — the MR that owns the switch must still merge", got) + } +} diff --git a/apps/web/components/gitlab/mr-automation-controls.automation.test.tsx b/apps/web/components/gitlab/mr-automation-controls.automation.test.tsx index 786a033de5..0f4345f070 100644 --- a/apps/web/components/gitlab/mr-automation-controls.automation.test.tsx +++ b/apps/web/components/gitlab/mr-automation-controls.automation.test.tsx @@ -2,7 +2,12 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { cleanup, render, screen, fireEvent } from "@testing-library/react"; import { TooltipProvider } from "@kandev/ui/tooltip"; import { ToastProvider } from "@/components/toast-provider"; -import type { TaskMR, TaskMRAutomationOptions, TaskMRLifecycleState } from "@/lib/types/gitlab"; +import type { + TaskMR, + TaskMRAutomationOptions, + TaskMRAutomationOptionsForMR, + TaskMRLifecycleState, +} from "@/lib/types/gitlab"; const hookMocks = vi.hoisted(() => ({ error: null as string | null, @@ -36,6 +41,26 @@ const AUTO_FIX_LABEL = "Auto-fix CI and address comments"; const AUTO_MERGE_LABEL = "Auto-merge when ready"; const EDIT_PROMPT_LABEL = "Edit auto-fix prompt for this task"; const PROMPT_TEXTAREA_LABEL = "Task auto-fix prompt"; +const PROJECT_PATH = "group/project"; + +function makeMROptions( + overrides: Partial = {}, +): TaskMRAutomationOptionsForMR { + return { + task_id: "task-1", + repository_id: "", + project_path: PROJECT_PATH, + mr_iid: 7, + auto_fix_enabled: false, + auto_merge_enabled: false, + prompt_on_review_requested: false, + prompt_on_merged: false, + prompt_on_closed: false, + created_at: "", + updated_at: "", + ...overrides, + }; +} function makeOptions(overrides: Partial = {}): TaskMRAutomationOptions { return { @@ -51,6 +76,7 @@ function makeOptions(overrides: Partial = {}): TaskMRAu review_reviewer_username: "", updated_at: "2026-06-18T10:00:00Z", mr_states: [], + mr_options: [makeMROptions()], ...overrides, }; } @@ -60,7 +86,7 @@ function makeMR(overrides: Partial = {}): TaskMR { id: "assoc-1", task_id: "task-1", host: "https://gitlab.com", - project_path: "group/project", + project_path: PROJECT_PATH, mr_iid: 7, mr_url: "https://gitlab.com/group/project/-/merge_requests/7", mr_title: "Test MR", @@ -89,7 +115,7 @@ function makeState(overrides: Partial = {}): TaskMRLifecyc return { task_id: "task-1", repository_id: "", - project_path: "group/project", + project_path: PROJECT_PATH, mr_iid: 7, review_request_initialized: false, last_review_requested: false, @@ -105,7 +131,7 @@ function makeState(overrides: Partial = {}): TaskMRLifecyc }; } -function renderControls(mr: TaskMR | undefined = makeMR()) { +function renderControls(mr: TaskMR = makeMR()) { return render( @@ -148,46 +174,69 @@ describe("MRAutomationControls — Automation section (AC1)", () => { it("toggling auto-fix on patches auto_fix_enabled", () => { renderControls(); fireEvent.click(screen.getByLabelText(AUTO_FIX_LABEL)); - expect(hookMocks.updateMock).toHaveBeenCalledWith({ auto_fix_enabled: true }); + expect(hookMocks.updateMock).toHaveBeenCalledWith({ + repository_id: "", + project_path: PROJECT_PATH, + mr_iid: 7, + auto_fix_enabled: true, + }); }); it("toggling auto-merge on patches auto_merge_enabled", () => { renderControls(); fireEvent.click(screen.getByLabelText(AUTO_MERGE_LABEL)); - expect(hookMocks.updateMock).toHaveBeenCalledWith({ auto_merge_enabled: true }); + expect(hookMocks.updateMock).toHaveBeenCalledWith({ + repository_id: "", + project_path: PROJECT_PATH, + mr_iid: 7, + auto_merge_enabled: true, + }); }); it("shows the round-help button only when auto-fix is enabled and a single MR is known", () => { - hookMocks.options = makeOptions({ auto_fix_enabled: false }); + hookMocks.options = makeOptions({ mr_options: [makeMROptions({ auto_fix_enabled: false })] }); renderControls(); expect(screen.queryByTestId("mr-auto-fix-round-help")).toBeNull(); cleanup(); hookMocks.options = makeOptions({ - auto_fix_enabled: true, + mr_options: [makeMROptions({ auto_fix_enabled: true })], mr_states: [makeState({ auto_fix_round_count: 3 })], }); renderControls(); expect(screen.getByTestId("mr-auto-fix-round-help")).toBeTruthy(); }); - it("still shows the round-help button when no single MR is provided", () => { - hookMocks.options = makeOptions({ auto_fix_enabled: true }); - renderControls(undefined); - expect(screen.getByTestId("mr-auto-fix-round-help")).toBeTruthy(); - }); - it("shows the auto-merge readiness help button only when auto-merge is enabled", () => { - hookMocks.options = makeOptions({ auto_merge_enabled: false }); + hookMocks.options = makeOptions({ + mr_options: [makeMROptions({ auto_merge_enabled: false })], + }); renderControls(); expect(screen.queryByTestId("mr-auto-merge-help")).toBeNull(); cleanup(); - hookMocks.options = makeOptions({ auto_merge_enabled: true }); + hookMocks.options = makeOptions({ mr_options: [makeMROptions({ auto_merge_enabled: true })] }); renderControls(); expect(screen.getByTestId("mr-auto-merge-help")).toBeTruthy(); }); + it("renders switches off when options have no entry for this MR", () => { + hookMocks.options = makeOptions({ + mr_options: [ + makeMROptions({ + mr_iid: 99, + auto_fix_enabled: true, + auto_merge_enabled: true, + }), + ], + }); + renderControls(); + expect(screen.getByLabelText(AUTO_FIX_LABEL).getAttribute("data-state")).toBe("unchecked"); + expect(screen.getByLabelText(AUTO_MERGE_LABEL).getAttribute("data-state")).toBe("unchecked"); + expect(screen.queryByTestId("mr-auto-fix-round-help")).toBeNull(); + expect(screen.queryByTestId("mr-auto-merge-help")).toBeNull(); + }); + it("disables both automation switches while loading", () => { hookMocks.loading = true; hookMocks.options = null; diff --git a/apps/web/components/gitlab/mr-automation-controls.test.tsx b/apps/web/components/gitlab/mr-automation-controls.test.tsx index 84ce665c6a..b42b9fa4d8 100644 --- a/apps/web/components/gitlab/mr-automation-controls.test.tsx +++ b/apps/web/components/gitlab/mr-automation-controls.test.tsx @@ -2,7 +2,11 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { cleanup, render, screen, fireEvent } from "@testing-library/react"; import { TooltipProvider } from "@kandev/ui/tooltip"; import { ToastProvider } from "@/components/toast-provider"; -import type { TaskMRAutomationOptions } from "@/lib/types/gitlab"; +import type { + TaskMR, + TaskMRAutomationOptions, + TaskMRAutomationOptionsForMR, +} from "@/lib/types/gitlab"; const hookMocks = vi.hoisted(() => ({ error: null as string | null, @@ -44,6 +48,25 @@ const CLOSED_LABEL = "MR closed without merging"; const REVIEW_REQUESTED_LABEL = "Your review is requested"; const FOLLOW_UP_TRIGGER = "mr-review-follow-up-trigger"; +function makeMROptions( + overrides: Partial = {}, +): TaskMRAutomationOptionsForMR { + return { + task_id: "task-1", + repository_id: "", + project_path: "group/project", + mr_iid: 7, + auto_fix_enabled: false, + auto_merge_enabled: false, + prompt_on_review_requested: false, + prompt_on_merged: false, + prompt_on_closed: false, + created_at: "", + updated_at: "", + ...overrides, + }; +} + function makeOptions(overrides: Partial = {}): TaskMRAutomationOptions { return { task_id: "task-1", @@ -58,6 +81,37 @@ function makeOptions(overrides: Partial = {}): TaskMRAu review_reviewer_username: "", updated_at: "2026-06-18T10:00:00Z", mr_states: [], + mr_options: [makeMROptions()], + ...overrides, + }; +} + +function makeMR(overrides: Partial = {}): TaskMR { + return { + id: "assoc-1", + task_id: "task-1", + host: "https://gitlab.com", + project_path: "group/project", + mr_iid: 7, + mr_url: "https://gitlab.com/group/project/-/merge_requests/7", + mr_title: "Test MR", + head_branch: "feature", + base_branch: "main", + author_username: "alice", + state: "open", + approval_state: "", + pipeline_state: "", + merge_status: "", + draft: false, + approval_count: 0, + required_approvals: 0, + pipeline_jobs_total: 0, + pipeline_jobs_pass: 0, + reviewer_count: 0, + unapproved_reviewers: 0, + unresolved_discussions: 0, + created_at: "", + updated_at: "", ...overrides, }; } @@ -66,7 +120,7 @@ function renderControls() { return render( - + , ); @@ -112,7 +166,7 @@ describe("MRAutomationControls", () => { }); it("auto-expands when a switch is already on", () => { - hookMocks.options = makeOptions({ prompt_on_merged: true }); + hookMocks.options = makeOptions({ mr_options: [makeMROptions({ prompt_on_merged: true })] }); renderControls(); expect(screen.getByTestId(FOLLOW_UP_TRIGGER).getAttribute("aria-expanded")).toBe("true"); expect(screen.getByLabelText(MERGED_LABEL)).not.toBeNull(); @@ -134,13 +188,13 @@ describe("MRAutomationControls", () => { fireEvent.click(screen.getByTestId(FOLLOW_UP_TRIGGER)); expect(screen.getByLabelText(REVIEW_REQUESTED_LABEL).getAttribute("aria-describedby")).toBe( - "task-mr-review-requested-prompt-task-1-description", + "task-mr-review-requested-prompt-task-1-assoc-1-description", ); expect(screen.getByLabelText(MERGED_LABEL).getAttribute("aria-describedby")).toBe( - "task-mr-terminal-help-task-1", + "task-mr-terminal-help-task-1-assoc-1", ); expect(screen.getByLabelText(CLOSED_LABEL).getAttribute("aria-describedby")).toBe( - "task-mr-terminal-help-task-1", + "task-mr-terminal-help-task-1-assoc-1", ); expect( screen.getByText( @@ -160,9 +214,23 @@ describe("MRAutomationControls", () => { fireEvent.click(screen.getByLabelText(MERGED_LABEL)); fireEvent.click(screen.getByLabelText(CLOSED_LABEL)); - expect(hookMocks.updateMock).toHaveBeenCalledWith({ prompt_on_review_requested: true }); - expect(hookMocks.updateMock).toHaveBeenCalledWith({ prompt_on_merged: true }); - expect(hookMocks.updateMock).toHaveBeenCalledWith({ prompt_on_closed: true }); + const mrIdentity = { repository_id: "", project_path: "group/project", mr_iid: 7 }; + expect(hookMocks.updateMock).toHaveBeenCalledWith({ + ...mrIdentity, + prompt_on_review_requested: true, + }); + expect(hookMocks.updateMock).toHaveBeenCalledWith({ ...mrIdentity, prompt_on_merged: true }); + expect(hookMocks.updateMock).toHaveBeenCalledWith({ ...mrIdentity, prompt_on_closed: true }); + }); +}); + +describe("MRAutomationControls error and loading states", () => { + beforeEach(() => { + resetHookMocks(); + }); + + afterEach(() => { + cleanup(); }); it("surfaces an error from the hook", () => { diff --git a/apps/web/components/gitlab/mr-automation-controls.tsx b/apps/web/components/gitlab/mr-automation-controls.tsx index 7266f9235c..0756e99b35 100644 --- a/apps/web/components/gitlab/mr-automation-controls.tsx +++ b/apps/web/components/gitlab/mr-automation-controls.tsx @@ -1,135 +1,35 @@ "use client"; -import { useCallback, useEffect, useState, type ReactNode } from "react"; +import { useCallback, useEffect, useState } from "react"; import { useTranslation } from "react-i18next"; import { IconChevronDown, IconEdit, IconInfoCircle } from "@tabler/icons-react"; import { Button } from "@kandev/ui/button"; import { Collapsible, CollapsibleContent, CollapsibleTrigger } from "@kandev/ui/collapsible"; -import { Label } from "@kandev/ui/label"; -import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover"; -import { Switch } from "@kandev/ui/switch"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; import { useToast } from "@/components/toast-provider"; import { useTaskMRAutomationOptions } from "@/hooks/domains/gitlab/use-task-mr-automation"; import { useResponsiveBreakpoint } from "@/hooks/use-responsive-breakpoint"; -import { autoFixRoundForState, findMRAutomationStateForMR } from "@/lib/gitlab/mr-automation"; +import { + findMRAutomationOptionsForMR, + findMRAutomationStateForMR, +} from "@/lib/gitlab/mr-automation"; import type { TaskMR, TaskMRAutomationOptions, + TaskMRAutomationOptionsForMR, TaskMRAutomationPatch, - TaskMRLifecycleState, } from "@/lib/types/gitlab"; import { MRAutoFixPromptDialog } from "./mr-auto-fix-prompt-dialog"; +import { + compactRowMinHeight, + MRAgentPromptRows, + MRAutomationOptionRows, +} from "./mr-automation-rows"; -/** Shared compact-row height: taller touch target on mobile/coarse pointers. */ -function compactRowMinHeight(isMobile: boolean, isFinePointer: boolean): string { - return isMobile || !isFinePointer ? "min-h-11" : "min-h-7"; -} - -/** True when any of the three #2125 lifecycle-notification switches is on. */ -function hasLifecycleSwitchEnabled(options: TaskMRAutomationOptions | null): boolean { - return Boolean( - options?.prompt_on_review_requested || options?.prompt_on_merged || options?.prompt_on_closed, - ); -} - -/** Resolves the round-help state for a single MR, when one is known. */ -function resolveAutomationState( - mr: TaskMR | undefined, - states: TaskMRLifecycleState[] | undefined, -): TaskMRLifecycleState | undefined { - return mr ? findMRAutomationStateForMR(states, mr) : undefined; -} - -/** - * Dual-mode help affordance: a tap popover on coarse pointers, a hover - * tooltip on fine pointers. Mirrors CIAutomationHelpButton (GitHub). - */ -function MRAutomationHelpButton({ - ariaLabel, - testId, - children, -}: { - ariaLabel: string; - testId: string; - children: ReactNode; -}) { - const { isFinePointer } = useResponsiveBreakpoint(); - const [open, setOpen] = useState(false); - const trigger = ( - - ); - if (!isFinePointer) { - return ( - - {trigger} - - {children} - - - ); - } - return ( - - {trigger} - - {children} - - - ); -} - -function MRAutomationRow({ - id, - label, - checked, - disabled, - onCheckedChange, - help, - describedBy, -}: { - id: string; - label: string; - checked: boolean; - disabled: boolean; - onCheckedChange: (checked: boolean) => void; - help?: ReactNode; - describedBy?: string; -}) { - const { isFinePointer, isMobile } = useResponsiveBreakpoint(); - const minHeight = compactRowMinHeight(isMobile, isFinePointer); - +/** True when any of the three lifecycle-notification switches is on for this MR. */ +function hasLifecycleSwitchEnabled(switches: TaskMRAutomationOptionsForMR): boolean { return ( -
-
- - {help} -
- -
+ switches.prompt_on_review_requested || switches.prompt_on_merged || switches.prompt_on_closed ); } @@ -172,50 +72,28 @@ function MRAutomationLoadErrorBanner({ error, onRetry }: { error: string; onRetr ); } -function MRAutoFixRoundHelpButton({ - state, - maxRounds, -}: { - state: TaskMRLifecycleState | undefined; - maxRounds: number | null | undefined; -}) { - const { t } = useTranslation(); - const round = autoFixRoundForState(state, maxRounds); - return ( - - - {t("gitlab:mrAutoFixRoundExplanation", { current: round.current, max: round.max })} - - - ); -} - -function MRAutoMergeHelpButton() { - const { t } = useTranslation(); - return ( - - {t("gitlab:mrAutoMergeReadyExplanation")} - - ); -} - function MRAutomationHeader({ + mrIID, disabled, onEditPrompt, }: { + mrIID: number; disabled: boolean; onEditPrompt: () => void; }) { const { t } = useTranslation(); return (
-
{t("gitlab:mrAutomation")}
+
+ {t("gitlab:mrAutomation")} + {/* The switches below are scoped to this merge request, so say which. */} + + {t("gitlab:mrAutomationAppliesToMR", { iid: mrIID })} + +
@@ -249,49 +127,6 @@ function MRAutomationHeader({ ); } -function MRAutomationOptionRows({ - taskId, - options, - disabled, - patchOption, - automationState, -}: { - taskId: string; - options: TaskMRAutomationOptions | null; - disabled: boolean; - patchOption: (patch: TaskMRAutomationPatch) => void; - automationState: TaskMRLifecycleState | undefined; -}) { - const { t } = useTranslation(); - return ( - <> - patchOption({ auto_fix_enabled: checked })} - help={ - options?.auto_fix_enabled ? ( - - ) : null - } - /> - patchOption({ auto_merge_enabled: checked })} - help={options?.auto_merge_enabled ? : null} - /> - - ); -} - /** * Encapsulates the auto-fix prompt editor's local state and save/reset * handlers, split out of MRAutomationControls to keep that component under @@ -328,6 +163,8 @@ function useMRAutoFixPromptEditor( setPromptOpen(true); }, [options]); + // The auto-fix prompt override is task-level (it applies to every linked + // MR), so these patches intentionally carry no MR identity. const savePrompt = useCallback(() => { const value = promptDraft.trim(); if (!value) return; @@ -355,15 +192,26 @@ function useMRAutoFixPromptEditor( /** * Wraps `update` with the standard "toast on failure" handling shared by - * every switch row. Split out of MRAutomationControls to keep that + * every switch row, and stamps `mr`'s identity onto each patch so the backend + * applies the change to this merge request alone instead of fanning it out to + * every MR linked to the task. Split out of MRAutomationControls to keep that * component under the file's complexity limit. */ -function useMRAutomationPatch(update: (patch: TaskMRAutomationPatch) => Promise) { +function useMRAutomationPatch( + mr: TaskMR, + update: (patch: TaskMRAutomationPatch) => Promise, +) { const { t } = useTranslation(); const { toast } = useToast(); return useCallback( (patch: TaskMRAutomationPatch) => { - update(patch).catch((err) => { + const scoped: TaskMRAutomationPatch = { + ...patch, + repository_id: mr.repository_id ?? "", + project_path: mr.project_path, + mr_iid: mr.mr_iid, + }; + update(scoped).catch((err) => { toast({ title: t("gitlab:mrAutomationUpdateFailedTitle"), description: @@ -372,127 +220,35 @@ function useMRAutomationPatch(update: (patch: TaskMRAutomationPatch) => Promise< }); }); }, - [t, toast, update], - ); -} - -function ReviewRequestedPromptRow({ - taskId, - options, - disabled, - patchOption, -}: { - taskId: string; - options: TaskMRAutomationOptions | null; - disabled: boolean; - patchOption: (patch: TaskMRAutomationPatch) => void; -}) { - const { t } = useTranslation(); - const helpID = `task-mr-review-requested-prompt-${taskId}-description`; - const help = t("gitlab:mrAutomationReviewRequestedHelp"); - return ( - <> - - {help} - - patchOption({ prompt_on_review_requested: checked })} - help={ - - {help} - - } - /> - - ); -} - -function MRAgentPromptRows({ - taskId, - options, - disabled, - patchOption, -}: { - taskId: string; - options: TaskMRAutomationOptions | null; - disabled: boolean; - patchOption: (patch: TaskMRAutomationPatch) => void; -}) { - const { t } = useTranslation(); - const terminalHelpID = `task-mr-terminal-help-${taskId}`; - const terminalHelp = t("gitlab:mrAutomationTerminalHelp"); - return ( - <> - - - {terminalHelp} - - patchOption({ prompt_on_merged: checked })} - help={ - - {terminalHelp} - - } - /> - patchOption({ prompt_on_closed: checked })} - /> - + [mr, t, toast, update], ); } /** - * MR automation controls: an "Automation" section (auto-fix CI + auto-merge) - * above a collapsible "Review follow-up" group of three compact switch rows. - * Renders inside the GitLab MR topbar dropdown, below the per-MR items. - * Auto-expands "Review follow-up" when any of its switches is already on so - * a previously configured task doesn't hide its active switches. Mirrors - * PRCIAutomationControls + ReviewFollowUpSection (GitHub), AC1, AC29. + * MR automation controls for one linked merge request: an "Automation" + * section (auto-fix CI + auto-merge) above a collapsible "Review follow-up" + * group of three compact switch rows. Renders inside the GitLab MR topbar + * dropdown and hover popover. Auto-expands "Review follow-up" when any of its + * switches is already on so a previously configured MR doesn't hide its active + * switches. Mirrors PRCIAutomationControls + ReviewFollowUpSection (GitHub). * - * `mr` is optional and only used to look up the per-MR auto-fix round state - * for the round-help button's count — when omitted (e.g. a task with - * multiple linked MRs and no single one to attribute rounds to), the - * round-help button still renders but reads 0 of max; the switches - * themselves remain task-scoped either way. + * `mr` is required: all five switches are per-MR, so there is no meaningful + * task-wide rendering of this group. A task with several linked MRs renders + * one instance per MR. */ -export function MRAutomationControls({ taskId, mr }: { taskId: string; mr?: TaskMR }) { +export function MRAutomationControls({ taskId, mr }: { taskId: string; mr: TaskMR }) { const { options, loading, saving, error, update, refresh, resetPrompt } = useTaskMRAutomationOptions(taskId); const { isFinePointer, isMobile } = useResponsiveBreakpoint(); const { t } = useTranslation(); const [open, setOpen] = useState(false); const promptEditor = useMRAutoFixPromptEditor(options, update, resetPrompt); - const patchOption = useMRAutomationPatch(update); - const lifecycleEnabled = hasLifecycleSwitchEnabled(options); + const patchOption = useMRAutomationPatch(mr, update); + const switches = findMRAutomationOptionsForMR(options?.mr_options, mr); + const lifecycleEnabled = hasLifecycleSwitchEnabled(switches); const minHeight = compactRowMinHeight(isMobile, isFinePointer); - const automationState = resolveAutomationState(mr, options?.mr_states); + const automationState = findMRAutomationStateForMR(options?.mr_states, mr); + const elementIDSuffix = `${taskId}-${mr.id}`; useEffect(() => { if (lifecycleEnabled) setOpen(true); @@ -502,7 +258,7 @@ export function MRAutomationControls({ taskId, mr }: { taskId: string; mr?: Task const disabled = saving || loading || !options; return ( -
+
{loadFailed ? ( ) : null} - + @@ -543,8 +304,8 @@ export function MRAutomationControls({ taskId, mr }: { taskId: string; mr?: Task {t("gitlab:mrAutomationDescription")}

diff --git a/apps/web/components/gitlab/mr-automation-rows.tsx b/apps/web/components/gitlab/mr-automation-rows.tsx new file mode 100644 index 0000000000..eb60274bf2 --- /dev/null +++ b/apps/web/components/gitlab/mr-automation-rows.tsx @@ -0,0 +1,280 @@ +"use client"; + +import { useState, type ReactNode } from "react"; +import { useTranslation } from "react-i18next"; +import { IconInfoCircle } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { Label } from "@kandev/ui/label"; +import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover"; +import { Switch } from "@kandev/ui/switch"; +import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; +import { useResponsiveBreakpoint } from "@/hooks/use-responsive-breakpoint"; +import { autoFixRoundForState } from "@/lib/gitlab/mr-automation"; +import type { + TaskMRAutomationOptionsForMR, + TaskMRAutomationPatch, + TaskMRLifecycleState, +} from "@/lib/types/gitlab"; + +/** Shared compact-row height: taller touch target on mobile/coarse pointers. */ +export function compactRowMinHeight(isMobile: boolean, isFinePointer: boolean): string { + return isMobile || !isFinePointer ? "min-h-11" : "min-h-7"; +} + +/** + * Dual-mode help affordance: a tap popover on coarse pointers, a hover + * tooltip on fine pointers. Mirrors CIAutomationHelpButton (GitHub). + */ +export function MRAutomationHelpButton({ + ariaLabel, + testId, + children, +}: { + ariaLabel: string; + testId: string; + children: ReactNode; +}) { + const { isFinePointer } = useResponsiveBreakpoint(); + const [open, setOpen] = useState(false); + const trigger = ( + + ); + if (!isFinePointer) { + return ( + + {trigger} + + {children} + + + ); + } + return ( + + {trigger} + + {children} + + + ); +} + +function MRAutomationRow({ + id, + label, + checked, + disabled, + onCheckedChange, + help, + describedBy, +}: { + id: string; + label: string; + checked: boolean; + disabled: boolean; + onCheckedChange: (checked: boolean) => void; + help?: ReactNode; + describedBy?: string; +}) { + const { isFinePointer, isMobile } = useResponsiveBreakpoint(); + const minHeight = compactRowMinHeight(isMobile, isFinePointer); + + return ( +
+
+ + {help} +
+ +
+ ); +} + +function MRAutoFixRoundHelpButton({ + state, + maxRounds, +}: { + state: TaskMRLifecycleState | undefined; + maxRounds: number | null | undefined; +}) { + const { t } = useTranslation(); + const round = autoFixRoundForState(state, maxRounds); + return ( + + + {t("gitlab:mrAutoFixRoundExplanation", { current: round.current, max: round.max })} + + + ); +} + +function MRAutoMergeHelpButton() { + const { t } = useTranslation(); + return ( + + {t("gitlab:mrAutoMergeReadyExplanation")} + + ); +} + +/** + * Props shared by every switch-row group. `elementIDSuffix` scopes the DOM ids + * to one merge request: the multi-MR dropdown renders one group per linked MR, + * so a task-only suffix would emit duplicate ids and point every `
))} - - {canLink ? ( <> @@ -381,7 +381,6 @@ function MRMenuButton({ { await apiClient.configureGitLab(workspaceId, host); + await seedGitLabMRData(apiClient, workspaceId, iid, title, host); +} + +// The MR-data half of seedGitLabReview, without the `configureGitLab` call. +// Configuring the connection invalidates and rebuilds the workspace's cached +// GitLab client (SetConfigForWorkspace -> invalidateWorkspaceClient), which +// discards the in-memory MockClient's previously seeded state — so seeding +// two-or-more MRs on one workspace must configure once, then call this for +// each MR, rather than calling seedGitLabReview (and its configureGitLab) +// per MR. +export async function seedGitLabMRData( + apiClient: ApiClient, + workspaceId: string, + iid: number, + title: string, + host = GITLAB_HOST, +): Promise { const mr = gitLabMR(iid, title, { url: `${host}/${GITLAB_PROJECT}/-/merge_requests/${iid}`, web_url: `${host}/${GITLAB_PROJECT}/-/merge_requests/${iid}`, @@ -108,3 +133,43 @@ export async function seedGitLabReview( }, ]); } + +export async function seedTaskWithLinkedGitLabMRs( + apiClient: ApiClient, + seedData: GitLabTaskSeedData, + title: string, + iids: number[], + mrTitlePrefix: string, +): Promise { + // Configuring the connection invalidates and rebuilds the workspace's cached + // mock client, so it must happen before seeding every MR for this task. + await apiClient.configureGitLab(seedData.workspaceId, GITLAB_HOST); + for (const iid of iids) { + await seedGitLabMRData(apiClient, seedData.workspaceId, iid, `${mrTitlePrefix} ${iid}`); + } + await apiClient.updateRepository(seedData.repositoryId, { + provider: "gitlab", + provider_host: GITLAB_HOST, + provider_owner: "platform", + provider_name: "kandev", + }); + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + title, + seedData.agentProfileId, + { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + for (const iid of iids) { + await apiClient.linkTaskGitLabMR(seedData.workspaceId, { + task_id: task.id, + repository_id: seedData.repositoryId, + mr_url: `${GITLAB_HOST}/${GITLAB_PROJECT}/-/merge_requests/${iid}`, + }); + } + return task.id; +} diff --git a/apps/web/e2e/helpers/submodule-review-helpers.test.ts b/apps/web/e2e/helpers/submodule-review-helpers.test.ts new file mode 100644 index 0000000000..dd91592813 --- /dev/null +++ b/apps/web/e2e/helpers/submodule-review-helpers.test.ts @@ -0,0 +1,16 @@ +import { describe, expect, it, vi } from "vitest"; +import { retryGitIndexLock } from "../tests/review/submodule-review-helpers"; + +describe("retryGitIndexLock", () => { + it("retries a transient Git index lock before returning the operation result", async () => { + const operation = vi + .fn<() => string>() + .mockImplementationOnce(() => { + throw new Error("fatal: Unable to create index.lock: File exists"); + }) + .mockReturnValueOnce("complete"); + + await expect(retryGitIndexLock(operation)).resolves.toBe("complete"); + expect(operation).toHaveBeenCalledTimes(2); + }); +}); diff --git a/apps/web/e2e/manual-seed-gitlab-mr-automation.ts b/apps/web/e2e/manual-seed-gitlab-mr-automation.ts new file mode 100644 index 0000000000..ee02331e5c --- /dev/null +++ b/apps/web/e2e/manual-seed-gitlab-mr-automation.ts @@ -0,0 +1,112 @@ +// One-off manual seed script for the "Scope GitLab MR automation switches +// per MR" task's isolated environment (STEP 3 of the Work phase). Not part +// of the automated test suite — run with `npx tsx` against a manually +// launched backend. Seeds a workspace/workflow/repo and a task with two +// linked GitLab MRs so the multi-MR dropdown independence UI can be +// exercised by hand. +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; +import { ApiClient } from "./helpers/api-client"; +import { seedGitLabMRData, GITLAB_HOST, GITLAB_PROJECT } from "./helpers/gitlab"; +import { dwell } from "./helpers/causal-waits"; + +const BASE_URL = process.env.KANDEV_BASE_URL || "http://localhost:18500"; +// Resolved from this file's own location so the script works in any checkout; +// `__dirname` matches how global-setup.ts and the e2e helpers locate paths. +const REPO_ROOT = + process.env.KANDEV_SEED_REPO_ROOT || path.resolve(__dirname, "../../backend/.manual-env/repos"); + +async function main() { + const apiClient = new ApiClient(BASE_URL); + + const workspace = await apiClient.createWorkspace("GitLab MR Automation Demo"); + const workflow = await apiClient.createWorkflow(workspace.id, "Demo Workflow", "simple"); + const { steps } = await apiClient.listWorkflowSteps(workflow.id); + const sorted = steps.sort((a, b) => a.position - b.position); + const startStep = sorted.find((s) => s.is_start_step) ?? sorted[0]; + + const remoteDir = path.join(REPO_ROOT, "e2e-remote.git"); + const repoDir = path.join(REPO_ROOT, "e2e-repo"); + fs.mkdirSync(REPO_ROOT, { recursive: true }); + if (!fs.existsSync(remoteDir)) { + execFileSync("git", ["init", "--bare", "-b", "main", remoteDir]); + fs.mkdirSync(repoDir, { recursive: true }); + execFileSync("git", ["init", "-b", "main"], { cwd: repoDir }); + execFileSync( + "git", + [ + "-c", + "user.name=Demo", + "-c", + "user.email=demo@test.local", + "commit", + "--allow-empty", + "-m", + "init", + ], + { cwd: repoDir }, + ); + execFileSync("git", ["remote", "add", "origin", `file://${remoteDir}`], { cwd: repoDir }); + execFileSync("git", ["push", "origin", "main"], { cwd: repoDir }); + } + const repo = await apiClient.createRepository(workspace.id, repoDir); + + let agentProfileId: string | undefined; + const deadline = Date.now() + 30_000; + while (Date.now() < deadline) { + const { agents } = await apiClient.listAgents(); + agentProfileId = agents[0]?.profiles[0]?.id; + if (agentProfileId) break; + await dwell( + 250, + "poll-interval", + "polling listAgents() until the backend's initial agent setup completes", + ); + } + if (!agentProfileId) throw new Error("no agent profile available after 30s"); + + const iidA = 220; + const iidB = 221; + await apiClient.configureGitLab(workspace.id, GITLAB_HOST); + await seedGitLabMRData(apiClient, workspace.id, iidA, "Fix pagination bug"); + await seedGitLabMRData(apiClient, workspace.id, iidB, "Add dark mode toggle"); + await apiClient.updateRepository(repo.id, { + provider: "gitlab", + provider_host: GITLAB_HOST, + provider_owner: "platform", + provider_name: "kandev", + }); + + const task = await apiClient.createTaskWithAgent( + workspace.id, + "Demo: two linked GitLab MRs", + agentProfileId, + { + description: "Demo task seeded for manual verification of per-MR automation scoping.", + workflow_id: workflow.id, + workflow_step_id: startStep.id, + repository_ids: [repo.id], + }, + ); + await apiClient.linkTaskGitLabMR(workspace.id, { + task_id: task.id, + repository_id: repo.id, + mr_url: `${GITLAB_HOST}/${GITLAB_PROJECT}/-/merge_requests/${iidA}`, + }); + await apiClient.linkTaskGitLabMR(workspace.id, { + task_id: task.id, + repository_id: repo.id, + mr_url: `${GITLAB_HOST}/${GITLAB_PROJECT}/-/merge_requests/${iidB}`, + }); + + console.log("Seed complete."); + console.log(`Workspace: ${workspace.id}`); + console.log(`Task: ${task.id}`); + console.log(`Open: ${BASE_URL}/t/${task.id}`); +} + +main().catch((err) => { + console.error(err); + process.exit(1); +}); diff --git a/apps/web/e2e/tests/gitlab/mobile-mr-automation-options.spec.ts b/apps/web/e2e/tests/gitlab/mobile-mr-automation-options.spec.ts index 1a8f3e5b1f..26730095c5 100644 --- a/apps/web/e2e/tests/gitlab/mobile-mr-automation-options.spec.ts +++ b/apps/web/e2e/tests/gitlab/mobile-mr-automation-options.spec.ts @@ -1,6 +1,11 @@ import { test, expect } from "../../fixtures/test-base"; import { SessionPage } from "../../pages/session-page"; -import { seedGitLabReview, GITLAB_HOST, GITLAB_PROJECT } from "../../helpers/gitlab"; +import { + seedGitLabReview, + seedTaskWithLinkedGitLabMRs, + GITLAB_HOST, + GITLAB_PROJECT, +} from "../../helpers/gitlab"; import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions"; import type { ApiClient } from "../../helpers/api-client"; import type { SeedData } from "../../fixtures/test-base"; @@ -188,4 +193,59 @@ test.describe("mobile GitLab MR automation options", () => { await assertNoDocumentHorizontalOverflow(testPage, "mobile MR automation load error"); }); + + test("dropdown renders one attributed automation block per linked MR, independently toggleable (AC1, AC26)", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + const iidA = 222; + const iidB = 223; + const taskId = await seedTaskWithLinkedGitLabMRs( + apiClient, + seedData, + "Mobile MR automation independence", + [iidA, iidB], + "Mobile MR automation independence", + ); + + await testPage.goto(`/t/${taskId}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + const mrButton = testPage.getByTestId("mr-topbar-button"); + await expect(mrButton).toBeVisible({ timeout: 15_000 }); + await mrButton.tap(); + await waitForDropdownSettled(testPage); + + const controlsA = testPage.locator( + `[data-testid="mr-automation-controls"][data-mr-iid="${iidA}"]`, + ); + const controlsB = testPage.locator( + `[data-testid="mr-automation-controls"][data-mr-iid="${iidB}"]`, + ); + await expect(controlsA).toBeVisible(); + await expect(controlsB).toBeVisible(); + await expect(controlsA.getByTestId("mr-automation-scope-label")).toHaveText( + `Applies to !${iidA}`, + ); + await expect(controlsB.getByTestId("mr-automation-scope-label")).toHaveText( + `Applies to !${iidB}`, + ); + + const autoFixA = controlsA.getByRole("switch", { name: "Auto-fix CI and address comments" }); + const autoFixB = controlsB.getByRole("switch", { name: "Auto-fix CI and address comments" }); + await autoFixA.tap(); + await expect + .poll(async () => { + const options = await apiClient.getTaskMRAutomationOptions(taskId); + return options.mr_options?.find((o) => o.mr_iid === iidA)?.auto_fix_enabled; + }) + .toBe(true); + const options = await apiClient.getTaskMRAutomationOptions(taskId); + expect(options.mr_options?.find((o) => o.mr_iid === iidB)?.auto_fix_enabled).toBe(false); + await expect(autoFixB).not.toBeChecked(); + + await assertNoDocumentHorizontalOverflow(testPage, "mobile MR automation multi-MR dropdown"); + }); }); diff --git a/apps/web/e2e/tests/gitlab/mr-automation-options.spec.ts b/apps/web/e2e/tests/gitlab/mr-automation-options.spec.ts index fb88494d73..ee305e7d0f 100644 --- a/apps/web/e2e/tests/gitlab/mr-automation-options.spec.ts +++ b/apps/web/e2e/tests/gitlab/mr-automation-options.spec.ts @@ -1,6 +1,11 @@ import { test, expect } from "../../fixtures/test-base"; import { SessionPage } from "../../pages/session-page"; -import { seedGitLabReview, GITLAB_HOST, GITLAB_PROJECT } from "../../helpers/gitlab"; +import { + seedGitLabReview, + seedTaskWithLinkedGitLabMRs, + GITLAB_HOST, + GITLAB_PROJECT, +} from "../../helpers/gitlab"; import type { ApiClient } from "../../helpers/api-client"; import type { SeedData } from "../../fixtures/test-base"; @@ -231,3 +236,149 @@ test.describe("GitLab MR automation options", () => { ).not.toBeChecked(); }); }); + +test.describe("GitLab MR automation — multi-MR independence (AC1-AC3, AC26)", () => { + test("toggling one linked MR's switches does not affect a second linked MR, and survives reload", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + const iidA = 220; + const iidB = 221; + const taskId = await seedTaskWithLinkedGitLabMRs( + apiClient, + seedData, + "MR automation independence", + [iidA, iidB], + "MR automation independence", + ); + await openTask(testPage, taskId); + + // 2+ linked MRs always render the click-only dropdown — never the + // single-MR hover popover — with one attributed Automation block per MR. + await testPage.getByTestId("mr-topbar-button").click(); + const controlsA = testPage.locator( + `[data-testid="mr-automation-controls"][data-mr-iid="${iidA}"]`, + ); + const controlsB = testPage.locator( + `[data-testid="mr-automation-controls"][data-mr-iid="${iidB}"]`, + ); + await expect(controlsA).toBeVisible(); + await expect(controlsB).toBeVisible(); + + // AC25: each block states which MR it applies to. + await expect(controlsA.getByTestId("mr-automation-scope-label")).toHaveText( + `Applies to !${iidA}`, + ); + await expect(controlsB.getByTestId("mr-automation-scope-label")).toHaveText( + `Applies to !${iidB}`, + ); + + const autoFixA = controlsA.getByRole("switch", { name: "Auto-fix CI and address comments" }); + const autoFixB = controlsB.getByRole("switch", { name: "Auto-fix CI and address comments" }); + await expect(autoFixA).not.toBeChecked(); + await expect(autoFixB).not.toBeChecked(); + + // AC27: unique element ids per MR, not duplicated across simultaneously + // mounted blocks. + const autoFixAID = await autoFixA.getAttribute("id"); + const autoFixBID = await autoFixB.getAttribute("id"); + expect(autoFixAID).not.toBeNull(); + expect(autoFixAID).not.toBe(autoFixBID); + + // AC1: enabling MR A's auto-fix switch does not enable MR B's. + await autoFixA.click(); + await expect + .poll(async () => { + const options = await apiClient.getTaskMRAutomationOptions(taskId); + return options.mr_options?.find((o) => o.mr_iid === iidA)?.auto_fix_enabled; + }) + .toBe(true); + const midOptions = await apiClient.getTaskMRAutomationOptions(taskId); + expect(midOptions.mr_options?.find((o) => o.mr_iid === iidB)?.auto_fix_enabled).toBe(false); + await expect(autoFixB).not.toBeChecked(); + + // AC2: same independence for auto-merge. + const autoMergeA = controlsA.getByRole("switch", { name: "Auto-merge when ready" }); + const autoMergeB = controlsB.getByRole("switch", { name: "Auto-merge when ready" }); + await autoMergeA.click(); + await expect + .poll(async () => { + const options = await apiClient.getTaskMRAutomationOptions(taskId); + return options.mr_options?.find((o) => o.mr_iid === iidA)?.auto_merge_enabled; + }) + .toBe(true); + const afterMergeOptions = await apiClient.getTaskMRAutomationOptions(taskId); + expect(afterMergeOptions.mr_options?.find((o) => o.mr_iid === iidB)?.auto_merge_enabled).toBe( + false, + ); + await expect(autoMergeB).not.toBeChecked(); + + // AC3: same independence for the three Review follow-up switches — MR A only. + await controlsA.getByTestId("mr-review-follow-up-trigger").click(); + await controlsA.getByRole("switch", { name: "Your review is requested" }).click(); + await controlsA.getByRole("switch", { name: "MR merged" }).click(); + await controlsA.getByRole("switch", { name: "MR closed without merging" }).click(); + await expect + .poll(async () => { + const options = await apiClient.getTaskMRAutomationOptions(taskId); + const option = options.mr_options?.find((o) => o.mr_iid === iidA); + return { + prompt_on_review_requested: option?.prompt_on_review_requested, + prompt_on_merged: option?.prompt_on_merged, + prompt_on_closed: option?.prompt_on_closed, + }; + }) + .toEqual({ + prompt_on_review_requested: true, + prompt_on_merged: true, + prompt_on_closed: true, + }); + const afterReviewOptions = await apiClient.getTaskMRAutomationOptions(taskId); + const optionB = afterReviewOptions.mr_options?.find((o) => o.mr_iid === iidB); + expect(optionB).toMatchObject({ + prompt_on_review_requested: false, + prompt_on_merged: false, + prompt_on_closed: false, + }); + + // AC1 (reload): !A stays on, !B stays off after a fresh mount. + await testPage.reload(); + await expect(testPage.getByTestId("mr-topbar-button")).toBeVisible({ timeout: 15_000 }); + await testPage.getByTestId("mr-topbar-button").click(); + const reloadedControlsA = testPage.locator( + `[data-testid="mr-automation-controls"][data-mr-iid="${iidA}"]`, + ); + const reloadedControlsB = testPage.locator( + `[data-testid="mr-automation-controls"][data-mr-iid="${iidB}"]`, + ); + await expect( + reloadedControlsA.getByRole("switch", { name: "Auto-fix CI and address comments" }), + ).toBeChecked(); + await expect( + reloadedControlsA.getByRole("switch", { name: "Auto-merge when ready" }), + ).toBeChecked(); + await expect( + reloadedControlsA.getByRole("switch", { name: "Your review is requested" }), + ).toBeChecked(); + await expect(reloadedControlsA.getByRole("switch", { name: "MR merged" })).toBeChecked(); + await expect( + reloadedControlsA.getByRole("switch", { name: "MR closed without merging" }), + ).toBeChecked(); + await expect( + reloadedControlsB.getByRole("switch", { name: "Auto-fix CI and address comments" }), + ).not.toBeChecked(); + await expect( + reloadedControlsB.getByRole("switch", { name: "Auto-merge when ready" }), + ).not.toBeChecked(); + await reloadedControlsB.getByTestId("mr-review-follow-up-trigger").click(); + await expect( + reloadedControlsB.getByRole("switch", { name: "Your review is requested" }), + ).not.toBeChecked(); + await expect(reloadedControlsB.getByRole("switch", { name: "MR merged" })).not.toBeChecked(); + await expect( + reloadedControlsB.getByRole("switch", { name: "MR closed without merging" }), + ).not.toBeChecked(); + }); +}); diff --git a/apps/web/e2e/tests/gitlab/mr-status-chip.spec.ts b/apps/web/e2e/tests/gitlab/mr-status-chip.spec.ts index a0eb3d48a7..6cf0d7b96f 100644 --- a/apps/web/e2e/tests/gitlab/mr-status-chip.spec.ts +++ b/apps/web/e2e/tests/gitlab/mr-status-chip.spec.ts @@ -232,6 +232,65 @@ test.describe("GitLab MR status chip", () => { await expect(chip.getByTestId("mr-status-auto-merge-chip")).toBeVisible(); }); + // The switches are per MR, so the response's top-level booleans are an + // aggregate ("on for every linked MR"). The chip used to read that + // aggregate, so enabling automation on one of two linked MRs rendered no + // badge row at all (mr-status-chip-trigger.tsx returns null when both + // flags are false) while auto-fix was genuinely running on that MR. + test("renders the badges when only one of two linked MRs has automation on", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + const ARMED_IID = 410; + const IDLE_IID = 411; + // Configure once, then seed both MRs: each configureGitLab call rebuilds + // the workspace's cached mock client and discards MRs seeded before it. + await apiClient.configureGitLab(seedData.workspaceId, GITLAB_HOST); + await apiClient.mockGitLabAddMRs(seedData.workspaceId, GITLAB_PROJECT, [ + mrSeed(ARMED_IID, "Armed MR"), + mrSeed(IDLE_IID, "Idle MR"), + ]); + await apiClient.mockGitLabAddPipelines(seedData.workspaceId, GITLAB_PROJECT, [ + pipelineSeed(ARMED_IID, "success"), + ]); + await apiClient.mockGitLabAddApprovals(seedData.workspaceId, GITLAB_PROJECT, ARMED_IID, [], 1); + await apiClient.mockGitLabAddApprovals(seedData.workspaceId, GITLAB_PROJECT, IDLE_IID, [], 1); + + const task = await createTask(apiClient, seedData, "MR chip per-MR badges"); + await linkMR(apiClient, seedData, task.id, ARMED_IID); + await linkMR(apiClient, seedData, task.id, IDLE_IID); + + await apiClient.updateTaskMRAutomationOptions(task.id, { + repository_id: seedData.repositoryId, + project_path: GITLAB_PROJECT, + mr_iid: ARMED_IID, + auto_fix_enabled: true, + auto_merge_enabled: true, + }); + + // Pin the precondition: the aggregate the chip used to read is false, + // so a passing assertion below cannot come from the old code path. + const options = await apiClient.getTaskMRAutomationOptions(task.id); + expect(options.auto_fix_enabled).toBe(false); + expect(options.auto_merge_enabled).toBe(false); + expect(options.mr_options?.find((o) => o.mr_iid === ARMED_IID)?.auto_fix_enabled).toBe(true); + expect(options.mr_options?.find((o) => o.mr_iid === IDLE_IID)?.auto_fix_enabled).toBe(false); + + const session = new SessionPage(testPage); + await openTask(testPage, session, task.id); + + const chip = session.mrStatusChip(); + await expect(chip).toBeVisible({ timeout: 15_000 }); + await expect(chip).toHaveAttribute("data-mr-count", "2"); + const autoFixBadge = chip.getByTestId("mr-status-auto-fix-chip"); + await expect(autoFixBadge).toBeVisible(); + // The round comes from the armed MR, which has no fix rounds yet. + await expect(autoFixBadge).toContainText("0/10"); + await expect(chip.getByTestId("mr-status-auto-merge-chip")).toBeVisible(); + }); + test("DOM order: pr-status-chip precedes mr-status-chip when a task has both an open PR and MR", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/review/mobile-submodule-review.spec.ts b/apps/web/e2e/tests/review/mobile-submodule-review.spec.ts index 1fed80d7de..d07ff208c4 100644 --- a/apps/web/e2e/tests/review/mobile-submodule-review.spec.ts +++ b/apps/web/e2e/tests/review/mobile-submodule-review.spec.ts @@ -25,7 +25,7 @@ test.describe("Nested submodule Review on mobile", () => { await session.waitForChatIdle({ timeout: 45_000 }); const worktreePath = await fixture.waitForWorktree(apiClient); - fixture.applyNestedChanges(worktreePath); + await fixture.applyNestedChanges(worktreePath); await testPage.getByRole("button", { name: "Changes" }).tap(); const changesPanel = testPage.getByTestId("mobile-changes-panel"); diff --git a/apps/web/e2e/tests/review/submodule-review-helpers.ts b/apps/web/e2e/tests/review/submodule-review-helpers.ts index 1d7e974159..db3362cb23 100644 --- a/apps/web/e2e/tests/review/submodule-review-helpers.ts +++ b/apps/web/e2e/tests/review/submodule-review-helpers.ts @@ -8,13 +8,15 @@ import type { SeedData } from "../../fixtures/test-base"; import { makeGitEnv } from "../../helpers/git-helper"; const GIT_PROTOCOL_ARGS = ["-c", "protocol.file.allow=always"]; +const GIT_INDEX_LOCK_ATTEMPTS = 3; +const GIT_INDEX_LOCK_RETRY_MS = 300; export type SubmoduleReviewFixture = { taskId: string; sessionId: string; sourceRoot: string; waitForWorktree: (apiClient: ApiClient) => Promise; - applyNestedChanges: (worktreePath: string) => void; + applyNestedChanges: (worktreePath: string) => Promise; cleanup: () => void; }; @@ -27,6 +29,26 @@ function runGit(repoPath: string, args: string[], env: NodeJS.ProcessEnv): strin }); } +/** Retries the short-lived index lock taken by the backend's Git status refresh. */ +export async function retryGitIndexLock(operation: () => T): Promise { + for (let attempt = 0; attempt < GIT_INDEX_LOCK_ATTEMPTS; attempt++) { + try { + return operation(); + } catch (error) { + const isLastAttempt = attempt === GIT_INDEX_LOCK_ATTEMPTS - 1; + if (!(error instanceof Error) || !error.message.includes("index.lock") || isLastAttempt) { + throw error; + } + await dwell( + GIT_INDEX_LOCK_RETRY_MS, + "poll-interval", + "the backend's periodic Git status refresh can briefly hold the submodule index lock without publishing a completion event", + ); + } + } + throw new Error("Git index lock retry exhausted"); +} + function initializeRepository( repoPath: string, env: NodeJS.ProcessEnv, @@ -48,9 +70,9 @@ export function readGitValue(repoPath: string, args: string[], tempRoot: string) }).trim(); } -function commit(repoPath: string, env: NodeJS.ProcessEnv, message: string): void { - runGit(repoPath, ["add", "-A"], env); - runGit(repoPath, ["commit", "-m", message], env); +async function commit(repoPath: string, env: NodeJS.ProcessEnv, message: string): Promise { + await retryGitIndexLock(() => runGit(repoPath, ["add", "-A"], env)); + await retryGitIndexLock(() => runGit(repoPath, ["commit", "-m", message], env)); } async function waitForWorktreePath( @@ -92,7 +114,7 @@ export async function createSubmoduleReviewFixture( initializeRepository(outerPath, env, "README.md", "outer base\n"); runGit(outerPath, [...GIT_PROTOCOL_ARGS, "submodule", "add", "../inner", "vendor/inner"], env); - commit(outerPath, env, "add nested inner submodule"); + await commit(outerPath, env, "add nested inner submodule"); initializeRepository(parentPath, env, "README.md", "parent base\n"); runGit(parentPath, [...GIT_PROTOCOL_ARGS, "submodule", "add", "../outer", "vendor/outer"], env); @@ -102,7 +124,7 @@ export async function createSubmoduleReviewFixture( [...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"], env, ); - commit(parentPath, env, "add outer submodule"); + await commit(parentPath, env, "add outer submodule"); const repository = await apiClient.createRepository(seedData.workspaceId, parentPath, "main", { name: "nested-submodule-parent", @@ -131,22 +153,24 @@ export async function createSubmoduleReviewFixture( sessionId: task.session_id, sourceRoot, waitForWorktree: (client) => waitForWorktreePath(client, task.id, task.session_id!), - applyNestedChanges(worktreePath: string) { + async applyNestedChanges(worktreePath: string) { const outerWorktree = path.join(worktreePath, "vendor/outer"); const innerWorktree = path.join(outerWorktree, "vendor/inner"); if (!fs.existsSync(innerWorktree)) { - runGit( - worktreePath, - [...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"], - env, + await retryGitIndexLock(() => + runGit( + worktreePath, + [...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"], + env, + ), ); } fs.appendFileSync(path.join(worktreePath, "README.md"), "parent working-tree change\n"); fs.appendFileSync(path.join(outerWorktree, "README.md"), "outer committed change\n"); - commit(outerWorktree, env, "change outer submodule"); + await commit(outerWorktree, env, "change outer submodule"); fs.appendFileSync(path.join(innerWorktree, "README.md"), "inner committed change\n"); - commit(innerWorktree, env, "change inner submodule"); - commit(outerWorktree, env, "record inner submodule change"); + await commit(innerWorktree, env, "change inner submodule"); + await commit(outerWorktree, env, "record inner submodule change"); }, cleanup, }; diff --git a/apps/web/e2e/tests/review/submodule-review.spec.ts b/apps/web/e2e/tests/review/submodule-review.spec.ts index f85bdb944d..6d7b2060e5 100644 --- a/apps/web/e2e/tests/review/submodule-review.spec.ts +++ b/apps/web/e2e/tests/review/submodule-review.spec.ts @@ -47,7 +47,7 @@ test.describe("Nested submodule Review", () => { await session.waitForChatIdle({ timeout: 45_000 }); const worktreePath = await fixture.waitForWorktree(apiClient); - fixture.applyNestedChanges(worktreePath); + await fixture.applyNestedChanges(worktreePath); const parentBaseSha = readGitValue(worktreePath, ["rev-parse", "HEAD"], backend.tmpDir); await session.clickTab("Changes"); diff --git a/apps/web/eslint.i18n.options.mjs b/apps/web/eslint.i18n.options.mjs index c57a8456dc..77ea900155 100644 --- a/apps/web/eslint.i18n.options.mjs +++ b/apps/web/eslint.i18n.options.mjs @@ -1421,6 +1421,9 @@ export const i18nGuardFiles = [ "components/github/pr-shared.tsx", "components/github/pr-status-chip.tsx", "components/github/pr-topbar-button.tsx", + "components/gitlab/mr-auto-fix-prompt-dialog.tsx", + "components/gitlab/mr-automation-controls.tsx", + "components/gitlab/mr-automation-rows.tsx", "components/gitlab/mr-commits-section.tsx", "components/gitlab/mr-detail-panel.tsx", "components/gitlab/mr-discussions-section.tsx", diff --git a/apps/web/hooks/domains/gitlab/use-task-mr-automation.test.tsx b/apps/web/hooks/domains/gitlab/use-task-mr-automation.test.tsx index bc7832c984..be4971280b 100644 --- a/apps/web/hooks/domains/gitlab/use-task-mr-automation.test.tsx +++ b/apps/web/hooks/domains/gitlab/use-task-mr-automation.test.tsx @@ -2,7 +2,7 @@ import { createElement, type ReactNode } from "react"; import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { StateProvider, useAppStore } from "@/components/state-provider"; -import type { TaskMRAutomationOptions } from "@/lib/types/gitlab"; +import type { TaskMRAutomationOptions, TaskMRAutomationOptionsForMR } from "@/lib/types/gitlab"; const api = vi.hoisted(() => ({ getTaskMRAutomation: vi.fn(), @@ -17,6 +17,25 @@ function wrapper({ children }: { children: ReactNode }) { return createElement(StateProvider, null, children); } +function defaultMROption( + overrides: Partial = {}, +): TaskMRAutomationOptionsForMR { + return { + task_id: "task-1", + repository_id: "", + project_path: "group/project", + mr_iid: 7, + auto_fix_enabled: false, + auto_merge_enabled: false, + prompt_on_review_requested: false, + prompt_on_merged: false, + prompt_on_closed: false, + created_at: "", + updated_at: "", + ...overrides, + }; +} + function baseOptions(overrides: Partial = {}): TaskMRAutomationOptions { return { task_id: "task-1", @@ -31,6 +50,7 @@ function baseOptions(overrides: Partial = {}): TaskMRAu review_reviewer_username: "", updated_at: "2026-01-01T00:00:00Z", mr_states: [], + mr_options: [defaultMROption()], ...overrides, }; } @@ -94,6 +114,30 @@ describe("useTaskMRAutomationOptions fetching", () => { }); expect(api.getTaskMRAutomation).toHaveBeenCalledTimes(1); }); + + it("issues one fetch when several instances mount together for the same task", async () => { + // The switches are per-MR, so a task with N linked MRs mounts N copies of + // MRAutomationControls — each with its own useTaskMRAutomationOptions. + // They all read the same store slot, so they must not each fire a GET. + const pending = deferred(); + api.getTaskMRAutomation.mockReturnValue(pending.promise); + const { result } = renderHook( + () => { + useTaskMRAutomationOptions("task-1"); + useTaskMRAutomationOptions("task-1"); + return useTaskMRAutomationOptions("task-1"); + }, + { wrapper }, + ); + + await waitFor(() => expect(api.getTaskMRAutomation).toHaveBeenCalledTimes(1)); + await act(async () => { + pending.resolve(baseOptions({ prompt_on_merged: true })); + await pending.promise; + }); + await waitFor(() => expect(result.current.options?.prompt_on_merged).toBe(true)); + expect(api.getTaskMRAutomation).toHaveBeenCalledTimes(1); + }); }); describe("useTaskMRAutomationOptions optimistic updates", () => { @@ -108,32 +152,44 @@ describe("useTaskMRAutomationOptions optimistic updates", () => { void result.current.update({ prompt_on_merged: true }); }); - // Optimistic reflect happens synchronously within the update call. - await waitFor(() => expect(result.current.options?.prompt_on_merged).toBe(true)); + // Optimistic reflect happens synchronously within the update call, into + // the targeted MR's entry in mr_options (the per-MR source of truth) — + // not the task-level aggregate, which is server-computed. + await waitFor(() => + expect(result.current.options?.mr_options?.[0]?.prompt_on_merged).toBe(true), + ); expect(result.current.saving).toBe(true); await act(async () => { - update.resolve(baseOptions({ prompt_on_merged: true })); + update.resolve(baseOptions({ mr_options: [defaultMROption({ prompt_on_merged: true })] })); }); await waitFor(() => expect(result.current.saving).toBe(false)); - expect(result.current.options?.prompt_on_merged).toBe(true); + expect(result.current.options?.mr_options?.[0]?.prompt_on_merged).toBe(true); expect(result.current.error).toBeNull(); }); it("reverts the optimistic update and surfaces an error on failure (AC27)", async () => { api.getTaskMRAutomation.mockResolvedValue(baseOptions()); - api.updateTaskMRAutomation.mockRejectedValue(new Error(NETWORK_DOWN_ERROR)); + const update = deferred(); + api.updateTaskMRAutomation.mockImplementation(() => update.promise); const { result } = renderHook(() => useTaskMRAutomationOptions("task-1"), { wrapper }); await waitFor(() => expect(result.current.options).not.toBeNull()); + let pending: Promise; + act(() => { + pending = result.current.update({ prompt_on_closed: true }); + }); + await waitFor(() => + expect(result.current.options?.mr_options?.[0]?.prompt_on_closed).toBe(true), + ); + await act(async () => { - await expect(result.current.update({ prompt_on_closed: true })).rejects.toThrow( - NETWORK_DOWN_ERROR, - ); + update.reject(new Error(NETWORK_DOWN_ERROR)); + await expect(pending!).rejects.toThrow(NETWORK_DOWN_ERROR); }); - expect(result.current.options?.prompt_on_closed).toBe(false); + expect(result.current.options?.mr_options?.[0]?.prompt_on_closed).toBe(false); expect(result.current.error).toBe(NETWORK_DOWN_ERROR); expect(result.current.saving).toBe(false); }); @@ -199,6 +255,89 @@ describe("useTaskMRAutomationOptions races", () => { }); expect(result.current.options?.prompt_on_merged).toBe(true); }); + + // A task with N linked MRs mounts N MRAutomationControls, each with its own + // useTaskMRAutomationOptions("task-1") instance. All instances read/write + // the same store slot, so their save-ordering guards must also be shared — + // otherwise an older instance's private counter never learns about a + // newer instance's save and treats its own stale response as current. + it("shares update ordering across multiple mounted instances for the same task, so an older instance's stale response cannot clobber a newer one's committed result", async () => { + api.getTaskMRAutomation.mockResolvedValue(baseOptions()); + const { result } = renderHook( + () => ({ + a: useTaskMRAutomationOptions("task-1"), + b: useTaskMRAutomationOptions("task-1"), + }), + { wrapper }, + ); + await waitFor(() => expect(result.current.a.options).not.toBeNull()); + + const updateA = deferred(); + const updateB = deferred(); + api.updateTaskMRAutomation + .mockImplementationOnce(() => updateA.promise) + .mockImplementationOnce(() => updateB.promise); + + // Instance A (e.g. !1's block) starts a save... + act(() => { + void result.current.a.update({ auto_fix_prompt_override: "from A" }); + }); + // ...then instance B (e.g. !2's block) starts a later save for the same + // task before A's resolves. + act(() => { + void result.current.b.update({ auto_fix_prompt_override: "from B" }); + }); + + // B's later, authoritative response lands first... + await act(async () => { + updateB.resolve(baseOptions({ auto_fix_prompt_override: "from B" })); + }); + expect(result.current.a.options?.auto_fix_prompt_override).toBe("from B"); + + // ...then A's now-stale response resolves after. It must not win. + await act(async () => { + updateA.resolve(baseOptions({ auto_fix_prompt_override: "from A" })); + }); + expect(result.current.a.options?.auto_fix_prompt_override).toBe("from B"); + expect(result.current.b.options?.auto_fix_prompt_override).toBe("from B"); + }); +}); + +describe("useTaskMRAutomationOptions revision ordering", () => { + it("commits a higher-revision response even when its request started before a lower-revision response", async () => { + api.getTaskMRAutomation.mockResolvedValue(baseOptions({ automation_revision: 1 })); + const { result } = renderHook(() => useTaskMRAutomationOptions("task-1"), { wrapper }); + await waitFor(() => expect(result.current.options).not.toBeNull()); + + const olderRequest = deferred(); + const newerRequest = deferred(); + api.updateTaskMRAutomation + .mockImplementationOnce(() => olderRequest.promise) + .mockImplementationOnce(() => newerRequest.promise); + + act(() => { + void result.current.update({ auto_fix_prompt_override: "older request" }); + void result.current.update({ auto_fix_prompt_override: "newer request" }); + }); + + await act(async () => { + newerRequest.resolve( + baseOptions({ automation_revision: 2, auto_fix_prompt_override: "revision two" }), + ); + }); + expect(result.current.options?.automation_revision).toBe(2); + + // The first request can commit later when the database transaction that + // owns it receives the next revision. Request-start order is not server + // revision order, so the higher revision must still win. + await act(async () => { + olderRequest.resolve( + baseOptions({ automation_revision: 3, auto_fix_prompt_override: "revision three" }), + ); + }); + expect(result.current.options?.automation_revision).toBe(3); + expect(result.current.options?.auto_fix_prompt_override).toBe("revision three"); + }); }); describe("useTaskMRAutomationOptions task switching", () => { @@ -420,13 +559,15 @@ describe("useTaskMRAutomationOptions refresh/save interaction", () => { // The refresh's pre-patch response resolves while the save is still // pending — it must not flip the optimistic switch back off. await act(async () => { - refreshCall.resolve(baseOptions({ prompt_on_merged: false })); + refreshCall.resolve( + baseOptions({ mr_options: [defaultMROption({ prompt_on_merged: false })] }), + ); }); - expect(result.current.options?.prompt_on_merged).toBe(true); + expect(result.current.options?.mr_options?.[0]?.prompt_on_merged).toBe(true); await act(async () => { - update.resolve(baseOptions({ prompt_on_merged: true })); + update.resolve(baseOptions({ mr_options: [defaultMROption({ prompt_on_merged: true })] })); }); - expect(result.current.options?.prompt_on_merged).toBe(true); + expect(result.current.options?.mr_options?.[0]?.prompt_on_merged).toBe(true); }); }); diff --git a/apps/web/hooks/domains/gitlab/use-task-mr-automation.ts b/apps/web/hooks/domains/gitlab/use-task-mr-automation.ts index 7bb98ed2ae..e03b474770 100644 --- a/apps/web/hooks/domains/gitlab/use-task-mr-automation.ts +++ b/apps/web/hooks/domains/gitlab/use-task-mr-automation.ts @@ -1,14 +1,40 @@ "use client"; -import { useCallback, useEffect, useRef, type RefObject } from "react"; +import { useCallback, useEffect, type RefObject } from "react"; import { getTaskMRAutomation, updateTaskMRAutomation } from "@/lib/api/domains/gitlab-api"; import { useAppStore, useAppStoreApi } from "@/components/state-provider"; import type { AppState } from "@/lib/state/store"; -import type { TaskMRAutomationOptions, TaskMRAutomationPatch } from "@/lib/types/gitlab"; +import type { + TaskMRAutomationOptions, + TaskMRAutomationOptionsForMR, + TaskMRAutomationPatch, +} from "@/lib/types/gitlab"; import { t } from "@/lib/i18n"; type AppStoreApi = ReturnType; +type MRAutomationRequestRefs = Pick< + MRAutomationRequestContext, + "refreshRequestRef" | "updateRequestRef" | "updateSettleCounterRef" +>; + +// One task can mount a control for every linked MR. Those controls share a +// store, so they must also share request ordering: an older control's full +// response must not overwrite a newer control's response for another MR. +const requestRefsByStore = new WeakMap(); + +function requestRefsForStore(storeApi: AppStoreApi): MRAutomationRequestRefs { + const existing = requestRefsByStore.get(storeApi); + if (existing) return existing; + const refs: MRAutomationRequestRefs = { + refreshRequestRef: { current: {} }, + updateRequestRef: { current: {} }, + updateSettleCounterRef: { current: {} }, + }; + requestRefsByStore.set(storeApi, refs); + return refs; +} + function errorMessage(error: unknown): string { return error instanceof Error ? error.message : t("gitlab:failedToLoadMrAutomationOptions"); } @@ -54,6 +80,14 @@ async function performRefresh( setLoading, setError, } = ctx; + // A task with several linked MRs mounts one MRAutomationControls per MR, + // and every instance's mount effect sees the same pre-fetch render snapshot + // (options null, loading false) — so without this guard, opening the + // dropdown would fire one identical GET per linked MR. The in-flight + // request commits to the shared store slot that all of them read. + if (storeApi.getState().taskMRAutomation.loading[taskId]) { + return storeApi.getState().taskMRAutomation.byTaskId[taskId] ?? null; + } const requestId = (refreshRequestRef.current[taskId] ?? 0) + 1; refreshRequestRef.current[taskId] = requestId; const settleCounterAtStart = updateSettleCounterRef.current[taskId] ?? 0; @@ -89,6 +123,84 @@ async function performRefresh( } } +/** + * Applies a patch to the cached options the way the backend will. The five + * switches live per-MR in `mr_options`, so a naive `{...previous, ...patch}` + * would write them onto the task-level *aggregate* instead — showing the + * switch as on for every linked MR until the response landed. Switch fields + * are therefore merged into the targeted MR's entry (or every entry, when the + * patch names no MR, which is the fan-out the backend performs); the + * remaining task-level fields merge at the top level as before. + */ +function applyMRAutomationPatchOptimistically( + previous: TaskMRAutomationOptions, + patch: TaskMRAutomationPatch, +): TaskMRAutomationOptions { + const { + repository_id: repositoryID, + project_path: projectPath, + mr_iid: mrIID, + ...fields + } = patch; + const switchKeys = [ + "auto_fix_enabled", + "auto_merge_enabled", + "prompt_on_review_requested", + "prompt_on_merged", + "prompt_on_closed", + ] as const; + const switchPatch: Partial = {}; + const taskLevel: Partial = {}; + for (const [key, value] of Object.entries(fields)) { + if ((switchKeys as readonly string[]).includes(key)) { + Object.assign(switchPatch, { [key]: value }); + } else { + Object.assign(taskLevel, { [key]: value }); + } + } + if (Object.keys(switchPatch).length === 0) { + return { ...previous, ...taskLevel }; + } + const targetsOneMR = + repositoryID !== undefined && projectPath !== undefined && mrIID !== undefined; + const mrOptions = (previous.mr_options ?? []).map((option) => { + const isTarget = + !targetsOneMR || + (option.repository_id === repositoryID && + option.project_path === projectPath && + option.mr_iid === mrIID); + return isTarget ? { ...option, ...switchPatch } : option; + }); + return { ...previous, ...taskLevel, mr_options: mrOptions }; +} + +type UpdateResponseCommitContext = { + automation: AppState["taskMRAutomation"]; + updateRequestRef: Record; + taskId: string; + requestId: number; + externalGenAtStart: number; + response: TaskMRAutomationOptions; +}; + +function shouldCommitUpdateResponse({ + automation, + updateRequestRef, + taskId, + requestId, + externalGenAtStart, + response, +}: UpdateResponseCommitContext): boolean { + const currentRevision = automation.byTaskId[taskId]?.automation_revision ?? 0; + const responseRevision = response.automation_revision ?? 0; + const isLatestRequest = updateRequestRef[taskId] === requestId; + const isNewerRevision = responseRevision > currentRevision; + return ( + (automation.externalGeneration[taskId] ?? 0) === externalGenAtStart && + (isLatestRequest || isNewerRevision) + ); +} + async function performUpdate( ctx: MRAutomationRequestContext, patch: TaskMRAutomationPatch, @@ -108,21 +220,25 @@ async function performUpdate( const previous = storeApi.getState().taskMRAutomation.byTaskId[taskId] ?? null; // Optimistic update: apply immediately, revert on failure (AC27). if (previous) { - setOptions(taskId, { ...previous, ...patch }); + setOptions(taskId, applyMRAutomationPatchOptimistically(previous, patch)); } setSaving(taskId, true); setError(taskId, null); try { const response = await updateTaskMRAutomation(taskId, patch, { cache: "no-store" }); const automation = storeApi.getState().taskMRAutomation; + // Request-start order is not server revision order. The helper allows an + // older request through only when its server revision is strictly newer; + // the store then applies its monotonic revision guard. if ( - isCurrentAndUnchangedExternally( + shouldCommitUpdateResponse({ automation, - updateRequestRef.current, + updateRequestRef: updateRequestRef.current, taskId, requestId, externalGenAtStart, - ) + response, + }) ) { setOptions(taskId, response); } @@ -189,11 +305,10 @@ async function performUpdate( * corrects it again. */ export function useTaskMRAutomationOptions(taskId: string | null) { - const refreshRequestRef = useRef>({}); - const updateRequestRef = useRef>({}); - const updateSettleCounterRef = useRef>({}); const storeApi = useAppStoreApi(); + const { refreshRequestRef, updateRequestRef, updateSettleCounterRef } = + requestRefsForStore(storeApi); const options = useAppStore((state) => taskId ? (state.taskMRAutomation.byTaskId[taskId] ?? null) : null, ); diff --git a/apps/web/lib/gitlab/mr-automation.ts b/apps/web/lib/gitlab/mr-automation.ts index 5f651954f4..a12f0827d1 100644 --- a/apps/web/lib/gitlab/mr-automation.ts +++ b/apps/web/lib/gitlab/mr-automation.ts @@ -1,7 +1,50 @@ -import type { TaskMR, TaskMRLifecycleState } from "@/lib/types/gitlab"; +import type { + TaskMR, + TaskMRAutomationOptionsForMR, + TaskMRLifecycleState, +} from "@/lib/types/gitlab"; const DEFAULT_AUTO_FIX_MAX_ROUNDS = 10; +export const DISABLED_MR_AUTOMATION_SWITCHES: Omit< + TaskMRAutomationOptionsForMR, + "task_id" | "repository_id" | "project_path" | "mr_iid" | "created_at" | "updated_at" +> = { + auto_fix_enabled: false, + auto_merge_enabled: false, + prompt_on_review_requested: false, + prompt_on_merged: false, + prompt_on_closed: false, +}; + +/** + * Selects the given MR's own automation switches out of the task-scoped + * `mr_options` array, falling back to all-off defaults when the MR has no + * stored row yet (never configured). Mirrors findMRAutomationStateForMR. + */ +export function findMRAutomationOptionsForMR( + options: TaskMRAutomationOptionsForMR[] | undefined, + mr: TaskMR, +): TaskMRAutomationOptionsForMR { + const repositoryID = mr.repository_id ?? ""; + const found = options?.find( + (option) => + option.mr_iid === mr.mr_iid && + option.project_path === mr.project_path && + option.repository_id === repositoryID, + ); + if (found) return found; + return { + task_id: mr.task_id, + repository_id: repositoryID, + project_path: mr.project_path, + mr_iid: mr.mr_iid, + created_at: "", + updated_at: "", + ...DISABLED_MR_AUTOMATION_SWITCHES, + }; +} + export type AutoFixRoundInfo = { current: number; max: number; diff --git a/apps/web/lib/state/slices/gitlab/gitlab-slice.test.ts b/apps/web/lib/state/slices/gitlab/gitlab-slice.test.ts index ce8d6cef94..62c0d1dbfb 100644 --- a/apps/web/lib/state/slices/gitlab/gitlab-slice.test.ts +++ b/apps/web/lib/state/slices/gitlab/gitlab-slice.test.ts @@ -49,6 +49,7 @@ function makeOptions(overrides: Partial = {}): TaskMRAu review_reviewer_username: "", updated_at: "2026-01-01T00:00:00Z", mr_states: [], + mr_options: [], ...overrides, }; } diff --git a/apps/web/lib/state/slices/gitlab/gitlab-slice.ts b/apps/web/lib/state/slices/gitlab/gitlab-slice.ts index 612bb517ab..bcb079b9ca 100644 --- a/apps/web/lib/state/slices/gitlab/gitlab-slice.ts +++ b/apps/web/lib/state/slices/gitlab/gitlab-slice.ts @@ -215,6 +215,10 @@ function taskMRAutomationActions(set: ImmerSet) { options: GitLabSliceState["taskMRAutomation"]["byTaskId"][string], ) => set((draft) => { + const existing = draft.taskMRAutomation.byTaskId[taskId]; + if (existing && (options.automation_revision ?? 0) < (existing.automation_revision ?? 0)) { + return; + } draft.taskMRAutomation.byTaskId[taskId] = options; }), setTaskMRAutomationLoading: (taskId: string, loading: boolean) => diff --git a/apps/web/lib/types/gitlab.ts b/apps/web/lib/types/gitlab.ts index 0e4ce06eb0..ad1fa8a142 100644 --- a/apps/web/lib/types/gitlab.ts +++ b/apps/web/lib/types/gitlab.ts @@ -411,9 +411,33 @@ export type TaskMRLifecycleState = { updated_at: string; }; -/** Task-level MR automation preferences: lifecycle switches (#2125) plus auto-fix CI and auto-merge. */ +/** + * The five automation switches for one linked merge request. This is the + * per-MR source of truth; the same-named booleans on TaskMRAutomationOptions + * are an aggregate that only reports "every linked MR has this on". + */ +export type TaskMRAutomationOptionsForMR = { + task_id: string; + repository_id: string; + project_path: string; + mr_iid: number; + auto_fix_enabled: boolean; + auto_merge_enabled: boolean; + prompt_on_review_requested: boolean; + prompt_on_merged: boolean; + prompt_on_closed: boolean; + created_at: string; + updated_at: string; +}; + +/** + * Task MR automation preferences: the task-level auto-fix prompt override and + * reviewer username, the per-MR switches in `mr_options`, and an aggregate of + * those switches in the top-level booleans. + */ export type TaskMRAutomationOptions = { task_id: string; + automation_revision?: number; auto_fix_enabled: boolean; auto_merge_enabled: boolean; auto_fix_prompt_override?: string | null; @@ -426,10 +450,16 @@ export type TaskMRAutomationOptions = { review_reviewer_username: string; updated_at: string; mr_states: TaskMRLifecycleState[]; + mr_options: TaskMRAutomationOptionsForMR[]; }; /** Partial update for task MR automation options. */ export type TaskMRAutomationPatch = { + // Target one linked MR's switches; omit all three to apply them to every MR + // currently linked to the task. + repository_id?: string; + project_path?: string; + mr_iid?: number; auto_fix_enabled?: boolean; auto_merge_enabled?: boolean; auto_fix_prompt_override?: string; diff --git a/apps/web/lib/ws/handlers/gitlab.test.ts b/apps/web/lib/ws/handlers/gitlab.test.ts index 44d88848c9..ce805108fb 100644 --- a/apps/web/lib/ws/handlers/gitlab.test.ts +++ b/apps/web/lib/ws/handlers/gitlab.test.ts @@ -6,12 +6,13 @@ import { registerGitLabHandlers } from "./gitlab"; const WORKSPACE_A = "workspace-a"; -function makeStore(activeWorkspaceId: string | null) { +function makeStore(activeWorkspaceId: string | null, options?: TaskMRAutomationOptions) { const setTaskMR = vi.fn(); const setTaskMRAutomationOptions = vi.fn(); const markTaskMRAutomationExternalUpdate = vi.fn(); const state = { workspaces: { activeId: activeWorkspaceId }, + taskMRAutomation: { byTaskId: options ? { [options.task_id]: options } : {} }, setTaskMR, setTaskMRAutomationOptions, markTaskMRAutomationExternalUpdate, @@ -40,6 +41,7 @@ function taskMRAutomationOptions( review_reviewer_username: "", updated_at: "2026-01-01T00:00:00Z", mr_states: [], + mr_options: [], ...overrides, }; } @@ -93,6 +95,24 @@ describe("GitLab WebSocket handlers", () => { expect(markTaskMRAutomationExternalUpdate).toHaveBeenCalledWith("task-1"); }); + it("drops an older MR automation snapshot pushed after a newer one", () => { + const current = taskMRAutomationOptions({ automation_revision: 2 }); + const { store, setTaskMRAutomationOptions, markTaskMRAutomationExternalUpdate } = makeStore( + WORKSPACE_A, + current, + ); + const handler = registerGitLabHandlers(store)["gitlab.task_mr_options.updated"]!; + + handler({ + type: "notification", + action: "gitlab.task_mr_options.updated", + payload: taskMRAutomationOptions({ automation_revision: 1 }), + }); + + expect(setTaskMRAutomationOptions).not.toHaveBeenCalled(); + expect(markTaskMRAutomationExternalUpdate).not.toHaveBeenCalled(); + }); + it("ignores a task MR automation options update with no task_id", () => { const { store, setTaskMRAutomationOptions } = makeStore(WORKSPACE_A); const handler = registerGitLabHandlers(store)["gitlab.task_mr_options.updated"]!; diff --git a/apps/web/lib/ws/handlers/gitlab.ts b/apps/web/lib/ws/handlers/gitlab.ts index 330cf5d44c..255651fe97 100644 --- a/apps/web/lib/ws/handlers/gitlab.ts +++ b/apps/web/lib/ws/handlers/gitlab.ts @@ -14,6 +14,10 @@ export function registerGitLabHandlers(store: StoreApi): WsHandlers { "gitlab.task_mr_options.updated": (message) => { const options = message.payload as TaskMRAutomationOptions; if (options.task_id) { + const current = store.getState().taskMRAutomation.byTaskId[options.task_id]; + if (current && (options.automation_revision ?? 0) < (current.automation_revision ?? 0)) { + return; + } store.getState().setTaskMRAutomationOptions(options.task_id, options); // Marks this write as externally-sourced so a slower in-flight local // refresh()/update() in useTaskMRAutomationOptions knows not to diff --git a/apps/web/src/locales/en/gitlab.json b/apps/web/src/locales/en/gitlab.json index d4ceb7305b..c1b7b892a5 100644 --- a/apps/web/src/locales/en/gitlab.json +++ b/apps/web/src/locales/en/gitlab.json @@ -294,6 +294,7 @@ "mrAutomationUpdateFailedDescription": "The setting was not saved.", "mrAutomationUpdateFailedTitle": "Failed to update MR automation", "mrAutomation": "Automation", + "mrAutomationAppliesToMR": "Applies to !{{iid}}", "mrAutoFixCiAndAddressComments": "Auto-fix CI and address comments", "mrAutoFixPrompt": "Auto-fix prompt", "mrAutoFixPromptDescription": "This prompt is used only for this task. Leave it blank to use the default prompt. Add <0>{{placeholder}} when you want Kandev to include its MR feedback snapshot.", diff --git a/apps/web/src/locales/pseudo/gitlab.json b/apps/web/src/locales/pseudo/gitlab.json index 73e9c9311d..fca6059c49 100644 --- a/apps/web/src/locales/pseudo/gitlab.json +++ b/apps/web/src/locales/pseudo/gitlab.json @@ -294,6 +294,7 @@ "mrAutomationUpdateFailedDescription": "Ţĥē śēţţĩńĝ ŵàś ńōţ śàvēď.", "mrAutomationUpdateFailedTitle": "Ƒàĩĺēď ţō ũƥďàţē ḾŔ àũţōḿàţĩōń", "mrAutomation": "Àũţōḿàţĩōń", + "mrAutomationAppliesToMR": "Àƥƥĺĩēś ţō !{{iid}}", "mrAutoFixCiAndAddressComments": "Àũţō-ƒĩx ĆĨ àńď àďďŕēśś ćōḿḿēńţś", "mrAutoFixPrompt": "Àũţō-ƒĩx ƥŕōḿƥţ", "mrAutoFixPromptDescription": "Ţĥĩś ƥŕōḿƥţ ĩś ũśēď ōńĺŷ ƒōŕ ţĥĩś ţàśķ. Ĺēàvē ĩţ ƀĺàńķ ţō ũśē ţĥē ďēƒàũĺţ ƥŕōḿƥţ. Àďď <0>{{placeholder}} ŵĥēń ŷōũ ŵàńţ Ķàńďēv ţō ĩńćĺũďē ĩţś ḾŔ ƒēēďƀàćķ śńàƥśĥōţ.", diff --git a/apps/web/src/locales/pt-pt/gitlab.json b/apps/web/src/locales/pt-pt/gitlab.json index 30d43853d6..a72db5cefd 100644 --- a/apps/web/src/locales/pt-pt/gitlab.json +++ b/apps/web/src/locales/pt-pt/gitlab.json @@ -281,6 +281,7 @@ "workflowThatReceivesNewTasks": "Fluxo de trabalho que recebe as novas tarefas.", "yourTokenMayStillBeValid": "O seu token pode continuar válido; isto parece um problema de rede ou do serviço.", "mrAutomationClosedLabel": "MR fechado sem merge", + "mrAutomationAppliesToMR": "Aplica-se a !{{iid}}", "mrAutomationDescription": "Acorda o agente desta tarefa quando a sua revisão é pedida num merge request associado ou quando um deles é integrado ou fechado. As notificações vão para a fila de sessão que estiver ativa nesta tarefa.", "mrAutomationMergedLabel": "MR integrado", "mrAutomationRetry": "Tentar novamente", diff --git a/apps/web/src/locales/zh-cn/gitlab.json b/apps/web/src/locales/zh-cn/gitlab.json index d6678aa042..b419aaf312 100644 --- a/apps/web/src/locales/zh-cn/gitlab.json +++ b/apps/web/src/locales/zh-cn/gitlab.json @@ -281,6 +281,7 @@ "workflowThatReceivesNewTasks": "接收新任务的工作流。", "yourTokenMayStillBeValid": "你的令牌可能仍然有效--这看起来是网络或上游服务问题。", "mrAutomationClosedLabel": "MR 已关闭未合并", + "mrAutomationAppliesToMR": "适用于 !{{iid}}", "mrAutomationDescription": "当关联的合并请求请求你的审阅，或该合并请求被合并、关闭时，唤醒此任务的智能体。通知会发送到该任务当前处于活动状态的会话队列。", "mrAutomationMergedLabel": "MR 已合并", "mrAutomationRetry": "重试", diff --git a/apps/web/src/locales/zh-hk/gitlab.json b/apps/web/src/locales/zh-hk/gitlab.json index f320db224c..7ea72458eb 100644 --- a/apps/web/src/locales/zh-hk/gitlab.json +++ b/apps/web/src/locales/zh-hk/gitlab.json @@ -281,6 +281,7 @@ "workflowThatReceivesNewTasks": "接收新任務的工作流程。", "yourTokenMayStillBeValid": "你的權杖可能仍然有效--這看起來是網絡或上游服務問題。", "mrAutomationClosedLabel": "MR 已關閉未合併", + "mrAutomationAppliesToMR": "適用於 !{{iid}}", "mrAutomationDescription": "當關聯的合併請求請求你的審閱，或該合併請求被合併、關閉時，喚醒此任務的代理程式。通知會傳送到該任務目前處於活動狀態的工作階段佇列。", "mrAutomationMergedLabel": "MR 已合併", "mrAutomationRetry": "重試", diff --git a/apps/web/src/locales/zh-tw/gitlab.json b/apps/web/src/locales/zh-tw/gitlab.json index 6189e5e8e9..84630a290a 100644 --- a/apps/web/src/locales/zh-tw/gitlab.json +++ b/apps/web/src/locales/zh-tw/gitlab.json @@ -281,6 +281,7 @@ "workflowThatReceivesNewTasks": "接收新任務的工作流程。", "yourTokenMayStillBeValid": "你的權杖可能仍然有效--這看起來是網路或上游服務問題。", "mrAutomationClosedLabel": "MR 已關閉未合併", + "mrAutomationAppliesToMR": "適用於 !{{iid}}", "mrAutomationDescription": "當關聯的合併請求請求你的審閱，或該合併請求被合併、關閉時，喚醒此任務的代理程式。通知會傳送到該任務目前處於活動狀態的工作階段佇列。", "mrAutomationMergedLabel": "MR 已合併", "mrAutomationRetry": "重試", diff --git a/docs/decisions/2026-08-01-gitlab-mr-lifecycle-notifications.md b/docs/decisions/2026-08-01-gitlab-mr-lifecycle-notifications.md index 599a160d3b..cbc27d1257 100644 --- a/docs/decisions/2026-08-01-gitlab-mr-lifecycle-notifications.md +++ b/docs/decisions/2026-08-01-gitlab-mr-lifecycle-notifications.md @@ -75,6 +75,22 @@ dispatch functions turned out byte-for-byte identical below the decision layer. - **`locked` state.** GitLab's MR state machine has a fourth value with no GitHub analogue. It is treated as non-terminal (fires neither `merged` nor `closed`) and is exercised as its own regression case in the decision-function tests. +- **Amended: the five switches are now per linked MR, not per task.** As written, + this ADR put `prompt_on_review_requested` / `prompt_on_merged` / `prompt_on_closed` + (and later auto-fix / auto-merge) on `gitlab_task_mr_options`, keyed by `task_id` + alone. That is wrong for a task with more than one linked MR: enabling a switch on + one MR silently enabled it on all of them. The five switches moved to + `gitlab_task_mr_automation_options`, keyed by the same + `(task_id, repository_id, project_path, mr_iid)` identity as + `gitlab_task_mr_state`, seeded once from the legacy task-wide values by an + `mr_scope_migrated_at`-guarded fan-out. `gitlab_task_mr_options` keeps only what is + genuinely task-level — the auto-fix prompt override and the server-resolved + reviewer username — so a reviewer-identity change still clears every linked MR's + review-request baseline at once, while a switch flip clears only its own MR's + checkpoints. A `PATCH` / `update_task_mr_automation_kandev` call naming an MR + targets it alone; omitting MR identity fans out to every linked MR, which preserves + the behavior of agents that have no MR identity to send. See + `docs/specs/gitlab-integration/spec.md`'s "MR automation" section. - **Discovered while integrating:** `executeQueuedMessage`'s lifecycle-prompt detection (`event_handlers_agent.go`) was hardcoded to the GitHub PR automation origin string. A GitLab-originated durable lifecycle entry was queued but never diff --git a/docs/public/integrations.md b/docs/public/integrations.md index 53d65c5a46..e9a41ce73e 100644 --- a/docs/public/integrations.md +++ b/docs/public/integrations.md @@ -364,7 +364,9 @@ These actions use the connected GitLab user's permissions and do not bypass prot ### Automate a linked merge request -For a task with a linked GitLab merge request, open the MR topbar control. The **Automation** group has the same two task-level controls as GitHub's PRs: **Auto-fix CI and address comments** and **Auto-merge when ready**. Below it, expand **Review follow-up** for three lifecycle booleans: **Your review is requested**, **MR merged**, and **MR closed without merging**. Enabling any control applies it to every MR linked to that task; Kandev tracks delivery and deduplication separately for each linked MR. +For a task with a linked GitLab merge request, open the MR topbar control. The **Automation** group has the same two controls as GitHub's PRs: **Auto-fix CI and address comments** and **Auto-merge when ready**. Below it, expand **Review follow-up** for three lifecycle booleans: **Your review is requested**, **MR merged**, and **MR closed without merging**. + +All five belong to a single merge request. A task with several linked MRs shows one **Automation** group per MR, each labelled with its MR number, so you can automate one MR and leave the rest untouched; Kandev tracks delivery and deduplication separately for each. The auto-fix prompt override is the one setting that stays task-level; editing it applies to every linked MR. An agent calling `update_task_mr_automation_kandev` can name a merge request to target it alone, or omit the merge-request fields to apply the change to every MR linked to the task. Kandev reuses the existing lightweight task MR poller, which checks linked MRs roughly once per minute; it does not add a separate scheduler. Saving enabled options also evaluates the task's current linked MRs without waiting for the next poll. diff --git a/docs/public/sessions-and-review.md b/docs/public/sessions-and-review.md index 82bbd05d56..c8abf776b1 100644 --- a/docs/public/sessions-and-review.md +++ b/docs/public/sessions-and-review.md @@ -265,7 +265,7 @@ The GitLab MR topbar control has an **Automation** group with the same two actio - **Auto-fix CI and address comments** sends the agent a new or changed failing pipeline job or unresolved discussion note once the pipeline settles, and stops after 10 repair rounds for that MR. Disable and re-enable it to reset the limit. - **Auto-merge when ready** merges only after the pipeline passes, unresolved discussions are cleared, and GitLab's own merge-readiness check agrees. -Below that, open **Review follow-up** for the same three notification switches GitHub uses, task-level and applying to every merge request linked to the task: +Below that, open **Review follow-up** for the same three notification switches GitHub uses. Every switch above belongs to one merge request: a task with several linked MRs shows an **Automation** group per MR, each labelled with its MR number, and turning a switch on for one leaves the others alone. - **Your review is requested** wakes the agent when the workspace's connected GitLab account is newly added as a reviewer on the MR. Staying assigned across MR updates does not re-fire it; being removed and re-added (for example, for a re-review after changes) does. - **MR merged** and **MR closed without merging** independently wake the agent when review work ends. diff --git a/docs/specs/gitlab-integration/spec.md b/docs/specs/gitlab-integration/spec.md index 3395df44cc..eda9b0a352 100644 --- a/docs/specs/gitlab-integration/spec.md +++ b/docs/specs/gitlab-integration/spec.md @@ -108,16 +108,18 @@ workflows are not usable end to end. switches — `Auto-fix CI and address comments` and `Auto-merge when ready` — above a collapsible `Review follow-up` group holding the three lifecycle notification switches (`Your review is requested`, `MR merged`, `MR closed - without merging`) introduced for MR lifecycle notifications. These switches - currently use task-level settings, so changing one linked MR's control also - affects the task's other linked MRs. Per-MR switch scoping, including - per-MR round/attempt state and identity-aware `PATCH`/MCP updates, is tracked - in the "Scope GitLab MR automation switches per MR" follow-up task. The - auto-fix prompt override remains task-level. See "Automation (lifecycle, - auto-fix, auto-merge)" below. + without merging`) introduced for MR lifecycle notifications. All five + switches are scoped per linked MR (see the per-MR amendment in + [the GitLab MR lifecycle ADR](../../decisions/2026-08-01-gitlab-mr-lifecycle-notifications.md)): + enabling a switch on one linked MR does not affect + any other linked MR's switches, and a `PATCH`/MCP update that omits MR + identity fans out to every linked MR, preserving prior agent behavior. + Auto-fix and auto-merge additionally track per-MR round/attempt state. The + auto-fix prompt override remains task-level, as does the resolved review + reviewer username. See "Automation (lifecycle, auto-fix, auto-merge)" below. - `Auto-fix CI and address comments` sends or queues an agent prompt when a linked MR's pipeline has a new or changed failing job, or a new or changed - unresolved discussion note, capped at 10 accepted rounds per task. + unresolved discussion note, capped at 10 accepted rounds per linked MR. `Auto-merge when ready` merges a linked MR only when it is open, not a draft, its pipeline succeeded, it has zero unresolved discussions, and GitLab's own merge-readiness verdict agrees. @@ -130,8 +132,11 @@ workflows are not usable end to end. Clicking the button opens the MR detail panel directly (no intermediate dropdown), also mirroring GitHub's single-PR topbar button. A task with 2+ linked MRs, and touch/coarse-pointer surfaces regardless of MR count, - keep the click-only dropdown (per-MR review/open/unlink rows, the - Automation group, and "Link another merge request") with no hover popover. + keep the click-only dropdown (per-MR review/open/unlink rows, one + Automation block per linked MR — each labeled with that MR's number, its + auto-fix/auto-merge rows always visible and its nested `Review follow-up` + group collapsed unless one of that MR's own three lifecycle switches is + already on — and "Link another merge request") with no hover popover. - The Kanban card shows a merge-request badge (`IconGitMerge`, coloured by state/pipeline/approval) next to the existing pull-request badge when the task has at least one linked MR. Multiple linked MRs collapse into one badge @@ -184,12 +189,23 @@ into the secret store. notable transitions. - GitLab notification subscription state is owned by GitLab. Kandev reads it live and does not duplicate it in SQLite. -- `gitlab_task_mr_options` is a per-task row: `task_id` (PK), the three - lifecycle booleans (`prompt_on_review_requested`, `prompt_on_merged`, - `prompt_on_closed`), `review_reviewer_username`, `auto_fix_enabled`, - `auto_merge_enabled`, `auto_fix_prompt_override` (nullable; empty/`NULL` - means use the built-in `mr-auto-fix` prompt), and timestamps. One row covers - every MR linked to the task. +- `gitlab_task_mr_options` is a per-task row: `task_id` (PK), the genuinely + task-level fields `review_reviewer_username` and `auto_fix_prompt_override` + (nullable; empty/`NULL` means use the built-in `mr-auto-fix` prompt), + `mr_scope_migrated_at` (nullable; guards the one-time fan-out into + `gitlab_task_mr_automation_options` below so a replay never re-enables a + switch a user has since turned off for one MR), and timestamps. Its five + boolean columns (`auto_fix_enabled`, `auto_merge_enabled`, + `prompt_on_review_requested`, `prompt_on_merged`, `prompt_on_closed`) are + legacy: no longer written, read only by that one-time migration. +- `gitlab_task_mr_automation_options` is the per-MR source of truth for the + five automation switches, keyed by `(task_id, repository_id, project_path, + mr_iid)`. A `PATCH`/MCP update naming one linked MR's identity writes only + that row; omitting MR identity fans the patch out to every row currently + linked to the task. The public `GET` response's top-level switch booleans + stay an aggregate ("on for every linked MR, and at least one MR linked") + for MCP/API read compatibility; the `mr_options` array in that response is + the per-MR source of truth the UI renders from. - `gitlab_task_mr_state` is a per-`(task_id, repository_id, project_path, mr_iid)` row carrying lifecycle dedupe fields (from MR lifecycle notifications) plus `last_fix_signature`, `last_fix_checkpoint_json`, @@ -265,12 +281,16 @@ validated against any supplied value. ### MR automation (lifecycle, auto-fix, auto-merge) - `GET /tasks/:taskID/mr-automation` returns the task's `TaskMRAutomationOptions`: - the three lifecycle booleans, `review_reviewer_username`, `auto_fix_enabled`, - `auto_merge_enabled`, `auto_fix_prompt_override` (`null` when unset), - `auto_fix_max_rounds` (`10`), `effective_auto_fix_prompt`, - `using_default_prompt`, `updated_at`, and `mr_states` (one - `TaskMRLifecycleState` per linked MR, carrying both the lifecycle dedupe - fields and the auto-fix/auto-merge checkpoint fields). + `automation_revision`, the three lifecycle booleans, + `review_reviewer_username`, `auto_fix_enabled`, `auto_merge_enabled`, + `auto_fix_prompt_override` (`null` when unset), `auto_fix_max_rounds` (`10`), + `effective_auto_fix_prompt`, `using_default_prompt`, `updated_at`, + `mr_options` (one row of the five switches per linked MR), and `mr_states` + (one `TaskMRLifecycleState` per linked MR, carrying both the lifecycle dedupe + fields and the auto-fix/auto-merge checkpoint fields). The top-level switch + booleans are compatibility aggregates: they are true only when at least one + MR is linked and every linked MR has the switch enabled. Clients that need + one MR's exact value must read its `mr_options` row. - `PATCH /tasks/:taskID/mr-automation` accepts a partial body with any of the same fields (excluding `auto_fix_max_rounds`, `effective_auto_fix_prompt`, `using_default_prompt`, `updated_at`, and `mr_states`, which are @@ -278,7 +298,11 @@ validated against any supplied value. field`; `auto_fix_enabled`/`auto_merge_enabled`/the three lifecycle booleans reject an explicit `null` (they are switches, not clearable values); `auto_fix_prompt_override: null` or `""` restores the built-in `mr-auto-fix` - prompt. + prompt. To target one linked MR, clients pass the complete + `repository_id`, `project_path`, and `mr_iid` selector tuple. If all three + selectors are omitted, the switch patch fans out to every linked MR. A + partial selector tuple or an unlinked MR returns `400` without a write. + The prompt override remains task-level and does not use these selectors. - Current-task MCP exposes `get_task_mr_automation_kandev` and `update_task_mr_automation_kandev` with the same shape, scoped to the connected task.