fix(submit): do not mistake GitHub 5xx errors for a missing PR

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.

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)
+	}
+}

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"))