fix: preserve GitHub review PR identity for auto-start (#3928)
This commit is contained in:
@@ -105,6 +105,17 @@ type repoFileEntry struct {
|
||||
Content []byte
|
||||
}
|
||||
|
||||
type mockPRDetailResult struct {
|
||||
pr *PR
|
||||
err error
|
||||
}
|
||||
|
||||
type mockReviewWatchResponses struct {
|
||||
details map[prKey]mockPRDetailResult
|
||||
searchResults []*PR
|
||||
searchResultsSet bool
|
||||
}
|
||||
|
||||
// MockClient implements Client with in-memory configurable data for E2E testing.
|
||||
// All data is protected by a sync.RWMutex for thread safety.
|
||||
type MockClient struct {
|
||||
@@ -161,6 +172,7 @@ type MockClient struct {
|
||||
// singleflight coalescing.
|
||||
probeEntered chan string
|
||||
probeRelease chan struct{}
|
||||
reviewWatch mockReviewWatchResponses
|
||||
}
|
||||
|
||||
// mockGist captures a gist that was created via the mock client so tests
|
||||
@@ -179,6 +191,7 @@ func NewMockClient() *MockClient {
|
||||
user: mockDefaultUser,
|
||||
authenticated: true,
|
||||
prs: make(map[prKey]*PR),
|
||||
reviewWatch: mockReviewWatchResponses{details: make(map[prKey]mockPRDetailResult)},
|
||||
issues: make(map[issueKey]*Issue),
|
||||
prsByBranch: make(map[branchKey]*PR),
|
||||
repos: make(map[string][]GitHubRepo),
|
||||
@@ -218,7 +231,11 @@ func (m *MockClient) GetAuthenticatedUser(context.Context) (string, error) {
|
||||
func (m *MockClient) GetPR(_ context.Context, owner, repo string, number int) (*PR, error) {
|
||||
m.mu.RLock()
|
||||
defer m.mu.RUnlock()
|
||||
pr, ok := m.prs[prKey{owner, repo, number}]
|
||||
key := prKey{owner, repo, number}
|
||||
if result, ok := m.reviewWatch.details[key]; ok {
|
||||
return result.pr, result.err
|
||||
}
|
||||
pr, ok := m.prs[key]
|
||||
if !ok {
|
||||
return nil, fmt.Errorf("mock: PR %s/%s#%d not found", owner, repo, number)
|
||||
}
|
||||
@@ -306,6 +323,9 @@ func (m *MockClient) ListAuthoredPRs(_ context.Context, owner, repo string) ([]*
|
||||
func (m *MockClient) ListReviewRequestedPRs(context.Context, string, string, string) ([]*PR, error) {
|
||||
m.mu.RLock()
|
||||
defer m.mu.RUnlock()
|
||||
if m.reviewWatch.searchResultsSet {
|
||||
return append([]*PR(nil), m.reviewWatch.searchResults...), nil
|
||||
}
|
||||
var result []*PR
|
||||
for _, pr := range m.prs {
|
||||
if len(pr.RequestedReviewers) > 0 {
|
||||
@@ -949,6 +969,23 @@ func (m *MockClient) AddPR(pr *PR) {
|
||||
}
|
||||
}
|
||||
|
||||
// SetReviewRequestedPRs overrides search-shaped review-requested results.
|
||||
// Use SetPRDetail to provide the separate GetPR response for each result.
|
||||
func (m *MockClient) SetReviewRequestedPRs(prs []*PR) {
|
||||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
m.reviewWatch.searchResultsSet = true
|
||||
m.reviewWatch.searchResults = append([]*PR(nil), prs...)
|
||||
}
|
||||
|
||||
// SetPRDetail overrides GetPR for one PR, allowing search and detail responses
|
||||
// to differ in tests that exercise provider enrichment.
|
||||
func (m *MockClient) SetPRDetail(owner, repo string, number int, pr *PR, err error) {
|
||||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
m.reviewWatch.details[prKey{owner, repo, number}] = mockPRDetailResult{pr: pr, err: err}
|
||||
}
|
||||
|
||||
func (m *MockClient) ensurePRHeadSHA(owner, repo string, number int, fallback string) (string, bool) {
|
||||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
@@ -1185,6 +1222,7 @@ func (m *MockClient) Reset() {
|
||||
m.authError = ""
|
||||
m.reposUnavailable = false
|
||||
m.prs = make(map[prKey]*PR)
|
||||
m.reviewWatch = mockReviewWatchResponses{details: make(map[prKey]mockPRDetailResult)}
|
||||
m.issues = make(map[issueKey]*Issue)
|
||||
m.prsByBranch = make(map[branchKey]*PR)
|
||||
m.orgs = nil
|
||||
|
||||
@@ -342,6 +342,7 @@ func TestMockClient_Reset(t *testing.T) {
|
||||
|
||||
m.SetUser("custom")
|
||||
m.AddPR(&PR{Number: 1, RepoOwner: "o", RepoName: "r", HeadBranch: "b"})
|
||||
m.SetPRDetail("o", "r", 1, &PR{Number: 1, Title: "detail override"}, nil)
|
||||
m.AddOrgs([]GitHubOrg{{Login: "org1"}})
|
||||
|
||||
m.Reset()
|
||||
|
||||
@@ -340,8 +340,8 @@ func (s *Service) CheckReviewWatch(ctx context.Context, watch *ReviewWatch) ([]*
|
||||
}
|
||||
}
|
||||
|
||||
// Enrich new PRs with full details (branch info) from the PR API,
|
||||
// since the search API does not return head/base branch.
|
||||
// Enrich new PRs with provider details because search results can omit
|
||||
// branch information and head repository identity.
|
||||
s.enrichPRDetails(ctx, resolved.Client, newPRs)
|
||||
|
||||
s.logger.Debug("review watch check complete",
|
||||
@@ -476,30 +476,46 @@ func repoFilterToQualifier(repo RepoFilter) string {
|
||||
return fmt.Sprintf("repo:%s/%s", repo.Owner, repo.Name)
|
||||
}
|
||||
|
||||
// enrichPRDetails fetches full PR details for PRs missing branch info (from the search API).
|
||||
// enrichPRDetails fetches full PR details for search results missing branch
|
||||
// information or head repository identity.
|
||||
func (s *Service) enrichPRDetails(ctx context.Context, client Client, prs []*PR) {
|
||||
for _, pr := range prs {
|
||||
if pr.HeadBranch != "" && pr.BaseBranch != "" {
|
||||
if pr.HeadBranch != "" && pr.BaseBranch != "" && pr.HeadRepoOwner != "" && pr.HeadRepoName != "" {
|
||||
continue
|
||||
}
|
||||
s.logger.Debug("enriching PR with full details (missing branch info)",
|
||||
s.logger.Debug("enriching PR with full details (missing required review metadata)",
|
||||
zap.String("repo", pr.RepoOwner+"/"+pr.RepoName),
|
||||
zap.Int("pr_number", pr.Number))
|
||||
|
||||
full, err := client.GetPR(ctx, pr.RepoOwner, pr.RepoName, pr.Number)
|
||||
if err != nil {
|
||||
s.logger.Warn("failed to fetch full PR details, branch info will be empty",
|
||||
s.logger.Warn("failed to fetch full PR details, review metadata will remain incomplete",
|
||||
zap.String("repo", pr.RepoOwner+"/"+pr.RepoName),
|
||||
zap.Int("pr_number", pr.Number),
|
||||
zap.Error(err))
|
||||
continue
|
||||
}
|
||||
if full == nil {
|
||||
continue
|
||||
}
|
||||
pr.HeadBranch = full.HeadBranch
|
||||
pr.HeadSHA = full.HeadSHA
|
||||
pr.BaseBranch = full.BaseBranch
|
||||
pr.BaseSHA = full.BaseSHA
|
||||
pr.Additions = full.Additions
|
||||
pr.Deletions = full.Deletions
|
||||
pr.Mergeable = full.Mergeable
|
||||
pr.MergeableState = full.MergeableState
|
||||
pr.HeadRepoID = full.HeadRepoID
|
||||
pr.HeadRepoNodeID = full.HeadRepoNodeID
|
||||
pr.HeadRepoOwner = full.HeadRepoOwner
|
||||
pr.HeadRepoName = full.HeadRepoName
|
||||
pr.HeadRepoCloneURL = full.HeadRepoCloneURL
|
||||
pr.BaseRepoID = full.BaseRepoID
|
||||
pr.BaseRepoOwner = full.BaseRepoOwner
|
||||
pr.BaseRepoName = full.BaseRepoName
|
||||
pr.BaseDefaultBranch = full.BaseDefaultBranch
|
||||
pr.MaintainerCanModify = full.MaintainerCanModify
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,212 @@
|
||||
package github
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"testing"
|
||||
|
||||
"github.com/kandev/kandev/internal/events"
|
||||
"github.com/kandev/kandev/internal/events/bus"
|
||||
)
|
||||
|
||||
type reviewWatchSearchClient struct {
|
||||
*MockClient
|
||||
searchPR *PR
|
||||
detailPR *PR
|
||||
detailErr error
|
||||
getPRCalls int
|
||||
getPROwner string
|
||||
getPRRepo string
|
||||
getPRNum int
|
||||
}
|
||||
|
||||
func (c *reviewWatchSearchClient) ListReviewRequestedPRs(context.Context, string, string, string) ([]*PR, error) {
|
||||
return []*PR{c.searchPR}, nil
|
||||
}
|
||||
|
||||
func (c *reviewWatchSearchClient) GetPR(_ context.Context, owner, repo string, number int) (*PR, error) {
|
||||
c.getPRCalls++
|
||||
c.getPROwner = owner
|
||||
c.getPRRepo = repo
|
||||
c.getPRNum = number
|
||||
return c.detailPR, c.detailErr
|
||||
}
|
||||
|
||||
type reviewPRDetails struct {
|
||||
headBranch string
|
||||
headSHA string
|
||||
baseBranch string
|
||||
baseSHA string
|
||||
additions int
|
||||
deletions int
|
||||
mergeable bool
|
||||
mergeableState string
|
||||
headRepoID int64
|
||||
headRepoNodeID string
|
||||
headRepoOwner string
|
||||
headRepoName string
|
||||
headRepoCloneURL string
|
||||
baseRepoID int64
|
||||
baseRepoOwner string
|
||||
baseRepoName string
|
||||
baseDefaultBranch string
|
||||
maintainerCanModify bool
|
||||
}
|
||||
|
||||
func reviewPRDetailsFrom(pr *PR) reviewPRDetails {
|
||||
return reviewPRDetails{
|
||||
headBranch: pr.HeadBranch,
|
||||
headSHA: pr.HeadSHA,
|
||||
baseBranch: pr.BaseBranch,
|
||||
baseSHA: pr.BaseSHA,
|
||||
additions: pr.Additions,
|
||||
deletions: pr.Deletions,
|
||||
mergeable: pr.Mergeable,
|
||||
mergeableState: pr.MergeableState,
|
||||
headRepoID: pr.HeadRepoID,
|
||||
headRepoNodeID: pr.HeadRepoNodeID,
|
||||
headRepoOwner: pr.HeadRepoOwner,
|
||||
headRepoName: pr.HeadRepoName,
|
||||
headRepoCloneURL: pr.HeadRepoCloneURL,
|
||||
baseRepoID: pr.BaseRepoID,
|
||||
baseRepoOwner: pr.BaseRepoOwner,
|
||||
baseRepoName: pr.BaseRepoName,
|
||||
baseDefaultBranch: pr.BaseDefaultBranch,
|
||||
maintainerCanModify: pr.MaintainerCanModify,
|
||||
}
|
||||
}
|
||||
|
||||
func TestCheckReviewWatchEnrichesSearchResultsBeforePublishingEvent(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
detailPR *PR
|
||||
detailErr error
|
||||
wantDetails reviewPRDetails
|
||||
}{
|
||||
{
|
||||
name: "same repository identity",
|
||||
detailPR: &PR{
|
||||
RepoOwner: "acme", RepoName: "widget",
|
||||
HeadBranch: "feature/detail", HeadSHA: "head-sha-detail",
|
||||
BaseBranch: "main-detail", BaseSHA: "base-sha-detail",
|
||||
Additions: 7, Deletions: 3, Mergeable: true, MergeableState: "clean",
|
||||
HeadRepoID: 101, HeadRepoNodeID: "H_repo", HeadRepoOwner: "acme",
|
||||
HeadRepoName: "widget", HeadRepoCloneURL: "https://github.com/acme/widget.git",
|
||||
BaseRepoID: 202, BaseRepoOwner: "acme", BaseRepoName: "widget",
|
||||
BaseDefaultBranch: "main", MaintainerCanModify: true,
|
||||
},
|
||||
wantDetails: reviewPRDetails{
|
||||
headBranch: "feature/detail", headSHA: "head-sha-detail",
|
||||
baseBranch: "main-detail", baseSHA: "base-sha-detail",
|
||||
additions: 7, deletions: 3, mergeable: true, mergeableState: "clean",
|
||||
headRepoID: 101, headRepoNodeID: "H_repo", headRepoOwner: "acme",
|
||||
headRepoName: "widget", headRepoCloneURL: "https://github.com/acme/widget.git",
|
||||
baseRepoID: 202, baseRepoOwner: "acme", baseRepoName: "widget",
|
||||
baseDefaultBranch: "main", maintainerCanModify: true,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "fork identity",
|
||||
detailPR: &PR{
|
||||
HeadBranch: "contrib/feature", HeadSHA: "fork-head-sha",
|
||||
BaseBranch: "main", BaseSHA: "base-sha",
|
||||
HeadRepoID: 303, HeadRepoNodeID: "H_fork", HeadRepoOwner: "contributor",
|
||||
HeadRepoName: "widget-fork", HeadRepoCloneURL: "https://github.com/contributor/widget-fork.git",
|
||||
BaseRepoID: 202, BaseRepoOwner: "acme", BaseRepoName: "widget",
|
||||
BaseDefaultBranch: "main", MaintainerCanModify: false,
|
||||
Mergeable: false, MergeableState: "blocked", Additions: 4, Deletions: 2,
|
||||
},
|
||||
wantDetails: reviewPRDetails{
|
||||
headBranch: "contrib/feature", headSHA: "fork-head-sha",
|
||||
baseBranch: "main", baseSHA: "base-sha", additions: 4, deletions: 2,
|
||||
mergeable: false, mergeableState: "blocked",
|
||||
headRepoID: 303, headRepoNodeID: "H_fork", headRepoOwner: "contributor",
|
||||
headRepoName: "widget-fork", headRepoCloneURL: "https://github.com/contributor/widget-fork.git",
|
||||
baseRepoID: 202, baseRepoOwner: "acme", baseRepoName: "widget",
|
||||
baseDefaultBranch: "main", maintainerCanModify: false,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "detail lookup failure keeps identity unavailable",
|
||||
detailErr: errors.New("GitHub detail lookup failed"),
|
||||
wantDetails: reviewPRDetails{
|
||||
headBranch: "feature/search", baseBranch: "main", additions: 1, deletions: 2,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "nil detail response keeps identity unavailable",
|
||||
wantDetails: reviewPRDetails{
|
||||
headBranch: "feature/search", baseBranch: "main", additions: 1, deletions: 2,
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
store := newTestStore(t)
|
||||
searchPR := &PR{
|
||||
Number: 42, Title: "Review this change", HTMLURL: "https://github.com/acme/widget/pull/42",
|
||||
RepoOwner: "acme", RepoName: "widget",
|
||||
HeadBranch: "feature/search", BaseBranch: "main", Additions: 1, Deletions: 2,
|
||||
}
|
||||
client := &reviewWatchSearchClient{
|
||||
MockClient: NewMockClient(), searchPR: searchPR, detailPR: tt.detailPR, detailErr: tt.detailErr,
|
||||
}
|
||||
eventBus := bus.NewMemoryEventBus(testLogger(t))
|
||||
t.Cleanup(eventBus.Close)
|
||||
var emitted *NewReviewPREvent
|
||||
subscription, err := eventBus.Subscribe(events.GitHubNewReviewPR, func(_ context.Context, event *bus.Event) error {
|
||||
payload, ok := event.Data.(*NewReviewPREvent)
|
||||
if !ok {
|
||||
return errors.New("event data is not a NewReviewPREvent")
|
||||
}
|
||||
emitted = payload
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("subscribe to review event: %v", err)
|
||||
}
|
||||
t.Cleanup(func() { _ = subscription.Unsubscribe() })
|
||||
|
||||
service := NewService(client, AuthMethodPAT, nil, store, eventBus, testLogger(t))
|
||||
t.Cleanup(service.Stop)
|
||||
configureTestWorkspaceAuth(t, service, client, "workspace-1")
|
||||
watch := &ReviewWatch{
|
||||
ID: "watch-1", WorkspaceID: "workspace-1", WorkflowID: "workflow-1",
|
||||
WorkflowStepID: "step-1", Repos: []RepoFilter{{Owner: "acme", Name: "widget"}},
|
||||
Enabled: true,
|
||||
}
|
||||
if err := store.CreateReviewWatch(ctx, watch); err != nil {
|
||||
t.Fatalf("create review watch: %v", err)
|
||||
}
|
||||
|
||||
prs, err := service.TriggerReviewWatch(ctx, watch)
|
||||
if err != nil {
|
||||
t.Fatalf("TriggerReviewWatch: %v", err)
|
||||
}
|
||||
if len(prs) != 1 {
|
||||
t.Fatalf("new review PR count = %d, want 1", len(prs))
|
||||
}
|
||||
if client.getPRCalls != 1 || client.getPROwner != "acme" || client.getPRRepo != "widget" || client.getPRNum != 42 {
|
||||
t.Fatalf("GetPR calls = (%d, %q, %q, %d), want one lookup for acme/widget#42",
|
||||
client.getPRCalls, client.getPROwner, client.getPRRepo, client.getPRNum)
|
||||
}
|
||||
if got := reviewPRDetailsFrom(prs[0]); got != tt.wantDetails {
|
||||
t.Errorf("enriched PR details = %+v, want %+v", got, tt.wantDetails)
|
||||
}
|
||||
if prs[0].RepoOwner != "acme" || prs[0].RepoName != "widget" {
|
||||
t.Errorf("search target repository = %s/%s, want acme/widget", prs[0].RepoOwner, prs[0].RepoName)
|
||||
}
|
||||
if emitted == nil || emitted.PR == nil {
|
||||
t.Fatal("review watch did not publish a PR event")
|
||||
}
|
||||
if got := reviewPRDetailsFrom(emitted.PR); got != tt.wantDetails {
|
||||
t.Errorf("event PR details = %+v, want %+v", got, tt.wantDetails)
|
||||
}
|
||||
if emitted.PR.RepoOwner != "acme" || emitted.PR.RepoName != "widget" {
|
||||
t.Errorf("event target repository = %s/%s, want acme/widget", emitted.PR.RepoOwner, emitted.PR.RepoName)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -3,12 +3,17 @@ package orchestrator
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
"time"
|
||||
"unicode/utf8"
|
||||
|
||||
"github.com/jmoiron/sqlx"
|
||||
"github.com/kandev/kandev/internal/db"
|
||||
"github.com/kandev/kandev/internal/events"
|
||||
"github.com/kandev/kandev/internal/events/bus"
|
||||
"github.com/kandev/kandev/internal/github"
|
||||
"github.com/kandev/kandev/internal/orchestrator/executor"
|
||||
"github.com/kandev/kandev/internal/orchestrator/watcher"
|
||||
@@ -97,6 +102,9 @@ func TestBuildReviewTaskRequest_ForkPRRequiresManualStart(t *testing.T) {
|
||||
evt.PR.HeadRepoOwner = tt.headOwner
|
||||
evt.PR.HeadRepoName = tt.headRepo
|
||||
|
||||
if got := reviewPRHeadMatchesBase(evt.PR); got != tt.wantAutostart {
|
||||
t.Errorf("reviewPRHeadMatchesBase() = %v, want %v", got, tt.wantAutostart)
|
||||
}
|
||||
req := buildReviewTaskRequest(evt, nil, "acme/widget")
|
||||
if req.Metadata[taskmodels.MetaKeyAutoStartGuard] != true {
|
||||
t.Fatalf("auto-start guard = %v, want true", req.Metadata[taskmodels.MetaKeyAutoStartGuard])
|
||||
@@ -105,14 +113,160 @@ func TestBuildReviewTaskRequest_ForkPRRequiresManualStart(t *testing.T) {
|
||||
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)
|
||||
manualStartMarker, requiresManualStart := req.Metadata[taskmodels.MetaKeyForkPRRequiresManualStart]
|
||||
if requiresManualStart != !tt.wantAutostart || (requiresManualStart && manualStartMarker != true) {
|
||||
t.Errorf("manual-start marker = (%v, present=%v), want (true, present=%v)",
|
||||
manualStartMarker, requiresManualStart, !tt.wantAutostart)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestReviewWatchSearchResultIdentityControlsAutoStart(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
detailPR *github.PR
|
||||
detailErr error
|
||||
wantHeadOwner string
|
||||
wantHeadRepo string
|
||||
wantAutostart bool
|
||||
}{
|
||||
{
|
||||
name: "internal PR",
|
||||
detailPR: &github.PR{
|
||||
HeadRepoOwner: "acme",
|
||||
HeadRepoName: "widget",
|
||||
BaseRepoOwner: "acme",
|
||||
BaseRepoName: "widget",
|
||||
},
|
||||
wantHeadOwner: "acme",
|
||||
wantHeadRepo: "widget",
|
||||
wantAutostart: true,
|
||||
},
|
||||
{
|
||||
name: "fork PR",
|
||||
detailPR: &github.PR{
|
||||
HeadRepoOwner: "contributor",
|
||||
HeadRepoName: "widget-fork",
|
||||
BaseRepoOwner: "acme",
|
||||
BaseRepoName: "widget",
|
||||
},
|
||||
wantHeadOwner: "contributor",
|
||||
wantHeadRepo: "widget-fork",
|
||||
},
|
||||
{
|
||||
name: "missing identity after detail lookup failure",
|
||||
detailErr: errors.New("GitHub detail lookup failed"),
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
emitted, prs := triggerReviewWatchForAutoStartTest(t, tt.detailPR, tt.detailErr)
|
||||
if len(prs) != 1 || emitted == nil || emitted.PR == nil {
|
||||
t.Fatalf("review watch result = (%d PRs, event=%v), want one PR event", len(prs), emitted != nil)
|
||||
}
|
||||
if emitted.PR != prs[0] {
|
||||
t.Fatal("published event PR is not the PR returned by TriggerReviewWatch")
|
||||
}
|
||||
|
||||
if emitted.PR.HeadRepoOwner != tt.wantHeadOwner || emitted.PR.HeadRepoName != tt.wantHeadRepo {
|
||||
t.Errorf("emitted PR head identity = %s/%s, want %s/%s",
|
||||
emitted.PR.HeadRepoOwner, emitted.PR.HeadRepoName, tt.wantHeadOwner, tt.wantHeadRepo)
|
||||
}
|
||||
if emitted.PR.RepoOwner != "acme" || emitted.PR.RepoName != "widget" {
|
||||
t.Errorf("emitted PR target identity = %s/%s, want acme/widget", emitted.PR.RepoOwner, emitted.PR.RepoName)
|
||||
}
|
||||
if got := reviewPRHeadMatchesBase(emitted.PR); got != tt.wantAutostart {
|
||||
t.Errorf("reviewPRHeadMatchesBase(emitted PR) = %v, want %v", got, tt.wantAutostart)
|
||||
}
|
||||
request := buildReviewTaskRequest(emitted, nil, "acme/widget")
|
||||
autoStartClaim, hasAutoStartClaim := request.Metadata[taskmodels.MetaKeyAutoStartClaimed]
|
||||
if hasAutoStartClaim != tt.wantAutostart || (hasAutoStartClaim && autoStartClaim != true) {
|
||||
t.Errorf("auto_start_claimed = (%v, present=%v), want true present=%v",
|
||||
autoStartClaim, hasAutoStartClaim, tt.wantAutostart)
|
||||
}
|
||||
manualMarker, hasManualMarker := request.Metadata[taskmodels.MetaKeyForkPRRequiresManualStart]
|
||||
if hasManualMarker != !tt.wantAutostart || (hasManualMarker && manualMarker != true) {
|
||||
t.Errorf("fork_pr_requires_manual_start = (%v, present=%v), want present=%v",
|
||||
manualMarker, hasManualMarker, !tt.wantAutostart)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func triggerReviewWatchForAutoStartTest(
|
||||
t *testing.T,
|
||||
detailPR *github.PR,
|
||||
detailErr error,
|
||||
) (*github.NewReviewPREvent, []*github.PR) {
|
||||
t.Helper()
|
||||
ctx := context.Background()
|
||||
store := newReviewWatchGitHubStore(t)
|
||||
mockClient := github.NewMockClient()
|
||||
mockClient.SetReviewRequestedPRs([]*github.PR{{
|
||||
Number: 42, Title: "Review this change", HTMLURL: "https://github.com/acme/widget/pull/42",
|
||||
RepoOwner: "acme", RepoName: "widget", HeadBranch: "feature", BaseBranch: "main",
|
||||
}})
|
||||
mockClient.SetPRDetail("acme", "widget", 42, detailPR, detailErr)
|
||||
if err := store.UpsertWorkspaceConnection(ctx, &github.WorkspaceConnection{
|
||||
WorkspaceID: "ws1", Source: github.ConnectionSourcePAT, GitHubHost: "github.com", Login: "reviewer",
|
||||
Status: github.ConnectionStatusActive,
|
||||
}); err != nil {
|
||||
t.Fatalf("create workspace connection: %v", err)
|
||||
}
|
||||
eventBus := bus.NewMemoryEventBus(testLogger())
|
||||
t.Cleanup(eventBus.Close)
|
||||
var emitted *github.NewReviewPREvent
|
||||
_, err := eventBus.Subscribe(events.GitHubNewReviewPR, func(_ context.Context, event *bus.Event) error {
|
||||
payload, ok := event.Data.(*github.NewReviewPREvent)
|
||||
if !ok {
|
||||
return errors.New("event data is not a NewReviewPREvent")
|
||||
}
|
||||
emitted = payload
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("subscribe to review event: %v", err)
|
||||
}
|
||||
githubService := github.NewService(mockClient, github.AuthMethodPAT, nil, store, eventBus, testLogger())
|
||||
t.Cleanup(githubService.Stop)
|
||||
github.NewMockController(mockClient, store, eventBus, githubService, testLogger())
|
||||
watch := &github.ReviewWatch{
|
||||
ID: "watch-1", WorkspaceID: "ws1", WorkflowID: "wf1", WorkflowStepID: "step1",
|
||||
AgentProfileID: testAgentProfileID, Repos: []github.RepoFilter{{Owner: "acme", Name: "widget"}}, Enabled: true,
|
||||
}
|
||||
if err := store.CreateReviewWatch(ctx, watch); err != nil {
|
||||
t.Fatalf("create review watch: %v", err)
|
||||
}
|
||||
prs, err := githubService.TriggerReviewWatch(ctx, watch)
|
||||
if err != nil {
|
||||
t.Fatalf("TriggerReviewWatch: %v", err)
|
||||
}
|
||||
return emitted, prs
|
||||
}
|
||||
|
||||
func newReviewWatchGitHubStore(t *testing.T) *github.Store {
|
||||
t.Helper()
|
||||
dbConn, err := db.OpenSQLite(filepath.Join(t.TempDir(), "review-watch.db"))
|
||||
if err != nil {
|
||||
t.Fatalf("open review watch database: %v", err)
|
||||
}
|
||||
sqlxDB := sqlx.NewDb(dbConn, "sqlite3")
|
||||
t.Cleanup(func() { _ = sqlxDB.Close() })
|
||||
if _, err := sqlxDB.Exec("CREATE TABLE workspaces (id TEXT PRIMARY KEY)"); err != nil {
|
||||
t.Fatalf("create workspace schema: %v", err)
|
||||
}
|
||||
if _, err := sqlxDB.Exec("INSERT INTO workspaces (id) VALUES (?)", "ws1"); err != nil {
|
||||
t.Fatalf("create workspace: %v", err)
|
||||
}
|
||||
store, err := github.NewStore(sqlxDB, sqlxDB)
|
||||
if err != nil {
|
||||
t.Fatalf("create GitHub store: %v", err)
|
||||
}
|
||||
return store
|
||||
}
|
||||
|
||||
// TestBuildIssueTaskTitle_TruncatesTitle mirrors the review-title regression for
|
||||
// the GitHub issue watcher's "Issue #<n>: <title>" builder.
|
||||
func TestBuildIssueTaskTitle_TruncatesTitle(t *testing.T) {
|
||||
|
||||
@@ -0,0 +1,103 @@
|
||||
---
|
||||
created: 2026-09-25
|
||||
status: complete
|
||||
requirements:
|
||||
- REQ-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001
|
||||
system_design:
|
||||
- ../../specs/integrations/system-design/github-fork-review-start.md
|
||||
legacy_specs: []
|
||||
---
|
||||
|
||||
# Implementation Plan: Review-watch PR identity
|
||||
|
||||
## Overview
|
||||
|
||||
Restore configured auto-start for same-repository GitHub review-watch PRs by
|
||||
preserving repository identity during search-result enrichment. One work order
|
||||
covers the provider-to-orchestrator path and its regression tests.
|
||||
|
||||
## Evidence and root cause
|
||||
|
||||
`CheckReviewWatch` obtains lightweight PRs through GitHub search, then calls
|
||||
`enrichPRDetails` before publishing `GitHubNewReviewPR`. The latter currently
|
||||
copies branch names, SHAs, diff counts, and mergeability, but omits head
|
||||
repository identity. `reviewPRHeadMatchesBase` requires that identity, so
|
||||
`buildReviewTaskRequest` marks an internal PR as requiring manual start and
|
||||
does not grant `auto_start_claimed`. The existing orchestrator unit test gives
|
||||
the event a complete PR directly, so it does not exercise enrichment.
|
||||
|
||||
This violates
|
||||
[`AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.1`](../../specs/integrations/requirements/github-fork-review-start.md).
|
||||
The existing requirement and fork security decision remain correct. The
|
||||
[system design](../../specs/integrations/system-design/github-fork-review-start.md)
|
||||
now states the provider-data contract that was missing.
|
||||
|
||||
## Scope
|
||||
|
||||
### In scope
|
||||
|
||||
- Copy head and base repository metadata and PR detail fields from `GetPR`
|
||||
into newly found review-watch PRs.
|
||||
- Fetch details when branch names exist but head repository identity is absent.
|
||||
- Prove that a same-repository search hit gains its auto-start token after
|
||||
enrichment, while a fork or failed detail fetch remains manual-start only.
|
||||
|
||||
### Out of scope
|
||||
|
||||
- Changes to fork trust policy, automated launch gates, workflow admission, or
|
||||
manual PR-link behavior.
|
||||
- Automatic repair of tasks already created with the manual-start marker or
|
||||
direct edits to existing SQLite task metadata.
|
||||
- Rendered UI changes or new public API fields.
|
||||
|
||||
## Technical approach
|
||||
|
||||
In `apps/backend/internal/github/service_reviews.go`, update
|
||||
`enrichPRDetails` to copy the detail response's head repository owner, name,
|
||||
node ID, clone URL, base repository owner/name, base SHA, mergeability state,
|
||||
and maintainer permission. Preserve all existing copied fields. Also carry
|
||||
repository numeric IDs and base default branch if supplied by the same full
|
||||
response, so the enriched `PR` stays coherent. Keep the detail fetch on missing
|
||||
branch or head identity; on fetch error leave the identity incomplete.
|
||||
|
||||
The review event already carries the returned `PR` without a second mapping.
|
||||
`buildReviewTaskRequest` remains the trust decision point. Do not infer a
|
||||
head repository from the search-result target.
|
||||
|
||||
## Tests
|
||||
|
||||
| Criterion | Evidence |
|
||||
| --- | --- |
|
||||
| AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.1 | `service_reviews_test.go` verifies detail fields are copied; `TestReviewWatchSearchResultIdentityControlsAutoStart` runs a search-shaped result through `TriggerReviewWatch`, then passes that same returned event PR to `buildReviewTaskRequest` and verifies internal identity grants `auto_start_claimed=true` without a manual-start marker. |
|
||||
| AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.2 | The same end-to-end test verifies a fork head and failed detail lookup retain the manual-start marker and omit the auto-start claim. |
|
||||
|
||||
The new provider regression must fail before the production change because
|
||||
the head repository fields remain empty.
|
||||
|
||||
## E2E tests
|
||||
|
||||
The backend integration test connects the watch search result to the review
|
||||
event and task request without a browser. Its mock client models distinct
|
||||
search and detail responses. Existing
|
||||
`apps/web/e2e/tests/pr/pr-watcher-dockview-layout.spec.ts` exercises a mock
|
||||
watch's visible auto-start behavior; no rendered UI behavior changes here.
|
||||
|
||||
## Work orders
|
||||
|
||||
- [x] [Task 01: Preserve review PR identity through enrichment](task-01-preserve-review-pr-identity.md)
|
||||
|
||||
## Verification results
|
||||
|
||||
The full GitHub package tests and targeted orchestrator tests passed, including
|
||||
the search-result enrichment event path and same-repository/fork/unknown
|
||||
identity launch decisions. Focused race tests for both packages, the backend
|
||||
build, and backend lint passed. The environment-isolated full backend suite did
|
||||
not pass because two real-process probe assertions in
|
||||
`internal/agentctl/server/process/probe` returned `live` where they expect
|
||||
`settled`; an isolated rerun reproduced both failures.
|
||||
|
||||
## Risks
|
||||
|
||||
- A failed or incomplete `GetPR` response must continue to fail closed.
|
||||
- Copying only owner/name would fix launch but leave downstream PR metadata
|
||||
incomplete; the field-completeness test guards the whole detail mapping.
|
||||
@@ -0,0 +1,119 @@
|
||||
---
|
||||
id: "01-preserve-review-pr-identity"
|
||||
title: "Preserve review PR identity through enrichment"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001
|
||||
acceptance_criteria:
|
||||
- AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.1
|
||||
- AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.2
|
||||
system_design:
|
||||
- ../../specs/integrations/system-design/github-fork-review-start.md
|
||||
---
|
||||
|
||||
# Task 01: Preserve review PR identity through enrichment
|
||||
|
||||
## Summary
|
||||
|
||||
Carry complete `GetPR` identity and related metadata onto review-watch search
|
||||
results before they become events. Prove that the resulting same-repository
|
||||
event grants an automatic launch token while fork and unknown identities retain
|
||||
the existing manual-start boundary.
|
||||
|
||||
## In scope
|
||||
|
||||
- Write a failing enrichment regression using a search-shaped PR and a fake
|
||||
`GetPR` result in `service_reviews_test.go`.
|
||||
- Copy the requested head/base repository, SHA, mergeability, and maintainer
|
||||
fields in `enrichPRDetails`, preserving the current branch and diff fields.
|
||||
- Exercise `TriggerReviewWatch` to event to `buildReviewTaskRequest` in backend
|
||||
tests, including same-repository, fork, and unavailable-detail cases. Pass the
|
||||
same emitted PR into the request builder.
|
||||
- Let `MockClient` provide separate review-search and detail responses so the
|
||||
test reproduces GitHub's lightweight search result.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- A changed fork policy, metadata backfill, database migration, and UI changes.
|
||||
|
||||
## Acceptance
|
||||
|
||||
1. The new enrichment regression is RED before the code change and GREEN after
|
||||
it; all requested fields and existing fields match the `GetPR` result.
|
||||
2. A same-repository review PR from the watch passes
|
||||
`reviewPRHeadMatchesBase`, has `auto_start_claimed`, and lacks
|
||||
`fork_pr_requires_manual_start`.
|
||||
3. A fork or identity-incomplete PR keeps `fork_pr_requires_manual_start` and
|
||||
has no auto-start token, including when `GetPR` fails.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
(cd apps/backend && go test ./internal/github -run 'TestEnrichPRDetails|TestCheckReviewWatch' -count=1)
|
||||
(cd apps/backend && go test ./internal/orchestrator -run 'TestReviewWatchSearchResultIdentityControlsAutoStart|TestBuildReviewTaskRequest_ForkPRRequiresManualStart' -count=1)
|
||||
(cd apps/backend && go test -race ./internal/github -run 'TestCheckReviewWatchEnrichesSearchResultsBeforePublishingEvent|TestMockClient_Reset' -count=1)
|
||||
(cd apps/backend && go test -race ./internal/orchestrator -run 'TestReviewWatchSearchResultIdentityControlsAutoStart|TestBuildReviewTaskRequest_ForkPRRequiresManualStart' -count=1)
|
||||
make -C apps/backend test
|
||||
make -C apps/backend lint
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/backend/internal/github/service_reviews.go`
|
||||
- `apps/backend/internal/github/mock_client.go`
|
||||
- `apps/backend/internal/github/service_reviews_test.go`
|
||||
- `apps/backend/internal/orchestrator/event_handlers_github_review_test.go`
|
||||
|
||||
## Dependencies
|
||||
|
||||
None.
|
||||
|
||||
## Risks
|
||||
|
||||
- Keep the lookup tied to the search result's target repository. Missing
|
||||
provider identity must never be inferred from that target.
|
||||
- Existing tests construct complete PRs and may miss a search-to-detail mapping
|
||||
regression unless the new tests use separate search and detail objects.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- [Fork review-start requirement](../../specs/integrations/requirements/github-fork-review-start.md),
|
||||
[design](../../specs/integrations/system-design/github-fork-review-start.md),
|
||||
and [security decision](../../decisions/2026-09-23-fork-pr-review-watch-manual-start.md).
|
||||
- `CheckReviewWatch`, `enrichPRDetails`, `publishNewReviewPREvent`,
|
||||
`buildReviewTaskRequest`, and nearby backend test helpers.
|
||||
|
||||
## Results
|
||||
|
||||
Implemented full PR detail enrichment for review-watch search results whose
|
||||
head identity is absent, even when both branch names are already present. The
|
||||
published event retains head/base repository identity, revisions, mergeability,
|
||||
maintainer permission, branch names, and diff counts. The regression was RED
|
||||
because `GetPR` was not called, then passed with the fix. The same-repository,
|
||||
fork, and failed-detail paths all preserve the expected launch boundary. The
|
||||
orchestrator test sends the actual `TriggerReviewWatch` event PR into
|
||||
`buildReviewTaskRequest` and asserts both the event identity and launch metadata.
|
||||
|
||||
Verification:
|
||||
|
||||
- PASS: `go test ./internal/github -run 'TestEnrichPRDetails|TestCheckReviewWatch' -count=1`
|
||||
- PASS: `go test ./internal/orchestrator -run 'TestBuildReviewTaskRequest|TestReviewWatch' -count=1`
|
||||
- PASS: `go test ./internal/github -count=1`
|
||||
- PASS: `go test ./internal/orchestrator -run 'TestReviewWatchSearchResultIdentityControlsAutoStart|TestBuildReviewTaskRequest_ForkPRRequiresManualStart' -count=1`
|
||||
- PASS: `go test -race ./internal/github -run 'TestCheckReviewWatchEnrichesSearchResultsBeforePublishingEvent|TestMockClient_Reset' -count=1`
|
||||
- PASS: `go test -race ./internal/orchestrator -run 'TestReviewWatchSearchResultIdentityControlsAutoStart|TestBuildReviewTaskRequest_ForkPRRequiresManualStart' -count=1`
|
||||
- PASS: `make -C apps/backend build`
|
||||
- PASS: `make -C apps/backend lint`
|
||||
- BLOCKED by unrelated existing probe failures: `make -C apps/backend test`
|
||||
- BLOCKED by the same two real-process probe failures after config isolation:
|
||||
`env -u KANDEV_INTERNAL_CONFIG_FILE -u KANDEV_INTERNAL_CONFIG_HOME_FILE -u KANDEV_INTERNAL_AGENTCTL_STARTUP_CONFIG make -C apps/backend test`
|
||||
- Confirmed: the isolated `TestProbeRealTree_AllDescendantsPreTurn_Settled` and
|
||||
`TestProbeRealTree_NewDescendantAfterTurnStart_Live` tests both return `live`
|
||||
where they expect `settled`.
|
||||
@@ -39,6 +39,16 @@ does not authorize execution.
|
||||
|
||||
## Identity and auto-start token
|
||||
|
||||
`CheckReviewWatch` receives lightweight PRs from GitHub search and enriches
|
||||
untracked results with `GetPR` before publishing review events. Enrichment
|
||||
must carry the full response's head and base repository identity, head and
|
||||
base revisions, mergeability details, and maintainer modification permission
|
||||
alongside the existing branch and diff fields. A PR with branch names but no
|
||||
head repository identity still needs enrichment. If the detail fetch fails or
|
||||
the provider has no head identity, the event retains incomplete identity and
|
||||
the launch decision remains fail-closed. Search-result target identity remains
|
||||
the repository used to look up the PR.
|
||||
|
||||
`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
|
||||
@@ -77,10 +87,11 @@ 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.
|
||||
Backend tests cover search-result enrichment through review event creation,
|
||||
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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user