diff --git a/apps/backend/internal/backendapp/pr_base_resolver_test.go b/apps/backend/internal/backendapp/pr_base_resolver_test.go index 31e18ebb03..839a556cb0 100644 --- a/apps/backend/internal/backendapp/pr_base_resolver_test.go +++ b/apps/backend/internal/backendapp/pr_base_resolver_test.go @@ -158,6 +158,33 @@ func TestGitHubPRBaseResolverUsesLinkedPRForLegacyAttachment(t *testing.T) { } } +func TestGitHubPRBaseResolverUsesAttachedTargetWhenAssociationIsMissing(t *testing.T) { + service := &githubPRBaseLookupServiceStub{pr: &githubpkg.PR{ + Number: 42, RepoOwner: "upstream", RepoName: "widgets", + BaseRepoOwner: "upstream", BaseRepoName: "widgets", + HeadRepoOwner: "fork-owner", HeadRepoName: "widgets", HeadBranch: "feature", + BaseBranch: "main", BaseSHA: "0123456789abcdef0123456789abcdef01234567", + }} + resolver := githubPRBaseResolver{service: service} + lookup := executorpkg.PRBaseLookup{ + TaskID: "task-1", TaskRepositoryID: "task-repo-1", RepositoryID: "repo-1", + Number: 42, CheckoutBranch: "feature", AttachedOwner: "upstream", AttachedRepository: "widgets", + } + base, err := resolver.ResolvePRBase(context.Background(), "workspace-1", lookup) + if err != nil { + t.Fatalf("ResolvePRBase() error: %v", err) + } + if base.Target.TargetRepository.Path != "upstream/widgets" || + base.Target.HeadRepository.Path != "fork-owner/widgets" || base.Target.HeadBranch != "feature" { + t.Fatalf("resolved target-attached PR = %#v, want upstream target and validated fork head", base.Target) + } + if !reflect.DeepEqual(service.getCalls, []string{"upstream/widgets#42"}) || + !reflect.DeepEqual(service.listCalls, [][]string{{"task-1"}}) { + t.Fatalf("provider lookups = (%v, %v), want empty association check then attached namespace lookup", + service.getCalls, service.listCalls) + } +} + func TestGitHubPRBaseResolverClassifiesKnownCrossRepositoryFailure(t *testing.T) { service := &githubPRBaseLookupServiceStub{ prs: []*githubpkg.TaskPR{{ diff --git a/apps/backend/internal/orchestrator/event_handlers_github.go b/apps/backend/internal/orchestrator/event_handlers_github.go index f226a596bc..f76e4d201b 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github.go +++ b/apps/backend/internal/orchestrator/event_handlers_github.go @@ -591,6 +591,27 @@ func (s *Service) attachTaskToReservation(ctx context.Context, evt *github.NewRe // buildReviewTaskRequest builds the ReviewTaskRequest payload from an event. func buildReviewTaskRequest(evt *github.NewReviewPREvent, repositories []ReviewTaskRepository, repoSlug string) *ReviewTaskRequest { pr := evt.PR + metadata := map[string]interface{}{ + "review_watch_id": evt.ReviewWatchID, + "pr_number": pr.Number, + "pr_url": pr.HTMLURL, + "pr_repo": repoSlug, + "pr_author": pr.AuthorLogin, + "pr_branch": pr.HeadBranch, + "agent_profile_id": evt.AgentProfileID, + "executor_profile_id": evt.ExecutorProfileID, + // Permanent guard: both the workflow-promotion path and this watcher's + // synchronous path must compete for the one-shot launch token. + models.MetaKeyAutoStartGuard: true, + } + if reviewPRHeadMatchesBase(pr) { + metadata[models.MetaKeyAutoStartClaimed] = true + } else { + // Fork-controlled files can be evaluated by the repository setup script, + // which receives executor-profile environment values. Leave fork review + // tasks for an explicit user start instead of granting an unattended token. + metadata[models.MetaKeyForkPRRequiresManualStart] = true + } return &ReviewTaskRequest{ WorkspaceID: evt.WorkspaceID, WorkflowID: evt.WorkflowID, @@ -598,29 +619,17 @@ func buildReviewTaskRequest(evt *github.NewReviewPREvent, repositories []ReviewT Title: service.TruncateTaskTitle(fmt.Sprintf("PR #%d: %s", pr.Number, pr.Title)), Description: interpolateReviewPrompt(evt.Prompt, pr), Repositories: repositories, - Metadata: map[string]interface{}{ - "review_watch_id": evt.ReviewWatchID, - "pr_number": pr.Number, - "pr_url": pr.HTMLURL, - "pr_repo": repoSlug, - "pr_author": pr.AuthorLogin, - "pr_branch": pr.HeadBranch, - "agent_profile_id": evt.AgentProfileID, - "executor_profile_id": evt.ExecutorProfileID, - // Permanent guard: signals that the auto-start idempotency protocol - // is active for this task. Both Path A (promotion, async) and Path B - // (watcher, sync) check this marker and then compete for the one-shot - // MetaKeyAutoStartClaimed token before calling StartTask. - models.MetaKeyAutoStartGuard: true, - // One-shot token: both paths atomically remove this key; only the - // first removal wins and proceeds to StartTask. Prevents the - // duplicate-agent bug when CreateTask synchronously promotes the task - // from its feeder into an auto-start destination. - models.MetaKeyAutoStartClaimed: true, - }, + Metadata: metadata, } } +func reviewPRHeadMatchesBase(pr *github.PR) bool { + if pr == nil || pr.RepoOwner == "" || pr.RepoName == "" || pr.HeadRepoOwner == "" || pr.HeadRepoName == "" { + return false + } + return strings.EqualFold(pr.RepoOwner, pr.HeadRepoOwner) && strings.EqualFold(pr.RepoName, pr.HeadRepoName) +} + // buildIssueTaskTitle builds the "Issue #: " task title for an issue // watcher, shortened to the task title limit so an over-long remote issue title // does not cause CreateIssueTask to reject the whole task. @@ -646,6 +655,11 @@ func (s *Service) shouldAutoStartStep(ctx context.Context, stepID string) bool { func (s *Service) autoStartReviewTask( ctx context.Context, evt *github.NewReviewPREvent, task *models.Task, ) { + if taskRequiresManualForkPRStart(task) { + s.logger.Info("fork review task is waiting for a manual start", + zap.String("task_id", task.ID), zap.Int("pr_number", evt.PR.Number)) + return + } if s.shouldSkipTerminalPRAutoStart(ctx, task) { return } @@ -655,7 +669,7 @@ func (s *Service) autoStartReviewTask( // autoStartTaskForStep (Path A) asynchronously; only the first claimer // launches so exactly one session is created for the task. if !s.claimAutoStart(ctx, task.ID, "review.auto_start") { - s.logger.Debug("review auto-start claim lost; promotion path will launch", + s.logger.Debug("review auto-start claim unavailable; skipping automatic launch", zap.String("task_id", task.ID), zap.Int("pr_number", evt.PR.Number)) return diff --git a/apps/backend/internal/orchestrator/event_handlers_github_review_test.go b/apps/backend/internal/orchestrator/event_handlers_github_review_test.go index 259b70e0f7..a1e87d893a 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_review_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_review_test.go @@ -80,6 +80,39 @@ func TestBuildReviewTaskRequest_TruncatesTitle(t *testing.T) { } } +func TestBuildReviewTaskRequest_ForkPRRequiresManualStart(t *testing.T) { + tests := []struct { + name string + headOwner string + headRepo string + wantAutostart bool + }{ + {name: "same repository", headOwner: "acme", headRepo: "widget", wantAutostart: true}, + {name: "fork repository", headOwner: "contributor", headRepo: "widget"}, + {name: "missing head repository identity"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + evt := newReviewEvent() + evt.PR.HeadRepoOwner = tt.headOwner + evt.PR.HeadRepoName = tt.headRepo + + req := buildReviewTaskRequest(evt, nil, "acme/widget") + if req.Metadata[taskmodels.MetaKeyAutoStartGuard] != true { + t.Fatalf("auto-start guard = %v, want true", req.Metadata[taskmodels.MetaKeyAutoStartGuard]) + } + _, gotAutostart := req.Metadata[taskmodels.MetaKeyAutoStartClaimed] + if gotAutostart != tt.wantAutostart { + t.Errorf("automatic launch token present = %v, want %v", gotAutostart, tt.wantAutostart) + } + _, requiresManualStart := req.Metadata[taskmodels.MetaKeyForkPRRequiresManualStart] + if requiresManualStart != !tt.wantAutostart { + t.Errorf("manual-start marker present = %v, want %v", requiresManualStart, !tt.wantAutostart) + } + }) + } +} + // TestBuildIssueTaskTitle_TruncatesTitle mirrors the review-title regression for // the GitHub issue watcher's "Issue #<n>: <title>" builder. func TestBuildIssueTaskTitle_TruncatesTitle(t *testing.T) { @@ -175,6 +208,7 @@ func newReviewEvent() *github.NewReviewPREvent { PR: &github.PR{ Number: 42, Title: "Some PR", HTMLURL: "https://gh/acme/widget/pull/42", RepoOwner: "acme", RepoName: "widget", + HeadRepoOwner: "acme", HeadRepoName: "widget", }, } } @@ -434,6 +468,110 @@ func TestAutoStart_BothPathsFireExactlyOnce(t *testing.T) { } } +func TestAutoStart_ForkReviewWaitsForManualStart(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + const taskID = "task-fork-review-42" + const stepID = "step-fork-review" + + sg := newMockStepGetter() + sg.steps[stepID] = &wfmodels.WorkflowStep{ + ID: stepID, WorkflowID: "wf1", Name: "Review", Position: 0, + Events: wfmodels.StepEvents{ + OnEnter: []wfmodels.OnEnterAction{{Type: wfmodels.OnEnterAutoStartAgent}}, + }, + } + evt := newReviewEvent() + evt.WorkflowStepID = stepID + evt.PR.HeadRepoOwner = "contributor" + request := buildReviewTaskRequest(evt, nil, "acme/widget") + if _, hasToken := request.Metadata[taskmodels.MetaKeyAutoStartClaimed]; hasToken { + t.Fatal("fork review task must not carry an unattended auto-start token") + } + if request.Metadata[taskmodels.MetaKeyForkPRRequiresManualStart] != true { + t.Fatal("fork review task must persist the manual-start requirement") + } + + now := time.Now().UTC() + if err := repo.CreateWorkspace(ctx, &taskmodels.Workspace{ID: "ws1", Name: "Test", CreatedAt: now, UpdatedAt: now}); err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + if err := repo.CreateWorkflow(ctx, &taskmodels.Workflow{ID: "wf1", WorkspaceID: "ws1", Name: "WF", CreatedAt: now, UpdatedAt: now}); err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + metadata := make(map[string]interface{}, len(request.Metadata)+1) + for key, value := range request.Metadata { + metadata[key] = value + } + metadata[taskmodels.MetaKeyAgentProfileID] = testAgentProfileID + task := &taskmodels.Task{ + ID: taskID, WorkspaceID: "ws1", WorkflowID: "wf1", WorkflowStepID: stepID, + Title: request.Title, Description: request.Description, State: v1.TaskStateInProgress, + Metadata: metadata, CreatedAt: now, UpdatedAt: now, + } + if err := repo.CreateTask(ctx, task); err != nil { + t.Fatalf("CreateTask: %v", err) + } + + taskRepo := newMockTaskRepo() + taskRepo.tasks[taskID] = &v1.Task{ + ID: taskID, State: v1.TaskStateInProgress, + Metadata: map[string]interface{}{ + taskmodels.MetaKeyAutoStartGuard: true, + taskmodels.MetaKeyAgentProfileID: testAgentProfileID, + }, + } + var launchCount atomic.Int32 + launched := make(chan struct{}, 2) + agentMgr := &mockAgentManager{ + repoForExecutionLookup: repo, + launchAgentFunc: func(_ context.Context, _ *executor.LaunchAgentRequest) (*executor.LaunchAgentResponse, error) { + launchCount.Add(1) + launched <- struct{}{} + return &executor.LaunchAgentResponse{}, nil + }, + } + svc := createTestServiceWithScheduler(repo, sg, taskRepo, agentMgr) + + dbTask, err := repo.GetTask(ctx, taskID) + if err != nil { + t.Fatalf("GetTask: %v", err) + } + svc.autoStartReviewTask(ctx, evt, dbTask) + svc.autoStartTaskForStep(ctx, taskID, stepID, "task.queue_promoted", 0, false) + svc.autoStartTaskForLoadedStep(ctx, dbTask, sg.steps[stepID], "task.moved", false, 0, false) + select { + case <-launched: + t.Fatal("fork review task launched automatically") + case <-time.After(250 * time.Millisecond): + } + if got := launchCount.Load(); got != 0 { + t.Fatalf("automatic launch count = %d, want 0", got) + } + sessions, err := repo.ListTaskSessions(ctx, taskID) + if err != nil { + t.Fatalf("ListTaskSessions before manual start: %v", err) + } + if len(sessions) != 0 { + t.Fatalf("sessions before manual start = %d, want 0", len(sessions)) + } + if _, err := svc.StartTask(ctx, taskID, testAgentProfileID, "", "", "", "Review", stepID, false, true, nil); err != errForkPRManualStartRequired { + t.Fatalf("automatic StartTask error = %v, want %v", err, errForkPRManualStartRequired) + } + + if _, err := svc.StartTask(ctx, taskID, testAgentProfileID, "", "", "", "Review", stepID, false, false, nil); err != nil { + t.Fatalf("manual StartTask: %v", err) + } + select { + case <-launched: + case <-time.After(2 * time.Second): + t.Fatal("manual start did not launch the fork review task") + } + if got := launchCount.Load(); got != 1 { + t.Fatalf("launch count after manual start = %d, want 1", got) + } +} + // TestAutoStart_ClaimIsAtomic verifies that two competing calls to claimAutoStart // for the same task produce exactly one winner. func TestAutoStart_ClaimIsAtomic(t *testing.T) { diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow.go b/apps/backend/internal/orchestrator/event_handlers_workflow.go index 434eb03ede..ec2d653f67 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow.go @@ -1716,6 +1716,9 @@ func (s *Service) autoStartTaskForStep(ctx context.Context, taskID, stepID, even } return } + if s.skipForkPRAutoStart(ctx, task, eventName, autoStartOnCreateClaimed) { + return + } if task.QueuedForStepID != "" { if autoStartOnCreateClaimed { s.restoreAutoStartOnCreate(ctx, taskID, eventName) @@ -1948,6 +1951,9 @@ func (s *Service) autoStartTaskForLoadedStep(ctx context.Context, task *models.T } return } + if s.skipForkPRAutoStart(ctx, task, eventName, autoStartOnCreateClaimed) { + return + } if models.HasAutoStartOnCreateIntent(task.Metadata) || models.HasAutoStartOnCreateInFlight(task.Metadata) || autoStartOnCreateClaimed { sessions, err := s.repo.ListTaskSessions(ctx, task.ID) if err != nil { @@ -2097,6 +2103,26 @@ func (s *Service) autoStartTaskForLoadedStep(ctx context.Context, task *models.T }() } +func taskRequiresManualForkPRStart(task *models.Task) bool { + if task == nil || task.Metadata == nil { + return false + } + required, _ := task.Metadata[models.MetaKeyForkPRRequiresManualStart].(bool) + return required +} + +func (s *Service) skipForkPRAutoStart(ctx context.Context, task *models.Task, eventName string, autoStartOnCreateClaimed bool) bool { + if !taskRequiresManualForkPRStart(task) { + return false + } + s.logger.Info(eventName+": fork review task is waiting for a manual start", + zap.String("task_id", task.ID)) + if autoStartOnCreateClaimed || models.HasAutoStartOnCreateIntent(task.Metadata) || models.HasAutoStartOnCreateInFlight(task.Metadata) { + s.discardAutoStartOnCreate(ctx, task.ID, eventName) + } + return true +} + // autoStartOfficeTaskForLoadedStep is the Office-aware counterpart of the // kanban launch path above. An Office task has no session-based launch route // (see validateOfficeRuntimeEnv) — StartTask can never succeed for it, it diff --git a/apps/backend/internal/orchestrator/executor/executor_pr_base_identity_test.go b/apps/backend/internal/orchestrator/executor/executor_pr_base_identity_test.go index df713134ab..9634481089 100644 --- a/apps/backend/internal/orchestrator/executor/executor_pr_base_identity_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_pr_base_identity_test.go @@ -2,6 +2,7 @@ package executor import ( "context" + "errors" "testing" "github.com/kandev/kandev/internal/task/models" @@ -73,6 +74,205 @@ func TestResolveTaskRepoInfo_PRBaseRejectsDifferentHeadForSameNumber(t *testing. } } +// @covers AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.11 +// @covers AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.15 +func TestResolveTaskRepoInfo_TargetAttachedForkPRWithoutContribution(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", WorkspaceID: "workspace-1", SourceType: sourceTypeLocal, + LocalPath: t.TempDir(), Provider: "github", ProviderOwner: "upstream", + ProviderName: "widgets", DefaultBranch: "main", + } + target := forkPRComparisonTarget() + target.TargetBranch = "main" + resolver := &recordingPRBaseResolver{result: &models.PRBase{ + Target: *target, OID: "0123456789abcdef0123456789abcdef01234567", + }} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + exec.SetPRBaseResolver(resolver) + + info, err := exec.resolveTaskRepoInfo(context.Background(), &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", + BaseBranch: "main", CheckoutBranch: "feature", + Metadata: map[string]interface{}{"pr_number": 42}, + }) + if err != nil { + t.Fatalf("resolveTaskRepoInfo() error: %v", err) + } + if info.PRBase == nil || info.PRBase.Target.HeadRepository.Path != "fork-owner/widgets" || + info.BaseBranch != "main" { + t.Fatalf("resolved target-attached fork PR = %#v, want transient fork identity and target base", info) + } + if info.QualifiedPRBase != nil || info.ComparisonTarget != nil { + t.Fatalf("ordinary target-attached PR added qualified metadata: base=%#v target=%#v", + info.QualifiedPRBase, info.ComparisonTarget) + } +} + +func TestResolveTaskRepoInfo_TargetAttachedForkPRRejectsInvalidProviderIdentity(t *testing.T) { + tests := []struct { + name string + checkoutBranch string + mutate func(*models.ComparisonTarget) + }{ + {name: "unrelated target", checkoutBranch: "feature", mutate: func(target *models.ComparisonTarget) { + target.TargetRepository = models.ComparisonTargetRepository{ + Host: "github.com", Path: "other/widgets", RemoteURL: "https://github.com/other/widgets.git", + } + }}, + {name: "wrong PR number", checkoutBranch: "feature", mutate: func(target *models.ComparisonTarget) { + target.Number = 43 + }}, + {name: "wrong head branch", checkoutBranch: "feature", mutate: func(target *models.ComparisonTarget) { + target.HeadBranch = "other" + }}, + {name: "missing head repository", checkoutBranch: "feature", mutate: func(target *models.ComparisonTarget) { + target.HeadRepository = models.ComparisonTargetRepository{} + }}, + {name: "empty checkout branch", checkoutBranch: "", mutate: func(*models.ComparisonTarget) {}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", WorkspaceID: "workspace-1", SourceType: sourceTypeLocal, + LocalPath: t.TempDir(), Provider: "github", ProviderOwner: "upstream", + ProviderName: "widgets", DefaultBranch: "main", + } + target := forkPRComparisonTarget() + target.TargetBranch = "main" + tt.mutate(target) + resolver := &recordingPRBaseResolver{result: &models.PRBase{Target: *target}} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + exec.SetPRBaseResolver(resolver) + _, err := exec.resolveTaskRepoInfo(context.Background(), &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", + BaseBranch: "main", CheckoutBranch: tt.checkoutBranch, + Metadata: map[string]interface{}{"pr_number": 42}, + }) + if err == nil { + t.Fatal("resolveTaskRepoInfo() accepted an invalid target-attached PR identity") + } + }) + } +} + +func TestResolveTaskRepoInfo_TargetAttachedForkPRDoesNotOverrideExplicitBindings(t *testing.T) { + t.Run("comparison target", func(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = targetAttachedPRRepository(t) + expected := forkPRComparisonTarget() + metadata := map[string]interface{}{"pr_number": 42} + if err := models.PutComparisonTarget(metadata, expected); err != nil { + t.Fatalf("PutComparisonTarget() error: %v", err) + } + resolved := *expected + resolved.TargetBranch = "live-main" + resolved.HeadRepository = models.ComparisonTargetRepository{ + Host: "github.com", Path: "other-fork/widgets", RemoteURL: "https://github.com/other-fork/widgets.git", + } + resolver := &recordingPRBaseResolver{result: &models.PRBase{Target: resolved}} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + exec.SetPRBaseResolver(resolver) + info, err := exec.resolveTaskRepoInfo(context.Background(), &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", + BaseBranch: "main", CheckoutBranch: "feature", Metadata: metadata, + }) + if err != nil { + t.Fatalf("resolveTaskRepoInfo() error: %v", err) + } + if info.BaseBranch != expected.TargetBranch || info.PRBase == nil || + !info.PRBase.Target.Equal(*expected) { + t.Fatalf("mismatched provider data replaced comparison binding: %#v", info) + } + }) + + t.Run("contribution source", func(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = targetAttachedPRRepository(t) + metadata := map[string]interface{}{"pr_number": 42} + binding := &models.RemoteContribution{ + Version: models.RemoteContributionVersion, Provider: models.RemoteContributionProviderGitHub, + Kind: models.RemoteContributionKindPullRequest, CanonicalURL: "https://github.com/upstream/widgets/pull/42", + Number: 42, State: models.RemoteContributionStateOpen, BaseBranch: "main", HeadBranch: "feature", + HeadSHA: "0123456789abcdef0123456789abcdef01234567", + SourceRepository: models.RemoteContributionRepository{ + Host: "github.com", Path: "fork-owner/widgets", RemoteURL: "https://github.com/fork-owner/widgets.git", + }, + } + if err := models.PutRemoteContribution(metadata, binding); err != nil { + t.Fatalf("PutRemoteContribution() error: %v", err) + } + target := forkPRComparisonTarget() + target.TargetBranch = "main" + target.HeadRepository = models.ComparisonTargetRepository{ + Host: "github.com", Path: "other-fork/widgets", RemoteURL: "https://github.com/other-fork/widgets.git", + } + resolver := &recordingPRBaseResolver{result: &models.PRBase{Target: *target}} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + exec.SetPRBaseResolver(resolver) + if _, err := exec.resolveTaskRepoInfo(context.Background(), &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", + BaseBranch: "main", CheckoutBranch: "feature", Metadata: metadata, + }); err == nil { + t.Fatal("resolveTaskRepoInfo() accepted a provider head outside the contribution source binding") + } + }) +} + +func TestResolveTaskRepoInfo_TargetAttachedForkPRPreservesCancellation(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = targetAttachedPRRepository(t) + exec := newTestExecutor(t, &mockAgentManager{}, repo) + exec.SetPRBaseResolver(&recordingPRBaseResolver{err: context.Canceled}) + info, err := exec.resolveTaskRepoInfo(context.Background(), &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", + BaseBranch: "main", CheckoutBranch: "feature", Metadata: map[string]interface{}{"pr_number": 42}, + }) + if info != nil || !errors.Is(err, context.Canceled) { + t.Fatalf("resolveTaskRepoInfo() = (%#v, %v), want cancellation", info, err) + } +} + +func TestResolveAllRepoInfoRejectsLaunchWhenOnePRBindingIsInvalid(t *testing.T) { + repo := newMockRepository() + for id, owner := range map[string]string{"repo-1": "upstream", "repo-2": "other"} { + repo.repositories[id] = &models.Repository{ + ID: id, WorkspaceID: "workspace-1", SourceType: sourceTypeLocal, + LocalPath: t.TempDir(), Provider: "github", ProviderOwner: owner, + ProviderName: "widgets", DefaultBranch: "main", + } + } + for i, id := range []string{"repo-1", "repo-2"} { + repo.taskRepositories[id] = &models.TaskRepository{ + ID: "task-repo-" + id, TaskID: "task-1", RepositoryID: id, + Position: i, BaseBranch: "main", CheckoutBranch: "feature", + Metadata: map[string]interface{}{"pr_number": 42}, + } + } + target := forkPRComparisonTarget() + target.TargetBranch = "main" + resolver := &recordingPRBaseResolver{result: &models.PRBase{Target: *target}} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + exec.SetPRBaseResolver(resolver) + infos, err := exec.resolveAllRepoInfo(context.Background(), "task-1") + if err == nil || infos != nil { + t.Fatalf("resolveAllRepoInfo() = (%#v, %v), want whole launch rejected", infos, err) + } + if len(resolver.calls) != 2 { + t.Fatalf("resolver calls = %d, want both repository bindings checked", len(resolver.calls)) + } +} + +func targetAttachedPRRepository(t *testing.T) *models.Repository { + t.Helper() + return &models.Repository{ + ID: "repo-1", WorkspaceID: "workspace-1", SourceType: sourceTypeLocal, + LocalPath: t.TempDir(), Provider: "github", ProviderOwner: "upstream", + ProviderName: "widgets", DefaultBranch: "main", + } +} + // @covers AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.15 func TestResolveTaskRepoInfo_LegacyForkPRQualifiesLiveLinkedBaseForMaterialization(t *testing.T) { target := forkPRComparisonTarget() diff --git a/apps/backend/internal/orchestrator/executor/executor_pr_base_materialization_test.go b/apps/backend/internal/orchestrator/executor/executor_pr_base_materialization_test.go index 3055b44c18..92c6fb193f 100644 --- a/apps/backend/internal/orchestrator/executor/executor_pr_base_materialization_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_pr_base_materialization_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "reflect" "strings" "testing" @@ -91,6 +92,135 @@ func TestLegacyLinkedPRBaseFlowsIntoWorktreeMaterialization(t *testing.T) { } } +func TestTargetAttachedForkPRBasePreparationEndToEnd(t *testing.T) { + root := t.TempDir() + gitConfig := filepath.Join(root, "empty-git-config") + if err := os.WriteFile(gitConfig, nil, 0o600); err != nil { + t.Fatalf("create isolated Git config: %v", err) + } + t.Setenv("GIT_CONFIG_GLOBAL", gitConfig) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + repoPath, targetOID, forkMainOID, prHeadOID := initTargetAttachedForkPRMaterializationRepos(t, root) + if targetOID == forkMainOID { + t.Fatal("upstream and fork main branches unexpectedly share an OID") + } + + target := forkPRComparisonTarget() + target.HeadBranch = "feature/pr-42" + target.TargetBranch = "main" + resolver := &recordingPRBaseResolver{result: &models.PRBase{ + Target: *target, OID: targetOID, + }} + repositoryStore := newMockRepository() + repositoryStore.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", WorkspaceID: "workspace-1", Name: "widgets", SourceType: sourceTypeLocal, + LocalPath: repoPath, Provider: "github", ProviderOwner: "upstream", ProviderName: "widgets", + DefaultBranch: "main", + } + exec := newTestExecutor(t, &mockAgentManager{}, repositoryStore) + exec.SetPRBaseResolver(resolver) + taskRepo := &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", BaseBranch: "main", + CheckoutBranch: "feature/pr-42", Metadata: map[string]interface{}{"pr_number": 42}, + } + info, err := exec.resolveTaskRepoInfo(context.Background(), taskRepo) + if err != nil { + t.Fatalf("resolve target-attached PR task: %v", err) + } + retriedInfo, err := exec.resolveTaskRepoInfo(context.Background(), taskRepo) + if err != nil { + t.Fatalf("retry target-attached PR task before preparation: %v", err) + } + if retriedInfo.PRBase == nil || info.PRBase == nil || !reflect.DeepEqual(retriedInfo.PRBase, info.PRBase) { + t.Fatalf("retried PR identity = %#v, want unchanged provider identity %#v", retriedInfo.PRBase, info.PRBase) + } + info = retriedInfo + if info.PRBase == nil || info.PRBase.Target.HeadRepository.Path != "fork-owner/widgets" || + info.QualifiedPRBase != nil || info.ComparisonTarget != nil { + t.Fatalf("resolved PR identity = %#v, want transient fork source without a qualified target", info) + } + specs := buildRepoSpecs([]*repoInfo{info}) + if len(specs) != 1 || specs[0].BaseBranch != "main" || specs[0].PRNumber != 42 || specs[0].QualifiedPRBase != nil { + t.Fatalf("target-attached repository spec = %#v, want ordinary main + PR-head preparation", specs) + } + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + if err != nil { + t.Fatalf("create test logger: %v", err) + } + t.Cleanup(func() { _ = log.Close() }) + mgr, err := worktree.NewManager(worktree.Config{ + Enabled: true, TasksBasePath: filepath.Join(root, "tasks"), BranchPrefix: "kandev/", + }, nil, log) + if err != nil { + t.Fatalf("create worktree manager: %v", err) + } + prepared, err := lifecycle.NewWorktreePreparer(mgr, log).Prepare(context.Background(), &lifecycle.EnvPrepareRequest{ + TaskID: "task-1", WorkspaceID: "workspace-1", SessionID: "session-1", TaskDirName: "task-1", + RepositoryID: specs[0].RepositoryID, TaskRepositoryID: specs[0].TaskRepositoryID, + RepositoryPath: specs[0].RepositoryPath, RepoName: specs[0].RepoName, + UseWorktree: true, BaseBranch: specs[0].BaseBranch, DefaultBranch: specs[0].DefaultBranch, + CheckoutBranch: specs[0].CheckoutBranch, PRNumber: specs[0].PRNumber, + }, nil) + if err != nil { + t.Fatalf("prepare target-attached fork PR worktree: %v", err) + } + if !prepared.Success || prepared.BaseBranch != "main" { + t.Fatalf("prepared target-attached PR worktree = %#v, want successful main-based preparation", prepared) + } + if got := gitOutputInTest(t, prepared.WorkspacePath, "rev-parse", "HEAD"); got != prHeadOID { + t.Fatalf("prepared HEAD = %q, want fork PR head %q", got, prHeadOID) + } + if got := gitOutputInTest(t, repoPath, "rev-parse", "refs/remotes/origin/main"); got != targetOID { + t.Fatalf("origin/main = %q, want attached upstream OID %q", got, targetOID) + } + if got := gitOutputInTest(t, repoPath, "config", "--get", "remote.origin.url"); got != filepath.Join(root, "upstream.git") { + t.Fatalf("origin URL = %q, want unchanged upstream remote", got) + } + if got := gitOutputInTest(t, repoPath, "config", "--get", "remote.origin.pushurl"); got != "https://push.example/upstream/widgets.git" { + t.Fatalf("origin push URL = %q, want unchanged upstream push route", got) + } +} + +func initTargetAttachedForkPRMaterializationRepos( + t *testing.T, root string, +) (repoPath, upstreamMainOID, forkMainOID, prHeadOID string) { + t.Helper() + upstreamBare := filepath.Join(root, "upstream.git") + forkBare := filepath.Join(root, "fork.git") + upstreamSeed := filepath.Join(root, "upstream-seed") + forkSeed := filepath.Join(root, "fork-seed") + gitCommandInTest(t, root, "init", "--bare", "-b", "main", upstreamBare) + gitCommandInTest(t, root, "init", "-b", "main", upstreamSeed) + configureLegacyForkPRCommitter(t, upstreamSeed) + writeLegacyForkPRFile(t, upstreamSeed, "base.txt", "upstream main\n") + gitCommandInTest(t, upstreamSeed, "add", "base.txt") + gitCommandInTest(t, upstreamSeed, "commit", "-m", "upstream main") + gitCommandInTest(t, upstreamSeed, "remote", "add", "origin", upstreamBare) + gitCommandInTest(t, upstreamSeed, "push", "origin", "main") + upstreamMainOID = gitOutputInTest(t, upstreamSeed, "rev-parse", "HEAD") + gitCommandInTest(t, root, "init", "--bare", "-b", "main", forkBare) + gitCommandInTest(t, root, "init", "-b", "main", forkSeed) + configureLegacyForkPRCommitter(t, forkSeed) + writeLegacyForkPRFile(t, forkSeed, "base.txt", "fork main\n") + gitCommandInTest(t, forkSeed, "add", "base.txt") + gitCommandInTest(t, forkSeed, "commit", "-m", "fork main") + gitCommandInTest(t, forkSeed, "remote", "add", "origin", forkBare) + gitCommandInTest(t, forkSeed, "push", "origin", "main") + forkMainOID = gitOutputInTest(t, forkSeed, "rev-parse", "HEAD") + gitCommandInTest(t, forkSeed, "checkout", "-b", "feature/pr-42") + writeLegacyForkPRFile(t, forkSeed, "pr.txt", "fork pull request head\n") + gitCommandInTest(t, forkSeed, "add", "pr.txt") + gitCommandInTest(t, forkSeed, "commit", "-m", "fork pull request head") + prHeadOID = gitOutputInTest(t, forkSeed, "rev-parse", "HEAD") + gitCommandInTest(t, forkSeed, "push", "origin", "feature/pr-42") + gitCommandInTest(t, forkSeed, "remote", "add", "upstream", upstreamBare) + gitCommandInTest(t, forkSeed, "push", "upstream", "HEAD:refs/pull/42/head") + repoPath = filepath.Join(root, "upstream-checkout") + gitCommandInTest(t, root, "clone", upstreamBare, repoPath) + gitCommandInTest(t, repoPath, "config", "remote.origin.pushurl", "https://push.example/upstream/widgets.git") + return repoPath, upstreamMainOID, forkMainOID, prHeadOID +} + func initLegacyForkPRMaterializationRepos(t *testing.T, root string) (repoPath, upstreamBaseOID, prHeadOID string) { t.Helper() upstreamBare := filepath.Join(root, "upstream.git") diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index ce23d6227f..4cc686854f 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -418,6 +418,19 @@ func validPRBaseIdentity( if checkoutBranch != "" && target.HeadBranch != checkoutBranch { return false } + if expected != nil || contribution { + return validBoundPRBaseIdentity(target, expected, attachedRepository, hasAttachedRepository, + headRepository, hasHeadRepository, contribution) + } + return validUnboundPRBaseIdentity(target, checkoutBranch, attachedRepository, hasAttachedRepository, + headRepository, hasHeadRepository) +} + +func validBoundPRBaseIdentity( + target models.ComparisonTarget, expected *models.ComparisonTarget, + attachedRepository models.ComparisonTargetRepository, hasAttachedRepository bool, + headRepository models.ComparisonTargetRepository, hasHeadRepository, contribution bool, +) bool { if !hasAttachedRepository || !hasHeadRepository || !models.ComparisonTargetRepositoriesEqual(target.HeadRepository, headRepository) { return false @@ -433,6 +446,24 @@ func validPRBaseIdentity( models.ComparisonTargetRepositoriesEqual(expected.HeadRepository, target.HeadRepository) } +func validUnboundPRBaseIdentity( + target models.ComparisonTarget, checkoutBranch string, + attachedRepository models.ComparisonTargetRepository, hasAttachedRepository bool, + headRepository models.ComparisonTargetRepository, hasHeadRepository bool, +) bool { + if !hasAttachedRepository || !hasHeadRepository { + return false + } + if models.ComparisonTargetRepositoriesEqual(target.TargetRepository, attachedRepository) { + if models.ComparisonTargetRepositoriesEqual(target.HeadRepository, attachedRepository) { + return true + } + return checkoutBranch != "" + } + // Fork-attached legacy tasks bind the attached repository to the PR head. + return models.ComparisonTargetRepositoriesEqual(target.HeadRepository, headRepository) +} + func githubComparisonRepositoryFromRepository(repo *models.Repository) (models.ComparisonTargetRepository, bool) { if repo == nil { return models.ComparisonTargetRepository{}, false diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index 647399c6ef..6a76e53899 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -1452,6 +1452,8 @@ func (s *Service) startTask(ctx context.Context, taskID string, agentProfileID s if autoStart { if task, taskErr := s.repo.GetTask(ctx, taskID); taskErr != nil { return nil, fmt.Errorf("failed to fetch task for automatic launch gate: %w", taskErr) + } else if taskRequiresManualForkPRStart(task) { + return nil, errForkPRManualStartRequired } else if s.shouldSkipTerminalPRAutoStart(ctx, task) { return nil, nil } @@ -1964,6 +1966,7 @@ func validateOfficeRuntimeEnv(env map[string]string) error { } var ( + errForkPRManualStartRequired = errors.New("fork pull request review requires a manual start") errOfficeTaskStartRequiresScheduler = errors.New( "office tasks must be started through Office; use StartTaskWithEnv with scheduler-injected credentials", ) diff --git a/apps/backend/internal/task/models/models.go b/apps/backend/internal/task/models/models.go index 7b17b36c3a..27a89dcc28 100644 --- a/apps/backend/internal/task/models/models.go +++ b/apps/backend/internal/task/models/models.go @@ -178,6 +178,10 @@ const ( // because the winner will (or already did) handle it. // Absent on ordinary (non-watcher) auto-start tasks, which launch normally. MetaKeyAutoStartClaimed = "auto_start_claimed" + // MetaKeyForkPRRequiresManualStart blocks unattended launches of fork PR + // review tasks. Their worktree setup scripts can read executor-profile secrets; + // a user can still explicitly start the task after reviewing it. + MetaKeyForkPRRequiresManualStart = "fork_pr_requires_manual_start" // MetaKeyInterruptedAt is set by reconciliation when a task's session lost // an active turn. Its presence makes the task DTO report `interrupted: true` // so task-list surfaces show the warning indicator. The orchestrator removes diff --git a/apps/web/e2e/tests/task/create-task-github-url.spec.ts b/apps/web/e2e/tests/task/create-task-github-url.spec.ts index 38b12e5310..500ad346cd 100644 --- a/apps/web/e2e/tests/task/create-task-github-url.spec.ts +++ b/apps/web/e2e/tests/task/create-task-github-url.spec.ts @@ -4,6 +4,12 @@ import { useRegularMode } from "../../helpers/regular-mode"; import { KanbanPage } from "../../pages/kanban-page"; import { SessionPage } from "../../pages/session-page"; import { createEmptyRemoteRepository } from "../../helpers/empty-remote-repository"; +import { waitForHttp } from "../../helpers/causal-waits"; +import { + cleanupPRLinkForkLaunchFixture, + createPRLinkForkLaunchFixture, + expectForkPRLaunchState, +} from "./pr-link-fork-launch-helpers"; // Exercises the regular task-create dialog (New Task in the sidebar); run with office off. useRegularMode(); @@ -174,6 +180,72 @@ test.describe("Task creation from GitHub URL", () => { await expect(session.idleInput()).toBeVisible({ timeout: 15_000 }); }); + test("starts a target-attached fork PR from its URL", async ({ + testPage, + apiClient, + seedData, + backend, + }) => { + test.setTimeout(120_000); + const fixture = await createPRLinkForkLaunchFixture( + apiClient, + seedData.workspaceId, + backend.tmpDir, + ); + let taskId: string | undefined; + + try { + const { executors } = await apiClient.listExecutors(); + const worktreeExec = executors.find((executor) => executor.type === "worktree"); + if (!worktreeExec?.profiles?.[0]) { + test.skip(true, "No worktree executor profile available"); + return; + } + + const taskTitle = `Fork PR launch ${fixture.repositoryName}`; + const kanban = new KanbanPage(testPage); + await kanban.goto(); + await kanban.createTaskButton.first().click(); + const dialog = testPage.getByTestId("create-task-dialog"); + await expect(dialog).toBeVisible(); + await openRemoteAndPasteURL(testPage, fixture.prURL); + await expect(testPage.getByTestId("remote-branch-chip-trigger").first()).toContainText( + fixture.headBranch, + ); + await testPage.getByTestId("task-title-input").fill(taskTitle); + await testPage.getByTestId("task-description-input").fill("/e2e:simple-message"); + + const startButton = testPage.getByTestId("submit-start-agent"); + await expect(startButton).toBeEnabled(); + await testPage.getByTestId("executor-profile-selector").click(); + await testPage.getByRole("option", { name: /Worktree/i }).click(); + const createdTaskResponse = waitForHttp(testPage, "POST", /\/api\/v1\/tasks$/); + await startButton.click(); + const response = await createdTaskResponse; + const responseBody = await response.text(); + expect(response.status(), responseBody).toBe(200); + const created = JSON.parse(responseBody) as { id: string }; + taskId = created.id; + const requestBody = response.request().postDataJSON() as { + repositories?: Array<Record<string, unknown>>; + }; + expect(requestBody.repositories?.[0]).not.toHaveProperty("remote_contribution"); + expect(requestBody.repositories?.[0]).not.toHaveProperty("comparison_target"); + + await expect(dialog).not.toBeVisible(); + await expect(testPage).toHaveURL(new RegExp(`/t/${taskId}$`)); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await expect(session.chat.getByText("simple mock response", { exact: false })).toBeVisible(); + await expect(session.idleInput()).toBeVisible(); + + await expectForkPRLaunchState(testPage, session, apiClient, fixture, taskId); + await expect(session.prTopbarButton()).toContainText("#3879", { timeout: 15_000 }); + } finally { + await cleanupPRLinkForkLaunchFixture(apiClient, fixture, taskId); + } + }); + // Three tests previously asserted the top-level `github-url-error` testid // surfaced on invalid URL, nonexistent repo, and "clears on valid URL". // After Task 5/8 the URL input moved into a per-chip popover and no diff --git a/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts b/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts index 699f2801e9..6da3720c90 100644 --- a/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts +++ b/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts @@ -1,6 +1,14 @@ import { test, expect } from "../../fixtures/test-base"; import type { Page } from "@playwright/test"; import { MobileKanbanPage } from "../../pages/mobile-kanban-page"; +import { SessionPage } from "../../pages/session-page"; +import { waitForHttp } from "../../helpers/causal-waits"; +import { switchToTerminalPanel, waitForShellReady } from "../terminal/mobile-terminal-helpers"; +import { + cleanupPRLinkForkLaunchFixture, + createPRLinkForkLaunchFixture, + expectForkPRLaunchState, +} from "./pr-link-fork-launch-helpers"; function expectedRemoteTitle(title: string): string { const characters = Array.from(title); @@ -96,6 +104,80 @@ test.describe("Create task Remote repo picker on mobile", () => { await expectNoDocumentHorizontalOverflow(testPage); }); + test("starts a target-attached fork PR from its URL", async ({ + testPage, + apiClient, + seedData, + backend, + }) => { + test.setTimeout(120_000); + const fixture = await createPRLinkForkLaunchFixture( + apiClient, + seedData.workspaceId, + backend.tmpDir, + ); + let taskId: string | undefined; + + try { + const { executors } = await apiClient.listExecutors(); + const worktreeExec = executors.find((executor) => executor.type === "worktree"); + if (!worktreeExec?.profiles?.[0]) { + test.skip(true, "No worktree executor profile available"); + return; + } + + const taskTitle = `Phone fork PR ${fixture.repositoryName}`; + const mobile = new MobileKanbanPage(testPage); + await mobile.goto(); + await mobile.mobileFab.tap(); + const dialog = testPage.getByTestId("create-task-dialog"); + await expect(dialog).toBeVisible(); + await testPage.getByTestId("source-mode-remote").tap(); + await testPage.getByTestId("remote-repo-chip-trigger").first().tap(); + const urlInput = testPage.getByTestId("remote-repo-input"); + await expect(urlInput).toBeVisible(); + await urlInput.fill(fixture.prURL); + await urlInput.press("Enter"); + await expect(testPage.getByTestId("remote-branch-chip-trigger").first()).toContainText( + fixture.headBranch, + ); + await testPage.getByTestId("task-title-input").fill(taskTitle); + await testPage.getByTestId("task-description-input").fill("/e2e:simple-message"); + + const startButton = testPage.getByTestId("submit-start-agent"); + await expect(startButton).toBeEnabled(); + await testPage.getByTestId("executor-profile-selector").tap(); + await testPage.getByRole("option", { name: /Worktree/i }).tap(); + const createdTaskResponse = waitForHttp(testPage, "POST", /\/api\/v1\/tasks$/); + await startButton.tap(); + const response = await createdTaskResponse; + const responseBody = await response.text(); + expect(response.status(), responseBody).toBe(200); + const created = JSON.parse(responseBody) as { id: string }; + taskId = created.id; + const requestBody = response.request().postDataJSON() as { + repositories?: Array<Record<string, unknown>>; + }; + expect(requestBody.repositories?.[0]).not.toHaveProperty("remote_contribution"); + expect(requestBody.repositories?.[0]).not.toHaveProperty("comparison_target"); + + await expect(dialog).not.toBeVisible(); + await expect(mobile.taskCard(taskId)).toBeVisible({ timeout: 15_000 }); + await mobile.taskCard(taskId).tap(); + await expect(testPage).toHaveURL(new RegExp(`/t/${taskId}$`)); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await expect(session.chat.getByText("simple mock response", { exact: false })).toBeVisible(); + await expect(session.idleInput()).toBeVisible(); + + await switchToTerminalPanel(testPage); + await waitForShellReady(testPage); + await expectForkPRLaunchState(testPage, session, apiClient, fixture, taskId); + } finally { + await cleanupPRLinkForkLaunchFixture(apiClient, fixture, taskId); + } + }); + test("keeps a failed URL row touch-usable and retries its branch resolution", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/task/pr-link-fork-launch-helpers.ts b/apps/web/e2e/tests/task/pr-link-fork-launch-helpers.ts new file mode 100644 index 0000000000..b3045e4f04 --- /dev/null +++ b/apps/web/e2e/tests/task/pr-link-fork-launch-helpers.ts @@ -0,0 +1,211 @@ +import { randomUUID } from "node:crypto"; +import { expect, type Page } from "@playwright/test"; +import { + createEmptyRemoteRepository, + removeTestRepository, +} from "../../helpers/empty-remote-repository"; +import { GitHelper } from "../../helpers/git-helper"; +import type { ApiClient } from "../../helpers/api-client"; +import type { SessionPage } from "../../pages/session-page"; + +const PR_NUMBER = 3879; +const HEAD_BRANCH = "feature/fork-pr-launch"; + +export async function expectTerminalCommit(page: Page, commitOID: string): Promise<void> { + const readUnwrappedTerminal = async () => + page + .locator('[data-testid="terminal-panel"]:visible') + .first() + .locator(".xterm") + .evaluate((xtermElement) => { + const container = xtermElement.parentElement as HTMLElement & { + __xtermReadBuffer?: () => string; + }; + return (container.__xtermReadBuffer?.() ?? "").replace(/\s+/gu, ""); + }); + + await expect + .poll(readUnwrappedTerminal, { + timeout: 10_000, + message: `Expected terminal to contain commit ${commitOID} without line wrapping`, + }) + .toContain(commitOID); +} + +export type PRLinkForkLaunchFixture = { + repositoryId: string; + repositoryName: string; + upstreamOwner: string; + upstreamRepository: string; + forkOwner: string; + forkRepository: string; + prURL: string; + headBranch: string; + headOID: string; + targetOID: string; + forkCleanup: () => void; + upstreamCleanup: () => void; +}; + +export async function expectForkPRLaunchState( + page: Page, + session: SessionPage, + apiClient: ApiClient, + fixture: PRLinkForkLaunchFixture, + taskId: string, +): Promise<void> { + await expect(session.terminal).toBeVisible(); + await session.typeInTerminal("git branch --show-current"); + await session.expectTerminalHasText(fixture.headBranch); + await session.typeInTerminal("git rev-parse HEAD"); + await expectTerminalCommit(page, fixture.headOID); + await session.typeInTerminal("git rev-parse refs/remotes/origin/main"); + await expectTerminalCommit(page, fixture.targetOID); + + await page.reload(); + await session.waitForLoad(); + const task = await apiClient.getTask(taskId); + expect(task.repositories?.[0]?.checkout_branch).toBe(fixture.headBranch); + expect(task.repositories?.[0]?.base_branch).toBe("main"); + await expect + .poll(async () => (await apiClient.getTask(taskId)).status_summary?.pull_request?.number, { + timeout: 15_000, + message: "waiting for the persisted PR summary to reach the task list", + }) + .toBe(PR_NUMBER); + await expect + .poll(async () => + (await apiClient.listTaskPRs(taskId)).map((pr) => ({ + owner: pr.owner, + repo: pr.repo, + pr_number: pr.pr_number, + head_branch: pr.head_branch, + })), + ) + .toEqual([ + { + owner: fixture.upstreamOwner, + repo: fixture.upstreamRepository, + pr_number: PR_NUMBER, + head_branch: fixture.headBranch, + }, + ]); +} + +export async function createPRLinkForkLaunchFixture( + apiClient: ApiClient, + workspaceId: string, + tmpDir: string, +): Promise<PRLinkForkLaunchFixture> { + const suffix = randomUUID().replaceAll("-", "").slice(0, 10); + const upstreamOwner = `e2e-upstream-${suffix}`; + const upstreamRepository = `pr-link-upstream-${suffix}`; + const forkOwner = `e2e-fork-${suffix}`; + const forkRepository = `pr-link-fork-${suffix}`; + const upstream = createEmptyRemoteRepository(tmpDir, `pr-link-upstream-${suffix}`); + const fork = createEmptyRemoteRepository(tmpDir, `pr-link-fork-${suffix}`); + let repositoryId = ""; + + try { + const upstreamGit = new GitHelper(upstream.localPath, upstream.gitEnv); + upstreamGit.createFile("base.txt", "upstream target main\n"); + upstreamGit.stageAll(); + const targetOID = upstreamGit.commit("upstream target main"); + upstreamGit.exec("git push origin main"); + + const forkGit = new GitHelper(fork.localPath, fork.gitEnv); + forkGit.createFile("base.txt", "fork main\n"); + forkGit.stageAll(); + forkGit.commit("fork main"); + forkGit.exec("git push origin main"); + forkGit.exec(`git checkout -b ${HEAD_BRANCH}`); + forkGit.createFile("pr.txt", "fork pull request head\n"); + forkGit.stageAll(); + const headOID = forkGit.commit("fork pull request head"); + forkGit.exec(`git push origin ${HEAD_BRANCH}`); + forkGit.exec(`git push "${upstream.remoteURL}" HEAD:refs/pull/${PR_NUMBER}/head`); + + repositoryId = ( + await apiClient.createRepository(workspaceId, upstream.localPath, "main", { + name: `${upstreamOwner}/${upstreamRepository}`, + provider: "github", + provider_owner: upstreamOwner, + provider_name: upstreamRepository, + pull_before_worktree: false, + }) + ).id; + await apiClient.mockGitHubAddBranches(upstreamOwner, upstreamRepository, [ + { name: "main" }, + { name: HEAD_BRANCH }, + ]); + await apiClient.mockGitHubAddPRs([ + { + number: PR_NUMBER, + title: "Fork PR task launch", + state: "open", + head_branch: HEAD_BRANCH, + head_sha: headOID, + head_repo_id: 387901, + head_repo_owner: forkOwner, + head_repo_name: forkRepository, + head_repo_clone_url: `https://github.com/${forkOwner}/${forkRepository}.git`, + base_branch: "main", + base_repo_id: 387902, + base_repo_owner: upstreamOwner, + base_repo_name: upstreamRepository, + base_default_branch: "main", + author_login: "e2e-fork-author", + repo_owner: upstreamOwner, + repo_name: upstreamRepository, + }, + ]); + + return { + repositoryId, + repositoryName: `${upstreamOwner}/${upstreamRepository}`, + upstreamOwner, + upstreamRepository, + forkOwner, + forkRepository, + prURL: `https://github.com/${upstreamOwner}/${upstreamRepository}/pull/${PR_NUMBER}`, + headBranch: HEAD_BRANCH, + headOID, + targetOID, + forkCleanup: fork.cleanup, + upstreamCleanup: upstream.cleanup, + }; + } catch (error) { + if (repositoryId) await removeTestRepository(apiClient, repositoryId).catch(() => undefined); + fork.cleanup(); + upstream.cleanup(); + throw error; + } +} + +export async function cleanupPRLinkForkLaunchFixture( + apiClient: ApiClient, + fixture: PRLinkForkLaunchFixture, + taskId?: string, +): Promise<void> { + let cleanupError: unknown; + if (taskId) { + try { + await apiClient.deleteTask(taskId); + } catch (error) { + cleanupError = error; + } + } + try { + await removeTestRepository(apiClient, fixture.repositoryId); + } catch (error) { + cleanupError ??= error; + } + for (const cleanup of [fixture.forkCleanup, fixture.upstreamCleanup]) { + try { + cleanup(); + } catch (error) { + cleanupError ??= error; + } + } + if (cleanupError) throw cleanupError; +} diff --git a/docs/decisions/2026-09-23-fork-pr-review-watch-manual-start.md b/docs/decisions/2026-09-23-fork-pr-review-watch-manual-start.md new file mode 100644 index 0000000000..9e84f24eb2 --- /dev/null +++ b/docs/decisions/2026-09-23-fork-pr-review-watch-manual-start.md @@ -0,0 +1,51 @@ +# ADR-2026-09-23: Require a manual start for fork PR review tasks + +**Status:** accepted +**Date:** 2026-09-23 +**Area:** backend, integrations, security, workflow + +## Context + +GitHub review watches create tasks attached to the PR target repository and may +auto-start them from a configured workflow step. Fork PR tasks check out +contributor-controlled files. Repository setup scripts run against that +checkout and receive executor-profile environment values, including secrets. + +The workspace base-resolution contract must support valid fork PR checkouts. +That must not make a background review watch execute contributor-controlled +setup inputs without a user action. + +## Decision + +GitHub review-watch tasks may auto-start only when provider data confirms that +the PR head and target are the same repository. A different or incomplete head +repository identity leaves the linked task waiting for an explicit manual +start. Manual starts remain available and do not grant later background +auto-starts. Same-repository review-watch behavior remains unchanged. Ordinary +PR-link task creation remains user initiated and is unaffected. + +The backend enforces this rule at review-task creation metadata and automated +launch boundaries. Fork PR base resolution and push routing remain unchanged. + +## Consequences + +- Fork PR reviews remain visible as tasks without running setup scripts in the + background. +- A user can inspect the task and manually start it when they accept the + repository setup and execution risk. +- Missing provider head identity fails closed for watcher auto-start. +- The integration system must retain the manual-start marker through task + persistence and check it in every automated launch path. + +## Alternatives Considered + +- **Reject fork PR review tasks.** Rejected because the watch can still create a + useful linked task for a user to start manually. +- **Run setup scripts without profile environment values.** Rejected because + configured setup commands may require those values, and removing them does + not sandbox the checked-out code. +- **Add a per-PR approval control or trust label.** Deferred because it adds a + new provider-specific approval lifecycle. An explicit task start already + supplies a clear user action. +- **Auto-start all valid fork identities.** Rejected because provider identity + proves which repository supplied the head; it does not make its code trusted. diff --git a/docs/plans/fork-pr-base-resolution/plan.md b/docs/plans/fork-pr-base-resolution/plan.md index 6c59e45689..128d1d0f24 100644 --- a/docs/plans/fork-pr-base-resolution/plan.md +++ b/docs/plans/fork-pr-base-resolution/plan.md @@ -152,6 +152,9 @@ materialization has its own agentctl API regression from Task 02. ## Companion packages +- [PR-link fork launch repair](../pr-link-fork-launch/plan.md): pending follow-up + for ordinary target-attached browser PR tasks. Historical results here remain unchanged. + - [Fork PR comparison targets](../fork-pr-comparison-targets/plan.md): implemented; reuse its identity, remote, and manual-selection invariants. - [Noninteractive comparison Git](../noninteractive-comparison-target-git/plan.md): diff --git a/docs/plans/pr-link-fork-launch/plan.md b/docs/plans/pr-link-fork-launch/plan.md new file mode 100644 index 0000000000..2e4de7ea28 --- /dev/null +++ b/docs/plans/pr-link-fork-launch/plan.md @@ -0,0 +1,213 @@ +--- +created: 2026-09-23 +status: done +requirements: + - REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001 + - REQ-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001 +system_design: + - ../../specs/workspaces/system-design/worktree-base-refresh.md + - ../../specs/integrations/system-design/github-fork-review-start.md +legacy_specs: [] +--- + +# Implementation Plan: PR-link fork launch + +## Overview + +Restore user-submitted task startup from a fork PR link. Task 01 repaired +attachment-aware base validation. Task 02 proves desktop and phone creation +with real Git fixtures and the mock provider. The PR security review also +preserves a manual-start boundary for unattended fork review-watch tasks. + +## Evidence and root cause + +Task `979b1b1b-3b52-44c2-8ea5-f317baf39839` failed on 2026-09-23 at +12:39:34 UTC, before agent startup. The backend reported: + +```text +pull request identity did not match the task repository binding +``` + +The retained attachment had `base_branch=main`, +`checkout_branch=feature/archive-force-remove-csd`, and only `pr_number=3879` +in metadata. GitHub returned source `nova28/kandev` and target `kdlbs/kandev`. +The source branch was `feature/archive-force-remove-csd`. The target was `main`. + +Commit `a2d4ee8c185` (#3878) added the rejecting identity check. +`resolvePRBaseForLaunch` assumes that the attached repository owns the head +unless `RemoteContribution` supplies another source. Browser creation emits +ordinary PR-link metadata. Its separate PR association runs asynchronously. +The MCP contribution coordinator does not run on this browser creation path. + +Evidence came from live task metadata, the GitHub API, source history, and +backend bundle `7841647e48331f84267ca55ea09d2bbd`. The running build was +`v0.95.1-20-gf2d52f18263`. The archive was size-limited but retained creation +and failure. No permanent reproduction test existed when the failure was +investigated. Task 01 added the regression and recorded its pre-fix RED result. + +## Requirement conformance + +The workspace system owns preparation identity and materialization. This is an +implementation regression against existing criteria, not a new product feature. +Reuse [REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001](../../specs/workspaces/requirements/worktree-base-refresh.md), +criteria .11 and .15 through .19. The security review identified a separate +integration contract: [REQ-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001](../../specs/integrations/requirements/github-fork-review-start.md) +keeps background review-watch starts disabled for fork or identity-incomplete +PRs. See its [system design](../../specs/integrations/system-design/github-fork-review-start.md) +and [decision](../../decisions/2026-09-23-fork-pr-review-watch-manual-start.md). +The [design clarification](../../specs/workspaces/system-design/worktree-base-refresh.md#ordinary-pr-link-launch-compatibility) +separates target-attached PR checkout from fork-attached checkout. + +The user confirmed that creating a task from a fork PR must work. +No material intent question remains. This package does not convert ordinary +browser tasks into the MCP contribution workflow. That workflow has additional +collaboration and push-policy semantics owned by the task system. + +## Scope + +### In scope + +- Validated target-attached fork PR launch with existing metadata. +- Launch before or after asynchronous PR association, and retry before preparation. +- Exact provider namespace, PR number, head branch, and repository checks. +- Compatibility with explicit contribution bindings and fork-attached checkouts. +- Desktop and phone PR-link startup evidence. +- Preventing unattended GitHub review-watch launches for fork PRs until a user + starts the linked task explicitly. + +### Out of scope + +- New controls, copy, public API fields, database schema, runtime flags, or permission model. +- Changing source remotes, push routing, or contribution collaboration policy. +- Resetting an existing checkout, repairing the live task, or automatic retries. +- Redesigning PR association ordering, GitLab behavior, or comparison reconciliation. +- Adding a fork trust UI, approval label, or setup-script sandbox. + +## Technical approach + +`apps/backend/internal/orchestrator/executor/executor_resume.go` owns the +attachment-aware decision in `resolvePRBaseForLaunch` and `validPRBaseIdentity`. +An unbound target-attached checkout can accept a provider-validated fork head +only when the target, PR number, and non-empty checkout branch match. +An explicit source binding always wins. A mismatch cannot enter this legacy path. + +`apps/backend/internal/backendapp/orchestrator.go` owns namespace lookup and +`selectLinkedTaskPRForBase`. Preserve rejection of ambiguous linked matches. +The no-association path queries the attached namespace. It must work during +the browser association race. Existing typed errors retain fallback decisions. + +GitHub review-watch task creation applies the separate trust contract. Confirmed +same-repository PRs retain their configured auto-start token. Fork or missing +head identities retain a manual-start marker and no unattended token. Workflow +gates and the automatic `StartTask` boundary enforce it; direct manual start +remains available. Ordinary user-submitted PR-link tasks do not carry the +marker. + +Keep the resolved source identity transient in `PRBase`. No database migration +or new durable binding is required. The target-attached checkout keeps the +ordinary target base and target PR-head fetch. Fork-attached checkouts retain +qualified upstream base handling. Do not derive permission from PR identity. + +## Tests + +Workspace criterion suffixes refer to `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001`; +the integration row below maps to its separately linked requirement. + +| Criteria | Implemented evidence | +| --- | --- | +| .11, .15 | `executor_pr_base_identity_test.go`: `TestResolveTaskRepoInfo_TargetAttachedForkPRWithoutContribution` recorded RED before the fix and passes after it | +| .15, .19 | Same test file: explicit source mismatch, unrelated target, wrong branch/number, incomplete identity, same-repository compatibility | +| .15, .17 | `backendapp/pr_base_resolver_test.go`: linked row absent/present, duplicate matches, provider failure, cancellation | +| .16, .18 | `apps/backend/internal/orchestrator/executor/executor_pr_base_materialization_test.go`: `TestTargetAttachedForkPRBasePreparationEndToEnd` proves exact head/base and unchanged push routing | +| .17, .19 | `executor_pr_base_identity_test.go`: `TestResolveAllRepoInfoRejectsLaunchWhenOnePRBindingIsInvalid` blocks a mixed launch without a fallback checkout | +| Integration AC 001.1-.4 | `apps/backend/internal/orchestrator/event_handlers_github_review_test.go`: `TestBuildReviewTaskRequest_ForkPRRequiresManualStart` and `TestAutoStart_ForkReviewWaitsForManualStart` cover same-repository auto-start, fork or missing-identity suppression, automatic launch rejection, and explicit manual start | + +## E2E tests + +Task 02 extends `apps/web/e2e/tests/task/create-task-github-url.spec.ts` +(project `chromium`) and `mobile-create-task-remote-repo.spec.ts` +(project `mobile-chrome`). Both paste an upstream PR URL, start the task, +observe the agent, reload, and retain the PR association and selected branch. +The fixture publishes a fork head into the upstream PR ref namespace. +It must not preseed `RemoteContribution` or `ComparisonTarget` metadata. +These flows cover .11, .15, .16, and .19. + +There is no rendered UI change, so no ASCII preview is required. Existing +Start Task dialog and mobile creation surface keep their controls, scrolling, +and navigation. Phone coverage uses touch submission on the existing surface. + +## Work orders + +- [x] [Task 01: Repair PR attachment identity validation](task-01-attachment-identity.md) +- [x] [Task 02: Prove browser PR-link startup](task-02-pr-link-startup.md) + +Task 02 depends on Task 01. Execution is sequential. No delegation is authorized. + +## Companion packages + +[Fork PR base resolution](../fork-pr-base-resolution/plan.md) introduced this +regression. Its completed work and command results remain historical evidence. +This package owns the additional cases and new verification results. +[Fork PR comparison targets](../fork-pr-comparison-targets/plan.md) retains +comparison ownership and push invariants. Neither package is reopened. + +## Verification results + +Task 01 implementation and focused backend verification passed: + +- The target-attached fork identity regression failed before the executor fix + with `pull request identity did not match the task repository binding`. +- Focused identity, invalid-identity, explicit-binding, cancellation, + multi-repository and real Git preparation tests passed. +- `go test ./internal/orchestrator/executor ./internal/backendapp ./internal/worktree -count=1`: passed. +- `python3 scripts/list-docs.py validate`, `python3 scripts/lint-spec-files.test.py`, + `python3 scripts/lint-spec-files.py --all`, and `git diff --check`: passed. + +Task 02 browser implementation and verification passed: + +- `(cd apps/web && pnpm e2e:run --project chromium tests/task/create-task-github-url.spec.ts)`: 10 passed. +- `(cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-create-task-remote-repo.spec.ts)`: 7 passed. +- Both suites ran against freshly built backend and pseudo-locale Vite assets. They created upstream and fork repositories with different `main` commits, submitted the ordinary PR URL without contribution/comparison bindings, launched the mock agent, and verified the exact fork `HEAD` and upstream target base commit. +- After reload, desktop shows the `#3879` PR topbar association. Desktop and phone both retain the checkout/base branches and persisted PR summary/association. The phone test submits with touch and opens the created task from the mobile card. +- Fixture cleanup removes its task, repository, and temporary Git remotes in `finally` paths. +- `make -C apps/backend build`, web `pnpm run typecheck`, and targeted ESLint on the three changed E2E files: passed. +- `python3 scripts/list-docs.py validate`, `python3 scripts/lint-spec-files.test.py` (36 tests), `python3 scripts/lint-spec-files.py --all`, and `git diff --check`: passed. +- An additional repository-wide `pnpm run lint:e2e-sleeps` check remains red on unrelated existing violations and unresolved rule references across other files; the changed E2E files pass targeted ESLint. + +The fork review-watch security follow-up adds a separate integration contract: + +- `go test ./internal/orchestrator -run 'TestBuildReviewTaskRequest_ForkPRRequiresManualStart|TestAutoStart_ForkReviewWaitsForManualStart' -count=1`: passed. +- `(cd apps/backend && go test ./internal/orchestrator ./internal/orchestrator/executor ./internal/backendapp ./internal/worktree -count=1)`: passed. +- `make -C apps/backend build`: passed. +- `(cd apps/web && pnpm e2e:run --project chromium tests/task/create-task-github-url.spec.ts)`: 10 passed. +- `(cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-create-task-remote-repo.spec.ts)`: 7 passed. +- `(cd apps/web && pnpm exec eslint e2e/tests/task/create-task-github-url.spec.ts e2e/tests/task/mobile-create-task-remote-repo.spec.ts e2e/tests/task/pr-link-fork-launch-helpers.ts)`: passed. +- `python3 scripts/list-docs.py validate`, `python3 scripts/lint-spec-files.test.py`, `python3 scripts/lint-spec-files.py --all`, and `git diff --check`: passed. +- Same-repository review watches retain their one-shot auto-start token. Fork or identity-incomplete watches stay available without an unattended launch token; both workflow launch paths and the central automatic `StartTask` boundary enforce the marker, while explicit manual start succeeds. +- The contract and its trust boundary are recorded in the integration requirement, system design, and decision linked above. Browser PR-link tasks do not carry the marker. + +Historical design-package checks at the 2026-09-23 handoff: + +- `python3 scripts/list-docs.py validate`: passed (299 decisions, 1124 specifications). +- `python3 scripts/lint-spec-files.test.py`: passed (36 tests). +- `python3 scripts/lint-spec-files.py --all`: passed. +- `git diff --check`: passed. +- Relative document links and referenced acceptance IDs: validated. +- At that handoff, the plan and both work orders were pending implementation. Production and test code were unchanged, and no product tests ran during planning. Task 01 and Task 02 were implemented and verified afterward as recorded above. + +## Risks + +- Accepting any fork head would weaken binding validation. Explicit bindings + must never fall through to the ordinary target-attached path. +- Requiring a linked row would make startup depend on asynchronous timing. +- A fixture with a same-repository head or preseeded binding would hide this defect. +- Local Git URL rewrites must remain inside disposable test repositories. + +## Documentation impact + +The implementation restores existing PR-link behavior without new user +instructions. The fork review-watch security boundary is recorded in the new +integration requirement, system design, and decision linked above. Public docs +remain unchanged because no new user-facing controls or instructions were added. +The existing repository-qualified comparison ADR remains authoritative for +comparison and push invariants. diff --git a/docs/plans/pr-link-fork-launch/task-01-attachment-identity.md b/docs/plans/pr-link-fork-launch/task-01-attachment-identity.md new file mode 100644 index 0000000000..2057a66ca1 --- /dev/null +++ b/docs/plans/pr-link-fork-launch/task-01-attachment-identity.md @@ -0,0 +1,168 @@ +--- +id: "01-attachment-identity" +title: "Repair PR attachment identity validation" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001 + - REQ-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001 +acceptance_criteria: + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.11 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.15 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.16 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.17 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.18 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.19 + - AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.1 + - AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.2 + - AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.3 + - AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.4 +system_design: + - ../../specs/workspaces/system-design/worktree-base-refresh.md + - ../../specs/integrations/system-design/github-fork-review-start.md +--- + +# Task 01: Repair PR attachment identity validation + +This work order preserves the original implementation sequence. The Results +section records the delivered implementation and verification. + +## Summary + +Accept a valid fork PR when the ordinary task attachment identifies its target +repository. Preserve exact identity checks for explicit bindings and fork-attached +checkouts. Resolve from existing metadata so unprepared failed tasks can retry. +Keep unattended GitHub review-watch execution disabled for fork or +identity-incomplete PRs until a user starts the linked task. + +## In scope + +- Add `TestResolveTaskRepoInfo_TargetAttachedForkPRWithoutContribution` in + `executor_pr_base_identity_test.go` before production edits. Use an upstream + attachment, a distinct fork head, matching checkout branch, and only PR metadata. + The initial run must fail with the reported repository-binding error. +- Update `resolvePRBaseForLaunch` and `validPRBaseIdentity` with the three + attachment forms from the design. Keep provider source identity transient. +- Cover target mismatch, wrong PR number/branch, missing head identity, and empty + checkout branch. Explicit contribution and comparison mismatches cannot enter + the unbound target-attached path. Keep current explicit-binding fallback rules. +- Cover resolver lookup with no linked row, one exact linked row, duplicate + matches, same-number foreign rows, cancellation, and provider errors. +- Add `TestTargetAttachedForkPRBasePreparationEndToEnd` in + `executor_pr_base_materialization_test.go`, beside existing real Git + materialization coverage. Use separate fork/upstream repositories with different + same-named base commits. Publish the fork head under upstream `refs/pull/N/head`. + Prove target base, PR head, and unchanged origin/push configuration. +- Repeat resolution for an unprepared task to represent retry. Include a mixed + two-repository preparation where one invalid binding blocks the whole launch. +- Keep same-repository GitHub review watches eligible for configured auto-start. + Persist a manual-start marker for fork or identity-incomplete review watches, + enforce it at automated launch boundaries, and prove a direct manual start + remains available. Leave ordinary browser PR-link tasks unaffected. + +## Out of scope + +New database schema or migrations, browser payload changes, contribution push +authorization, automatic repair of live tasks, and PR-association lifecycle +changes. + +## Acceptance + +1. The ordinary upstream-attached fixture fails before the fix and succeeds + after it, with or without a completed linked association. +2. Explicit binding mismatches and ambiguous identities keep their existing + rejection or stored-qualified-target behavior. No unrelated base is applied. +3. Real Git preparation uses the correct head and target commits. Retry and + mixed-repository checks preserve cancellation, fallback, and push invariants. + +## Planned verification sequence + +This is the original RED/GREEN sequence. The Results section records the +commands and outcomes from implementation. + +```bash +(cd apps/backend && go test ./internal/orchestrator/executor -run '^TestResolveTaskRepoInfo_TargetAttachedForkPRWithoutContribution$' -count=1 -v) +(cd apps/backend && go test ./internal/orchestrator/executor -run '^TestTargetAttachedForkPRBasePreparationEndToEnd$' -count=1 -v) +(cd apps/backend && go test ./internal/orchestrator/executor ./internal/backendapp ./internal/worktree -count=1) +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +The first test records the expected pre-fix RED; the second proves real Git +materialization. Both tests reside in `internal/orchestrator/executor`. The +final package run also covers existing same-repository, explicit contribution, +resolver, and qualified-base regressions. + +## Files touched + +- `apps/backend/internal/orchestrator/executor/executor_resume.go` +- `apps/backend/internal/orchestrator/executor/executor_pr_base_identity_test.go` +- `apps/backend/internal/orchestrator/executor/executor_pr_base_materialization_test.go` +- `apps/backend/internal/backendapp/pr_base_resolver_test.go` +- `apps/backend/internal/orchestrator/event_handlers_github.go` +- `apps/backend/internal/orchestrator/event_handlers_workflow.go` +- `apps/backend/internal/orchestrator/task_operations.go` +- `apps/backend/internal/orchestrator/event_handlers_github_review_test.go` +- `apps/backend/internal/task/models/models.go` +- `docs/specs/integrations/requirements/github-fork-review-start.md` +- `docs/specs/integrations/system-design/github-fork-review-start.md` +- `docs/decisions/2026-09-23-fork-pr-review-watch-manual-start.md` + +## Dependencies + +None. + +## Risks + +A loose target match can bypass an explicit source binding. A linked-row +requirement can reintroduce the browser association race. Tests must exercise +production resolution and preparation, not only a copied predicate. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirement](../../specs/workspaces/requirements/worktree-base-refresh.md), .11 and .15-.19. +- [Design clarification](../../specs/workspaces/system-design/worktree-base-refresh.md#ordinary-pr-link-launch-compatibility). +- [Fork review-start requirement](../../specs/integrations/requirements/github-fork-review-start.md). +- [Fork review-start design](../../specs/integrations/system-design/github-fork-review-start.md) and [decision](../../decisions/2026-09-23-fork-pr-review-watch-manual-start.md). +- `executor_pr_base_identity_test.go` and + `executor_pr_base_materialization_test.go`. +- [Comparison target ADR](../../decisions/2026-08-19-repository-qualified-comparison-targets.md). + +## Results + +Completed. The ordinary target-attached fork regression failed before the fix +with the reported repository-binding error. It passes after the change, along +with wrong target/number/branch, missing head repository, empty checkout, +explicit comparison/source mismatches, cancellation, retry, mixed-repository, +resolver association and real Git preparation coverage. + +Commands and results: + +- `go test ./internal/orchestrator/executor -run '^TestResolveTaskRepoInfo_TargetAttachedForkPRWithoutContribution$' -count=1 -v`: expected RED before production change. +- `go test ./internal/orchestrator/executor -run '^TestTargetAttachedForkPRBasePreparationEndToEnd$' -count=1 -v`: expected RED before production change; passed after the test exercised normal PR-head fetch. +- Focused new executor and resolver tests: passed. +- `go test ./internal/orchestrator/executor ./internal/backendapp ./internal/worktree -count=1`: passed. +- Specification catalog, spec linter tests, all-spec lint and `git diff --check`: passed. + +Security follow-up results: + +- `go test ./internal/orchestrator -run 'TestBuildReviewTaskRequest_ForkPRRequiresManualStart|TestAutoStart_ForkReviewWaitsForManualStart' -count=1`: passed. +- `go test ./internal/orchestrator ./internal/orchestrator/executor ./internal/backendapp ./internal/worktree -count=1`: passed. +- `make -C apps/backend build`: passed. +- `(cd apps/web && pnpm e2e:run --project chromium tests/task/create-task-github-url.spec.ts)`: 10 passed. +- `(cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-create-task-remote-repo.spec.ts)`: 7 passed. +- `python3 scripts/list-docs.py validate`, `python3 scripts/lint-spec-files.test.py`, `python3 scripts/lint-spec-files.py --all`, and `git diff --check`: passed. +- Same-repository review watches retain automatic start. Fork and identity-incomplete review watches remain available without unattended execution; workflow and central automatic-start gates enforce the marker, and direct manual start launches. +- Requirement, system design, and decision links above record the delivered security boundary. Browser PR-link launch remains unchanged. + +The test fixture uses separate fork/upstream bare repositories with different +same-named `main` commits. It publishes the fork head under the upstream PR +ref and preserves the upstream origin and push URL during target-attached +preparation. diff --git a/docs/plans/pr-link-fork-launch/task-02-pr-link-startup.md b/docs/plans/pr-link-fork-launch/task-02-pr-link-startup.md new file mode 100644 index 0000000000..2c93bb3694 --- /dev/null +++ b/docs/plans/pr-link-fork-launch/task-02-pr-link-startup.md @@ -0,0 +1,127 @@ +--- +id: "02-pr-link-startup" +title: "Prove browser PR-link startup" +status: done +wave: 2 +depends_on: + - "01-attachment-identity" +plan: "plan.md" +requirements: + - REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001 +acceptance_criteria: + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.11 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.15 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.16 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.17 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.18 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.19 +system_design: + - ../../specs/workspaces/system-design/worktree-base-refresh.md +--- + +# Task 02: Prove browser PR-link startup + +## Summary + +Prove that the existing desktop and phone task-creation flows launch a fork PR. +Use the normal browser payload and backend preparation, without preseeded +contribution or comparison bindings. + +## In scope + +- Extend `create-task-github-url.spec.ts` with a test named + `starts a target-attached fork PR from its URL`. +- Extend `mobile-create-task-remote-repo.spec.ts` with the same scenario. + Reuse the phone remote picker and touch submission patterns from that file. +- Extract shared Git/provider fixture setup into + `pr-link-fork-launch-helpers.ts` in the same directory. +- Create disposable upstream and fork repositories. Give same-named base + branches different commits. Publish the fork head in upstream `refs/pull/N/head`. + Configure provider metadata with distinct head and target repository identities. +- Paste the upstream PR URL through the existing UI, select the worktree + executor, and submit. Assert agent output, exact checkout head, target base, + and the visible PR association. Reload and assert retained branch/association. +- Confirm that the fixture does not insert `RemoteContribution` or + `ComparisonTarget` metadata. The browser must use its ordinary creation path. +- Preserve existing local-executor, same-repository, and missing-PR-ref scenarios. + Restore fixture changes and remove owned repositories even after assertions fail. + +## Out of scope + +New UI layout, controls, copy, translation keys, page objects unrelated to this +flow, live-instance testing, and broad E2E runs. + +## Acceptance + +1. Desktop creation launches the mock agent from the fork head and target base. + Reload retains the selected branch and PR association. +2. Phone creation proves the same outcome through touch-accessible controls. + Existing layout, navigation, and scroll ownership remain unchanged. +3. Both focused suites pass with freshly built assets. Existing scenarios remain + present. Results identify the discovered and executed test counts. + +## Verification + +Run from the repository root. Install dependencies once in a fresh worktree. +Managed E2E commands build and tear down their own isolated instances. + +```bash +(cd apps && pnpm install --frozen-lockfile) +(cd apps/web && pnpm e2e:run --project chromium tests/task/create-task-github-url.spec.ts) +(cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-create-task-remote-repo.spec.ts) +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.test.py +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +Task 01 owns the deterministic pre-fix RED test. Browser tests verify the same +regression through production builds after that correction. Do not claim RED +from a missing selector, unavailable binary, or failed fixture setup. + +## Files likely touched + +- `apps/web/e2e/tests/task/create-task-github-url.spec.ts` +- `apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts` +- `apps/web/e2e/tests/task/pr-link-fork-launch-helpers.ts` (new). +- `docs/plans/pr-link-fork-launch/plan.md` and these work-order result sections. + +## Dependencies + +Task 01. Its backend correction must pass its targeted commands first. + +## Risks + +Existing mock PR defaults can accidentally make the head and target identical. +A contribution-binding fixture would bypass the failed path. Shared Git changes +can leak into later tests unless fixtures own and remove their temporary paths. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirement](../../specs/workspaces/requirements/worktree-base-refresh.md), .11 and .15-.19. +- [Design clarification](../../specs/workspaces/system-design/worktree-base-refresh.md#ordinary-pr-link-launch-compatibility). +- Existing worktree PR launch in `create-task-github-url.spec.ts`. +- Existing phone remote creation in `mobile-create-task-remote-repo.spec.ts`. +- `.agents/skills/e2e/SKILL.md` and `.agents/skills/mobile-parity/SKILL.md`. + +## Results + +Completed. The fixture uses disposable upstream and fork repositories with +different `main` commits, publishes the fork head under the upstream PR ref, +and seeds distinct provider head/target identities. The ordinary creation +request contains neither `remote_contribution` nor `comparison_target`. + +Commands and results: + +- `(cd apps/web && pnpm e2e:run --project chromium tests/task/create-task-github-url.spec.ts)`: 10 passed. +- `(cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-create-task-remote-repo.spec.ts)`: 7 passed. +- Both suites used freshly built backend and pseudo-locale Vite assets. The mock agent launched from the expected checkout branch. Terminal checks verified the full fork `HEAD` and upstream target `main` commit OIDs. +- After reload, both flows retained `checkout_branch`, `base_branch`, PR summary number, and exact persisted PR association. The desktop flow also displays `#3879` in the session PR topbar. The phone flow uses touch submission and opens the created task from its mobile task card. +- Cleanup is in `finally` paths and removes the task, test repository, and temporary upstream/fork Git directories. +- `make -C apps/backend build` and web `pnpm run typecheck`: passed. Targeted ESLint for both changed specs and the shared fixture: passed. +- `python3 scripts/list-docs.py validate`, `python3 scripts/lint-spec-files.test.py` (36 tests), `python3 scripts/lint-spec-files.py --all`, and `git diff --check`: passed. +- Additional broad `pnpm run lint:e2e-sleeps` check: failed on existing errors outside the changed E2E files, including unsanctioned waits in unrelated tests and unresolved rule references. Targeted ESLint for changed files passes. diff --git a/docs/specs/integrations/requirements/github-fork-review-start.md b/docs/specs/integrations/requirements/github-fork-review-start.md new file mode 100644 index 0000000000..23db4397a8 --- /dev/null +++ b/docs/specs/integrations/requirements/github-fork-review-start.md @@ -0,0 +1,46 @@ +--- +status: active +system: integrations +created: 2026-09-23 +updated: 2026-09-23 +owners: + - kandev +--- + +# GitHub fork review task start + +## Overview + +GitHub review watches can create tasks from pull requests whose code comes from +a fork. The linked task must remain available while unattended execution waits +for a user to start it. + +## Requirements + +### REQ-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001: Safe fork review launch + +**Intent:** Preserve automatic review of same-repository pull requests while +requiring an explicit user start before a review watch runs fork-controlled +checkout content. + +#### Acceptance criteria + +- **AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.1:** When provider data + confirms that a watched pull request's head and target are in the same + repository, the review task shall retain its configured auto-start behavior. +- **AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.2:** When the watched pull + request head belongs to another repository, or its head repository identity + is unavailable, the review task shall be created and associated without an + unattended agent start. +- **AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.3:** A user shall be able to + start a fork review task explicitly. The manual start shall not make later + background auto-starts eligible. +- **AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.4:** User-submitted PR-link + task creation shall keep its existing launch behavior. + +## Exclusions + +- Adding a fork-approval UI, trust label, or provider setting. +- Sandboxing setup scripts or changing their environment. +- Changing repository identity validation, contribution authorization, or push + routing. diff --git a/docs/specs/integrations/system-design/github-fork-review-start.md b/docs/specs/integrations/system-design/github-fork-review-start.md new file mode 100644 index 0000000000..9e0f8249e2 --- /dev/null +++ b/docs/specs/integrations/system-design/github-fork-review-start.md @@ -0,0 +1,88 @@ +--- +status: current +system: integrations +requirements: + - REQ-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001 +created: 2026-09-23 +updated: 2026-09-23 +owners: + - kandev +--- + +# GitHub fork review task start design + +## Ownership and mapping + +The integration system owns GitHub review-watch task creation and its +provider-specific auto-start decision. The task system owns task persistence. +The [workspace worktree design](../../workspaces/system-design/worktree-base-refresh.md) +continues to own repository-qualified PR preparation. + +| Acceptance criterion | Design section | +| --- | --- | +| `AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.1` | [Identity and auto-start token](#identity-and-auto-start-token) | +| `AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.2` | [Automated launch gates](#automated-launch-gates) | +| `AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.3` | [Manual start](#manual-start) | +| `AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.4` | [Manual PR-link creation](#manual-pr-link-creation) | + +## Trust boundary + +A fork PR head controls files read by repository setup commands such as package +install scripts. Those commands run before the agent starts and can receive +executor-profile environment values. Provider validation proves the head +identity and target relationship; it does not establish trust in the source +code. + +The [manual-start decision](../../../decisions/2026-09-23-fork-pr-review-watch-manual-start.md) +records why the watcher leaves fork tasks idle and why provider identity alone +does not authorize execution. + +## Identity and auto-start token + +`buildReviewTaskRequest` compares the PR head owner and repository name with the +target owner and name using case-insensitive GitHub identity. If both identities +are present and equal, it creates the normal one-shot auto-start token. If the +head differs or either identity is missing, it persists +`fork_pr_requires_manual_start` and does not create that token. The permanent +auto-start guard remains so competing watcher and workflow paths cannot bypass +the decision. + +The check does not alter `checkout_branch`, `base_branch`, PR association, +comparison target, contribution binding, or Git push configuration. + +## Automated launch gates + +The task metadata marker is checked before deferred workflow launch and at the +central automatic `StartTask` boundary. Review-watch and workflow auto-start +paths leave the task available without preparing a worktree or running setup. +The central check protects other automated callers that reach `StartTask` +directly. A missing marker preserves existing behavior. + +The marker is durable for the task. Manual launch does not clear it, so later +background automation remains blocked. This avoids converting one user action +into ongoing approval for future unattended runs. + +## Manual start + +A direct user start uses the existing non-automatic task launch path. It is not +blocked by `fork_pr_requires_manual_start`, so no approval screen, setting, or +new API field is required. The existing task state and PR association remain +available while the task waits. + +## Manual PR-link creation + +Browser PR-link creation is a user-submitted task flow rather than a review +watch. It does not carry the review-watch marker. Existing explicit task +creation and launch behavior therefore remains unchanged for fork PR links. + +## Verification + +Backend tests cover same-repository auto-start tokens, fork and missing-head +identity suppression, both review-watch auto-start paths, rejection at the +central automatic launch boundary, and successful manual start. Existing +browser tests continue to prove manual PR-link launch from a fork head. + +## Related decisions + +- [Fork PR review watch manual start](../../../decisions/2026-09-23-fork-pr-review-watch-manual-start.md) +- [Repository secrets](../../../decisions/2026-08-03-scope-and-merge-repository-secrets.md) diff --git a/docs/specs/workspaces/system-design/worktree-base-refresh.md b/docs/specs/workspaces/system-design/worktree-base-refresh.md index d9186c5693..2334f44d20 100644 --- a/docs/specs/workspaces/system-design/worktree-base-refresh.md +++ b/docs/specs/workspaces/system-design/worktree-base-refresh.md @@ -127,7 +127,7 @@ Resolve the provider namespace from the exact attachment's existing binding: checkout. For a `RemoteContribution`, compare the PR head to the validated source repository and require the PR target repository to match the attachment. An ambiguous or mismatched response cannot resolve a qualified - PR base. + PR base. Ordinary target-attached PR links use the compatibility path below. Never choose the first PR in a task-wide list. Validate current binding ownership before applying a live retarget. Preserve manual comparison selections and @@ -148,6 +148,57 @@ provider snapshot. The fetched commit supplies the current OID. Without a validated target identity, a known cross-repository request remains unresolved. Same-repository offline branch fallback retains criterion .11. +### Ordinary PR-link launch compatibility + +The [PR-link fork launch repair](../../../plans/pr-link-fork-launch/plan.md) +clarifies criterion .15 for existing browser-created attachments. This amendment +is implemented. Its plan and work orders record the implementation and +verification status. + +The browser PR-link path stores the target repository, PR number, checkout +branch, and base branch. It does not create a `RemoteContribution` binding. +`associatePRFromRepoInputs` links the PR asynchronously. Launch must therefore +resolve identity correctly before and after that association exists. + +Base validation distinguishes these attachment forms: + +- With `RemoteContribution`, the persisted source repository and head branch + remain authoritative. The provider target must match the attached repository. +- With a fork-attached checkout, the provider head must match the attached + repository and checkout branch. Existing comparison-target checks still apply. +- With an ordinary target-attached PR checkout, provider resolution must use + the attached target namespace and exact PR number. The returned target must + match that attachment, and the returned head branch must match the checkout. + The validated provider response supplies the source repository for this + launch. Missing, malformed, unrelated, or ambiguous identities are rejected. + +The last form requires a positive PR number and a non-empty checkout branch. +It applies only without an explicit contribution or comparison binding. +It cannot override an explicit source binding after a mismatch. An unrelated +linked PR cannot authorize this form merely by sharing a number or branch. + +`validPRBaseIdentity` must evaluate the attachment form before applying its head +repository comparison. Do not remove repository checks or accept a branch-only +match. `githubPRBaseResolver` retains provider namespace validation and exact +linked-association selection. Without a linked row, it resolves the attached +namespace directly. Multiple matching linked rows remain an error. + +For a target-attached checkout, the existing PR-head fetch uses the target +repository's PR namespace. The comparison base remains that target repository's +branch. A fork-attached checkout retains qualified upstream materialization. +No contribution binding, comparison binding, credential scope, or push destination +is synthesized by this compatibility path. Provider resolution is read-only. + +Retry of an unprepared legacy task uses the same validation. Valid worktree +reuse and explicit manual-base overrides retain their existing rules. +A provider outage retains the existing fallback policy. Known cross-repository +errors and cancellation must not become branch-only fallback successes. + +Coverage includes ordinary PR-link metadata with and without a linked row, +explicit source mismatch, unrelated target, malformed identity, colliding PR +numbers, and two attachments where only one is valid. A real Git fixture proves +that PR head and base commits come from their respective repositories. + ### Preparation and materialization Carry the qualified target through `repoInfo`, lifecycle launch and preparation