Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions internal/run/orchestrator.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ type Orchestrator struct {
// PullRequests is the side-effect seam Final success is checked against: an
// open pull request for the Run branch after the Final Agent exits 0.
type PullRequests interface {
HasOpenPR(ctx context.Context, branch string) (bool, error)
// OpenPRURL returns the URL of an open pull request for branch, or "" if
// none exists.
OpenPRURL(ctx context.Context, branch string) (string, error)
}

// Run executes one Ship Run in the current checkout.
Expand Down Expand Up @@ -180,13 +182,14 @@ func (r Orchestrator) final(ctx context.Context, branch string, done []ticket.Ti
if err := r.runPhase(ctx, throbber.Status{Phase: "Final"}, finalPrompt); err != nil {
return fmt.Errorf("Abort: Final Phase: %w", err)
}
open, err := r.PRs.HasOpenPR(ctx, branch)
prURL, err := r.PRs.OpenPRURL(ctx, branch)
if err != nil {
return fmt.Errorf("Abort: check Final pull request for %s: %w", branch, err)
}
if !open {
if prURL == "" {
return fmt.Errorf("Abort: Final Phase produced no open pull request for branch %s", branch)
}
fmt.Fprintf(r.stdout(), "Run complete. Agents finished and opened a pull request:\n%s\n", prURL)
return nil
}

Expand Down
37 changes: 24 additions & 13 deletions internal/run/orchestrator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -555,8 +555,8 @@ func TestRun_skipsFinalWhenNoSuccessfulIteration(t *testing.T) {
if len(tickets.doneList) != 0 {
t.Errorf("done = %v, want none", tickets.doneList)
}
open, _ := prs.HasOpenPR(context.Background(), "ship/run")
if open {
url, _ := prs.OpenPRURL(context.Background(), "ship/run")
if url != "" {
t.Error("no PR should be recorded when Final never runs")
}
}
Expand Down Expand Up @@ -597,6 +597,13 @@ func TestRun_finalPhaseRunsAfterQueueDrains(t *testing.T) {
if !strings.Contains(out.String(), "Queue drained") {
t.Errorf("stdout = %q, want drain summary", out.String())
}
wantPR := "https://example.com/ship/run/pull/1"
if !strings.Contains(out.String(), "Run complete. Agents finished and opened a pull request:") {
t.Errorf("stdout = %q, want Run complete message", out.String())
}
if !strings.Contains(out.String(), wantPR) {
t.Errorf("stdout = %q, want linked pull request %q", out.String(), wantPR)
}
}

func TestRun_finalWithoutOpenedPRFails(t *testing.T) {
Expand Down Expand Up @@ -665,8 +672,8 @@ func TestRun_finalPhaseFailureAbortsRun(t *testing.T) {
if got := tickets.doneList; !equalInts(got, []int{7}) {
t.Errorf("done = %v, want [7]", got)
}
open, _ := prs.HasOpenPR(context.Background(), "ship/run")
if open {
url, _ := prs.OpenPRURL(context.Background(), "ship/run")
if url != "" {
t.Error("no PR should be recorded when Final Agent fails")
}
}
Expand Down Expand Up @@ -710,6 +717,9 @@ func TestRun_stopsAtMaxIterationsWithPartialProgress(t *testing.T) {
if !strings.Contains(finalPrompt, "2") {
t.Errorf("Final prompt must include max iterations (2):\n%s", finalPrompt)
}
if !strings.Contains(out.String(), "https://example.com/ship/run/pull/1") {
t.Errorf("stdout = %q, want linked pull request after Partial Progress Final", out.String())
}
}

func TestRun_implementWithoutCommitFailsAndDoesNotMarkDone(t *testing.T) {
Expand Down Expand Up @@ -1399,23 +1409,24 @@ func (f *fakeTickets) Done(_ context.Context, t ticket.Ticket) error {
}

// fakePRs reports which branches have an open pull request. Final success
// requires HasOpenPR to return true for the Run branch after the Agent exits.
// requires OpenPRURL to return a non-empty URL for the Run branch after the
// Agent exits.
type fakePRs struct {
open map[string]bool
urls map[string]string
}

func (f *fakePRs) HasOpenPR(_ context.Context, branch string) (bool, error) {
if f.open == nil {
return false, nil
func (f *fakePRs) OpenPRURL(_ context.Context, branch string) (string, error) {
if f.urls == nil {
return "", nil
}
return f.open[branch], nil
return f.urls[branch], nil
}

func (f *fakePRs) openPR(branch string) {
if f.open == nil {
f.open = map[string]bool{}
if f.urls == nil {
f.urls = map[string]string{}
}
f.open[branch] = true
f.urls[branch] = "https://example.com/" + branch + "/pull/1"
}

// fakeAgent records Phase requests and defers behavior to handler. The 1-based
Expand Down
20 changes: 12 additions & 8 deletions internal/ticket/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,24 +220,28 @@ func (g *GitHub) Done(ctx context.Context, t Ticket) error {
return err
}

// HasOpenPR reports whether an open pull request exists for branch.
// Ship uses this to verify the Final Phase opened a PR.
func (g *GitHub) HasOpenPR(ctx context.Context, branch string) (bool, error) {
// OpenPRURL returns the URL of an open pull request for branch, or "" if none.
// Ship uses this to verify the Final Phase opened a PR and to link it in the
// Run completion message.
func (g *GitHub) OpenPRURL(ctx context.Context, branch string) (string, error) {
out, err := g.exec()(ctx, "pr", "list",
"--head", branch,
"--state", "open",
"--json", "number",
"--json", "url",
)
if err != nil {
return false, err
return "", err
}
var prs []struct {
Number int `json:"number"`
URL string `json:"url"`
}
if err := json.Unmarshal(out, &prs); err != nil {
return false, fmt.Errorf("parse pr list: %w", err)
return "", fmt.Errorf("parse pr list: %w", err)
}
return len(prs) > 0, nil
if len(prs) == 0 {
return "", nil
}
return prs[0].URL, nil
}

var _ Port = (*GitHub)(nil)
Expand Down
25 changes: 13 additions & 12 deletions internal/ticket/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,31 +241,32 @@ func TestDone_removesShipAndClosesIssue(t *testing.T) {
}
}

func TestHasOpenPR_trueWhenOpenPRExistsForBranch(t *testing.T) {
func TestOpenPRURL_returnsURLWhenOpenPRExistsForBranch(t *testing.T) {
gh := &ticket.GitHub{Exec: scriptedExec(t, map[string]string{
"pr list --head ship/run --state open --json number": `[{"number":42}]`,
"pr list --head ship/run --state open --json url": `[{"url":"https://github.com/acme/ship/pull/42"}]`,
})}

open, err := gh.HasOpenPR(context.Background(), "ship/run")
got, err := gh.OpenPRURL(context.Background(), "ship/run")
if err != nil {
t.Fatalf("HasOpenPR: %v", err)
t.Fatalf("OpenPRURL: %v", err)
}
if !open {
t.Fatal("HasOpenPR = false, want true when an open PR exists")
want := "https://github.com/acme/ship/pull/42"
if got != want {
t.Fatalf("OpenPRURL = %q, want %q", got, want)
}
}

func TestHasOpenPR_falseWhenNone(t *testing.T) {
func TestOpenPRURL_emptyWhenNone(t *testing.T) {
gh := &ticket.GitHub{Exec: scriptedExec(t, map[string]string{
"pr list --head ship/run --state open --json number": `[]`,
"pr list --head ship/run --state open --json url": `[]`,
})}

open, err := gh.HasOpenPR(context.Background(), "ship/run")
got, err := gh.OpenPRURL(context.Background(), "ship/run")
if err != nil {
t.Fatalf("HasOpenPR: %v", err)
t.Fatalf("OpenPRURL: %v", err)
}
if open {
t.Fatal("HasOpenPR = true, want false when no open PR")
if got != "" {
t.Fatalf("OpenPRURL = %q, want empty when no open PR", got)
}
}

Expand Down
Loading