diff --git a/internal/engine/github.go b/internal/engine/github.go index f15379a..804b091 100644 --- a/internal/engine/github.go +++ b/internal/engine/github.go @@ -40,8 +40,38 @@ func ghCheckInstalled() error { } // ghPRForBranch checks whether a PR exists for the given branch. -// Returns nil, nil when no PR exists (gh exits with code 1). +// Returns nil, nil when no PR exists. +// +// gh exits 1 both for "no pull requests found" and for server-side failures +// (HTTP 503 and friends), so the exit code cannot distinguish "no PR" from +// "GitHub didn't answer". Mistaking the second for the first made submit +// offer to create PRs that already existed — the answer has to come from the +// error text. Server-side failures are retried before giving up, since one +// flaky call otherwise poisons a whole stack survey. func ghPRForBranch(branch string) (*PRResult, error) { + const attempts = 3 + var lastErr error + for i := 0; i < attempts; i++ { + if i > 0 { + time.Sleep(time.Duration(i) * time.Second) + } + result, stderr, err := ghPRView(branch) + switch { + case err == nil: + return result, nil + case ghNoPRFound(stderr): + return nil, nil + case !ghServerError(stderr): + return nil, err + } + lastErr = err + } + return nil, lastErr +} + +// ghPRView runs one `gh pr view` and returns its stderr alongside the error, +// so the caller can classify the failure. It never interprets exit codes. +func ghPRView(branch string) (*PRResult, string, error) { ctx, cancel := context.WithTimeout(context.Background(), ghTimeout) defer cancel() @@ -55,20 +85,30 @@ func ghPRForBranch(branch string) (*PRResult, error) { if err := cmd.Run(); err != nil { if ctx.Err() != nil { - return nil, fmt.Errorf("gh pr view timed out after %s", ghTimeout) + return nil, "", fmt.Errorf("gh pr view timed out after %s", ghTimeout) } - // gh exits 1 when no PR exists for the branch. - if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { - return nil, nil - } - return nil, fmt.Errorf("gh pr view failed: %s: %w", strings.TrimSpace(stderr.String()), err) + msg := strings.TrimSpace(stderr.String()) + return nil, msg, fmt.Errorf("gh pr view failed: %s: %w", msg, err) } var result PRResult if err := json.Unmarshal(stdout.Bytes(), &result); err != nil { - return nil, fmt.Errorf("failed to parse gh output: %w", err) + return nil, "", fmt.Errorf("failed to parse gh output: %w", err) } - return &result, nil + return &result, "", nil +} + +// ghNoPRFound reports whether a gh failure means the branch genuinely has no +// pull request — gh prints `no pull requests found for branch "x"` for it. +func ghNoPRFound(stderr string) bool { + return strings.Contains(stderr, "no pull requests found") +} + +// ghServerError reports whether a gh failure is GitHub's, not ours: an HTTP +// 5xx from the API (gh prints "HTTP 503: ..."). Worth retrying; everything +// else (auth, rate limits, bad requests) fails the same way again. +func ghServerError(stderr string) bool { + return strings.Contains(stderr, "HTTP 5") } // ghUpdatePRBase retargets an existing PR's base branch on GitHub. diff --git a/internal/engine/github_test.go b/internal/engine/github_test.go new file mode 100644 index 0000000..cb8539d --- /dev/null +++ b/internal/engine/github_test.go @@ -0,0 +1,115 @@ +package engine + +import ( + "fmt" + "os" + "path/filepath" + "testing" +) + +// gh exits 1 for "no PR exists" and for server failures alike, so the +// classification must come from the error text. Getting it wrong in one +// direction offers to create duplicate PRs; in the other, it turns a plain +// "no PR yet" into a spurious failure. + +func TestGHNoPRFound(t *testing.T) { + cases := []struct { + name string + stderr string + want bool + }{ + {"no PR for branch", `no pull requests found for branch "feature-x"`, true}, + {"server unavailable", "HTTP 503: No server is currently available to service your request. (https://api.github.com/graphql)", false}, + {"auth failure", "HTTP 401: Bad credentials (https://api.github.com/graphql)", false}, + {"empty stderr", "", false}, + } + for _, tc := range cases { + if got := ghNoPRFound(tc.stderr); got != tc.want { + t.Errorf("%s: ghNoPRFound = %v, want %v", tc.name, got, tc.want) + } + } +} + +func TestGHServerError(t *testing.T) { + cases := []struct { + name string + stderr string + want bool + }{ + {"503 unavailable", "HTTP 503: No server is currently available to service your request. (https://api.github.com/graphql)", true}, + {"502 bad gateway", "HTTP 502: Bad gateway (https://api.github.com/graphql)", true}, + {"500 internal", "HTTP 500: Internal server error (https://api.github.com/graphql)", true}, + {"401 is the caller's problem", "HTTP 401: Bad credentials (https://api.github.com/graphql)", false}, + {"403 rate limit is the caller's problem", "HTTP 403: API rate limit exceeded (https://api.github.com/graphql)", false}, + {"no PR is not a server error", `no pull requests found for branch "feature-x"`, false}, + {"empty stderr", "", false}, + } + for _, tc := range cases { + if got := ghServerError(tc.stderr); got != tc.want { + t.Errorf("%s: ghServerError = %v, want %v", tc.name, got, tc.want) + } + } +} + +// stubGHScript puts a `gh` on PATH backed by the given shell script body. +func stubGHScript(t *testing.T, body string) { + t.Helper() + dir := t.TempDir() + script := filepath.Join(dir, "gh") + if err := os.WriteFile(script, []byte("#!/bin/sh\n"+body), 0o755); err != nil { + t.Fatalf("write gh stub: %v", err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +// A 503 on the first call must not be read as "no PR" — it should be retried, +// and the PR found on the second attempt returned as if nothing happened. +func TestGHPRForBranch_RetriesServerErrorThenSucceeds(t *testing.T) { + state := t.TempDir() + stubGHScript(t, fmt.Sprintf(` +count=%q +if [ ! -f "$count" ]; then + touch "$count" + echo 'HTTP 503: No server is currently available (https://api.github.com/graphql)' >&2 + exit 1 +fi +echo '{"number":42,"url":"https://example.com/pull/42","state":"OPEN","title":"t","isDraft":false}' +`, filepath.Join(state, "count"))) + + pr, err := ghPRForBranch("feature") + if err != nil { + t.Fatalf("ghPRForBranch after transient 503: %v", err) + } + if pr == nil || pr.Number != 42 { + t.Fatalf("ghPRForBranch = %+v, want PR #42", pr) + } +} + +// "no pull requests found" is a real answer, not a failure: nil PR, nil error, +// no retries. +func TestGHPRForBranch_NoPRIsNotAnError(t *testing.T) { + stubGHScript(t, `echo 'no pull requests found for branch "feature"' >&2 +exit 1 +`) + + pr, err := ghPRForBranch("feature") + if err != nil { + t.Fatalf("ghPRForBranch on no-PR branch: %v", err) + } + if pr != nil { + t.Fatalf("ghPRForBranch = %+v, want nil", pr) + } +} + +// A non-transient failure (bad credentials) must surface as an error — the old +// behavior of reading it as "no PR" is what offered to create duplicate PRs. +func TestGHPRForBranch_AuthFailureIsAnError(t *testing.T) { + stubGHScript(t, `echo 'HTTP 401: Bad credentials (https://api.github.com/graphql)' >&2 +exit 1 +`) + + pr, err := ghPRForBranch("feature") + if err == nil { + t.Fatalf("ghPRForBranch on auth failure returned %+v, want error", pr) + } +} diff --git a/internal/engine/prepare_test.go b/internal/engine/prepare_test.go index 0de6836..e1e7de9 100644 --- a/internal/engine/prepare_test.go +++ b/internal/engine/prepare_test.go @@ -11,12 +11,16 @@ import ( ) // stubGH puts a `gh` on PATH that always reports "no PR for this branch", so -// PrepareAI never reaches the network from a test. +// PrepareAI never reaches the network from a test. It prints the same message +// real gh does: a bare exit 1 is no longer read as "no PR" — it is +// indistinguishable from a server failure, which is exactly the confusion +// ghPRForBranch now refuses to make. func stubGH(t *testing.T) { t.Helper() dir := t.TempDir() script := filepath.Join(dir, "gh") - if err := os.WriteFile(script, []byte("#!/bin/sh\nexit 1\n"), 0o755); err != nil { + body := "#!/bin/sh\necho 'no pull requests found for branch' >&2\nexit 1\n" + if err := os.WriteFile(script, []byte(body), 0o755); err != nil { t.Fatalf("write gh stub: %v", err) } t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH"))