Skip to content

Commit 7f0e7fb

Browse files
fix: log metadata overwrites in batched workflows and deduplicate copier PRs
#3: workflow_processor now logs when a subsequent workflow in the same batch overwrites the commit message or PR title, showing both old and new values so the "last wins" behavior is transparent. #5: addFilesViaPR now checks for an existing open PR from a copier/* branch before creating a new one. If found, pushes to the existing branch and updates the PR title/body instead of creating a duplicate. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent ab8ea0c commit 7f0e7fb

5 files changed

Lines changed: 190 additions & 11 deletions

File tree

docs/RECOMMENDATIONS.md

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,13 @@ When two instances process the same webhook (e.g., local + Cloud Run), the secon
2727

2828
**Files:** `services/github_write_to_target.go`
2929

30-
### 3. PR title/body "last wins" is opaque
30+
### 3. ~~PR title/body "last wins" is opaque~~ (RESOLVED)
3131

32-
**Priority:** Low
33-
34-
When multiple workflows batch into one PR, the commit message, PR title, and body come from whichever workflow ran last. There's no logging indicating which workflow's metadata was used.
32+
**Priority:** Low — **Status: Fixed**
3533

36-
**Fix:** Log which workflow's strategy was selected during the write phase.
34+
~~When multiple workflows batch into one PR, the commit message, PR title, and body come from whichever workflow ran last. There's no logging indicating which workflow's metadata was used.~~
3735

38-
**Files:** `services/github_write_to_target.go`
36+
**Resolution:** `workflow_processor.go` now logs when a subsequent workflow overwrites a batched commit message or PR title, including the previous and new values and the workflow name responsible.
3937

4038
## Reliability
4139

@@ -47,13 +45,13 @@ The in-memory `DeliveryTracker` prevents duplicate processing within a single in
4745

4846
**Files:** `services/delivery_tracker.go`
4947

50-
### 5. PR deduplication in target repos
48+
### 5. ~~PR deduplication in target repos~~ (RESOLVED)
5149

52-
**Priority:** Low
50+
**Priority:** Low**Status: Fixed**
5351

54-
If the app processes two source PRs in quick succession, it can create duplicate open PRs in the target repo. Before creating a new PR, check for an existing open PR from a `copier/*` branch and update it instead.
52+
~~If the app processes two source PRs in quick succession, it can create duplicate open PRs in the target repo.~~
5553

56-
**Files:** `services/github_write_to_target.go`
54+
**Resolution:** `addFilesViaPR` now calls `findExistingCopierPR` before creating a new branch. If an open PR from a `copier/*` branch targeting the same base branch exists, the app pushes new commits to that branch and updates the PR title/body instead of creating a duplicate.
5755

5856
### 6. Graceful partial failure
5957

services/github_write_to_target.go

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,14 +185,72 @@ func createPullRequest(ctx context.Context, client *github.Client, defaultOwner,
185185
return created, nil
186186
}
187187

