From 6d344c74a887a4a81f37dfbbddb43a8691eb0c7d Mon Sep 17 00:00:00 2001 From: Mathias Date: Sat, 4 Jul 2026 08:30:30 +0200 Subject: [PATCH] =?UTF-8?q?feat(tbd=5Fship):=20idempotent=20re-invoke=20?= =?UTF-8?q?=E2=80=94=20resume=20existing=20branch/PR=20(#48)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second tbd_ship for the same change no longer errors on "branch exists": CreateBranch conflict is tolerated, and if a PR is already open for the head, CreatePullRequest's conflict/validation error resolves it via ListPullRequests (matching head.ref). Identical file content on the branch skips the write, so a resume produces no redundant empty-diff commit. Then the same CI gate runs and merges if now green — so "poll or re-invoke" (the #40 UX) actually works. Test: TestTBDShip_Resume_ExistingBranchAndPR (branch+PR exist, content unchanged → no write, merges when green). First-call paths unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/tools/tbd_ship.go | 52 ++++++++++++++++++++++++------- internal/tools/tbd_ship_test.go | 54 +++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 11 deletions(-) diff --git a/internal/tools/tbd_ship.go b/internal/tools/tbd_ship.go index 3253f7c..dba459b 100644 --- a/internal/tools/tbd_ship.go +++ b/internal/tools/tbd_ship.go @@ -171,25 +171,34 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag return nil, fmt.Errorf("branch protection probe: %w", err) } - // Branch from base, write the change, open the PR. - if err := t.c.CreateBranch(ctx, args.Owner, args.Repo, branch, base); err != nil { + // Branch from base. A conflict means the branch already exists — resume on it + // (idempotent re-invoke, #48) rather than failing. + if err := t.c.CreateBranch(ctx, args.Owner, args.Repo, branch, base); err != nil && !errors.Is(err, gitea.ErrConflict) { return nil, fmt.Errorf("create branch %s: %w", branch, err) } - // A pre-existing file at path needs its blob sha to update; a new file does not. + // A pre-existing file at path needs its blob sha to update; a new file does + // not. If the content already on the branch is identical, skip the write so a + // resume doesn't produce a redundant empty-diff commit. sha := "" + needWrite := true if fc, ferr := t.c.GetFileContents(ctx, args.Owner, args.Repo, args.Path, branch); ferr == nil { sha = fc.Sha + if decoded, derr := base64.StdEncoding.DecodeString(fc.Content); derr == nil && string(decoded) == args.Content { + needWrite = false + } } else if !errors.Is(ferr, gitea.ErrNotFound) { return nil, fmt.Errorf("read %s on %s: %w", args.Path, branch, ferr) } - if _, err := t.c.UpsertFile(ctx, args.Owner, args.Repo, args.Path, gitea.UpsertFileArgs{ - Branch: branch, - Content: base64.StdEncoding.EncodeToString([]byte(args.Content)), - Message: args.Message, - Sha: sha, - }); err != nil { - return nil, fmt.Errorf("write %s on %s: %w", args.Path, branch, err) + if needWrite { + if _, err := t.c.UpsertFile(ctx, args.Owner, args.Repo, args.Path, gitea.UpsertFileArgs{ + Branch: branch, + Content: base64.StdEncoding.EncodeToString([]byte(args.Content)), + Message: args.Message, + Sha: sha, + }); err != nil { + return nil, fmt.Errorf("write %s on %s: %w", args.Path, branch, err) + } } prTitle := args.PRTitle @@ -203,7 +212,13 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag Base: base, }) if err != nil { - return nil, fmt.Errorf("open PR: %w", err) + // An open PR for this head already exists → resume it (#48). + if errors.Is(err, gitea.ErrConflict) || errors.Is(err, gitea.ErrValidation) { + pr, err = t.findOpenPR(ctx, args.Owner, args.Repo, branch) + } + if err != nil { + return nil, fmt.Errorf("open PR: %w", err) + } } // CI gate on the PR head commit. @@ -267,6 +282,21 @@ func (t *TBDShip) pollRuns(ctx context.Context, owner, repo, headSHA string, tim } } +// findOpenPR returns the open PR whose head is the given branch — used to resume +// an existing PR when CreatePullRequest reports one already exists (#48). +func (t *TBDShip) findOpenPR(ctx context.Context, owner, repo, branch string) (*gitea.PullRequest, error) { + prs, err := t.c.ListPullRequests(ctx, owner, repo, "open", branch, 1, 50) + if err != nil { + return nil, err + } + for i := range prs { + if prs[i].Head.Ref == branch { + return &prs[i], nil + } + } + return nil, fmt.Errorf("no open PR found for head %s: %w", branch, gitea.ErrNotFound) +} + func (t *TBDShip) listRuns(ctx context.Context, owner, repo, headSHA string) []gitea.WorkflowRun { resp, err := t.c.ListWorkflowRuns(ctx, owner, repo, gitea.ListWorkflowRunsArgs{HeadSHA: headSHA, Limit: 50}) if err != nil || resp == nil { diff --git a/internal/tools/tbd_ship_test.go b/internal/tools/tbd_ship_test.go index 452fd6a..7d8c7eb 100644 --- a/internal/tools/tbd_ship_test.go +++ b/internal/tools/tbd_ship_test.go @@ -150,6 +150,60 @@ func TestTBDShip_MergeConflict_FailsClosed(t *testing.T) { assert.False(t, deleted.Load(), "branch is NOT deleted on a failed merge") } +// Re-invoking with the same change must resume the existing branch/PR (not +// error on "branch exists"), skip the redundant write when content is +// unchanged, and merge once CI is green (#48). +func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) { + var merged, deleted, wrote atomic.Bool + branch := "tbd/resume-me" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + p := r.URL.Path + w.Header().Set("Content-Type", "application/json") + switch { + case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"): + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"not found"}`)) + case r.Method == http.MethodPost && strings.HasSuffix(p, "/branches"): + w.WriteHeader(http.StatusConflict) // branch already exists + _, _ = w.Write([]byte(`{"message":"branch already exists"}`)) + case r.Method == http.MethodGet && strings.Contains(p, "/contents/"): + // existing file with identical content ("hi") → write should be skipped + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"path":"docs/x.md","sha":"s","content":"aGk=","encoding":"base64"}`)) + case (r.Method == http.MethodPost || r.Method == http.MethodPut) && strings.Contains(p, "/contents/"): + wrote.Store(true) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"content":{"path":"x","sha":"s2"},"commit":{"sha":"c"}}`)) + case r.Method == http.MethodPost && strings.HasSuffix(p, "/pulls"): + w.WriteHeader(http.StatusConflict) // PR already exists for this head + _, _ = w.Write([]byte(`{"message":"pull request already exists"}`)) + case r.Method == http.MethodGet && strings.HasSuffix(p, "/pulls"): + _, _ = w.Write([]byte(`[{"number":7,"html_url":"http://x/pulls/7","state":"open","head":{"ref":"` + branch + `","sha":"abc"},"base":{"ref":"main"}}]`)) + case r.Method == http.MethodGet && strings.Contains(p, "/actions/runs"): + _, _ = w.Write([]byte(`{"total_count":1,"workflow_runs":[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]}`)) + case r.Method == http.MethodPost && strings.HasSuffix(p, "/pulls/7/merge"): + merged.Store(true) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + case r.Method == http.MethodDelete && strings.Contains(p, "/branches/"): + deleted.Store(true) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + default: + t.Errorf("unexpected request: %s %s", r.Method, p) + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + + res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","branch":"`+branch+`"}`) + + assert.Equal(t, true, res["merged"], "resume must merge when CI is green") + assert.Equal(t, float64(7), res["pr_number"], "must reuse the existing PR #7") + assert.False(t, wrote.Load(), "identical content must not trigger a redundant write") + assert.True(t, merged.Load()) +} + func TestTBDShip_AllowlistRejects(t *testing.T) { tool := tools.NewTBDShip(gitea.NewClient("http://unused", ""), allowlist.New([]string{"mathias"})) _, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"evil","repo":"r","path":"p","content":"c","message":"m"}`))