fix: make remote repository entry reliable (#1936)

Author: carlosflorencio

apps/backend/internal/github/controller_test.go

@@ -936,23 +936,46 @@ func TestHttpGetPRInfo_ServiceError(t *testing.T) {
 }
 
 func TestHttpGetPRInfo_NoClient(t *testing.T) {
+	api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != http.MethodGet {
+			t.Errorf("method = %s, want GET", r.Method)
+		}
+		if r.URL.Path != "/repos/acme/widget/pulls/99" {
+			t.Errorf("path = %q, want public PR endpoint", r.URL.Path)
+		}
+		if got := r.Header.Get("Accept"); got != githubAccept {
+			t.Errorf("Accept = %q, want %q", got, githubAccept)
+		}
+		if got := r.Header.Get("X-GitHub-Api-Version"); got != githubAPIVersion {
+			t.Errorf("X-GitHub-Api-Version = %q, want %q", got, githubAPIVersion)
+		}
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"number":99,"title":"Public widget","state":"open","user":{"login":"octo"},"head":{"ref":"feature/public","sha":"abc123"},"base":{"ref":"main"}}`))
+	}))
+	t.Cleanup(api.Close)
+
+	originalAPIBase := anonymousAPIBase
+	anonymousAPIBase = api.URL
+	t.Cleanup(func() { anonymousAPIBase = originalAPIBase })
+
 	router, _ := setupControllerTest(nil)
 
 	req := httptest.NewRequest(http.MethodGet, "/api/v1/github/prs/acme/widget/99/info", nil)
 	w := httptest.NewRecorder()
 	router.ServeHTTP(w, req)
 
-	if w.Code != http.StatusServiceUnavailable {
-		t.Fatalf("expected 503, got %d: %s", w.Code, w.Body.String())
-	}
-	var got struct {
-		Code string `json:"code"`
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
 	}
+	var got PR
 	if err := json.NewDecoder(w.Body).Decode(&got); err != nil {
 		t.Fatalf("decode response: %v", err)
 	}
-	if got.Code != "github_not_configured" {
-		t.Fatalf("expected github_not_configured code, got %q", got.Code)
+	if got.Number != 99 || got.Title != "Public widget" {
+		t.Fatalf("PR = %#v, want public PR details", got)
+	}
+	if got.RepoOwner != "acme" || got.RepoName != "widget" || got.HeadBranch != "feature/public" || got.BaseBranch != "main" || got.AuthorLogin != "octo" {
+		t.Fatalf("PR fallback fields = %#v", got)
 	}
 }
 

apps/backend/internal/github/service_pr.go

@@ -2,6 +2,7 @@ package github
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 	"sync"
@@ -116,19 +117,25 @@ func pickDefaultMergeMethod(m RepoMergeMethods) string {
 
 // GetPR fetches basic PR details from GitHub.
 func (s *Service) GetPR(ctx context.Context, owner, repo string, number int) (*PR, error) {
-	if s.client == nil {
-		return nil, ErrNoClient
+	if s.client != nil {
+		pr, err := s.client.GetPR(ctx, owner, repo, number)
+		if !errors.Is(err, ErrNoClient) {
+			return pr, err
+		}
 	}
-	return s.client.GetPR(ctx, owner, repo, number)
+	return getPRAnonymous(ctx, owner, repo, number)
 }
 
 // GetIssue fetches basic issue details from GitHub. The create-task dialog is
 // currently the only caller and dedupes requests per URL on the frontend.
 func (s *Service) GetIssue(ctx context.Context, owner, repo string, number int) (*Issue, error) {
-	if s.client == nil {
-		return nil, ErrNoClient
+	if s.client != nil {
+		issue, err := s.client.GetIssue(ctx, owner, repo, number)
+		if !errors.Is(err, ErrNoClient) {
+			return issue, err
+		}
 	}
-	return s.client.GetIssue(ctx, owner, repo, number)
+	return getIssueAnonymous(ctx, owner, repo, number)
 }
 
 // GetPRFeedback fetches live PR feedback from GitHub. Cached briefly with

apps/backend/internal/github/service_reviews.go

@@ -539,6 +539,47 @@ func listRepoBranchesAnonymous(ctx context.Context, owner, repo string) ([]RepoB
 	return branches, nil
 }
 
+func getPRAnonymous(ctx context.Context, owner, repo string, number int) (*PR, error) {
+	var raw patPR
+	endpoint := fmt.Sprintf("/repos/%s/%s/pulls/%d", url.PathEscape(owner), url.PathEscape(repo), number)
+	if err := getAnonymous(ctx, endpoint, &raw); err != nil {
+		return nil, fmt.Errorf("get PR #%d: %w", number, err)
+	}
+	return convertPatPR(&raw, owner, repo), nil
+}
+
+func getIssueAnonymous(ctx context.Context, owner, repo string, number int) (*Issue, error) {
+	var raw patIssue
+	endpoint := fmt.Sprintf("/repos/%s/%s/issues/%d", url.PathEscape(owner), url.PathEscape(repo), number)
+	if err := getAnonymous(ctx, endpoint, &raw); err != nil {
+		return nil, fmt.Errorf("get issue #%d: %w", number, err)
+	}
+	return convertPatIssue(&raw, owner, repo), nil
+}
+
+func getAnonymous(ctx context.Context, endpoint string, result any) error {
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, anonymousAPIBase+endpoint, nil)
+	if err != nil {
+		return ErrNoClient
+	}
+	req.Header.Set("Accept", githubAccept)
+	req.Header.Set("X-GitHub-Api-Version", githubAPIVersion)
+
+	resp, err := anonymousHTTPClient.Do(req)
+	if err != nil {
+		return ErrNoClient
+	}
+	defer func() { _ = resp.Body.Close() }()
+	if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
+		body, _ := io.ReadAll(resp.Body)
+		return &GitHubAPIError{StatusCode: resp.StatusCode, Endpoint: endpoint, Body: string(body)}
+	}
+	if err := json.NewDecoder(resp.Body).Decode(result); err != nil {
+		return fmt.Errorf("decode GitHub response: %w", err)
+	}
+	return nil
+}
+
 // parseLinkNext extracts the URL for rel="next" from a GitHub Link header.
 // Returns "" if no next page is present.
 func parseLinkNext(link string) string {

apps/backend/internal/github/service_test.go

@@ -133,6 +133,99 @@ func TestListRepoBranches_NoopClientFallback_NotFound(t *testing.T) {
 	}
 }
 
+func TestGetPRAndIssue_FallBackWithoutClient(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		switch r.URL.Path {
+		case "/repos/owner/repo/pulls/7":
+			_, _ = w.Write([]byte(`{"number":7,"title":"Public PR","state":"open","head":{"ref":"feature"},"base":{"ref":"main"},"user":{"login":"alice"}}`))
+		case "/repos/owner/repo/issues/8":
+			_, _ = w.Write([]byte(`{"number":8,"title":"Public issue","state":"open","user":{"login":"alice"}}`))
+		default:
+			http.NotFound(w, r)
+		}
+	}))
+	defer srv.Close()
+
+	orig := anonymousAPIBase
+	anonymousAPIBase = srv.URL
+	defer func() { anonymousAPIBase = orig }()
+
+	for _, client := range []Client{nil, &NoopClient{}} {
+		svc := &Service{client: client}
+		pr, err := svc.GetPR(context.Background(), "owner", "repo", 7)
+		if err != nil {
+			t.Fatalf("GetPR() error = %v", err)
+		}
+		if pr.Title != "Public PR" {
+			t.Fatalf("GetPR() title = %q, want public PR", pr.Title)
+		}
+
+		issue, err := svc.GetIssue(context.Background(), "owner", "repo", 8)
+		if err != nil {
+			t.Fatalf("GetIssue() error = %v", err)
+		}
+		if issue.Title != "Public issue" {
+			t.Fatalf("GetIssue() title = %q, want public issue", issue.Title)
+		}
+	}
+}
+
+func TestGetPRAndIssue_AuthenticatedErrorsAreAuthoritative(t *testing.T) {
+	orig := anonymousAPIBase
+	anonymousAPIBase = "http://127.0.0.1:1"
+	defer func() { anonymousAPIBase = orig }()
+
+	prErr := &GitHubAPIError{StatusCode: http.StatusForbidden}
+	issueErr := &GitHubAPIError{StatusCode: http.StatusNotFound}
+	svc := &Service{client: &stubClient{
+		getPRFunc: func(context.Context, string, string, int) (*PR, error) { return nil, prErr },
+		getIssueFunc: func(context.Context, string, string, int) (*Issue, error) {
+			return nil, issueErr
+		},
+	}}
+
+	if _, err := svc.GetPR(context.Background(), "owner", "repo", 7); !errors.Is(err, prErr) {
+		t.Fatalf("GetPR() error = %v, want authenticated 403", err)
+	}
+	if _, err := svc.GetIssue(context.Background(), "owner", "repo", 8); !errors.Is(err, issueErr) {
+		t.Fatalf("GetIssue() error = %v, want authenticated 404", err)
+	}
+}
+
+func TestGetPRAndIssue_AnonymousStatusIsPreserved(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if strings.Contains(r.URL.Path, "/pulls/") {
+			w.WriteHeader(http.StatusForbidden)
+			return
+		}
+		w.WriteHeader(http.StatusNotFound)
+	}))
+	defer srv.Close()
+
+	orig := anonymousAPIBase
+	anonymousAPIBase = srv.URL
+	defer func() { anonymousAPIBase = orig }()
+
+	svc := &Service{}
+	for _, request := range []struct {
+		name   string
+		call   func() error
+		status int
+	}{
+		{name: "PR", call: func() error { _, err := svc.GetPR(t.Context(), "owner", "repo", 7); return err }, status: http.StatusForbidden},
+		{name: "issue", call: func() error { _, err := svc.GetIssue(t.Context(), "owner", "repo", 8); return err }, status: http.StatusNotFound},
+	} {
+		t.Run(request.name, func(t *testing.T) {
+			err := request.call()
+			var apiErr *GitHubAPIError
+			if !errors.As(err, &apiErr) || apiErr.StatusCode != request.status {
+				t.Fatalf("error = %v, want GitHub API status %d", err, request.status)
+			}
+		})
+	}
+}
+
 func TestSortBranchesMainFirst(t *testing.T) {
 	tests := []struct {
 		input []string
@@ -173,17 +266,6 @@ func TestSortBranchesMainFirst(t *testing.T) {
 	}
 }
 
-func TestGetPR_NilClient(t *testing.T) {
-	svc := &Service{client: nil}
-	_, err := svc.GetPR(context.Background(), "owner", "repo", 1)
-	if err == nil {
-		t.Fatal("expected error when client is nil")
-	}
-	if !errors.Is(err, ErrNoClient) {
-		t.Errorf("err = %v, want ErrNoClient", err)
-	}
-}
-
 func TestGetPRFeedback_NilClient(t *testing.T) {
 	svc := &Service{client: nil}
 	_, err := svc.GetPRFeedback(context.Background(), "owner", "repo", 1)

apps/backend/internal/gitlab/controller_test.go

@@ -1,6 +1,7 @@
 package gitlab
 
 import (
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -11,6 +12,12 @@ import (
 	"github.com/gin-gonic/gin"
 )
 
+type roundTripFunc func(*http.Request) (*http.Response, error)
+
+func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) {
+	return f(req)
+}
+
 // newControllerFixture wires a real PATClient + Controller against an
 // httptest.NewServer GitLab stub. The returned *requestLog captures every
 // path + query the stub observed so tests can assert exactly which params
@@ -100,6 +107,95 @@ func hit(router *gin.Engine, target string) *httptest.ResponseRecorder {
 	return w
 }
 
+func TestHttpListProjectBranches_UnconfiguredPublicGitLab(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	store := newTestStore(t)
+	seedWorkspace(t, store, "workspace-test")
+	svc := NewService(DefaultHost, NewNoopClient(DefaultHost), AuthMethodNone, nil, newTestLogger(t))
+	svc.SetStore(store)
+	router := gin.New()
+	NewController(svc, newTestLogger(t)).RegisterHTTPRoutes(router)
+
+	originalTransport := http.DefaultTransport
+	http.DefaultTransport = roundTripFunc(func(req *http.Request) (*http.Response, error) {
+		if req.URL.Host != "gitlab.com" {
+			t.Fatalf("anonymous request host = %q, want gitlab.com", req.URL.Host)
+		}
+		if got := req.Header.Get("PRIVATE-TOKEN"); got != "" {
+			t.Fatalf("PRIVATE-TOKEN = %q, want absent for anonymous read", got)
+		}
+		return &http.Response{
+			StatusCode: http.StatusOK,
+			Header:     make(http.Header),
+			Body:       io.NopCloser(strings.NewReader(`[{"name":"main"}]`)),
+			Request:    req,
+		}, nil
+	})
+	t.Cleanup(func() { http.DefaultTransport = originalTransport })
+
+	resp := hit(router, "/api/v1/gitlab/projects/branches?project=group%2Fproject&expected_host=https%3A%2F%2Fgitlab.com")
+	if resp.Code != http.StatusOK {
+		t.Fatalf("status = %d, want 200; body=%s", resp.Code, resp.Body.String())
+	}
+	if !strings.Contains(resp.Body.String(), `"name":"main"`) {
+		t.Fatalf("response = %s, want main branch", resp.Body.String())
+	}
+}
+
+func TestHttpListProjectBranches_ConfiguredUpstreamNotFound(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	host, stop := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusNotFound)
+	}))
+	t.Cleanup(stop)
+
+	store := newTestStore(t)
+	seedWorkspace(t, store, "workspace-test")
+	if err := store.UpsertConfigForWorkspace(t.Context(), "workspace-test", &GitLabConfig{
+		Host: host, AuthMethod: AuthMethodPAT,
+	}); err != nil {
+		t.Fatalf("seed GitLab config: %v", err)
+	}
+	svc := NewService(host, NewNoopClient(host), AuthMethodNone, nil, newTestLogger(t))
+	svc.SetStore(store)
+	svc.SetWorkspaceSecretStore(&configTestSecrets{values: map[string]string{
+		SecretKeyForWorkspace("workspace-test"): "token",
+	}})
+	router := gin.New()
+	NewController(svc, newTestLogger(t)).RegisterHTTPRoutes(router)
+
+	resp := hit(router, "/api/v1/gitlab/projects/branches?project=group%2Fmissing&expected_host="+url.QueryEscape(host))
+	if resp.Code != http.StatusNotFound {
+		t.Fatalf("status = %d, want 404; body=%s", resp.Code, resp.Body.String())
+	}
+}
+
+func TestHttpListProjectBranches_UnconfiguredPublicNotFound(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	store := newTestStore(t)
+	seedWorkspace(t, store, "workspace-test")
+	svc := NewService(DefaultHost, NewNoopClient(DefaultHost), AuthMethodNone, nil, newTestLogger(t))
+	svc.SetStore(store)
+	router := gin.New()
+	NewController(svc, newTestLogger(t)).RegisterHTTPRoutes(router)
+
+	originalTransport := http.DefaultTransport
+	http.DefaultTransport = roundTripFunc(func(req *http.Request) (*http.Response, error) {
+		return &http.Response{
+			StatusCode: http.StatusNotFound,
+			Header:     make(http.Header),
+			Body:       io.NopCloser(strings.NewReader(`{"message":"not found"}`)),
+			Request:    req,
+		}, nil
+	})
+	t.Cleanup(func() { http.DefaultTransport = originalTransport })
+
+	resp := hit(router, "/api/v1/gitlab/projects/branches?project=group%2Fmissing&expected_host=https%3A%2F%2Fgitlab.com")
+	if resp.Code != http.StatusNotFound {
+		t.Fatalf("status = %d, want 404; body=%s", resp.Code, resp.Body.String())
+	}
+}
+
 // Regression for the /gitlab page tabs: each tab value must reach GitLab
 // as a real scoping param. Before the translator was added the bare token
 // became an empty-value key (e.g. `assigned_to_me=`) and the page served