feat(tbd_ship): idempotent re-invoke — resume existing branch/PR (#48)
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) <noreply@anthropic.com>
This commit is contained in:
@@ -171,18 +171,26 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
return nil, fmt.Errorf("branch protection probe: %w", err)
|
return nil, fmt.Errorf("branch protection probe: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Branch from base, write the change, open the PR.
|
// Branch from base. A conflict means the branch already exists — resume on it
|
||||||
if err := t.c.CreateBranch(ctx, args.Owner, args.Repo, branch, base); err != nil {
|
// (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)
|
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 := ""
|
sha := ""
|
||||||
|
needWrite := true
|
||||||
if fc, ferr := t.c.GetFileContents(ctx, args.Owner, args.Repo, args.Path, branch); ferr == nil {
|
if fc, ferr := t.c.GetFileContents(ctx, args.Owner, args.Repo, args.Path, branch); ferr == nil {
|
||||||
sha = fc.Sha
|
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) {
|
} else if !errors.Is(ferr, gitea.ErrNotFound) {
|
||||||
return nil, fmt.Errorf("read %s on %s: %w", args.Path, branch, ferr)
|
return nil, fmt.Errorf("read %s on %s: %w", args.Path, branch, ferr)
|
||||||
}
|
}
|
||||||
|
if needWrite {
|
||||||
if _, err := t.c.UpsertFile(ctx, args.Owner, args.Repo, args.Path, gitea.UpsertFileArgs{
|
if _, err := t.c.UpsertFile(ctx, args.Owner, args.Repo, args.Path, gitea.UpsertFileArgs{
|
||||||
Branch: branch,
|
Branch: branch,
|
||||||
Content: base64.StdEncoding.EncodeToString([]byte(args.Content)),
|
Content: base64.StdEncoding.EncodeToString([]byte(args.Content)),
|
||||||
@@ -191,6 +199,7 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
}); err != nil {
|
}); err != nil {
|
||||||
return nil, fmt.Errorf("write %s on %s: %w", args.Path, branch, err)
|
return nil, fmt.Errorf("write %s on %s: %w", args.Path, branch, err)
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
prTitle := args.PRTitle
|
prTitle := args.PRTitle
|
||||||
if prTitle == "" {
|
if prTitle == "" {
|
||||||
@@ -202,9 +211,15 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
Head: branch,
|
Head: branch,
|
||||||
Base: base,
|
Base: base,
|
||||||
})
|
})
|
||||||
|
if err != nil {
|
||||||
|
// 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 {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("open PR: %w", err)
|
return nil, fmt.Errorf("open PR: %w", err)
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// CI gate on the PR head commit.
|
// CI gate on the PR head commit.
|
||||||
runs := t.pollRuns(ctx, args.Owner, args.Repo, pr.Head.Sha, args.CITimeoutSec)
|
runs := t.pollRuns(ctx, args.Owner, args.Repo, pr.Head.Sha, args.CITimeoutSec)
|
||||||
@@ -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 {
|
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})
|
resp, err := t.c.ListWorkflowRuns(ctx, owner, repo, gitea.ListWorkflowRunsArgs{HeadSHA: headSHA, Limit: 50})
|
||||||
if err != nil || resp == nil {
|
if err != nil || resp == nil {
|
||||||
|
|||||||
@@ -150,6 +150,60 @@ func TestTBDShip_MergeConflict_FailsClosed(t *testing.T) {
|
|||||||
assert.False(t, deleted.Load(), "branch is NOT deleted on a failed merge")
|
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) {
|
func TestTBDShip_AllowlistRejects(t *testing.T) {
|
||||||
tool := tools.NewTBDShip(gitea.NewClient("http://unused", ""), allowlist.New([]string{"mathias"}))
|
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"}`))
|
_, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"evil","repo":"r","path":"p","content":"c","message":"m"}`))
|
||||||
|
|||||||
Reference in New Issue
Block a user