188+
// findExistingCopierPR searches for an open PR whose head branch starts with "copier/"
189+
// targeting the given base branch. Returns nil if none found.
190+
func findExistingCopierPR(ctx context.Context, client *github.Client, owner, repoName, baseBranch string) *github.PullRequest {
191+
prs, _, err := client.PullRequests.List(ctx, owner, repoName, &github.PullRequestListOptions{
192+
State: "open",
193+
Base: baseBranch,
194+
ListOptions: github.ListOptions{
195+
PerPage: 50,
196+
},
197+
})
198+
if err != nil {
199+
LogWarning("Failed to list PRs for dedup check; will create new PR", "repo", owner+"/"+repoName, "error", err)
200+
return nil
201+
}
202+
for _, pr := range prs {
203+
if strings.HasPrefix(pr.GetHead().GetRef(), "copier/") {
204+
return pr
205+
}
206+
}
207+
return nil
208+
}
209+
188210
// addFilesViaPR creates a temporary branch, commits files to it using the provided commitMessage,
189211
// opens a pull request with prTitle and prBody, and optionally merges it automatically.
212+
// If an existing open PR from a copier/* branch is found, the files are pushed to that
213+
// branch and the PR is updated instead of creating a duplicate.
190214
func addFilesViaPR(ctx context.Context, config *configs.Config, client *github.Client, key types.UploadKey,
191215
files []github.RepositoryContent, commitMessage string, prTitle string, prBody string, mergeWithoutReview bool,
192216
) error {
193217
defaultOwner := config.ConfigRepoOwner
194-
tempBranch := "copier/" + time.Now().UTC().Format("20060102-150405")
195218
baseBranch := strings.TrimPrefix(key.BranchPath, "refs/heads/")
219+
owner, repoName := parseRepoPath(key.RepoName, defaultOwner)
220+
221+
// 0. Check for an existing open copier PR targeting this base branch.
222+
existingPR := findExistingCopierPR(ctx, client, owner, repoName, baseBranch)
223+
if existingPR != nil {
224+
existingBranch := existingPR.GetHead().GetRef()
225+
LogInfo("Found existing open copier PR; updating instead of creating new",
226+
"pr_number", existingPR.GetNumber(),
227+
"branch", existingBranch,
228+
"repo", key.RepoName,
229+
)
230+
231+
// Push new files to the existing branch
232+
if err := commitFilesToBranch(ctx, config, client, key, files, existingBranch, commitMessage); err != nil {
233+
return fmt.Errorf("commit to existing copier branch %s: %w", existingBranch, err)
234+
}
235+
236+
// Update the PR title/body to reflect the latest content
237+
_, _, err := client.PullRequests.Edit(ctx, owner, repoName, existingPR.GetNumber(), &github.PullRequest{
238+
Title: github.Ptr(prTitle),
239+
Body: github.Ptr(prBody),
240+
})
241+
if err != nil {
242+
LogWarning("Failed to update existing PR title/body", "pr_number", existingPR.GetNumber(), "error", err)
243+
}
244+
245+
if mergeWithoutReview {
246+
return autoMergePR(ctx, config, client, key.RepoName, defaultOwner, existingPR.GetNumber(), existingBranch)
247+
}
248+
LogInfo("Existing PR updated and awaiting review", "pr_number", existingPR.GetNumber())
249+
return nil
250+
}
251+
252+
// No existing PR — create a new temp branch and PR.
253+
tempBranch := "copier/" + time.Now().UTC().Format("20060102-150405")
196254

197255
// 1. Create branch off the target
198256
if _, err := createBranch(ctx, client, defaultOwner, key.RepoName, tempBranch, baseBranch); err != nil {

services/github_write_to_target_test.go

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,9 @@ func TestAddFilesToTargetRepos_ViaPR_Succeeds(t *testing.T) {
115115

116116
test.SetupOrgToken(owner, "test-token")
117117

118+
// No existing open PRs
119+
test.MockListOpenPRs(owner, repo, nil)
120+
118121
// Base ref used to create temp branch
119122
httpmock.RegisterRegexpResponder("GET",
120123
regexp.MustCompile(`^https://api\.github\.com/repos/`+owner+`/`+repo+`/git/ref/(?:refs/)?heads/`+baseBranch+`$`),
@@ -263,6 +266,7 @@ func TestAddFiles_ViaPR_MergeConflict_Dirty_NotMerged(t *testing.T) {
263266
require.NoError(t, err, "ConfigurePermissions should succeed")
264267

265268
test.SetupOrgToken(owner, "test-token")
269+
test.MockListOpenPRs(owner, repo, nil)
266270

267271
httpmock.RegisterRegexpResponder("GET",
268272
regexp.MustCompile(`^https://api\.github\.com/repos/`+owner+`/`+repo+`/git/ref/(?:refs/)?heads/`+baseBranch+`$`),
@@ -395,6 +399,7 @@ func TestPriority_PRTitleDefaultsToCommitMessage_And_NoAutoMergeWhenConfigPresen
395399
require.NoError(t, err, "ConfigurePermissions should succeed")
396400

397401
test.SetupOrgToken(owner, "test-token")
402+
test.MockListOpenPRs(owner, repo, nil)
398403

399404
httpmock.RegisterRegexpResponder("GET",
400405
regexp.MustCompile(`^https://api\.github\.com/repos/`+owner+`/`+repo+`/git/ref/(?:refs/)?heads/`+baseBranch+`$`),
@@ -470,6 +475,7 @@ func TestAddFilesToTargetRepos_MixedStrategies_ProducesSeparateOperations(t *tes
470475
err := services.ConfigurePermissions(context.Background(), cfg)
471476
require.NoError(t, err, "ConfigurePermissions should succeed")
472477
test.SetupOrgToken(owner, "test-token")
478+
test.MockListOpenPRs(owner, repo, nil)
473479

474480
// --- Mock direct-commit endpoints ---
475481
baseRefURL, directCommitsURL, updateRefURL := test.MockGitHubWriteEndpoints(owner, repo, baseBranch)
@@ -532,6 +538,89 @@ func TestAddFilesToTargetRepos_MixedStrategies_ProducesSeparateOperations(t *tes
532538
), "PR path: POST create PR")
533539
}
534540

541+
// TestAddFilesViaPR_ReusesExistingCopierPR verifies that when an open PR from a
542+
// copier/* branch already exists, the app pushes to that branch and updates the PR
543+
// title/body instead of creating a duplicate PR.
544+
func TestAddFilesViaPR_ReusesExistingCopierPR(t *testing.T) {
545+
_ = test.WithHTTPMock(t)
546+
t.Setenv("COPIER_COMMIT_STRATEGY", "pr")
547+
548+
owner, repo := test.EnvOwnerRepo(t)
549+
baseBranch := "main"
550+
existingBranch := "copier/20260101-120000"
551+
552+
cfg := test.TestConfig()
553+
services.DefaultTokenManager().SetInstallationAccessToken("")
554+
test.MockGitHubAppTokenEndpoint(cfg.InstallationId)
555+
err := services.ConfigurePermissions(context.Background(), cfg)
556+
require.NoError(t, err, "ConfigurePermissions should succeed")
557+
558+
test.SetupOrgToken(owner, "test-token")
559+
560+
// Return an existing open PR from a copier/* branch
561+
test.MockListOpenPRs(owner, repo, []map[string]any{
562+
{
563+
"number": 77,
564+
"head": map[string]any{"ref": existingBranch},
565+
"base": map[string]any{"ref": baseBranch},
566+
},
567+
})
568+
569+
// Mock the existing copier branch ref (for commit push)
570+
httpmock.RegisterRegexpResponder("GET",
571+
regexp.MustCompile(`^https://api\.github\.com/repos/`+owner+`/`+repo+`/git/ref/(?:refs/)?heads/`+regexp.QuoteMeta(existingBranch)+`$`),
572+
httpmock.NewJsonResponderOrPanic(200, map[string]any{
573+
"ref": "refs/heads/" + existingBranch, "object": map[string]any{"sha": "existingSha"},
574+
}),
575+
)
576+
httpmock.RegisterRegexpResponder("POST",
577+
regexp.MustCompile(`^https://api\.github\.com/repos/`+owner+`/`+repo+`/git/trees(\?.*)?$`),
578+
httpmock.NewJsonResponderOrPanic(201, map[string]any{"sha": "newTreeSha"}),
579+
)
580+
commitsURL := "https://api.github.com/repos/" + owner + "/" + repo + "/git/commits"
581+
httpmock.RegisterResponder("POST", commitsURL,
582+
httpmock.NewJsonResponderOrPanic(201, map[string]any{"sha": "newCommitSha"}),
583+
)
584+
httpmock.RegisterRegexpResponder("PATCH",
585+
regexp.MustCompile(`^https://api\.github\.com/repos/`+owner+`/`+repo+`/git/refs/heads/`+regexp.QuoteMeta(existingBranch)+`$`),
586+
httpmock.NewStringResponder(200, "{}"),
587+
)
588+
// Mock PR Edit (title/body update)
589+
editPRURL := "https://api.github.com/repos/" + owner + "/" + repo + "/pulls/77"
590+
httpmock.RegisterResponder("PATCH", editPRURL,
591+
httpmock.NewJsonResponderOrPanic(200, map[string]any{"number": 77}),
592+
)
593+
594+
files := []github.RepositoryContent{{
595+
Name: github.Ptr("updated-file.txt"),
596+
Path: github.Ptr("updated-file.txt"),
597+
Content: github.Ptr(base64.StdEncoding.EncodeToString([]byte("new content"))),
598+
}}
599+
filesToUpload := map[types.UploadKey]types.UploadFileContent{
600+
{RepoName: repo, BranchPath: "refs/heads/" + baseBranch, CommitStrategy: "pull_request"}: {
601+
TargetBranch: baseBranch,
602+
Content: files,
603+
CommitStrategy: "pr",
604+
},
605+
}
606+
607+
services.AddFilesToTargetRepos(context.Background(), cfg, filesToUpload, nil, nil)
608+
609+
info := httpmock.GetCallCountInfo()
610+
611+
// Should NOT create a new branch or new PR
612+
require.Equal(t, 0, test.CountByMethodAndURLRegexp("POST",
613+
regexp.MustCompile(`/repos/`+regexp.QuoteMeta(owner)+`/`+regexp.QuoteMeta(repo)+`/git/refs$`),
614+
), "should not create a new branch ref")
615+
require.Equal(t, 0, test.CountByMethodAndURLRegexp("POST",
616+
regexp.MustCompile(`/repos/`+regexp.QuoteMeta(owner)+`/`+regexp.QuoteMeta(repo)+`/pulls$`),
617+
), "should not create a new PR")
618+
619+
// Should commit to the existing branch and update the PR
620+
require.Equal(t, 1, info["POST "+commitsURL], "should commit to existing branch")
621+
require.Equal(t, 1, info["PATCH "+editPRURL], "should update PR title/body")
622+
}
623+
535624
func TestDeleteBranchIfExists_NilReference(t *testing.T) {
536625
_ = test.WithHTTPMock(t)
537626

services/workflow_processor.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,10 @@ func (wp *workflowProcessor) addToUploadQueue(
374374
msgCtx.CommitSHA = sourceCommitSHA
375375
msgCtx.FileCount = len(content.Content)
376376

377+
// Track previous metadata so we can log when a later workflow overwrites it.
378+
prevCommitMsg := content.CommitMessage
379+
prevPRTitle := content.PRTitle
380+
377381
// Render commit message
378382
if workflow.CommitStrategy != nil && workflow.CommitStrategy.CommitMessage != "" {
379383
content.CommitMessage = wp.messageTemplater.RenderCommitMessage(workflow.CommitStrategy.CommitMessage, msgCtx)
@@ -393,6 +397,24 @@ func (wp *workflowProcessor) addToUploadQueue(
393397
content.PRBody = wp.messageTemplater.RenderPRBody(workflow.CommitStrategy.PRBody, msgCtx)
394398
}
395399

400+
// Log when a subsequent workflow in the same batch overwrites PR metadata.
401+
if exists && prevCommitMsg != "" && prevCommitMsg != content.CommitMessage {
402+
LogInfo("Workflow overwrites batched commit message (last wins)",
403+
"workflow", workflow.Name,
404+
"target", workflow.Destination.Repo,
405+
"prev_commit_message", prevCommitMsg,
406+
"new_commit_message", content.CommitMessage,
407+
)
408+
}
409+
if exists && prevPRTitle != "" && prevPRTitle != content.PRTitle {
410+
LogInfo("Workflow overwrites batched PR title (last wins)",
411+
"workflow", workflow.Name,
412+
"target", workflow.Destination.Repo,
413+
"prev_pr_title", prevPRTitle,
414+
"new_pr_title", content.PRTitle,
415+
)
416+
}
417+
396418
// Add back to FileStateService
397419
wp.fileStateService.AddFileToUpload(key, content)
398420

tests/utils.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,18 @@ func MockCreateRef(owner, repo string) string {
174174
return url
175175
}
176176

177+
// MockListOpenPRs mocks the "list open PRs" endpoint, returning the supplied PRs.
178+
// Pass nil or empty to simulate no existing open PRs.
179+
func MockListOpenPRs(owner, repo string, prs []map[string]any) {
180+
if prs == nil {
181+
prs = []map[string]any{}
182+
}
183+
httpmock.RegisterRegexpResponder("GET",
184+
regexp.MustCompile(`^https://api\.github\.com/repos/`+regexp.QuoteMeta(owner)+`/`+regexp.QuoteMeta(repo)+`/pulls\?`),
185+
httpmock.NewJsonResponderOrPanic(200, prs),
186+
)
187+
}
188+
177189
// MockPullsAndMerge mocks creating and merging a PR.
178190
func MockPullsAndMerge(owner, repo string, number int) {
179191
httpmock.RegisterResponder("POST",

0 commit comments

Comments
 (0)