feat: remote tab in task-create dialog — multi-row GitHub repo picker (#1116)

This commit is contained in:
Carlos Florêncio
2026-05-30 15:47:02 +01:00
committed by GitHub
parent d288bd6a6d
commit d66d9e6f67
81 changed files with 8990 additions and 1441 deletions
+12
View File
@@ -35,6 +35,18 @@ type Client interface {
// SearchOrgRepos searches repositories in an organization, optionally filtered by a query string.
SearchOrgRepos(ctx context.Context, org, query string, limit int) ([]GitHubRepo, error)
// ListUserRepos lists repositories the authenticated user has access to,
// optionally filtered by a query string. limit is a per-page upper bound
// (clamped to GitHub's 100 max by implementations).
ListUserRepos(ctx context.Context, query string, limit int) ([]GitHubRepo, error)
// ListAccessibleRepos lists every repo the authenticated user can access —
// their own repos plus collaborator and org-member repos — in a single
// GET /user/repos call on the core REST quota (no per-org search fan-out).
// query is applied as a case-insensitive substring filter on full_name
// after fetching; limit bounds the page size (clamped to GitHub's 100 max).
ListAccessibleRepos(ctx context.Context, query string, limit int) ([]GitHubRepo, error)
// ListPRReviews lists reviews on a pull request.
ListPRReviews(ctx context.Context, owner, repo string, number int) ([]PRReview, error)
@@ -64,6 +64,7 @@ func (c *Controller) RegisterHTTPRoutes(router *gin.Engine) {
api.POST("/cleanup/issue-tasks", c.httpCleanupIssueTasks)
api.GET("/orgs", c.httpListUserOrgs)
api.GET("/repos", c.httpListAccessibleRepos)
api.GET("/repos/search", c.httpSearchRepos)
api.GET("/repos/:owner/:repo/branches", c.httpListRepoBranches)
api.GET("/repos/:owner/:repo/merge-methods", c.httpGetRepoMergeMethods)
@@ -484,6 +485,44 @@ func (c *Controller) httpListUserOrgs(ctx *gin.Context) {
ctx.JSON(http.StatusOK, gin.H{"orgs": orgs})
}
// httpListAccessibleRepos returns the merged list of repos the authenticated
// user can access (own repos + each org they belong to). Accepts:
// - q: optional GitHub search qualifier appended to the per-source query
// - limit: 1..100, defaults to 50 when missing / non-positive
//
// Returns 503 with `{"code":"github_not_configured"}` (matches every other
// GitHub-not-configured 503 in this file) when GitHub is not configured /
// not authenticated.
func (c *Controller) httpListAccessibleRepos(ctx *gin.Context) {
query := ctx.Query("q")
limit, _ := strconv.Atoi(ctx.Query("limit"))
repos, err := c.service.ListAccessibleRepos(ctx.Request.Context(), query, limit)
if err != nil {
if errors.Is(err, ErrNoClient) {
ctx.JSON(http.StatusServiceUnavailable, gin.H{
"error": "GitHub is not configured. Install the gh CLI and run 'gh auth login', or add a GITHUB_TOKEN secret.",
"code": "github_not_configured",
})
return
}
// Preserve GitHub-side status codes (401 / 403 / 404) so the
// frontend can render the appropriate UX — auth re-prompt for 401,
// permission notice for 403, etc. — instead of collapsing every
// upstream failure into an opaque 500.
status := http.StatusInternalServerError
var apiErr *GitHubAPIError
if errors.As(err, &apiErr) {
switch apiErr.StatusCode {
case http.StatusUnauthorized, http.StatusForbidden, http.StatusNotFound:
status = apiErr.StatusCode
}
}
ctx.JSON(status, gin.H{"error": err.Error()})
return
}
ctx.JSON(http.StatusOK, gin.H{"repos": repos})
}
func (c *Controller) httpSearchRepos(ctx *gin.Context) {
org := ctx.Query("org")
query := ctx.Query("q")
@@ -4,9 +4,11 @@ import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
@@ -51,6 +53,12 @@ func (s *stubClient) ListUserOrgs(context.Context) ([]GitHubOrg, error) { return
func (s *stubClient) SearchOrgRepos(context.Context, string, string, int) ([]GitHubRepo, error) {
return nil, nil
}
func (s *stubClient) ListUserRepos(context.Context, string, int) ([]GitHubRepo, error) {
return nil, nil
}
func (s *stubClient) ListAccessibleRepos(context.Context, string, int) ([]GitHubRepo, error) {
return nil, nil
}
func (s *stubClient) ListPRReviews(context.Context, string, string, int) ([]PRReview, error) {
return nil, nil
}
@@ -465,6 +473,179 @@ func TestHttpSubmitReview_SelfApproveReturns422(t *testing.T) {
}
}
// listAccessibleReposClient is a per-test client tuned for the
// /api/v1/github/repos handler tests. Kept separate from the larger
// stubClient so the existing PR/merge tests above keep their minimal shape.
type listAccessibleReposClient struct {
stubClient
repos []GitHubRepo
}
func (c *listAccessibleReposClient) ListAccessibleRepos(_ context.Context, query string, _ int) ([]GitHubRepo, error) {
return filterReposByQuery(c.repos, query), nil
}
func TestHandleListAccessibleRepos_OK(t *testing.T) {
sc := &listAccessibleReposClient{
repos: []GitHubRepo{
{FullName: "alice/personal", Owner: "alice", Name: "personal", DefaultBranch: "main", Description: "Personal repo", PushedAt: func() *time.Time { t := time.Unix(200, 0); return &t }()},
{FullName: "acme/widget", Owner: "acme", Name: "widget", DefaultBranch: "trunk", PushedAt: func() *time.Time { t := time.Unix(100, 0); return &t }()},
},
}
router, _ := setupControllerTest(sc)
req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos?q=&limit=10", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
if w.Code != http.StatusOK {
t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
}
raw := w.Body.String()
var body struct {
Repos []GitHubRepo `json:"repos"`
}
if err := json.NewDecoder(strings.NewReader(raw)).Decode(&body); err != nil {
t.Fatalf("decode: %v", err)
}
if len(body.Repos) != 2 {
t.Fatalf("got %d repos, want 2", len(body.Repos))
}
// Sorted by pushed_at desc → personal (200) first, widget (100) second.
if body.Repos[0].FullName != "alice/personal" {
t.Errorf("first repo = %q, want alice/personal", body.Repos[0].FullName)
}
if body.Repos[0].DefaultBranch != "main" {
t.Errorf("first repo default_branch = %q, want main", body.Repos[0].DefaultBranch)
}
if body.Repos[0].Description != "Personal repo" {
t.Errorf("first repo description = %q, want Personal repo", body.Repos[0].Description)
}
if body.Repos[1].FullName != "acme/widget" {
t.Errorf("second repo = %q, want acme/widget", body.Repos[1].FullName)
}
if body.Repos[1].DefaultBranch != "trunk" {
t.Errorf("second repo default_branch = %q, want trunk", body.Repos[1].DefaultBranch)
}
// Empty description must be omitted (omitempty) — verify via the raw JSON
// so a future struct-tag regression dropping omitempty is caught.
if strings.Contains(raw, `"description":""`) {
t.Errorf("expected empty description to be omitted, got body: %s", raw)
}
}
func TestHandleListAccessibleRepos_503_WhenUnavailable(t *testing.T) {
gin.SetMode(gin.TestMode)
log := newControllerTestLogger()
// nil client triggers ErrNoClient via Service.ListAccessibleRepos.
svc := NewService(nil, "none", nil, nil, nil, log)
ctrl := NewController(svc, log)
router := gin.New()
ctrl.RegisterHTTPRoutes(router)
req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos", 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 body struct {
Error string `json:"error"`
Code string `json:"code"`
}
if err := json.NewDecoder(w.Body).Decode(&body); err != nil {
t.Fatalf("decode: %v", err)
}
if body.Code != "github_not_configured" {
t.Errorf("code = %q, want github_not_configured", body.Code)
}
wantMsg := "GitHub is not configured. Install the gh CLI and run 'gh auth login', or add a GITHUB_TOKEN secret."
if body.Error != wantMsg {
t.Errorf("error = %q, want %q", body.Error, wantMsg)
}
}
// listAccessibleReposEmptyClient: orgs+user repos both empty. Verifies the
// handler emits the literal `{"repos":[]}` body so a future regression that
// emits `null` (omitempty + nil slice) is caught.
func TestHandleListAccessibleRepos_Empty(t *testing.T) {
sc := &listAccessibleReposClient{repos: []GitHubRepo{}}
router, _ := setupControllerTest(sc)
req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
if w.Code != http.StatusOK {
t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
}
got := strings.TrimSpace(w.Body.String())
if got != `{"repos":[]}` {
t.Errorf("body = %q, want %q (regression guard for null repos)", got, `{"repos":[]}`)
}
}
// listAccessibleReposErrClient surfaces an unexpected error from the repo
// fetch — the handler must map it to a 500 rather than swallowing it.
type listAccessibleReposErrClient struct {
stubClient
reposErr error
}
func (c *listAccessibleReposErrClient) ListAccessibleRepos(context.Context, string, int) ([]GitHubRepo, error) {
return nil, c.reposErr
}
func TestHandleListAccessibleRepos_500_OnUnknownError(t *testing.T) {
sc := &listAccessibleReposErrClient{reposErr: errors.New("boom")}
router, _ := setupControllerTest(sc)
req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
if w.Code != http.StatusInternalServerError {
t.Fatalf("expected 500, got %d: %s", w.Code, w.Body.String())
}
}
// TestHandleListAccessibleRepos_PreservesAPIErrorStatus verifies that when the
// upstream GitHub API returns 401 / 403 / 404, the handler surfaces the same
// status to the frontend rather than collapsing it into a generic 500. The
// frontend needs the distinct status to render the right UX (auth re-prompt,
// permission notice, "not found").
func TestHandleListAccessibleRepos_PreservesAPIErrorStatus(t *testing.T) {
cases := []struct {
name string
statusCode int
}{
{"unauthorized", http.StatusUnauthorized},
{"forbidden", http.StatusForbidden},
{"not_found", http.StatusNotFound},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
sc := &listAccessibleReposErrClient{
reposErr: &GitHubAPIError{
StatusCode: tc.statusCode,
Endpoint: "/user/repos",
Body: "{}",
},
}
router, _ := setupControllerTest(sc)
req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
if w.Code != tc.statusCode {
t.Fatalf("expected %d, got %d: %s", tc.statusCode, w.Code, w.Body.String())
}
})
}
}
func TestHttpMergePR_Conflict(t *testing.T) {
sc := &stubClient{
mergePRFn: func(context.Context, string, string, int, string) error {
+94 -10
View File
@@ -13,6 +13,17 @@ import (
"time"
)
// ghSearchReposPath is the GitHub REST search-repositories path that gh CLI
// invocations target via `gh api`. Centralised to keep the search-bucket
// rate-limit dispatch (`resourceForGHArgs`) and the repo search helpers in
// sync.
const ghSearchReposPath = "search/repositories"
// ghAccessibleReposPath is the GET /user/repos endpoint (with affiliation +
// sort + per_page baked in) that backs ListAccessibleRepos. It returns a flat
// JSON array on the core REST quota, replacing the per-org search fan-out.
const ghAccessibleReposPathFmt = "/user/repos?affiliation=%s&sort=pushed&per_page=%d"
// GHClient implements Client using the gh CLI.
type GHClient struct {
rateTracker *RateTracker
@@ -337,10 +348,8 @@ func (c *GHClient) SearchOrgRepos(ctx context.Context, org, query string, limit
if query != "" {
q += " " + query
}
if limit <= 0 {
limit = 20
}
out, err := c.run(ctx, "api", "search/repositories",
limit = clampRepoSearchLimit(limit)
out, err := c.run(ctx, "api", ghSearchReposPath,
"-X", "GET",
"-f", "q="+q,
"-f", fmt.Sprintf("per_page=%d", limit),
@@ -351,14 +360,83 @@ func (c *GHClient) SearchOrgRepos(ctx context.Context, org, query string, limit
return parseGHSearchRepos(out)
}
func (c *GHClient) ListUserRepos(ctx context.Context, query string, limit int) ([]GitHubRepo, error) {
limit = clampRepoSearchLimit(limit)
login, err := c.GetAuthenticatedUser(ctx)
if err != nil {
return nil, fmt.Errorf("list user repos: %w", err)
}
args := buildUserReposGHArgs(login, query, limit)
out, err := c.run(ctx, args...)
if err != nil {
return nil, fmt.Errorf("list user repos: %w", err)
}
return parseGHSearchRepos(out)
}
// buildUserReposGHArgs constructs the `gh api search/repositories` argv used
// to list repos for the authenticated user. Keeping this pure makes the
// endpoint + qualifier construction unit-testable without spawning gh. The
// `q` value mirrors the PAT path (`user:<login> <query>`) so both clients
// honour the same GitHub search syntax (e.g. `language:go`, `in:name`).
// Callers must clamp `limit` via clampRepoSearchLimit beforehand.
func buildUserReposGHArgs(login, query string, limit int) []string {
q := "user:" + login
if query != "" {
q += " " + query
}
return []string{
"api", ghSearchReposPath,
"-X", "GET",
"-f", "q=" + q,
"-f", fmt.Sprintf("per_page=%d", limit),
"--jq", ".items",
}
}
// ListAccessibleRepos lists every repo the authenticated user can access via a
// single `gh api /user/repos` call on the core REST quota. The endpoint returns
// a flat JSON array (parsed by parseGHSearchRepos, which already decodes a
// top-level array). No --paginate: one page of up to 100 is what the picker
// needs (the frontend caps at 100 and filters client-side).
//
// query is a BEST-EFFORT substring filter over only that single un-paginated
// page — a match beyond the cap returns nothing. The frontend does the
// canonical client-side filtering, so this server filter is just optional
// narrowing; do not rely on it for completeness.
func (c *GHClient) ListAccessibleRepos(ctx context.Context, query string, limit int) ([]GitHubRepo, error) {
limit = clampRepoSearchLimit(limit)
out, err := c.run(ctx, buildAccessibleReposGHArgs(limit)...)
if err != nil {
return nil, fmt.Errorf("list accessible repos: %w", err)
}
repos, err := parseGHSearchRepos(out)
if err != nil {
return nil, err
}
return filterReposByQuery(repos, query), nil
}
// buildAccessibleReposGHArgs constructs the `gh api /user/repos?...` argv for
// ListAccessibleRepos. Keeping it pure makes the endpoint/query construction
// unit-testable without spawning gh. Callers must clamp `limit` via
// clampRepoSearchLimit beforehand.
func buildAccessibleReposGHArgs(limit int) []string {
endpoint := fmt.Sprintf(ghAccessibleReposPathFmt, accessibleReposAffiliation, limit)
return []string{"api", endpoint}
}
func parseGHSearchRepos(data string) ([]GitHubRepo, error) {
var items []struct {
FullName string `json:"full_name"`
Owner struct {
Login string `json:"login"`
} `json:"owner"`
Name string `json:"name"`
Private bool `json:"private"`
Name string `json:"name"`
Private bool `json:"private"`
DefaultBranch string `json:"default_branch"`
Description string `json:"description"`
PushedAt time.Time `json:"pushed_at"`
}
if err := json.Unmarshal([]byte(data), &items); err != nil {
return nil, fmt.Errorf("parse search repos: %w", err)
@@ -366,10 +444,16 @@ func parseGHSearchRepos(data string) ([]GitHubRepo, error) {
repos := make([]GitHubRepo, len(items))
for i, item := range items {
repos[i] = GitHubRepo{
FullName: item.FullName,
Owner: item.Owner.Login,
Name: item.Name,
Private: item.Private,
FullName: item.FullName,
Owner: item.Owner.Login,
Name: item.Name,
Private: item.Private,
DefaultBranch: item.DefaultBranch,
Description: item.Description,
}
if !item.PushedAt.IsZero() {
t := item.PushedAt
repos[i].PushedAt = &t
}
}
return repos, nil
@@ -2,6 +2,7 @@ package github
import (
"errors"
"strings"
"testing"
"time"
)
@@ -390,6 +391,139 @@ func TestResourceForGHArgs(t *testing.T) {
}
}
func TestBuildUserReposGHArgs(t *testing.T) {
cases := []struct {
name string
login string
query string
limit int
wantQ string
wantP string
}{
{
name: "empty query",
login: "alice",
query: "",
limit: 20,
wantQ: "q=user:alice",
wantP: "per_page=20",
},
{
name: "with query",
login: "alice",
query: "language:go",
limit: 50,
wantQ: "q=user:alice language:go",
wantP: "per_page=50",
},
{
name: "clamped limit at upper bound",
login: "bob",
query: "",
limit: 100,
wantQ: "q=user:bob",
wantP: "per_page=100",
},
{
name: "default clamp value",
login: "bob",
query: "in:name foo",
limit: 20,
wantQ: "q=user:bob in:name foo",
wantP: "per_page=20",
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
args := buildUserReposGHArgs(tc.login, tc.query, tc.limit)
// Sanity: first two args pin the gh subcommand to `api search/repositories`.
if len(args) < 2 || args[0] != "api" || args[1] != "search/repositories" {
t.Fatalf("args prefix = %v, want [api search/repositories ...]", args)
}
// Find -f q=... and -f per_page=... values without depending on
// exact positional layout (so future arg additions don't break the test).
gotQ, gotP := "", ""
for i := 0; i < len(args)-1; i++ {
if args[i] != "-f" {
continue
}
switch {
case strings.HasPrefix(args[i+1], "q="):
gotQ = args[i+1]
case strings.HasPrefix(args[i+1], "per_page="):
gotP = args[i+1]
}
}
if gotQ != tc.wantQ {
t.Errorf("q flag = %q, want %q", gotQ, tc.wantQ)
}
if gotP != tc.wantP {
t.Errorf("per_page flag = %q, want %q", gotP, tc.wantP)
}
})
}
}
func TestParseGHSearchRepos(t *testing.T) {
// Mixed payload: one public repo with a description and a default branch
// other than "main", one private repo with a `null` description that must
// decode to an empty string, and one repo with no pushed_at to verify the
// nil-pointer branch.
data := `[
{"full_name":"octocat/hello","owner":{"login":"octocat"},"name":"hello","private":false,"default_branch":"trunk","description":"Hello world","pushed_at":"2025-03-01T10:00:00Z"},
{"full_name":"octocat/secret","owner":{"login":"octocat"},"name":"secret","private":true,"default_branch":"main","description":null,"pushed_at":"2025-02-01T10:00:00Z"},
{"full_name":"octocat/new","owner":{"login":"octocat"},"name":"new","private":false,"default_branch":"main"}
]`
repos, err := parseGHSearchRepos(data)
if err != nil {
t.Fatalf("parseGHSearchRepos: %v", err)
}
if len(repos) != 3 {
t.Fatalf("len = %d, want 3", len(repos))
}
if repos[0].FullName != "octocat/hello" || repos[0].DefaultBranch != "trunk" || repos[0].Description != "Hello world" {
t.Errorf("repo[0] unexpected: %#v", repos[0])
}
if repos[0].PushedAt == nil {
t.Errorf("repo[0] PushedAt nil, want non-nil")
}
if !repos[1].Private || repos[1].DefaultBranch != "main" || repos[1].Description != "" {
t.Errorf("repo[1] unexpected: %#v", repos[1])
}
if repos[2].DefaultBranch != "main" || repos[2].PushedAt != nil {
t.Errorf("repo[2] unexpected: %#v", repos[2])
}
}
func TestBuildUserReposGHArgs_LimitClamping(t *testing.T) {
// Mirrors the PAT client test: ListUserRepos must clamp before calling
// buildUserReposGHArgs, so verify a full round-trip via clampRepoSearchLimit.
cases := []struct {
name string
inLimit int
wantPerPage string
}{
{"zero defaults to 20", 0, "per_page=20"},
{"negative defaults to 20", -5, "per_page=20"},
{"in range passes through", 42, "per_page=42"},
{"exceeds cap clamps to 100", 500, "per_page=100"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
args := buildUserReposGHArgs("alice", "", clampRepoSearchLimit(tc.inLimit))
gotP := ""
for i := 0; i < len(args)-1; i++ {
if args[i] == "-f" && strings.HasPrefix(args[i+1], "per_page=") {
gotP = args[i+1]
}
}
if gotP != tc.wantPerPage {
t.Errorf("per_page flag = %q, want %q", gotP, tc.wantPerPage)
}
})
}
}
// Regression: a 429 on `gh api repos/...` (REST) must mark Core, not GraphQL.
// Previously this would pause the GraphQL PR monitor incorrectly.
func TestGHClient_InspectRateStderr_RestEndpointMarksCore(t *testing.T) {
@@ -403,3 +537,95 @@ func TestGHClient_InspectRateStderr_RestEndpointMarksCore(t *testing.T) {
t.Errorf("REST 429 must not pause graphql bucket")
}
}
func TestBuildAccessibleReposGHArgs(t *testing.T) {
cases := []struct {
name string
inLimit int
wantPath string
}{
{"in range", 50, "/user/repos?affiliation=owner,collaborator,organization_member&sort=pushed&per_page=50"},
{"cap clamps to 100", 100, "/user/repos?affiliation=owner,collaborator,organization_member&sort=pushed&per_page=100"},
{"small page", 5, "/user/repos?affiliation=owner,collaborator,organization_member&sort=pushed&per_page=5"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
args := buildAccessibleReposGHArgs(clampRepoSearchLimit(tc.inLimit))
if len(args) != 2 || args[0] != "api" {
t.Fatalf("args = %v, want [api <path>]", args)
}
if args[1] != tc.wantPath {
t.Errorf("path = %q, want %q", args[1], tc.wantPath)
}
// Must NOT include --paginate: the picker wants a single page.
for _, a := range args {
if a == "--paginate" {
t.Errorf("args must not contain --paginate, got %v", args)
}
}
})
}
}
func TestBuildAccessibleReposGHArgs_ClampsPerPage(t *testing.T) {
cases := []struct {
name string
inLimit int
wantPerPage string
}{
{"zero defaults to 20", 0, "per_page=20"},
{"negative defaults to 20", -5, "per_page=20"},
{"exceeds cap clamps to 100", 500, "per_page=100"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
args := buildAccessibleReposGHArgs(clampRepoSearchLimit(tc.inLimit))
if !strings.Contains(args[1], tc.wantPerPage) {
t.Errorf("path %q must contain %q", args[1], tc.wantPerPage)
}
})
}
}
// TestParseGHSearchRepos_FlatArray confirms the flat /user/repos array shape
// (used by ListAccessibleRepos) decodes correctly: happy path, empty, and a
// null description mapping to empty string.
func TestParseGHSearchRepos_FlatArray(t *testing.T) {
t.Run("happy", func(t *testing.T) {
data := `[
{"full_name":"kdlbs/kandev","owner":{"login":"kdlbs"},"name":"kandev","private":false,"default_branch":"main","description":"the app","pushed_at":"2025-05-01T10:00:00Z"}
]`
repos, err := parseGHSearchRepos(data)
if err != nil {
t.Fatalf("parse: %v", err)
}
if len(repos) != 1 || repos[0].FullName != "kdlbs/kandev" || repos[0].Owner != "kdlbs" {
t.Fatalf("unexpected repos: %#v", repos)
}
if repos[0].PushedAt == nil {
t.Errorf("PushedAt nil, want non-nil")
}
})
t.Run("empty", func(t *testing.T) {
repos, err := parseGHSearchRepos(`[]`)
if err != nil {
t.Fatalf("parse: %v", err)
}
if len(repos) != 0 {
t.Errorf("len = %d, want 0", len(repos))
}
})
t.Run("null description", func(t *testing.T) {
data := `[{"full_name":"o/r","owner":{"login":"o"},"name":"r","private":true,"default_branch":"main","description":null}]`
repos, err := parseGHSearchRepos(data)
if err != nil {
t.Fatalf("parse: %v", err)
}
if repos[0].Description != "" {
t.Errorf("description = %q, want empty", repos[0].Description)
}
if repos[0].PushedAt != nil {
t.Errorf("PushedAt = %v, want nil", repos[0].PushedAt)
}
})
}
+1 -1
View File
@@ -31,7 +31,7 @@ func RegisterMockRoutes(router *gin.Engine, svc *Service, log *logger.Logger) {
if !ok {
return
}
ctrl := NewMockController(mock, svc.TestStore(), svc.TestEventBus(), log)
ctrl := NewMockController(mock, svc.TestStore(), svc.TestEventBus(), svc, log)
ctrl.RegisterRoutes(router)
log.Info("registered GitHub mock control endpoints")
}
+79 -5
View File
@@ -58,10 +58,17 @@ type repoKey struct {
// MockClient implements Client with in-memory configurable data for E2E testing.
// All data is protected by a sync.RWMutex for thread safety.
type MockClient struct {
mu sync.RWMutex
user string
authenticated bool
authError string
mu sync.RWMutex
user string
authenticated bool
authError string
// reposUnavailable, when true, makes the org/user listing methods that
// back ListAccessibleRepos return ErrNoClient — driving the
// `/api/v1/github/repos` handler to respond with 503
// `github_not_configured`. Used by e2e tests that need to verify the
// "Connect GitHub" banner in the Remote-tab chip popover without ripping
// the whole mock client out of the wiring.
reposUnavailable bool
prs map[prKey]*PR
prsByBranch map[branchKey]*PR
orgs []GitHubOrg
@@ -235,6 +242,9 @@ func (m *MockClient) GetIssueState(context.Context, string, string, int) (string
func (m *MockClient) ListUserOrgs(context.Context) ([]GitHubOrg, error) {
m.mu.RLock()
defer m.mu.RUnlock()
if m.reposUnavailable {
return nil, ErrNoClient
}
if m.orgs == nil {
return []GitHubOrg{}, nil
}
@@ -244,6 +254,9 @@ func (m *MockClient) ListUserOrgs(context.Context) ([]GitHubOrg, error) {
func (m *MockClient) SearchOrgRepos(_ context.Context, org, query string, _ int) ([]GitHubRepo, error) {
m.mu.RLock()
defer m.mu.RUnlock()
if m.reposUnavailable {
return nil, ErrNoClient
}
repos := m.repos[org]
if query == "" {
return repos, nil
@@ -257,6 +270,54 @@ func (m *MockClient) SearchOrgRepos(_ context.Context, org, query string, _ int)
return filtered, nil
}
// ListUserRepos returns repos seeded for the authenticated user via AddRepos,
// keyed by the current user login. The query parameter is matched
// case-insensitively against the repo full_name; an empty query returns
// every repo for the user.
func (m *MockClient) ListUserRepos(_ context.Context, query string, _ int) ([]GitHubRepo, error) {
m.mu.RLock()
defer m.mu.RUnlock()
if m.reposUnavailable {
return nil, ErrNoClient
}
repos := m.repos[m.user]
if query == "" {
return repos, nil
}
var filtered []GitHubRepo
for _, r := range repos {
if strings.Contains(strings.ToLower(r.FullName), strings.ToLower(query)) {
filtered = append(filtered, r)
}
}
return filtered, nil
}
// ListAccessibleRepos returns the union of every seeded repo (the user's own
// repos plus every org's repos), deduped by full_name, applying the same
// case-insensitive full_name substring filter the real clients use. Honours the
// reposUnavailable toggle by returning ErrNoClient so the 503/banner e2e path
// still works. Mirrors the single GET /user/repos call the real clients make.
func (m *MockClient) ListAccessibleRepos(_ context.Context, query string, _ int) ([]GitHubRepo, error) {
m.mu.RLock()
defer m.mu.RUnlock()
if m.reposUnavailable {
return nil, ErrNoClient
}
seen := make(map[string]struct{})
var all []GitHubRepo
for _, repos := range m.repos {
for _, r := range repos {
if _, ok := seen[r.FullName]; ok {
continue
}
seen[r.FullName] = struct{}{}
all = append(all, r)
}
}
return filterReposByQuery(all, query), nil
}
func (m *MockClient) ListPRReviews(_ context.Context, owner, repo string, number int) ([]PRReview, error) {
m.mu.RLock()
defer m.mu.RUnlock()
@@ -442,6 +503,16 @@ func (m *MockClient) SetAuthHealth(authenticated bool, authError string) {
m.authError = authError
}
// SetReposUnavailable toggles whether the org/user repo-listing methods that
// back ListAccessibleRepos return ErrNoClient. Used by e2e tests to drive
// the `/api/v1/github/repos` handler into its 503 `github_not_configured`
// branch without rewiring the mock client out of the factory.
func (m *MockClient) SetReposUnavailable(unavailable bool) {
m.mu.Lock()
defer m.mu.Unlock()
m.reposUnavailable = unavailable
}
// AddPR adds a PR to the mock data store, indexed by owner/repo/number and branch.
func (m *MockClient) AddPR(pr *PR) {
m.mu.Lock()
@@ -468,7 +539,9 @@ func (m *MockClient) AddBranches(owner, repo string, branches []RepoBranch) {
m.branches[repoKey{owner, repo}] = cp
}
// AddRepos appends repositories for an organization.
// AddRepos adds repos under a key (an org login OR the authenticated user's
// login). The mock's ListUserRepos reads m.repos[m.user], so the same store
// backs both SearchOrgRepos and ListUserRepos in tests.
func (m *MockClient) AddRepos(org string, repos []GitHubRepo) {
m.mu.Lock()
defer m.mu.Unlock()
@@ -550,6 +623,7 @@ func (m *MockClient) Reset() {
m.user = mockDefaultUser
m.authenticated = true
m.authError = ""
m.reposUnavailable = false
m.prs = make(map[prKey]*PR)
m.prsByBranch = make(map[branchKey]*PR)
m.orgs = nil
@@ -22,11 +22,12 @@ type MockController struct {
store *Store
eventBus bus.EventBus
logger *logger.Logger
service *Service
}
// NewMockController creates a new MockController.
func NewMockController(mock *MockClient, store *Store, eventBus bus.EventBus, log *logger.Logger) *MockController {
return &MockController{mock: mock, store: store, eventBus: eventBus, logger: log}
func NewMockController(mock *MockClient, store *Store, eventBus bus.EventBus, svc *Service, log *logger.Logger) *MockController {
return &MockController{mock: mock, store: store, eventBus: eventBus, service: svc, logger: log}
}
// RegisterRoutes registers all mock control HTTP routes.
@@ -45,9 +46,45 @@ func (c *MockController) RegisterRoutes(router *gin.Engine) {
api.POST("/task-prs", c.associateTaskPR)
api.POST("/pr-feedback", c.seedPRFeedback)
api.PUT("/auth-health", c.setAuthHealth)
api.PUT("/repos-unavailable", c.setReposUnavailable)
api.DELETE("/reset", c.reset)
}
// setReposUnavailable toggles the mock client's "list accessible repos
// unavailable" branch so e2e tests can drive the Remote-tab chip popover
// into its "Connect GitHub" banner state. Also clears the service's
// accessible-repos / user-orgs TTL caches so a previously-cached success
// doesn't shadow the toggle for up to 60s.
func (c *MockController) setReposUnavailable(ctx *gin.Context) {
var req struct {
Unavailable bool `json:"unavailable"`
}
if err := ctx.ShouldBindJSON(&req); err != nil {
respondInvalidPayload(ctx)
return
}
c.mock.SetReposUnavailable(req.Unavailable)
if c.service != nil {
c.service.ClearAccessibleReposCaches()
}
ctx.JSON(http.StatusOK, gin.H{"unavailable": req.Unavailable})
}
// errKey is the JSON key used for error responses in this controller. Pulled
// into a constant so goconst (min-occurrences=3 on new code) stops flagging
// every new gin.H{"error":...} the file grows. Mirrors `commitStatusError`
// in scope-of-meaning but is distinct because that constant means a GitHub
// commit-status state, not a generic JSON error key.
const errKey = "error"
// respondInvalidPayload is a tiny helper so callers in this file (and any
// future mock handlers added here) don't each spell out the gin.H{<errKey>:
// "invalid payload"} pair. Reuses the package-level errMsgInvalidPayload
// constant from handlers.go to keep wording (and goconst) in sync.
func respondInvalidPayload(ctx *gin.Context) {
ctx.JSON(http.StatusBadRequest, gin.H{errKey: errMsgInvalidPayload})
}
func (c *MockController) setUser(ctx *gin.Context) {
var req struct {
Username string `json:"username"`
@@ -391,5 +428,8 @@ func mockHeadSHA(owner, repo string, prNumber int) string {
func (c *MockController) reset(ctx *gin.Context) {
c.mock.Reset()
if c.service != nil {
c.service.ClearAccessibleReposCaches()
}
ctx.JSON(http.StatusOK, gin.H{"reset": true})
}
+16 -4
View File
@@ -284,11 +284,23 @@ type GitHubOrg struct {
}
// GitHubRepo represents a GitHub repository (lightweight, for autocomplete).
// PushedAt is the timestamp of the latest push, used to sort the
// list-accessible-repos result (most-recently-active first). It is a pointer
// so `omitempty` actually drops it from JSON when unset — a zero `time.Time`
// struct would otherwise serialize as `"0001-01-01T00:00:00Z"`.
//
// DefaultBranch lets the Remote picker pre-fill the row's branch immediately
// on selection (skips the branch-list round-trip for the common case). The
// GitHub API returns it on every repo, so it is required. Description is
// omitempty because GitHub may return null/empty.
type GitHubRepo struct {
FullName string `json:"full_name"`
Owner string `json:"owner"`
Name string `json:"name"`
Private bool `json:"private"`
FullName string `json:"full_name"`
Owner string `json:"owner"`
Name string `json:"name"`
Private bool `json:"private"`
DefaultBranch string `json:"default_branch"`
Description string `json:"description,omitempty"`
PushedAt *time.Time `json:"pushed_at,omitempty"`
}
// RepoBranch represents a branch in a GitHub repository.
@@ -45,6 +45,14 @@ func (c *NoopClient) SearchOrgRepos(context.Context, string, string, int) ([]Git
return nil, ErrNoClient
}
func (c *NoopClient) ListUserRepos(context.Context, string, int) ([]GitHubRepo, error) {
return nil, ErrNoClient
}
func (c *NoopClient) ListAccessibleRepos(context.Context, string, int) ([]GitHubRepo, error) {
return nil, ErrNoClient
}
func (c *NoopClient) ListPRReviews(context.Context, string, string, int) ([]PRReview, error) {
return nil, ErrNoClient
}
+141 -10
View File
@@ -19,6 +19,13 @@ const (
githubAPIVersion = "2022-11-28"
)
// accessibleReposAffiliation is the GitHub /user/repos affiliation filter that
// returns every repo the authenticated user can reach in one call: their own
// repos (owner), repos they collaborate on, and repos in orgs they belong to.
// This replaces the per-org search/repositories fan-out (which burns the 30/min
// search quota) with a single call on the 5000/min core quota.
const accessibleReposAffiliation = "owner,collaborator,organization_member"
// GitHubAPIError represents an error response from the GitHub API with a status code.
type GitHubAPIError struct {
StatusCode int
@@ -296,35 +303,159 @@ func (c *PATClient) SearchOrgRepos(ctx context.Context, org, query string, limit
if query != "" {
q += " " + query
}
if limit <= 0 {
limit = 20
limit = clampRepoSearchLimit(limit)
endpoint := fmt.Sprintf("/search/repositories?q=%s&per_page=%d", url.QueryEscape(q), limit)
repos, err := c.fetchRepoSearch(ctx, endpoint)
if err != nil {
return nil, fmt.Errorf("search org repos: %w", err)
}
return repos, nil
}
func (c *PATClient) ListUserRepos(ctx context.Context, query string, limit int) ([]GitHubRepo, error) {
user, err := c.GetAuthenticatedUser(ctx)
if err != nil {
return nil, fmt.Errorf("list user repos: %w", err)
}
q := "user:" + user
if query != "" {
q += " " + query
}
limit = clampRepoSearchLimit(limit)
endpoint := fmt.Sprintf("/search/repositories?q=%s&per_page=%d", url.QueryEscape(q), limit)
repos, err := c.fetchRepoSearch(ctx, endpoint)
if err != nil {
return nil, fmt.Errorf("list user repos: %w", err)
}
return repos, nil
}
// ListAccessibleRepos lists every repo the authenticated user can access via a
// single GET /user/repos call on the core REST quota. The response is a flat
// JSON array (not a search wrapper), so it decodes differently from
// fetchRepoSearch.
//
// query is a BEST-EFFORT substring filter applied only over the first `limit`
// repos returned by this single (un-paginated) page — a query matching a repo
// beyond the cap returns nothing here. This is intentional: the picker fetches
// limit=100 and the frontend performs the canonical, comprehensive client-side
// filtering over that page, so the server filter is just an optional narrowing.
// Do NOT rely on it for completeness, and do NOT add pagination to "fix" it
// without revisiting the picker contract.
func (c *PATClient) ListAccessibleRepos(ctx context.Context, query string, limit int) ([]GitHubRepo, error) {
limit = clampRepoSearchLimit(limit)
endpoint := fmt.Sprintf("/user/repos?affiliation=%s&sort=pushed&per_page=%d",
url.QueryEscape(accessibleReposAffiliation), limit)
var items []repoListItem
if err := c.get(ctx, endpoint, &items); err != nil {
return nil, fmt.Errorf("list accessible repos: %w", err)
}
return filterReposByQuery(convertRepoListItems(items), query), nil
}
// repoListItem is the per-repo JSON shape returned by the flat-array endpoints
// (GET /user/repos). The search endpoints wrap the same fields under `.items`.
type repoListItem struct {
FullName string `json:"full_name"`
Owner struct {
Login string `json:"login"`
} `json:"owner"`
Name string `json:"name"`
Private bool `json:"private"`
DefaultBranch string `json:"default_branch"`
Description string `json:"description"`
PushedAt time.Time `json:"pushed_at"`
}
// convertRepoListItems maps the raw flat-array items into the lightweight
// GitHubRepo shape, preserving PushedAt as a pointer so callers can sort by
// recency (a zero pushed_at becomes nil).
func convertRepoListItems(items []repoListItem) []GitHubRepo {
repos := make([]GitHubRepo, len(items))
for i, item := range items {
repos[i] = GitHubRepo{
FullName: item.FullName,
Owner: item.Owner.Login,
Name: item.Name,
Private: item.Private,
DefaultBranch: item.DefaultBranch,
Description: item.Description,
}
if !item.PushedAt.IsZero() {
t := item.PushedAt
repos[i].PushedAt = &t
}
}
return repos
}
// filterReposByQuery returns the repos whose full_name contains query
// (case-insensitive). An empty query returns the input unchanged. Best-effort:
// it filters only the already-fetched (capped) slice — see ListAccessibleRepos.
func filterReposByQuery(repos []GitHubRepo, query string) []GitHubRepo {
if query == "" {
return repos
}
needle := strings.ToLower(query)
filtered := make([]GitHubRepo, 0, len(repos))
for _, r := range repos {
if strings.Contains(strings.ToLower(r.FullName), needle) {
filtered = append(filtered, r)
}
}
return filtered
}
// fetchRepoSearch executes a /search/repositories request and decodes the
// items into the lightweight GitHubRepo shape used for autocomplete and the
// list-accessible-repos endpoint. PushedAt is captured so callers can sort
// merged results by recency.
func (c *PATClient) fetchRepoSearch(ctx context.Context, endpoint string) ([]GitHubRepo, error) {
var result struct {
Items []struct {
FullName string `json:"full_name"`
Owner struct {
Login string `json:"login"`
} `json:"owner"`
Name string `json:"name"`
Private bool `json:"private"`
Name string `json:"name"`
Private bool `json:"private"`
DefaultBranch string `json:"default_branch"`
Description string `json:"description"`
PushedAt time.Time `json:"pushed_at"`
} `json:"items"`
}
endpoint := fmt.Sprintf("/search/repositories?q=%s&per_page=%d", url.QueryEscape(q), limit)
if err := c.get(ctx, endpoint, &result); err != nil {
return nil, fmt.Errorf("search org repos: %w", err)
return nil, err
}
repos := make([]GitHubRepo, len(result.Items))
for i, item := range result.Items {
repos[i] = GitHubRepo{
FullName: item.FullName,
Owner: item.Owner.Login,
Name: item.Name,
Private: item.Private,
FullName: item.FullName,
Owner: item.Owner.Login,
Name: item.Name,
Private: item.Private,
DefaultBranch: item.DefaultBranch,
Description: item.Description,
}
if !item.PushedAt.IsZero() {
t := item.PushedAt
repos[i].PushedAt = &t
}
}
return repos, nil
}
// clampRepoSearchLimit applies the default and GitHub's per_page=100 cap.
func clampRepoSearchLimit(limit int) int {
if limit <= 0 {
return 20
}
if limit > 100 {
return 100
}
return limit
}
func (c *PATClient) ListPRReviews(ctx context.Context, owner, repo string, number int) ([]PRReview, error) {
var raw []struct {
ID int64 `json:"id"`
@@ -2,6 +2,7 @@ package github
import (
"context"
"errors"
"net/http"
"net/http/httptest"
"strconv"
@@ -371,6 +372,198 @@ func TestPATClient_FetchBranchProtection(t *testing.T) {
}
}
func TestListUserRepos_Success(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/user":
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`{"login":"octocat"}`))
case "/search/repositories":
q := r.URL.Query().Get("q")
if !strings.Contains(q, "user:octocat") {
t.Errorf("q = %q, want to contain user:octocat", q)
}
if !strings.Contains(q, "demo") {
t.Errorf("q = %q, want to contain demo", q)
}
if got := r.URL.Query().Get("per_page"); got != "50" {
t.Errorf("per_page = %q, want 50", got)
}
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`{"items":[
{"full_name":"octocat/demo","owner":{"login":"octocat"},"name":"demo","private":false,"default_branch":"main","description":"Public demo"},
{"full_name":"octocat/demo-private","owner":{"login":"octocat"},"name":"demo-private","private":true,"default_branch":"trunk","description":null}
]}`))
default:
t.Fatalf("unexpected path %q", r.URL.Path)
}
}))
t.Cleanup(srv.Close)
c := newPATClientPointingAt(t, srv.URL)
repos, err := c.ListUserRepos(context.Background(), "demo", 50)
if err != nil {
t.Fatalf("ListUserRepos: %v", err)
}
if len(repos) != 2 {
t.Fatalf("repos = %d, want 2", len(repos))
}
if repos[0].FullName != "octocat/demo" || repos[0].Owner != "octocat" || repos[0].Name != "demo" || repos[0].Private {
t.Errorf("unexpected first repo: %#v", repos[0])
}
if repos[0].DefaultBranch != "main" {
t.Errorf("first repo default_branch = %q, want main", repos[0].DefaultBranch)
}
if repos[0].Description != "Public demo" {
t.Errorf("first repo description = %q, want Public demo", repos[0].Description)
}
if !repos[1].Private {
t.Errorf("expected second repo private")
}
if repos[1].DefaultBranch != "trunk" {
t.Errorf("second repo default_branch = %q, want trunk", repos[1].DefaultBranch)
}
// JSON `null` description must decode to an empty string (omitempty drops it on serialize).
if repos[1].Description != "" {
t.Errorf("second repo description = %q, want empty string", repos[1].Description)
}
}
func TestListUserRepos_EmptyQuery(t *testing.T) {
var gotQuery string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/user":
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`{"login":"alice"}`))
case "/search/repositories":
gotQuery = r.URL.Query().Get("q")
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`{"items":[]}`))
default:
t.Fatalf("unexpected path %q", r.URL.Path)
}
}))
t.Cleanup(srv.Close)
c := newPATClientPointingAt(t, srv.URL)
repos, err := c.ListUserRepos(context.Background(), "", 0)
if err != nil {
t.Fatalf("ListUserRepos: %v", err)
}
if len(repos) != 0 {
t.Fatalf("repos = %d, want 0", len(repos))
}
if gotQuery != "user:alice" {
t.Errorf("q = %q, want exactly %q", gotQuery, "user:alice")
}
}
func TestListUserRepos_LimitClamping(t *testing.T) {
cases := []struct {
name string
inLimit int
wantPerPage string
}{
{"zero defaults to 20", 0, "20"},
{"negative defaults to 20", -5, "20"},
{"in range passes through", 42, "42"},
{"exceeds cap clamps to 100", 500, "100"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
var gotPerPage string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/user":
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`{"login":"alice"}`))
case "/search/repositories":
gotPerPage = r.URL.Query().Get("per_page")
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`{"items":[]}`))
default:
t.Fatalf("unexpected path %q", r.URL.Path)
}
}))
t.Cleanup(srv.Close)
c := newPATClientPointingAt(t, srv.URL)
if _, err := c.ListUserRepos(context.Background(), "", tc.inLimit); err != nil {
t.Fatalf("ListUserRepos: %v", err)
}
if gotPerPage != tc.wantPerPage {
t.Errorf("per_page = %q, want %q", gotPerPage, tc.wantPerPage)
}
})
}
}
func TestListUserRepos_APIError(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/user":
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`{"login":"alice"}`))
case "/search/repositories":
w.WriteHeader(http.StatusInternalServerError)
_, _ = w.Write([]byte(`{"message":"boom"}`))
default:
t.Fatalf("unexpected path %q", r.URL.Path)
}
}))
t.Cleanup(srv.Close)
c := newPATClientPointingAt(t, srv.URL)
repos, err := c.ListUserRepos(context.Background(), "", 0)
if err == nil {
t.Fatal("expected error from 500, got nil")
}
if repos != nil {
t.Errorf("expected nil repos on error, got %v", repos)
}
}
func TestListUserRepos_AuthError(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/user" {
t.Fatalf("unexpected path %q", r.URL.Path)
}
w.WriteHeader(http.StatusUnauthorized)
_, _ = w.Write([]byte(`{"message":"bad creds"}`))
}))
t.Cleanup(srv.Close)
c := newPATClientPointingAt(t, srv.URL)
repos, err := c.ListUserRepos(context.Background(), "", 0)
if err == nil {
t.Fatal("expected error when /user returns 401")
}
if repos != nil {
t.Errorf("expected nil repos on auth error, got %v", repos)
}
}
func TestClampRepoSearchLimit(t *testing.T) {
cases := []struct {
name string
in int
want int
}{
{"zero", 0, 20},
{"negative", -1, 20},
{"small", 10, 10},
{"exact cap", 100, 100},
{"over cap", 1000, 100},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
if got := clampRepoSearchLimit(tc.in); got != tc.want {
t.Errorf("clampRepoSearchLimit(%d) = %d, want %d", tc.in, got, tc.want)
}
})
}
}
// newPATClientPointingAt builds a PATClient whose underlying HTTP client
// reroutes any github API URL to the given test server.
func newPATClientPointingAt(t *testing.T, baseURL string) *PATClient {
@@ -394,3 +587,116 @@ func (rt *rewriteTransport) RoundTrip(req *http.Request) (*http.Response, error)
req2.Header = req.Header.Clone()
return http.DefaultTransport.RoundTrip(req2)
}
func TestListAccessibleRepos_Success(t *testing.T) {
var gotAffiliation, gotSort, gotPerPage string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/user/repos" {
// t.Fatalf must not run in a handler goroutine (it's a data race);
// use Errorf+return and let the main goroutine fail the assertion.
t.Errorf("unexpected path %q", r.URL.Path)
http.Error(w, "unexpected path", http.StatusInternalServerError)
return
}
gotAffiliation = r.URL.Query().Get("affiliation")
gotSort = r.URL.Query().Get("sort")
gotPerPage = r.URL.Query().Get("per_page")
w.WriteHeader(http.StatusOK)
// Flat array (NOT a search wrapper with .items).
_, _ = w.Write([]byte(`[
{"full_name":"kdlbs/kandev","owner":{"login":"kdlbs"},"name":"kandev","private":false,"default_branch":"main","description":"the app","pushed_at":"2025-05-01T10:00:00Z"},
{"full_name":"alice/secret","owner":{"login":"alice"},"name":"secret","private":true,"default_branch":"trunk","description":null}
]`))
}))
t.Cleanup(srv.Close)
c := newPATClientPointingAt(t, srv.URL)
repos, err := c.ListAccessibleRepos(context.Background(), "", 50)
if err != nil {
t.Fatalf("ListAccessibleRepos: %v", err)
}
if gotAffiliation != "owner,collaborator,organization_member" {
t.Errorf("affiliation = %q, want owner,collaborator,organization_member", gotAffiliation)
}
if gotSort != "pushed" {
t.Errorf("sort = %q, want pushed", gotSort)
}
if gotPerPage != "50" {
t.Errorf("per_page = %q, want 50", gotPerPage)
}
if len(repos) != 2 {
t.Fatalf("repos = %d, want 2", len(repos))
}
if repos[0].FullName != "kdlbs/kandev" || repos[0].Owner != "kdlbs" || repos[0].DefaultBranch != "main" {
t.Errorf("unexpected first repo: %#v", repos[0])
}
if repos[0].Description != "the app" {
t.Errorf("first repo description = %q, want 'the app'", repos[0].Description)
}
if repos[0].PushedAt == nil {
t.Errorf("first repo PushedAt nil, want non-nil")
}
if !repos[1].Private || repos[1].DefaultBranch != "trunk" {
t.Errorf("unexpected second repo: %#v", repos[1])
}
// JSON null description must decode to empty string.
if repos[1].Description != "" {
t.Errorf("second repo description = %q, want empty", repos[1].Description)
}
}
func TestListAccessibleRepos_QueryFilterAndClamp(t *testing.T) {
var gotPerPage string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/user/repos" {
t.Errorf("unexpected path %q", r.URL.Path)
http.Error(w, "unexpected path", http.StatusInternalServerError)
return
}
gotPerPage = r.URL.Query().Get("per_page")
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`[
{"full_name":"acme/widget","owner":{"login":"acme"},"name":"widget"},
{"full_name":"acme/gadget","owner":{"login":"acme"},"name":"gadget"},
{"full_name":"u/other","owner":{"login":"u"},"name":"other"}
]`))
}))
t.Cleanup(srv.Close)
c := newPATClientPointingAt(t, srv.URL)
// limit above cap must clamp per_page to 100; query filters client-side.
repos, err := c.ListAccessibleRepos(context.Background(), "WIDGET", 5000)
if err != nil {
t.Fatalf("ListAccessibleRepos: %v", err)
}
if gotPerPage != "100" {
t.Errorf("per_page = %q, want 100 (clamped)", gotPerPage)
}
if len(repos) != 1 || repos[0].FullName != "acme/widget" {
t.Fatalf("got %v, want [acme/widget] (case-insensitive substring on full_name)", repos)
}
}
func TestListAccessibleRepos_AuthError(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/user/repos" {
t.Errorf("unexpected path %q", r.URL.Path)
}
w.WriteHeader(http.StatusUnauthorized)
_, _ = w.Write([]byte(`{"message":"bad creds"}`))
}))
t.Cleanup(srv.Close)
c := newPATClientPointingAt(t, srv.URL)
repos, err := c.ListAccessibleRepos(context.Background(), "", 0)
if err == nil {
t.Fatal("expected error on 401")
}
if repos != nil {
t.Errorf("expected nil repos on error, got %v", repos)
}
var apiErr *GitHubAPIError
if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusUnauthorized {
t.Errorf("err = %v, want *GitHubAPIError with 401", err)
}
}
+19 -17
View File
@@ -70,23 +70,24 @@ type SecretManager interface {
// Service coordinates GitHub integration operations.
type Service struct {
mu sync.Mutex
client Client
authMethod string
secrets SecretProvider
secretManager SecretManager
store *Store
eventBus bus.EventBus
logger *logger.Logger
taskDeleter TaskDeleter
taskSessionChecker TaskSessionChecker
syncGroup singleflight.Group
taskEventSubs []bus.Subscription
searchCache *ttlCache
prStatusCache *ttlCache
mergeMethodsCache *ttlCache
protectionCache *branchProtectionCache
rateTracker *RateTracker
mu sync.Mutex
client Client
authMethod string
secrets SecretProvider
secretManager SecretManager
store *Store
eventBus bus.EventBus
logger *logger.Logger
taskDeleter TaskDeleter
taskSessionChecker TaskSessionChecker
syncGroup singleflight.Group
taskEventSubs []bus.Subscription
searchCache *ttlCache
prStatusCache *ttlCache
mergeMethodsCache *ttlCache
accessibleReposCache *ttlCache
protectionCache *branchProtectionCache
rateTracker *RateTracker
// cleanupFailureMu guards cleanupFailureCounts; the cleanup loop is the
// only writer but the global sweep + per-watch sweep can run concurrently
@@ -107,6 +108,7 @@ func NewService(client Client, authMethod string, secrets SecretProvider, store
searchCache: newTTLCache(),
prStatusCache: newTTLCache(),
mergeMethodsCache: newMergeMethodsCache(),
accessibleReposCache: newAccessibleReposCache(),
protectionCache: newBranchProtectionCache(),
rateTracker: NewRateTracker(eventBus, log),
cleanupFailureCounts: make(map[string]int),
@@ -0,0 +1,129 @@
package github
import (
"context"
"fmt"
"sort"
"time"
)
// Repo-list defaults for the list-accessible-repos endpoint.
//
// The default of 50 mirrors the autocomplete pickers in the web UI; the cap
// of 100 matches GitHub's per_page maximum and the frontend picker's own cap.
const (
defaultAccessibleReposLimit = 50
maxAccessibleReposLimit = 100
)
// accessibleReposFetchTimeout bounds the single GET /user/repos fetch. It
// matches the gh CLI's default 30s timeout. The fetch runs under a detached
// context (see ListAccessibleRepos) so this deadline — not a caller's request
// cancellation — is what stops the work.
const accessibleReposFetchTimeout = 30 * time.Second
// clampAccessibleReposLimit normalises caller-supplied limit values: <=0 falls
// back to the default, anything above the cap is clamped down. Mirrors the
// existing clampRepoSearchLimit shape so callers see consistent behaviour
// between this endpoint and the autocomplete /repos/search endpoint.
func clampAccessibleReposLimit(limit int) int {
if limit <= 0 {
return defaultAccessibleReposLimit
}
if limit > maxAccessibleReposLimit {
return maxAccessibleReposLimit
}
return limit
}
// ListAccessibleRepos returns the repos the authenticated user can access —
// their own repos plus collaborator and org-member repos — via a single
// GET /user/repos call on GitHub's core REST quota (5000/min). Results are
// sorted by most-recently-pushed first and returned as a non-nil (possibly
// empty) slice when err == nil.
//
// The result is cached per (query, limit) for 60s so picker re-renders and
// typeahead bursts don't hit GitHub on every keystroke. Returns ErrNoClient
// (untouched) when GitHub is not configured / not authenticated; the HTTP
// handler maps that to 503 with the `github_not_configured` code. Any other
// error is returned to the caller and NOT cached, so the next call retries.
//
// The cached fetch runs under a context detached from the caller's request
// (context.WithoutCancel). This is the core correctness fix: the picker's
// frontend aborts in-flight requests via an AbortController, and a `gh`
// subprocess started under the request context would be SIGKILLed on that
// abort — surfacing as `signal: killed` to every coalesced caller. Detaching
// lets the fetch outlive any single request; only the fetch timeout stops it.
func (s *Service) ListAccessibleRepos(ctx context.Context, query string, limit int) ([]GitHubRepo, error) {
if s.client == nil {
return nil, ErrNoClient
}
limit = clampAccessibleReposLimit(limit)
key := accessibleReposCacheKey(query, limit)
if v, ok := s.accessibleReposCache.get(key); ok {
return v.([]GitHubRepo), nil
}
// Snapshot the cache generation BEFORE the fetch. If a clear() runs while
// this fetch is in flight (token swap via ConfigureToken/ClearToken, or the
// e2e mock toggle), the generation bumps and the post-fetch write is
// dropped — otherwise a fetch that started under the previous user/token
// could write that user's stale repos back into the just-cleared cache.
gen := s.accessibleReposCache.generation()
// Detach from the caller's request context so a client-side abort can't
// cancel (and SIGKILL) the shared fetch; bound it with our own timeout.
fetchCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), accessibleReposFetchTimeout)
defer cancel()
repos, err := s.client.ListAccessibleRepos(fetchCtx, query, limit)
if err != nil {
return nil, err
}
sortReposByPushedAt(repos)
if len(repos) > limit {
repos = repos[:limit]
}
s.accessibleReposCache.setIfCurrentGeneration(key, repos, gen)
return repos, nil
}
// accessibleReposCacheKey composes a cache key with length-prefixed string
// fields so a user-supplied query containing the separator can't collide with
// another key.
func accessibleReposCacheKey(query string, limit int) string {
return fmt.Sprintf("%d:%s|%d", len(query), query, limit)
}
// ClearAccessibleReposCaches drops every cached entry from the accessible-repos
// cache. Used by the e2e mock controller so flipping the mock's "repos
// unavailable" toggle takes effect immediately instead of waiting for the 60s
// TTL on a prior cached success to expire, and by ConfigureToken / ClearToken
// so an auth change invalidates the user-scoped cache synchronously.
//
// Nil-guards the cache because tests construct Service literals without going
// through NewService (so the cache stays nil).
func (s *Service) ClearAccessibleReposCaches() {
if s.accessibleReposCache != nil {
s.accessibleReposCache.clear()
}
}
// sortReposByPushedAt sorts repos in place by pushed_at desc, tiebroken
// alphabetically on full_name for determinism. A nil PushedAt sorts last
// (unknown timestamp is treated as "oldest"). GitHub's /user/repos already
// returns sort=pushed order, but we re-sort defensively so the contract holds
// even if the upstream order changes or the query filter reorders nothing.
func sortReposByPushedAt(repos []GitHubRepo) {
sort.SliceStable(repos, func(i, j int) bool {
switch {
case repos[i].PushedAt == nil && repos[j].PushedAt == nil:
return repos[i].FullName < repos[j].FullName
case repos[i].PushedAt == nil:
return false
case repos[j].PushedAt == nil:
return true
}
if !repos[i].PushedAt.Equal(*repos[j].PushedAt) {
return repos[i].PushedAt.After(*repos[j].PushedAt)
}
return repos[i].FullName < repos[j].FullName
})
}
@@ -0,0 +1,408 @@
package github
import (
"context"
"errors"
"strconv"
"sync/atomic"
"testing"
"time"
"github.com/kandev/kandev/internal/common/logger"
)
// accessibleReposStubClient lets each test wire its own ListAccessibleRepos
// behaviour without touching the broader stubClient (which other tests depend
// on with its current shape). It also records the context each call observed so
// the detached-context regression test can assert the fetch is not cancelled
// when the caller's request is.
type accessibleReposStubClient struct {
stubClient
repos []GitHubRepo
reposErr error
calls atomic.Int32
// lastCtxErr records ctx.Err() observed at call time. The detached-context
// test cancels the caller context before calling and asserts this stays nil
// — proving the service ran the fetch under a context that survives the
// request abort.
lastCtxErr atomic.Value // error
}
func (s *accessibleReposStubClient) ListAccessibleRepos(ctx context.Context, query string, _ int) ([]GitHubRepo, error) {
s.calls.Add(1)
s.lastCtxErr.Store(errBox{ctx.Err()})
if s.reposErr != nil {
return nil, s.reposErr
}
return filterReposByQuery(s.repos, query), nil
}
// errBox wraps an error so it can be stored in an atomic.Value even when nil.
type errBox struct{ err error }
func (s *accessibleReposStubClient) observedCtxErr() error {
v, ok := s.lastCtxErr.Load().(errBox)
if !ok {
return nil
}
return v.err
}
func newAccessibleReposTestService(client Client) *Service {
return &Service{
client: client,
authMethod: AuthMethodPAT,
logger: logger.Default(),
accessibleReposCache: newAccessibleReposCache(),
}
}
// ptrTime is a tiny helper to take the address of a time.Time literal for the
// pointer-typed GitHubRepo.PushedAt field used in these tests.
func ptrTime(t time.Time) *time.Time { return &t }
func TestListAccessibleRepos_ReturnsRepos(t *testing.T) {
t0 := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)
sc := &accessibleReposStubClient{
repos: []GitHubRepo{
{FullName: "alice/personal", Owner: "alice", Name: "personal", DefaultBranch: "main", Description: "Alice's personal repo", PushedAt: ptrTime(t0.Add(3 * time.Hour))},
{FullName: "acme/widget", Owner: "acme", Name: "widget", DefaultBranch: "trunk", Description: "Widget service", PushedAt: ptrTime(t0.Add(2 * time.Hour))},
{FullName: "globex/foo", Owner: "globex", Name: "foo", DefaultBranch: "main", PushedAt: ptrTime(t0.Add(time.Hour))},
},
}
svc := newAccessibleReposTestService(sc)
got, err := svc.ListAccessibleRepos(context.Background(), "", 50)
if err != nil {
t.Fatalf("ListAccessibleRepos: %v", err)
}
if len(got) != 3 {
t.Fatalf("len = %d, want 3", len(got))
}
wantByName := map[string]struct {
defaultBranch string
description string
}{
"alice/personal": {"main", "Alice's personal repo"},
"acme/widget": {"trunk", "Widget service"},
"globex/foo": {"main", ""},
}
for _, r := range got {
want, ok := wantByName[r.FullName]
if !ok {
t.Errorf("unexpected repo %q in result", r.FullName)
continue
}
if r.DefaultBranch != want.defaultBranch {
t.Errorf("repo %q DefaultBranch = %q, want %q", r.FullName, r.DefaultBranch, want.defaultBranch)
}
if r.Description != want.description {
t.Errorf("repo %q Description = %q, want %q", r.FullName, r.Description, want.description)
}
}
}
func TestListAccessibleRepos_SortsByPushedAt(t *testing.T) {
t0 := time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC)
sc := &accessibleReposStubClient{
repos: []GitHubRepo{
{FullName: "u/middle", Owner: "u", Name: "middle", PushedAt: ptrTime(t0.Add(2 * time.Hour))},
{FullName: "acme/oldest", Owner: "acme", Name: "oldest", PushedAt: ptrTime(t0)},
{FullName: "acme/newest", Owner: "acme", Name: "newest", PushedAt: ptrTime(t0.Add(5 * time.Hour))},
},
}
svc := newAccessibleReposTestService(sc)
got, err := svc.ListAccessibleRepos(context.Background(), "", 50)
if err != nil {
t.Fatalf("ListAccessibleRepos: %v", err)
}
wantOrder := []string{"acme/newest", "u/middle", "acme/oldest"}
if len(got) != len(wantOrder) {
t.Fatalf("len = %d, want %d", len(got), len(wantOrder))
}
for i, name := range wantOrder {
if got[i].FullName != name {
t.Errorf("pos %d = %q, want %q", i, got[i].FullName, name)
}
}
}
func TestListAccessibleRepos_QueryFilter(t *testing.T) {
sc := &accessibleReposStubClient{
repos: []GitHubRepo{
{FullName: "acme/widget", Owner: "acme", Name: "widget"},
{FullName: "acme/gadget", Owner: "acme", Name: "gadget"},
{FullName: "u/other", Owner: "u", Name: "other"},
},
}
svc := newAccessibleReposTestService(sc)
// Case-insensitive substring match on full_name.
got, err := svc.ListAccessibleRepos(context.Background(), "ACME/W", 50)
if err != nil {
t.Fatalf("ListAccessibleRepos: %v", err)
}
if len(got) != 1 || got[0].FullName != "acme/widget" {
t.Fatalf("got %v, want [acme/widget]", got)
}
}
func TestListAccessibleRepos_LimitClamp(t *testing.T) {
repos := make([]GitHubRepo, 150)
base := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
for i := range repos {
repos[i] = GitHubRepo{
FullName: "acme/repo" + strconv.Itoa(i),
Owner: "acme",
Name: "repo" + strconv.Itoa(i),
PushedAt: ptrTime(base.Add(time.Duration(i) * time.Minute)),
}
}
sc := &accessibleReposStubClient{repos: repos}
svc := newAccessibleReposTestService(sc)
got, err := svc.ListAccessibleRepos(context.Background(), "", 10000)
if err != nil {
t.Fatalf("ListAccessibleRepos: %v", err)
}
if len(got) != maxAccessibleReposLimit {
t.Errorf("len = %d, want %d (clamped to cap)", len(got), maxAccessibleReposLimit)
}
}
func TestListAccessibleRepos_DefaultLimit(t *testing.T) {
// limit=0 must resolve to the default (50); we verify by feeding 60
// distinct repos and observing the truncation to exactly 50.
repos := make([]GitHubRepo, 60)
base := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
for i := range repos {
repos[i] = GitHubRepo{
FullName: "acme/r" + strconv.Itoa(i),
Owner: "acme",
Name: "r" + strconv.Itoa(i),
PushedAt: ptrTime(base.Add(time.Duration(i) * time.Minute)),
}
}
sc := &accessibleReposStubClient{repos: repos}
svc := newAccessibleReposTestService(sc)
got, err := svc.ListAccessibleRepos(context.Background(), "", 0)
if err != nil {
t.Fatalf("ListAccessibleRepos: %v", err)
}
if len(got) != defaultAccessibleReposLimit {
t.Errorf("len = %d, want %d (default limit)", len(got), defaultAccessibleReposLimit)
}
}
func TestListAccessibleRepos_NotAuthenticated(t *testing.T) {
// nil client => ErrNoClient straight from the guard (matches the
// behaviour required by the 503-mapping handler test).
svc := newAccessibleReposTestService(nil)
got, err := svc.ListAccessibleRepos(context.Background(), "", 50)
if !errors.Is(err, ErrNoClient) {
t.Fatalf("err = %v, want ErrNoClient", err)
}
if got != nil {
t.Errorf("got = %v, want nil", got)
}
// Same sentinel must bubble up when the client itself is the noop
// (e.g. GH CLI absent and no PAT configured).
svcNoop := newAccessibleReposTestService(&NoopClient{})
gotNoop, errNoop := svcNoop.ListAccessibleRepos(context.Background(), "", 50)
if !errors.Is(errNoop, ErrNoClient) {
t.Fatalf("noop err = %v, want ErrNoClient", errNoop)
}
if gotNoop != nil {
t.Errorf("noop got = %v, want nil", gotNoop)
}
}
func TestListAccessibleRepos_UsesCache(t *testing.T) {
// Subsequent calls within the cache window must coalesce — the fetch runs
// once and later calls hit the 60s cache.
sc := &accessibleReposStubClient{
repos: []GitHubRepo{
{FullName: "u/own", Owner: "u", Name: "own"},
{FullName: "acme/widget", Owner: "acme", Name: "widget"},
},
}
svc := newAccessibleReposTestService(sc)
for i := 0; i < 3; i++ {
if _, err := svc.ListAccessibleRepos(context.Background(), "", 50); err != nil {
t.Fatalf("call %d: %v", i, err)
}
}
if got := sc.calls.Load(); got != 1 {
t.Errorf("ListAccessibleRepos called %d times, want 1 (cached)", got)
}
}
// TestListAccessibleRepos_DetachedContext is the key regression test for the
// `signal: killed` 500: the picker frontend aborts its request via an
// AbortController, which previously cancelled the shared fetch's context and
// SIGKILLed the `gh` subprocess. The service must run the fetch under a context
// detached from the caller's request, so an already-cancelled caller context
// does NOT propagate into the client call.
func TestListAccessibleRepos_DetachedContext(t *testing.T) {
sc := &accessibleReposStubClient{
repos: []GitHubRepo{{FullName: "u/own", Owner: "u", Name: "own"}},
}
svc := newAccessibleReposTestService(sc)
// Cancel the caller context BEFORE calling the service.
ctx, cancel := context.WithCancel(context.Background())
cancel()
if ctx.Err() == nil {
t.Fatal("expected caller context to be cancelled")
}
got, err := svc.ListAccessibleRepos(ctx, "", 50)
if err != nil {
t.Fatalf("ListAccessibleRepos with cancelled caller ctx: %v", err)
}
if len(got) != 1 || got[0].FullName != "u/own" {
t.Fatalf("got %v, want [u/own]", got)
}
// The fetch must have observed a NON-cancelled context.
if ctxErr := sc.observedCtxErr(); ctxErr != nil {
t.Errorf("fetch observed ctx.Err() = %v, want nil (context must be detached)", ctxErr)
}
if got := sc.calls.Load(); got != 1 {
t.Errorf("fetch called %d times, want 1", got)
}
}
// TestListAccessibleRepos_ErrorNotCached verifies a failed fetch is NOT cached:
// the next call must retry rather than serve a poisoned cache entry.
func TestListAccessibleRepos_ErrorNotCached(t *testing.T) {
sc := &accessibleReposStubClient{
reposErr: errors.New("simulated rate limit"),
}
svc := newAccessibleReposTestService(sc)
if _, err := svc.ListAccessibleRepos(context.Background(), "", 50); err == nil {
t.Fatal("first call: expected error, got nil")
}
if _, err := svc.ListAccessibleRepos(context.Background(), "", 50); err == nil {
t.Fatal("second call: expected error, got nil")
}
if got := sc.calls.Load(); got != 2 {
t.Errorf("fetch called %d times, want 2 (error must not be cached)", got)
}
}
// TestListAccessibleRepos_SuccessIsCached guards against accidentally turning
// off caching while reworking the endpoint.
func TestListAccessibleRepos_SuccessIsCached(t *testing.T) {
sc := &accessibleReposStubClient{
repos: []GitHubRepo{{FullName: "u/own", Owner: "u", Name: "own"}},
}
svc := newAccessibleReposTestService(sc)
if _, err := svc.ListAccessibleRepos(context.Background(), "", 50); err != nil {
t.Fatalf("first call: %v", err)
}
if _, err := svc.ListAccessibleRepos(context.Background(), "", 50); err != nil {
t.Fatalf("second call: %v", err)
}
if got := sc.calls.Load(); got != 1 {
t.Errorf("fetch called %d times, want 1 (second call should hit cache)", got)
}
}
// TestListAccessibleRepos_CacheClearedOnTokenChange verifies the
// accessible-repos cache is invalidated when authentication changes. The
// Service lives across token swaps (ConfigureToken / ClearToken mutate s.client
// in place rather than rebuilding the Service), so without an explicit
// invalidation a token swap would surface the previous user's repos for up to
// the 60s TTL. We exercise the invalidation path directly via
// ClearAccessibleReposCaches — both ConfigureToken and ClearToken call it.
func TestListAccessibleRepos_CacheClearedOnTokenChange(t *testing.T) {
scA := &accessibleReposStubClient{
repos: []GitHubRepo{{FullName: "user-a/repo", Owner: "user-a", Name: "repo"}},
}
svc := newAccessibleReposTestService(scA)
if _, err := svc.ListAccessibleRepos(context.Background(), "", 50); err != nil {
t.Fatalf("initial ListAccessibleRepos: %v", err)
}
if got := scA.calls.Load(); got != 1 {
t.Fatalf("user-a fetch called %d times, want 1", got)
}
// Swap the client to a "user B" stub and clear caches — this mirrors what
// ConfigureToken / ClearToken do when auth changes.
scB := &accessibleReposStubClient{
repos: []GitHubRepo{{FullName: "user-b/repo", Owner: "user-b", Name: "repo"}},
}
svc.client = scB
svc.ClearAccessibleReposCaches()
got, err := svc.ListAccessibleRepos(context.Background(), "", 50)
if err != nil {
t.Fatalf("post-swap ListAccessibleRepos: %v", err)
}
if calls := scB.calls.Load(); calls != 1 {
t.Errorf("user-b fetch called %d times, want 1 (cache miss expected)", calls)
}
names := map[string]bool{}
for _, r := range got {
names[r.FullName] = true
}
if !names["user-b/repo"] {
t.Errorf("missing user-b repo in result: %v", names)
}
if names["user-a/repo"] {
t.Errorf("user-a repo leaked across token swap: %v", names)
}
}
// blockingAccessibleReposClient blocks inside ListAccessibleRepos until the test
// releases it, letting the test race a ClearAccessibleReposCaches() with an
// in-flight fetch.
type blockingAccessibleReposClient struct {
stubClient
repos []GitHubRepo
started chan struct{}
release chan struct{}
}
func (c *blockingAccessibleReposClient) ListAccessibleRepos(_ context.Context, query string, _ int) ([]GitHubRepo, error) {
close(c.started)
<-c.release
return filterReposByQuery(c.repos, query), nil
}
// TestListAccessibleRepos_ClearDuringFetchDropsStaleWrite is the service-level
// mirror of the ttl_cache TestTTLCache_ClearInvalidatesInFlightFetch regression:
// a fetch that started BEFORE a clear() (token swap) must not write its stale
// result back into the just-cleared cache. The fix snapshots the cache
// generation before the fetch and writes via setIfCurrentGeneration so the bump
// from clear() drops the late write.
func TestListAccessibleRepos_ClearDuringFetchDropsStaleWrite(t *testing.T) {
sc := &blockingAccessibleReposClient{
repos: []GitHubRepo{{FullName: "user-a/repo", Owner: "user-a", Name: "repo"}},
started: make(chan struct{}),
release: make(chan struct{}),
}
svc := newAccessibleReposTestService(sc)
done := make(chan struct{})
go func() {
_, _ = svc.ListAccessibleRepos(context.Background(), "", 50)
close(done)
}()
// Wait until the fetch is in flight, then clear (simulating a token swap),
// then let the stale fetch complete.
<-sc.started
svc.ClearAccessibleReposCaches()
close(sc.release)
<-done
// The stale write must have been dropped: the cache key must miss so the
// next call refetches under the new generation rather than serving the
// pre-clear (prior-user) repos.
key := accessibleReposCacheKey("", 50)
if v, ok := svc.accessibleReposCache.get(key); ok {
t.Fatalf("expected cache to miss after clear() during in-flight fetch; got %v", v)
}
}
@@ -26,11 +26,12 @@ func (c *countingSearchClient) ListIssuesPaged(ctx context.Context, filter, cust
func newTestService(client Client) *Service {
return &Service{
client: client,
authMethod: AuthMethodPAT,
logger: logger.Default(),
searchCache: newTTLCache(),
prStatusCache: newTTLCache(),
client: client,
authMethod: AuthMethodPAT,
logger: logger.Default(),
searchCache: newTTLCache(),
prStatusCache: newTTLCache(),
accessibleReposCache: newAccessibleReposCache(),
}
}
@@ -202,6 +202,12 @@ func (s *Service) ConfigureToken(ctx context.Context, token string) error {
s.authMethod = AuthMethodPAT
s.mu.Unlock()
// Drop user-scoped caches — the previous token's user/orgs and merged
// repo list are no longer valid under the new identity. Without this,
// the picker would surface the prior user's repos for up to 60s after
// a token swap.
s.ClearAccessibleReposCaches()
return nil
}
@@ -227,6 +233,9 @@ func (s *Service) ClearToken(ctx context.Context) error {
s.authMethod = AuthMethodNone
s.mu.Unlock()
// Drop user-scoped caches — see ConfigureToken for rationale.
s.ClearAccessibleReposCaches()
// Try to re-establish connection via other methods
s.retryClientCreation(ctx)
+71 -2
View File
@@ -24,6 +24,12 @@ type ttlEntry struct {
// ttlCache is a tiny TTL map guarded by singleflight to coalesce concurrent
// misses for the same key. When size exceeds the cap, entries with the
// earliest expiry are dropped — good enough for a sub-minute window.
//
// `gen` is bumped on every `clear()`. A fetch in flight at the time of a
// clear (e.g. token swap) is allowed to finish, but its result is dropped on
// the floor by `setIfCurrentGeneration` instead of being written back into
// the cache — otherwise the new user could see stale repos from the prior
// user for up to one TTL.
type ttlCache struct {
mu sync.Mutex
entries map[string]ttlEntry
@@ -31,6 +37,7 @@ type ttlCache struct {
ttl time.Duration
maxSize int
now func() time.Time
gen uint64
}
func newTTLCache() *ttlCache {
@@ -51,6 +58,16 @@ func newMergeMethodsCache() *ttlCache {
return c
}
// newAccessibleReposCache backs the list-accessible-repos endpoint and the
// org-list lookup it composes from. 60s is enough to make repeated picker
// opens / typeahead bursts cheap without staling out "I just got added to a
// new org" too long.
func newAccessibleReposCache() *ttlCache {
c := newTTLCache()
c.ttl = 60 * time.Second
return c
}
func (c *ttlCache) get(key string) (any, bool) {
c.mu.Lock()
defer c.mu.Unlock()
@@ -65,6 +82,29 @@ func (c *ttlCache) get(key string) (any, bool) {
return entry.value, true
}
// clear drops every cached entry and bumps the generation counter so any
// fetch that was already in flight is prevented from writing its result back
// (its post-fetch setIfCurrentGeneration becomes a no-op). Used by the e2e
// mock controller to make a "GitHub repos unavailable" toggle visible
// immediately instead of waiting for the 60s TTL on a prior cached success
// to expire, and by the token-swap path so the new user never sees the old
// user's cached repos through a late-resolving singleflight.
func (c *ttlCache) clear() {
c.mu.Lock()
defer c.mu.Unlock()
c.entries = make(map[string]ttlEntry)
c.gen++
}
// generation returns the current generation counter. Callers snapshot this
// before launching a fetch and pass it back into setIfCurrentGeneration so a
// concurrent clear() can invalidate a still-pending write.
func (c *ttlCache) generation() uint64 {
c.mu.Lock()
defer c.mu.Unlock()
return c.gen
}
func (c *ttlCache) set(key string, value any) {
c.mu.Lock()
defer c.mu.Unlock()
@@ -74,6 +114,22 @@ func (c *ttlCache) set(key string, value any) {
c.entries[key] = ttlEntry{value: value, expiresAt: c.now().Add(c.ttl)}
}
// setIfCurrentGeneration writes `value` for `key` only if the cache's
// generation counter still matches `gen`. If a clear() ran since `gen` was
// snapshotted, the write is dropped on the floor — preventing a stale fetch
// from clobbering the post-clear empty cache.
func (c *ttlCache) setIfCurrentGeneration(key string, value any, gen uint64) {
c.mu.Lock()
defer c.mu.Unlock()
if c.gen != gen {
return
}
if len(c.entries) >= c.maxSize {
c.evictLocked()
}
c.entries[key] = ttlEntry{value: value, expiresAt: c.now().Add(c.ttl)}
}
// evictLocked first drops expired entries; if still over the cap, drops the
// entries with the earliest expiry until back under the limit. Caller must
// hold c.mu.
@@ -107,11 +163,24 @@ func (c *ttlCache) evictLocked() {
// doOrFetch returns a cached value when fresh; otherwise runs fetch under a
// singleflight guard, caches the result, and returns it. Errors are not
// cached. The returned value is shared — callers must not mutate it.
//
// The generation snapshot is taken BEFORE launching the singleflight: if a
// clear() races with this fetch, the post-fetch write becomes a no-op so the
// caller still receives the freshly-fetched value but the cache stays empty
// (the next ensure() will re-fetch under the new generation).
func (c *ttlCache) doOrFetch(key string, fetch func() (any, error)) (any, error) {
if v, ok := c.get(key); ok {
return v, nil
}
v, err, _ := c.sf.Do(key, func() (any, error) {
gen := c.generation()
// Key the singleflight on the generation as well as the cache key. Without
// the generation prefix, a caller arriving after clear() would join an
// already-in-flight fetch from the previous generation and receive its
// stale result — singleflight's whole point is shared results. Bumping
// gen on clear() guarantees the post-clear caller mints its own fetch
// instead of inheriting the old one.
sfKey := fmt.Sprintf("%d|%s", gen, key)
v, err, _ := c.sf.Do(sfKey, func() (any, error) {
if v, ok := c.get(key); ok {
return v, nil
}
@@ -119,7 +188,7 @@ func (c *ttlCache) doOrFetch(key string, fetch func() (any, error)) (any, error)
if err != nil {
return nil, err
}
c.set(key, v)
c.setIfCurrentGeneration(key, v, gen)
return v, nil
})
return v, err
@@ -149,6 +149,105 @@ func TestTTLCache_Singleflight(t *testing.T) {
}
}
// Regression: a clear() that races with an in-flight singleflight fetch must
// invalidate the late write so the cache stays empty afterward. Otherwise a
// token swap (the user-facing trigger for clear()) could surface the
// previous user's repos for up to one TTL window — the cleared entries get
// re-populated by the still-pending fetch from the old generation.
func TestTTLCache_ClearInvalidatesInFlightFetch(t *testing.T) {
cache := newTTLCache()
fetchStarted := make(chan struct{})
releaseFetch := make(chan struct{})
fetchDone := make(chan struct{})
go func() {
_, _ = cache.doOrFetch("k", func() (any, error) {
close(fetchStarted)
<-releaseFetch
return "stale", nil
})
close(fetchDone)
}()
<-fetchStarted
cache.clear()
close(releaseFetch)
<-fetchDone
if v, ok := cache.get("k"); ok {
t.Fatalf("expected cache to remain empty after clear(); got %v", v)
}
}
// Regression: a caller arriving after clear() must not join a still-in-flight
// fetch from the previous generation via singleflight's shared result. Before
// keying singleflight by generation, B's doOrFetch joined A's in-flight call
// and received A's stale value — the post-clear cache stayed empty, but the
// caller still saw old data. After the fix, B mints its own fetch under the
// new generation.
func TestTTLCache_ClearMakesPostClearCallersMintNewFetch(t *testing.T) {
cache := newTTLCache()
var calls atomic.Int32
fetchAStarted := make(chan struct{})
releaseA := make(chan struct{})
// Caller A: starts a fetch, blocks on releaseA, eventually returns "A".
resultA := make(chan any, 1)
go func() {
v, _ := cache.doOrFetch("k", func() (any, error) {
calls.Add(1)
close(fetchAStarted)
<-releaseA
return "A", nil
})
resultA <- v
}()
<-fetchAStarted
// Clear races between A's fetch starting and finishing. After this, any
// new caller must NOT receive A's value via singleflight.
cache.clear()
// Caller B arrives after clear() while A is still in flight. B's fetch
// returns "B" immediately (it does not block on releaseA), so under the
// generation-keyed singleflight B must mint its own fetch and resolve right
// away rather than joining A's still-blocked call.
resultB := make(chan any, 1)
go func() {
v, _ := cache.doOrFetch("k", func() (any, error) {
calls.Add(1)
return "B", nil
})
resultB <- v
}()
// Block on B's result with a timeout guard instead of a sleep busy-loop. If
// generation-keyed singleflight regressed (B joining A's blocked fetch), B
// would never resolve and the timeout fires deterministically.
select {
case v := <-resultB:
if v != "B" {
t.Fatalf("caller B got %v, expected 'B' (joined A's stale fetch?)", v)
}
case <-time.After(2 * time.Second):
t.Fatal("caller B never completed — generation-keyed singleflight may still be blocking")
}
// Now let A finish; its write must be dropped via the generation guard.
close(releaseA)
<-resultA
if v, ok := cache.get("k"); ok {
// We expect either empty (A's setIfCurrentGeneration was a no-op AND B
// fetched but landed under the new generation) or "B" — never "A".
if v == "A" {
t.Fatalf("stale A result leaked into cache: %v", v)
}
}
if got := calls.Load(); got < 2 {
t.Fatalf("expected at least 2 fetches (A then B); got %d", got)
}
}
func TestTTLCache_MaxSizeEviction(t *testing.T) {
cache := newTTLCache()
cache.maxSize = 3