diff --git a/cmd/codeaf/appfactory.go b/cmd/codeaf/appfactory.go index e437306f2b..62dc761515 100644 --- a/cmd/codeaf/appfactory.go +++ b/cmd/codeaf/appfactory.go @@ -844,6 +844,7 @@ func (b *appBackend) FactoryCapabilities(context.Context) (appwire.FactoryCapabi Local: true, Runner: true, Writes: false, Admin: true, Links: true, CopyLink: b.copier().Available(), ReposManage: true, RunnerManage: true, Rules: true, PipelinePR: true, PipelinePRs: true, PipelinePRUpdate: true, PipelineSourceRef: true, SpendByRepo: true, SpendByModel: true, PipelineEdit: true, PipelineEditRuntime: true, PipelineEditStructure: true, PipelineEditRules: true, RepoRead: true, RepoFacts: true, GroupPolicy: true, WaitsOn: true, MaxConnections: appwire.MaxFactoryConnections, MaxFlows: appwire.MaxFactoryFlows, + RepoIssues: true, }, nil } diff --git a/cmd/codeaf/appfactory_admin_test.go b/cmd/codeaf/appfactory_admin_test.go index fd7e37d699..e499f9be4b 100644 --- a/cmd/codeaf/appfactory_admin_test.go +++ b/cmd/codeaf/appfactory_admin_test.go @@ -46,6 +46,9 @@ type adminFactory struct { // no way to save the list at all. afterStore error noRepoSwitch bool + // tokens is the factory's access-token file, for tests that mint a row as + // `codeaf factory token add` does. + tokens string } func startAdminFactory(t *testing.T) *adminFactory { return startAdminFactoryAt(t, true) } @@ -75,6 +78,7 @@ func startAdminFactoryWith(t *testing.T, forwarded, noRepoSwitch bool) *adminFac t.Fatal(err) } f.st = st + f.tokens = filepath.Join(dir, "tokens.json") seam := factory.Seam{Load: func() (factory.Snapshot, error) { return factory.Snapshot{}, nil }} if !f.noRepoSwitch { seam.SetRepos = func(repos []string) error { @@ -87,7 +91,7 @@ func startAdminFactoryWith(t *testing.T, forwarded, noRepoSwitch bool) *adminFac h := serve.New(seam, serve.Config{ Version: "test", Name: "Acme factory", ServedBy: "factory", Address: adminFactoryAddress, AccessToken: testHostToken, Mode: api.FactoryTeam, GitHubClientID: "client", GitHubHTTPClient: f.github.client(), Now: f.clock.now, Store: st, Record: rec, Root: st.Root(), - TokensPath: filepath.Join(dir, "tokens.json"), + TokensPath: f.tokens, }) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/v1/about" { diff --git a/cmd/codeaf/appfactory_repo_issues.go b/cmd/codeaf/appfactory_repo_issues.go new file mode 100644 index 0000000000..e3ecede25d --- /dev/null +++ b/cmd/codeaf/appfactory_repo_issues.go @@ -0,0 +1,96 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/url" + "strconv" + "strings" + + "github.com/Agent-Field/codeaf/internal/appwire" + "github.com/Agent-Field/codeaf/internal/factory/api" +) + +var _ appwire.FactoryRepoIssuesReader = (*appBackend)(nil) + +// FactoryRepoIssues reads one fixed route through the caller's connection. +func (b *appBackend) FactoryRepoIssues(ctx context.Context, p appwire.FactoryRepoIssuesParams) (appwire.FactoryRepoIssues, error) { + p, err := appwire.CheckRepoIssues(p) + if err != nil { + return appwire.FactoryRepoIssues{}, err + } + owner, name, _ := strings.Cut(p.Repo, "/") + query := url.Values{"page": {strconv.Itoa(*p.Page)}} + if p.Since != "" { + query.Set("since", p.Since) + } + path := appFactoryRepoRoute + url.PathEscape(owner) + "/" + url.PathEscape(name) + "/issues?" + query.Encode() + got, err := b.factoryHub().readRepoIssues(ctx, p.Factory, path) + if err != nil { + return appwire.FactoryRepoIssues{}, err + } + if got.Issues == nil { + got.Issues = []api.RepoIssue{} + } + return appwire.FactoryRepoIssues{Factory: p.Factory, RepoIssues: got}, nil +} + +// This read keeps the response body until refusals have been decoded, because +// the ordinary read mapping has no rate-limit retry detail. +func (h *appFactoryHub) readRepoIssues(ctx context.Context, id, path string) (api.RepoIssues, error) { + var out api.RepoIssues + row, token, err := h.credentialed(ctx, id) + if err != nil { + return out, err + } + r, err := h.send(ctx, row.URL, token, http.MethodGet, path, nil, appFactoryBody, nil) + if err != nil { + return out, err + } + if err := h.staleCredential(id, token); err != nil { + return out, err + } + if r.status != http.StatusOK { + err := repoIssuesFailure(r) + var wire *appwire.Error + if errors.As(err, &wire) && wire.Code == appwire.CodeSignedOut { + h.refused(id, token) + } + return out, err + } + if json.Unmarshal(r.body, &out) != nil { + return out, appwire.Errorf(appwire.CodeEngine, "the factory sent an answer this app cannot read") + } + h.touch(id) + return out, nil +} + +// repoIssuesFailure keeps signed_out, which forgets a restored sign-in, for the +// Factory's sign-in gate refusing the bearer. That gate always sends a bearer +// challenge. A 401 without one comes from the route's own GitHub step: the +// sign-in stands but has no GitHub access the route can use, such as a pasted +// access token with no GitHub sign-in. That is github_expired, including from a +// Factory that sent the step without a reason. +func repoIssuesFailure(r appFactoryReply) error { + if r.status == http.StatusUnauthorized && r.header.Get("WWW-Authenticate") == "" { + return appwire.Errorf(appwire.CodeGitHubExpired, "GitHub access for this sign-in is missing or expired; reconnect GitHub") + } + err := appFactoryFailure(r) + if routeMissing(err) { + var wire *appwire.Error + if errors.As(factoryTooOld(), &wire) { + wire.Detail = &appwire.FactoryRefusal{Reason: appwire.FactoryReasonTooOld} + return wire + } + } + if r.status == http.StatusTooManyRequests { + var retry api.RepoIssuesRetry + var wire *appwire.Error + if json.Unmarshal(r.body, &retry) == nil && !retry.RetryAt.IsZero() && errors.As(err, &wire) { + wire.Detail = &retry + } + } + return err +} diff --git a/cmd/codeaf/appfactory_repo_issues_test.go b/cmd/codeaf/appfactory_repo_issues_test.go new file mode 100644 index 0000000000..f398144101 --- /dev/null +++ b/cmd/codeaf/appfactory_repo_issues_test.go @@ -0,0 +1,290 @@ +package main + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/appwire" + "github.com/Agent-Field/codeaf/internal/factory/api" + "github.com/Agent-Field/codeaf/internal/factory/tokens" +) + +type repoIssuesFactory struct { + mu sync.Mutex + asks []string +} + +func attachRepoIssuesFactory(t *testing.T, status int, body string) (*factoryWire, string, *repoIssuesFactory) { + t.Helper() + return attachRepoIssuesFactoryHeader(t, status, nil, body) +} + +func attachRepoIssuesFactoryHeader(t *testing.T, status int, header http.Header, body string) (*factoryWire, string, *repoIssuesFactory) { + t.Helper() + f := &repoIssuesFactory{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/v1/about", "/v1/me": + _ = json.NewEncoder(w).Encode(api.Me{Contract: api.Contract, Factory: api.Factory{Name: "acme"}, Mode: api.FactoryTeam}) + case "/v1/repos/acme/api/issues": + if r.Method != "GET" || r.Header.Get("Authorization") != "Bearer "+testHostToken { + t.Errorf("issue request = %s, authorization %q", r.Method, r.Header.Get("Authorization")) + } + f.mu.Lock() + f.asks = append(f.asks, r.URL.RawQuery) + f.mu.Unlock() + for key, values := range header { + w.Header()[key] = values + } + w.WriteHeader(status) + _, _ = io.WriteString(w, body) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + wire := newFactoryWire(t, testFactoryHub(t)) + var conn appwire.FactoryConnection + wire.must("factory.attach", map[string]any{"url": srv.URL, "token": testHostToken}, &conn) + return wire, conn.ID, f +} + +// C1, C2, C3, C4: The real wire advertises the read and carries its fields, +// caller credential, default, cursor, row count and PR-inclusive newest. +func TestAppFactoryRepoIssuesAdvertisesAndReadsThroughTheConnection(t *testing.T) { + body := `{"repo":"acme/api","issues":[{"id":9007199254740993,"number":7,"title":"Retry","body":"details","body_cut":true,"url":"https://github.com/acme/api/issues/7","state":"closed","created_at":"2026-10-01T00:00:00Z","updated_at":"2026-11-01T01:00:00Z"}],"rows":2,"newest":"2026-11-01T02:00:00Z"}` + wire, id, f := attachRepoIssuesFactory(t, 200, body) + if !strings.Contains(wire.hello, appwire.FeatureFactoryRepoIssues) { + t.Fatalf("hello = %s", wire.hello) + } + var caps appwire.FactoryCapabilities + wire.must("factory.capabilities", map[string]any{}, &caps) + if !caps.RepoIssues { + t.Fatalf("capabilities = %+v", caps) + } + const since = "2026-11-01T01:02:03.123+05:30" + var got appwire.FactoryRepoIssues + wire.must("factory.repo_issues", map[string]any{"factory": id, "repo": " acme/api ", "since": since, "page": 10}, &got) + if got.Factory != id || got.Repo != "acme/api" || got.Rows != 2 || got.Newest == nil || len(got.Issues) != 1 { + t.Fatalf("result = %+v", got) + } + if i := got.Issues[0]; i.ID != 9007199254740993 || i.Number != 7 || i.Title != "Retry" || i.Body != "details" || !i.BodyCut || + i.State != "closed" || i.URL != "https://github.com/acme/api/issues/7" || i.CreatedAt.Format(time.RFC3339) != "2026-10-01T00:00:00Z" || + i.UpdatedAt.Format(time.RFC3339) != "2026-11-01T01:00:00Z" || got.Newest.Format(time.RFC3339) != "2026-11-01T02:00:00Z" { + t.Fatalf("issue = %+v, newest = %v", i, got.Newest) + } + wire.must("factory.repo_issues", map[string]any{"factory": id, "repo": "acme/api"}, &got) + f.mu.Lock() + defer f.mu.Unlock() + want := url.Values{"since": {since}, "page": {"10"}}.Encode() + " page=1" + if strings.Join(f.asks, " ") != want { + t.Fatalf("queries = %v, want %s", f.asks, want) + } +} + +// C4: Invalid params never reach the Factory's HTTP route. +func TestAppFactoryRepoIssuesBadParamsSendNothing(t *testing.T) { + wire, id, f := attachRepoIssuesFactory(t, 200, `{"repo":"acme/api","issues":[],"rows":0}`) + for _, bad := range []map[string]any{ + {"factory": id, "repo": "acme/api", "page": 11}, + {"factory": id, "repo": "acme/api", "page": 0}, + {"factory": id, "repo": "acme/api", "since": "yesterday"}, + {"factory": id, "repo": "acme"}, + {"factory": id, "repo": "acme/api", "state": "open"}, + } { + wire.mustFail("factory.repo_issues", bad, appwire.CodeBadParams) + } + f.mu.Lock() + defer f.mu.Unlock() + if len(f.asks) != 0 { + t.Fatalf("refused reads sent %v", f.asks) + } +} + +func repoIssuesErrorDetail(t *testing.T, wire *factoryWire, id, want string) json.RawMessage { + t.Helper() + wire.mustFail("factory.repo_issues", map[string]any{"factory": id, "repo": "acme/api"}, want) + wire.mu.Lock() + defer wire.mu.Unlock() + for i := len(wire.lines) - 1; i >= 0; i-- { + var frame struct { + ID int `json:"id"` + Error *struct { + Detail json.RawMessage `json:"detail"` + } `json:"error"` + } + if json.Unmarshal([]byte(wire.lines[i]), &frame) == nil && frame.ID == wire.next && frame.Error != nil { + return frame.Error.Detail + } + } + t.Fatal("no error frame") + return nil +} + +// C6: The wire's busy refusal preserves the Factory's RFC 3339 retry time. +func TestAppFactoryRepoIssuesPreservesRateLimitRetryAt(t *testing.T) { + wire, id, _ := attachRepoIssuesFactory(t, 429, `{"error":"GitHub rate limit","retry_at":"2026-11-01T03:00:00Z"}`) + raw := repoIssuesErrorDetail(t, wire, id, appwire.CodeBusy) + var detail api.RepoIssuesRetry + if json.Unmarshal(raw, &detail) != nil || detail.RetryAt.Format(time.RFC3339) != "2026-11-01T03:00:00Z" { + t.Fatalf("detail = %s", raw) + } +} + +// C5, C7: A missing route is too old; a repository's 404 remains not_found. +func TestAppFactoryRepoIssuesDistinguishesAnOlderFactoryFromAMissingRepository(t *testing.T) { + for _, tc := range []struct{ body, code, reason string }{ + {`{"error":"factory endpoint not found"}`, appwire.CodeConflict, appwire.FactoryReasonTooOld}, + {`{"error":"repository not found"}`, appwire.CodeNotFound, ""}, + } { + t.Run(tc.code, func(t *testing.T) { + wire, id, _ := attachRepoIssuesFactory(t, 404, tc.body) + raw := repoIssuesErrorDetail(t, wire, id, tc.code) + var detail appwire.FactoryRefusal + if len(raw) > 0 && json.Unmarshal(raw, &detail) != nil { + t.Fatalf("detail = %s", raw) + } + if detail.Reason != tc.reason { + t.Fatalf("reason = %q, want %q", detail.Reason, tc.reason) + } + }) + } +} + +// The Factory's sign-in gate refusing the bearer stays signed_out and forgets +// a restored sign-in. Its gate always challenges with WWW-Authenticate. A 401 +// with no challenge, or with github_expired, is the route's own GitHub step: the +// sign-in has no GitHub access the route can use. That is github_expired and the +// sign-in is kept, including from a Factory that sent that step without a reason. +func TestAppFactoryRepoIssuesKeepsSignInRefusals(t *testing.T) { + challenge := http.Header{"Www-Authenticate": {`Bearer realm="factory"`}} + for _, tc := range []struct { + name string + header http.Header + body string + code string + forget bool + }{ + {"gate refuses the bearer", challenge, `{"error":"this link is not recognized","reason":"link_unknown"}`, appwire.CodeSignedOut, true}, + {"gate refuses without a reason", challenge, `{"error":"sign in"}`, appwire.CodeSignedOut, true}, + {"GitHub sign-in expired", challenge, `{"error":"reconnect","reason":"github_expired"}`, appwire.CodeGitHubExpired, false}, + {"no GitHub credential", nil, `{"error":"Reconnect GitHub to read repository issues","reason":"github_expired"}`, appwire.CodeGitHubExpired, false}, + {"no GitHub credential without a reason", nil, `{"error":"Reconnect GitHub to read repository issues"}`, appwire.CodeGitHubExpired, false}, + } { + t.Run(tc.name, func(t *testing.T) { + wire, id, _ := attachRepoIssuesFactoryHeader(t, 401, tc.header, tc.body) + first := &person{w: wire, conn: appwire.FactoryConnection{ID: id}} + kept := first.take(t) + if len(kept) != 1 { + t.Fatalf("attach kept %+v", kept) + } + second := testFactoryHub(t) + second.dir = wire.backend.factoryHub().dir + after := &person{w: newFactoryWire(t, second), conn: first.conn} + var restored appwire.FactorySessionRestored + after.w.must("factory.session_restore", map[string]any{"factory": id, "url": kept[0].URL, "token": kept[0].Token}, &restored) + if !restored.Kept { + t.Fatal("the kept sign-in was not taken back") + } + after.w.mustFail("factory.repo_issues", map[string]any{"factory": id, "repo": "acme/api"}, tc.code) + gone := after.take(t) + if forgot := len(gone) == 1 && gone[0].Removed && after.held() == ""; forgot != tc.forget || (!tc.forget && (len(gone) != 0 || after.held() != testHostToken)) { + t.Fatalf("forget = %t, want %t: changes %+v, held %t", forgot, tc.forget, gone, after.held() != "") + } + }) + } +} + +// A KEPT TOKEN SIGN-IN WITHOUT GITHUB SURVIVES A BACKGROUND ISSUE READ. A team +// Factory attached with a plain access token (the host's access token, or a +// row minted as `codeaf factory token add` does) and never signed in with +// GitHub is restored after a restart. The Factory cannot read issues for that +// sign-in, so the read is github_expired; the sign-in is still held, nothing is +// queued for removal, and the connection still reads ok and reads its queue. +func TestAppFactoryRepoIssuesWithoutGitHubKeepsARestoredTokenSignIn(t *testing.T) { + for _, kind := range []string{"host access token", "minted access token"} { + t.Run(kind, func(t *testing.T) { + f := startAdminFactory(t) + secret := testHostToken + if kind == "minted access token" { + row, err := tokens.Mint() + if err != nil { + t.Fatal(err) + } + now := f.clock.now() + row.Created, row.Expires = now, now.Add(7*24*time.Hour) + secret = row.Token + if err := tokens.Add(f.tokens, row); err != nil { + t.Fatal(err) + } + } + hub := testFactoryHub(t) + hub.now = f.clock.now + before := &person{w: newFactoryWire(t, hub)} + before.w.must("factory.attach", map[string]any{"url": f.URL, "token": secret}, &before.conn) + kept := before.take(t) + if len(kept) != 1 || kept[0].Token != secret || kept[0].Removed { + t.Fatalf("attach kept %+v", kept) + } + + // The restart: a new engine over the same saved connections, handed + // the kept token back as the app does at launch. + second := testFactoryHub(t) + second.dir = hub.dir + second.now = f.clock.now + after := &person{w: newFactoryWire(t, second), conn: before.conn} + var restored appwire.FactorySessionRestored + after.w.must("factory.session_restore", map[string]any{"factory": kept[0].Factory, "url": kept[0].URL, "token": kept[0].Token, "expires": kept[0].Expires}, &restored) + if !restored.Kept { + t.Fatal("the kept token was not taken back") + } + var queue api.Queue + after.w.must("factory.queue", map[string]any{"factory": after.conn.ID}, &queue) + + if _, code, msg := after.w.call("factory.repo_issues", map[string]any{"factory": after.conn.ID, "repo": "acme/api"}); code != appwire.CodeGitHubExpired { + t.Errorf("issue read without GitHub = %q (%s), want %q", code, msg, appwire.CodeGitHubExpired) + } + if after.held() != secret { + t.Error("an issue read without GitHub dropped the held sign-in") + } + if changes := after.take(t); len(changes) != 0 { + t.Errorf("an issue read without GitHub changed the kept sign-in: %+v", changes) + } + if c := after.listed(t); c.State != appwire.FactoryOK { + t.Fatalf("after the issue read the connection reads %s", c.State) + } + after.w.must("factory.queue", map[string]any{"factory": after.conn.ID}, &queue) + }) + } +} + +// A RESTORED SIGN-IN THE FACTORY ITSELF REFUSES IS STILL FORGOTTEN by an issue +// read: the Factory's sign-in gate answers it, not the route's GitHub step. +func TestAppFactoryRepoIssuesForgetsARestoredSignInTheFactoryRefuses(t *testing.T) { + f := startAdminFactory(t) + bob := f.signIn(t, 1, "alice") + bob.take(t) + second := testFactoryHub(t) + second.dir = bob.w.backend.factoryHub().dir + second.now = f.clock.now + after := &person{w: newFactoryWire(t, second), handle: "alice", conn: bob.conn} + var restored appwire.FactorySessionRestored + after.w.must("factory.session_restore", map[string]any{"factory": bob.conn.ID, "url": f.URL, "token": "dead-session-token-0123456789"}, &restored) + if !restored.Kept { + t.Fatal("a hand-back for a saved connection was refused before it was tried") + } + after.w.mustFail("factory.repo_issues", map[string]any{"factory": bob.conn.ID, "repo": "acme/api"}, appwire.CodeSignedOut) + gone := after.take(t) + if len(gone) != 1 || !gone[0].Removed || gone[0].Factory != bob.conn.ID || after.held() != "" { + t.Fatalf("the refused sign-in was not removed: %+v, held %q", gone, after.held()) + } +} diff --git a/docs/changes/unreleased/2618-factory-repo-issues.md b/docs/changes/unreleased/2618-factory-repo-issues.md new file mode 100644 index 0000000000..9634f1943b --- /dev/null +++ b/docs/changes/unreleased/2618-factory-repo-issues.md @@ -0,0 +1,46 @@ +--- +kind: added +title: Apps read a repository's issues through the Factory; a GitHub rate limit answers retry +pr: 2618 +surface: [engine, factory, app] +invalidates: + - "Was true: an app could read a repository's issues only through GitHub's public API or the Factory's open-issue search. Is true now: factory.repo_issues lists issues in every state, updated since a time, through the Factory's GitHub connection, so private repositories work." + - "Was true: a rate limit during the Factory's repository visibility check answered not_found on every repository route, and the denial was cached for ten minutes. Is true now: it answers busy with detail.retry_at on factory.repo_issues (busy on other calls), and GitHub is asked again once retry_at passes, or after ten minutes at most." + - "Was true: a GitHub 429 on the Factory's visibility check without X-RateLimit-Remaining or Retry-After read as repository not found and was cached as a denial. Is true now: every 429 answers busy with retry_at, and GitHub is asked again once retry_at passes." +--- + +`factory.repo_issues {factory, repo, since?, page?}` lists one repository's +GitHub issues in every state, oldest update first, through the caller's Factory +connection. The hello feature is `factory.repo_issues`; the capability is +`repo_issues`. The read accepts an optional RFC 3339 lower bound and `page` +from 1–10, default 1, with at most 50 GitHub rows each. Bodies are cut at +8,192 bytes on a rune boundary, marked by `body_cut: true`. Pull requests are +excluded from `issues` while still contributing to `rows` and `newest`, so a +client can advance its cursor through them. A missing Factory route answers +`conflict` with reason `factory_too_old`, distinct from a repository's +`not_found`. The route stores nothing and appends no activity. + +A sign-in with no GitHub credential the route can use, such as a team Factory +attached with a pasted access token and no GitHub sign-in, answers +`github_expired` and keeps the sign-in: the Factory names the reason and sends +no bearer challenge, and the engine reads any unchallenged 401 from this route +that way. Only the Factory refusing the sign-in itself is `signed_out`, which +forgets a restored sign-in, so a background issue poll can no longer sign a +person out of a Factory that still accepts them. + +A spent GitHub rate limit, on the issues request or on the repository +visibility check behind any repository route, is any 429, or a 403 with +`X-RateLimit-Remaining: 0` or a `Retry-After`. It answers HTTP 429 with +`retry_at`: GitHub's `Retry-After`, or its reset when `X-RateLimit-Remaining` +is `0` or absent (the later when both apply), otherwise one minute after the +Factory's clock. A secondary limit with requests remaining retries after its +own hint, not the primary window's reset up to an hour away. It is never +cached as a denial. Until `retry_at`, for at most ten minutes, the Factory +answers the same token's other repository checks with that limit instead of +asking GitHub again, so queue paints, event ticks and issue polls stop calling +GitHub while it has said to wait. Job lookups behind visibility still answer a +hidden job the same as a missing one. + +This app wire read adds no chat or resident surface, so neither shipped manual +corpus changes. Carried from #2419, with the visibility parts of #2439 and +#2460, which merged only into the closed integration branch of #2306. diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index 897d253df7..9b6a8439c5 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -338,6 +338,7 @@ keeps the value in a private file the agent's commands read (the model's own | `factory.read_on_arrival` | `in.FactoryReadOnArrivalParams` | `FactoryReadOnArrival` | | `factory.rename` | `in.FactoryRenameParams` | `Empty` | | `factory.repo_facts` | `in.FactoryRepoFactsParams` | `FactoryRepoFacts` | +| `factory.repo_issues` | `in.FactoryRepoIssuesParams` | `FactoryRepoIssues` | | `factory.repo_read` | `in.FactoryRepoReadParams` | `FactoryRepoRead` | | `factory.repo_set` | `in.FactoryRepoSetParams` | `FactoryRepoSetDone` | | `factory.repos` | `in.FactoryRef` | `FactoryRepos` | @@ -897,6 +898,52 @@ last `months` calendar months (1 to 12, default 3) by the rule of areas and 20 stuck steps are answered, busiest first (at most 97 facts); `truncated` counts the rest. A repository the caller cannot read is not found. +An engine whose hello carries `factory.repo_issues` and whose capabilities +answer `repo_issues: true` serves `factory.repo_issues {factory, repo, since?, +page?}`. `repo` is `owner/name`, `since` is RFC 3339 (absent means no lower +bound), and `page` is 1–10, default 1. It reads only `GET +/v1/repos/{owner}/{repo}/issues?since=&page=` through the caller's Factory +connection. The route accepts only those two query keys, once each, and uses +the caller's GitHub sign-in, or a solo Factory's own profile. + +The result is `{factory, repo, issues, rows, newest?}`. `issues` contains at +most 50 entries, oldest update first, in every state: `{id, number, title, +body, url, state, created_at, updated_at, body_cut?}`. `id` is GitHub's int64 +issue identity, `url` is its HTML URL, and `state` is `open` or `closed`. +`body` is cut at 8,192 bytes on a rune boundary; `body_cut: true` marks a +cut. Pull requests are excluded from `issues`, but `rows` counts all rows +GitHub returned (0–50) and `newest` is their latest `updated_at`, including +pull requests. An empty result has `issues: []`, `rows: 0` and no `newest`. + +Invalid params are `bad_params` before any request. A repository the caller +cannot see, or GitHub's repository 404, is `not_found`. A spent GitHub rate +limit on the visibility check or issues request is `busy` with +`detail.retry_at` in RFC 3339: GitHub's `Retry-After` hint, or its +`X-RateLimit-Reset` when `X-RateLimit-Remaining` is `0` or absent, the later +of the two when both apply. A secondary limit, which leaves requests +remaining, therefore never waits for the primary window's reset. +Every GitHub HTTP 429 is a rate limit, even without headers; a 403 is a +rate limit only with `X-RateLimit-Remaining: 0` or `Retry-After`. +A rate-limited visibility check is not cached as a repository denial. +Until `retry_at`, and for at most the ten-minute visibility cache life, the +Factory answers that GitHub token's uncached repository checks as the same +rate limit without asking GitHub; the next check after that asks again. +Without a usable reset or retry hint, `detail.retry_at` is one minute after +the Factory's clock. +A sign-in the Factory accepts but that has no GitHub access the route can use +(a pasted access token with no GitHub sign-in, or a solo Factory whose host +has no GitHub profile) gets HTTP 401 with reason `github_expired` and no +bearer challenge. The engine answers `github_expired` for it, and for any 401 +from this route without a `WWW-Authenticate` challenge, and keeps the sign-in. +Only the Factory's sign-in gate refusing the bearer, which always challenges, +is `signed_out`; that forgets a restored sign-in as every read does. +A Factory missing the route is `conflict` with +`detail.reason: "factory_too_old"`; its own +"repository not found" remains `not_found`. An engine without the reader +answers `unknown_method`. +The route itself stores nothing and appends no activity; the Factory's +ordinary per-request housekeeping runs as it does for every authenticated request. + An engine whose hello carries the feature `factory.pipeline_pr_update` (and capabilities `pipeline_pr_update: true`) lets `factory.pipeline_pr` take `pr`, the number of an open pull request from a branch of the same repository into diff --git a/internal/appwire/factory.go b/internal/appwire/factory.go index d24550a00b..8d55f7784a 100644 --- a/internal/appwire/factory.go +++ b/internal/appwire/factory.go @@ -162,6 +162,8 @@ type FactoryCapabilities struct { // True only for an engine that implements [FactoryRepoFactsReader]; a // factory older than the route answers not found. RepoFacts bool `json:"repo_facts"` + // RepoIssues reads one repository's GitHub issues through its Factory. + RepoIssues bool `json:"repo_issues"` // RepoRead: factory.repo_read is served (feature factory.repo_read). True // only for an engine that implements [FactoryRepoReader]; whether the // factory has a checkout of a repository is answered by the call. diff --git a/internal/appwire/factory_repo_issues.go b/internal/appwire/factory_repo_issues.go new file mode 100644 index 0000000000..e62208dbfd --- /dev/null +++ b/internal/appwire/factory_repo_issues.go @@ -0,0 +1,83 @@ +package appwire + +import ( + "context" + "encoding/json" + + "github.com/Agent-Field/codeaf/internal/factory/api" +) + +// FeatureFactoryRepoIssues announces a read of one repository's GitHub issues. +const FeatureFactoryRepoIssues = "factory.repo_issues" + +// FactoryRepoIssuesParams selects one bounded read, with no lower bound when +// Since is absent. A nil Page defaults to 1; an explicit zero is invalid. +type FactoryRepoIssuesParams struct { + Factory string `json:"factory"` + Repo string `json:"repo"` + Since string `json:"since,omitempty"` + Page *int `json:"page,omitempty"` +} + +// FactoryRepoIssues names the connection that supplied the repository issues. +type FactoryRepoIssues struct { + Factory string `json:"factory"` + api.RepoIssues +} + +// FactoryRepoIssuesReader is the engine's read-only repository issue interface. +type FactoryRepoIssuesReader interface { + FactoryRepoIssues(context.Context, FactoryRepoIssuesParams) (FactoryRepoIssues, error) +} + +func withFactoryRepoIssuesFeature(hello Hello, e Engine) Hello { + if _, ok := e.(Factory); !ok { + return hello + } + if _, ok := e.(FactoryRepoIssuesReader); ok { + hello = withFeature(hello, FeatureFactoryRepoIssues) + } + return hello +} + +// CheckRepoIssues checks the repository and cursor before any request is sent. +func CheckRepoIssues(p FactoryRepoIssuesParams) (FactoryRepoIssuesParams, error) { + var err error + if p.Factory, p.Repo, err = checkPipelineRepo(p.Factory, p.Repo); err != nil { + return p, err + } + if p.Page == nil { + n := 1 + p.Page = &n + } + if err := api.CheckRepoIssues(p.Since, *p.Page); err != nil { + return p, Errorf(CodeBadParams, "%s", err) + } + return p, nil +} + +func init() { + methods[FeatureFactoryRepoIssues] = shaped[FactoryRepoIssuesParams, FactoryRepoIssues](readRepoIssues) +} + +func readRepoIssues(e Engine, raw json.RawMessage) (*FactoryRepoIssues, error) { + r, ok := e.(FactoryRepoIssuesReader) + if _, isFactory := e.(Factory); !ok || !isFactory { + return nil, Errorf(CodeUnknownMethod, "this engine does not serve %s", FeatureFactoryRepoIssues) + } + var p FactoryRepoIssuesParams + if err := decodeStrict(FeatureFactoryRepoIssues, raw, &p); err != nil { + return nil, err + } + p, err := CheckRepoIssues(p) + if err != nil { + return nil, err + } + ctx, cancel := factoryCall() + defer cancel() + out, err := r.FactoryRepoIssues(ctx, p) + if err != nil { + return nil, err + } + return &out, nil +} diff --git a/internal/appwire/factory_repo_issues_test.go b/internal/appwire/factory_repo_issues_test.go new file mode 100644 index 0000000000..cfee32c145 --- /dev/null +++ b/internal/appwire/factory_repo_issues_test.go @@ -0,0 +1,138 @@ +package appwire + +import ( + "context" + "encoding/json" + "reflect" + "testing" +) + +type repoIssuesEngine struct { + factoryEngine + got []FactoryRepoIssuesParams +} + +func (e *repoIssuesEngine) FactoryRepoIssues(_ context.Context, p FactoryRepoIssuesParams) (FactoryRepoIssues, error) { + e.got = append(e.got, p) + return FactoryRepoIssues{Factory: p.Factory}, nil +} + +type onlyRepoIssuesReader struct{ fakeEngine } + +func (*onlyRepoIssuesReader) FactoryRepoIssues(context.Context, FactoryRepoIssuesParams) (FactoryRepoIssues, error) { + return FactoryRepoIssues{}, nil +} + +// C1: Only a Factory with this reader announces and serves the method. +func TestRepoIssuesNeedsItsFactoryReader(t *testing.T) { + for _, e := range []Engine{&fakeEngine{}, &factoryEngine{}, &onlyRepoIssuesReader{}} { + _, err := methods[FeatureFactoryRepoIssues].call(e, json.RawMessage(`{"factory":"f","repo":"acme/api"}`)) + if wireCode(t, err) != CodeUnknownMethod { + t.Errorf("%T: %v", e, err) + } + if h := withFactoryRepoIssuesFeature(Hello{}, e); len(h.Features) != 0 { + t.Errorf("%T announced %v", e, h.Features) + } + } + h := withFactoryRepoIssuesFeature(Hello{}, &repoIssuesEngine{}) + if len(h.Features) != 1 || h.Features[0] != FeatureFactoryRepoIssues { + t.Fatalf("hello = %+v", h) + } +} + +// C4: Defaults and valid cursors reach the reader without changing since. +func TestRepoIssuesChecksAndDefaultsItsParams(t *testing.T) { + e := &repoIssuesEngine{} + for _, raw := range []string{ + `{"factory":" f ","repo":" acme/api "}`, + `{"factory":"f","repo":"acme/api","since":"2026-11-01T02:03:04.123+05:30","page":10}`, + } { + if _, err := methods[FeatureFactoryRepoIssues].call(e, json.RawMessage(raw)); err != nil { + t.Fatal(err) + } + } + if len(e.got) != 2 || e.got[0].Factory != "f" || e.got[0].Repo != "acme/api" || *e.got[0].Page != 1 || + *e.got[1].Page != 10 || e.got[1].Since != "2026-11-01T02:03:04.123+05:30" { + t.Fatalf("reader got %+v", e.got) + } +} + +// C4: Invalid input is refused before the reader can send a request. +func TestRepoIssuesRefusesBadParamsBeforeReading(t *testing.T) { + e := &repoIssuesEngine{} + for _, raw := range []string{ + `{}`, `{"factory":"f"}`, `{"repo":"acme/api"}`, + `{"factory":"f","repo":"acme"}`, `{"factory":"f","repo":"acme/../api"}`, + `{"factory":"f","repo":"acme/api?x=1"}`, + `{"factory":"f","repo":"acme/api","since":"yesterday"}`, + `{"factory":"f","repo":"acme/api","since":"2026-11-01"}`, + `{"factory":"f","repo":"acme/api","page":0}`, + `{"factory":"f","repo":"acme/api","page":-1}`, + `{"factory":"f","repo":"acme/api","page":11}`, + `{"factory":"f","repo":"acme/api","page":1.5}`, + `{"factory":"f","repo":"acme/api","page":"2"}`, + `{"factory":"f","repo":"acme/api","state":"open"}`, + } { + if _, err := methods[FeatureFactoryRepoIssues].call(e, json.RawMessage(raw)); wireCode(t, err) != CodeBadParams { + t.Errorf("%s: %v", raw, err) + } + } + if len(e.got) != 0 { + t.Fatalf("refused input reached the reader: %+v", e.got) + } +} + +// C1, C2, C3, C4: The independently registered method publishes its params, +// result, issue states, timestamps, optional fields and literal capability key. +func TestFactoryRepoIssuesSchemaNamesTheMethodAndTheCapability(t *testing.T) { + doc := Schema() + wantMethod := map[string]any{ + "params": map[string]any{"$ref": "#/$defs/in.FactoryRepoIssuesParams"}, + "result": map[string]any{"$ref": "#/$defs/FactoryRepoIssues"}, + } + if got := doc["methods"].(map[string]any)["factory.repo_issues"]; !reflect.DeepEqual(got, wantMethod) { + t.Fatalf("method schema = %#v, want %#v", got, wantMethod) + } + defs := doc["$defs"].(map[string]any) + text := map[string]any{"type": "string"} + integer := map[string]any{"type": "integer"} + stamp := map[string]any{"type": "string", "format": "date-time"} + null := map[string]any{"type": "null"} + for _, tc := range []struct { + name string + fields map[string]any + required []string + }{ + {"in.FactoryRepoIssuesParams", map[string]any{ + "factory": text, "repo": text, "since": text, + "page": map[string]any{"anyOf": []any{integer, null}}, + }, nil}, + {"FactoryRepoIssues", map[string]any{ + "factory": text, "repo": text, "rows": integer, + "issues": map[string]any{"anyOf": []any{ + map[string]any{"type": "array", "items": map[string]any{"$ref": "#/$defs/RepoIssue"}}, null, + }}, + "newest": map[string]any{"anyOf": []any{stamp, null}}, + }, []string{"factory", "repo", "issues", "rows"}}, + {"RepoIssue", map[string]any{ + "id": integer, "number": integer, "title": text, "body": text, "url": text, + "state": map[string]any{"type": "string", "enum": []string{"open", "closed"}}, + "created_at": stamp, "updated_at": stamp, + "body_cut": map[string]any{"type": "boolean"}, + }, []string{"id", "number", "title", "body", "url", "state", "created_at", "updated_at"}}, + } { + t.Run(tc.name, func(t *testing.T) { + want := map[string]any{"type": "object", "properties": tc.fields, "additionalProperties": false} + if tc.required != nil { + want["required"] = tc.required + } + if got := defs[tc.name]; !reflect.DeepEqual(got, want) { + t.Fatalf("schema = %#v, want %#v", got, want) + } + }) + } + caps := defs["FactoryCapabilities"].(map[string]any)["properties"].(map[string]any) + if got := caps["repo_issues"]; !reflect.DeepEqual(got, map[string]any{"type": "boolean"}) { + t.Fatalf("repo_issues capability schema = %#v", got) + } +} diff --git a/internal/appwire/factory_too_old.go b/internal/appwire/factory_too_old.go new file mode 100644 index 0000000000..ed63fb8b47 --- /dev/null +++ b/internal/appwire/factory_too_old.go @@ -0,0 +1,5 @@ +package appwire + +// FactoryReasonTooOld is the refusal reason for a Factory that cannot carry a +// field or serve a route this engine asked for; nothing was changed there. +const FactoryReasonTooOld = "factory_too_old" diff --git a/internal/appwire/server.go b/internal/appwire/server.go index 8f0ea8a99a..6c35b04c2e 100644 --- a/internal/appwire/server.go +++ b/internal/appwire/server.go @@ -297,6 +297,7 @@ func (s *Server) Serve(ctx context.Context, in io.Reader, hello Hello, engine En hello = withFactoryPipelineEditFeature(hello, engine) hello = withFactoryPipelineSourceRefFeature(hello, engine) hello = withFactoryPipelineCopyFeature(hello, engine) + hello = withFactoryRepoIssuesFeature(hello, engine) hello = withFactorySpendByRepoFeature(hello, engine) hello = withFactorySpendByModelFeature(hello, engine) hello = withFactoryRepoFactsFeature(hello, engine) diff --git a/internal/factory/api/repo_issues.go b/internal/factory/api/repo_issues.go new file mode 100644 index 0000000000..4f1f5da5d9 --- /dev/null +++ b/internal/factory/api/repo_issues.go @@ -0,0 +1,54 @@ +package api + +import ( + "fmt" + "time" +) + +// Repository issue reads have the same bounds on both sides of the wire. +const ( + RepoIssuesPerPage = 50 + RepoIssuesPageMax = 10 + RepoIssueBodyMax = 8192 // The limit counts bytes, preserving whole runes. +) + +// RepoIssue is a GitHub issue, excluding pull requests. ID is GitHub's +// permanent numeric identity; Number is its number within the repository. +type RepoIssue struct { + ID int64 `json:"id"` + Number int `json:"number"` + Title string `json:"title"` + Body string `json:"body"` + URL string `json:"url"` + State string `json:"state" enum:"open,closed"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + BodyCut bool `json:"body_cut,omitempty"` +} + +// RepoIssues counts all GitHub rows, including omitted pull requests, so a +// caller can advance its cursor even when there are no issues in the answer. +type RepoIssues struct { + Repo string `json:"repo"` + Issues []RepoIssue `json:"issues"` + Rows int `json:"rows"` + Newest *time.Time `json:"newest,omitempty"` +} + +// RepoIssuesRetry is the retry detail of a rate-limited repository read. +type RepoIssuesRetry struct { + RetryAt time.Time `json:"retry_at"` +} + +// CheckRepoIssues validates the cursor without rewriting its timestamp. +func CheckRepoIssues(since string, n int) error { + if n < 1 || n > RepoIssuesPageMax { + return fmt.Errorf("page is from 1 to %d", RepoIssuesPageMax) + } + if since != "" { + if _, err := time.Parse(time.RFC3339, since); err != nil { + return fmt.Errorf("since must be an RFC 3339 timestamp") + } + } + return nil +} diff --git a/internal/factory/api/repo_issues_test.go b/internal/factory/api/repo_issues_test.go new file mode 100644 index 0000000000..d3ae6d537f --- /dev/null +++ b/internal/factory/api/repo_issues_test.go @@ -0,0 +1,27 @@ +package api + +import ( + "encoding/json" + "strings" + "testing" +) + +// C2, C3: The issue identity remains int64 and an empty result has no cursor. +func TestRepoIssuesJSONKeepsIdentityAndEmptyShape(t *testing.T) { + var got RepoIssues + raw := `{"repo":"acme/api","issues":[{"id":9007199254740993,"number":7,"title":"Retry","body":"details","url":"https://github.com/acme/api/issues/7","state":"closed","created_at":"2026-10-01T00:00:00Z","updated_at":"2026-11-01T00:00:00Z"}],"rows":1,"newest":"2026-11-01T00:00:00Z"}` + if err := json.Unmarshal([]byte(raw), &got); err != nil { + t.Fatal(err) + } + if len(got.Issues) != 1 || got.Issues[0].ID != 9007199254740993 || got.Newest == nil || !got.Newest.Equal(got.Issues[0].UpdatedAt) { + t.Fatalf("decoded %+v", got) + } + encoded, err := json.Marshal(RepoIssues{Repo: "acme/api", Issues: []RepoIssue{}, Rows: 0}) + if err != nil || string(encoded) != `{"repo":"acme/api","issues":[],"rows":0}` { + t.Fatalf("empty result = %s, %v", encoded, err) + } + encoded, err = json.Marshal(got) + if err != nil || !strings.Contains(string(encoded), `"id":9007199254740993`) || strings.Contains(string(encoded), "body_cut") { + t.Fatalf("issue result = %s, %v", encoded, err) + } +} diff --git a/internal/factory/serve/job_screens.go b/internal/factory/serve/job_screens.go index 1ab801e74e..513132f567 100644 --- a/internal/factory/serve/job_screens.go +++ b/internal/factory/serve/job_screens.go @@ -317,7 +317,7 @@ func (h *Handler) searchIssues(w http.ResponseWriter, r *http.Request) { return } if !h.repoVisible(r, repo) { - if !h.authFailed(w, r) { + if !h.authFailed(w, r) && !h.githubVisibilityLimited(w, r) { h.fail(w, 404, "repository not found") } return diff --git a/internal/factory/serve/repo_issues.go b/internal/factory/serve/repo_issues.go new file mode 100644 index 0000000000..3ff883e507 --- /dev/null +++ b/internal/factory/serve/repo_issues.go @@ -0,0 +1,204 @@ +package serve + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strconv" + "time" + "unicode/utf8" + + "github.com/Agent-Field/codeaf/internal/factory/api" +) + +// GitHub can return fifty full issue bodies before the public body cut. +const repoIssuesGitHubMax = 32 << 20 + +type githubRepoIssue struct { + ID int64 `json:"id"` + Number int `json:"number"` + Title string `json:"title"` + Body string `json:"body"` + URL string `json:"html_url"` + State string `json:"state"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + PullRequest json.RawMessage `json:"pull_request"` +} + +// repoIssues reads GitHub as this caller, after the usual repository check. +func (h *Handler) repoIssues(w http.ResponseWriter, r *http.Request) { + query, err := repoIssuesQuery(r.URL.RawQuery) + if err != nil { + h.fail(w, http.StatusUnprocessableEntity, err.Error()) + return + } + owner, name, ok := h.splitRepo(w, r) + if !ok { + return + } + token := h.repoIssuesToken(w, r) + if token == "" { + return + } + path := "/repos/" + url.PathEscape(owner) + "/" + url.PathEscape(name) + "/issues?" + query.Encode() + rows, ok := h.githubRepoIssues(w, r, path, token) + if ok { + h.write(w, http.StatusOK, repoIssuesOf(owner+"/"+name, rows)) + } +} + +func repoIssuesQuery(raw string) (url.Values, error) { + query, err := url.ParseQuery(raw) + if err != nil { + return nil, fmt.Errorf("issues takes since and page, once each") + } + for key, values := range query { + if (key != "since" && key != "page") || len(values) != 1 { + return nil, fmt.Errorf("issues takes since and page, once each") + } + } + n := 1 + if query.Has("page") { + if n, err = strconv.Atoi(query.Get("page")); err != nil { + return nil, fmt.Errorf("page is a whole number from 1 to %d", api.RepoIssuesPageMax) + } + } + if err := api.CheckRepoIssues(query.Get("since"), n); err != nil { + return nil, err + } + out := url.Values{"state": {"all"}, "sort": {"updated"}, "direction": {"asc"}, + "per_page": {strconv.Itoa(api.RepoIssuesPerPage)}, "page": {strconv.Itoa(n)}} + if since := query.Get("since"); since != "" { + out.Set("since", since) + } + return out, nil +} + +// The credential rule matches searchIssues: a person's sign-in, or only a +// solo Factory's own profile. A team's host credential cannot stand in for it. +// +// A sign-in with no GitHub credential (a pasted access token, or a solo +// Factory whose host has no GitHub profile) is still a sign-in this Factory +// accepts. Its refusal names github_expired and carries no bearer challenge, +// so an app asks for GitHub again rather than forgetting the sign-in. +func (h *Handler) repoIssuesToken(w http.ResponseWriter, r *http.Request) string { + session, _ := h.credential(r) + token := session.githubToken + if token == "" && h.mode() == api.FactorySolo && h.config.GitHubToken != nil { + token = h.config.GitHubToken(r.Context()) + } + if h.authFailed(w, r) { + return "" + } + if token == "" { + h.refuse(w, http.StatusUnauthorized, "Reconnect GitHub to read repository issues", api.ReasonGitHubExpired, "", nil, "") + } + return token +} + +func (h *Handler) githubRepoIssues(w http.ResponseWriter, r *http.Request, path, token string) ([]githubRepoIssue, bool) { + req, err := http.NewRequestWithContext(r.Context(), http.MethodGet, "https://api.github.com"+path, nil) + if h.failed(w, err) { + return nil, false + } + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("Content-Type", "application/json") + req.Header.Set("User-Agent", "codeaf-factory") + client := h.githubClient() + response, err := client.Do(req) + if err != nil { + h.fail(w, http.StatusBadGateway, "GitHub isn't answering; retry this issue read") + return nil, false + } + defer response.Body.Close() + if !h.repoIssuesStatus(w, r, response, token) { + return nil, false + } + var rows []githubRepoIssue + raw, err := io.ReadAll(io.LimitReader(response.Body, repoIssuesGitHubMax+1)) + if err != nil || len(raw) > repoIssuesGitHubMax || json.Unmarshal(raw, &rows) != nil || len(rows) > api.RepoIssuesPerPage { + h.fail(w, http.StatusBadGateway, "GitHub returned an invalid repository issues response") + return nil, false + } + return rows, true +} + +func (h *Handler) repoIssuesStatus(w http.ResponseWriter, r *http.Request, response *http.Response, token string) bool { + status, header := response.StatusCode, response.Header + switch { + case status == http.StatusOK: + return true + case githubLimitSpent(status, header): + h.write(w, http.StatusTooManyRequests, struct { + Factory api.Factory `json:"factory"` + Error string `json:"error"` + api.RepoIssuesRetry + }{h.factory, "GitHub's rate limit is spent; retry after the reset", api.RepoIssuesRetry{RetryAt: h.repoIssuesRetryAt(header)}}) + case status == http.StatusNotFound: + h.fail(w, http.StatusNotFound, "repository not found") + case status == http.StatusUnauthorized: + h.endGitHub(r, token) + h.authFailed(w, r) + default: + h.fail(w, http.StatusBadGateway, fmt.Sprintf("GitHub refused the repository issues read (HTTP %d)", status)) + } + return false +} + +// repoIssuesRetryAt is when GitHub says a spent limit ends. Retry-After names +// it for a secondary limit. X-RateLimit-Reset comes on every response but ends +// the primary window, so it counts only when that window is spent or GitHub +// does not say what remains. When both count, the later one wins: a request +// before either time would be refused again. +func (h *Handler) repoIssuesRetryAt(header http.Header) time.Time { + var at time.Time + if seconds, err := strconv.ParseInt(header.Get("Retry-After"), 10, 32); err == nil && seconds >= 0 { + at = h.clock().Add(time.Duration(seconds) * time.Second).UTC() + } else if retry, err := http.ParseTime(header.Get("Retry-After")); err == nil { + at = retry.UTC() + } + if remaining, err := strconv.ParseInt(header.Get("X-RateLimit-Remaining"), 10, 64); err != nil || remaining <= 0 { + if reset, err := strconv.ParseInt(header.Get("X-RateLimit-Reset"), 10, 64); err == nil && reset > 0 { + if primary := time.Unix(reset, 0).UTC(); primary.After(at) { + at = primary + } + } + } + if at.IsZero() { + // A spent limit without either clock hint still needs a usable retry time. + return h.clock().Add(time.Minute).UTC() + } + return at +} + +func repoIssuesOf(repo string, rows []githubRepoIssue) api.RepoIssues { + out := api.RepoIssues{Repo: repo, Issues: []api.RepoIssue{}, Rows: len(rows)} + for _, row := range rows { + if out.Newest == nil || row.UpdatedAt.After(*out.Newest) { + stamp := row.UpdatedAt + out.Newest = &stamp + } + if len(row.PullRequest) != 0 && string(row.PullRequest) != "null" { + continue + } + body, cut := repoIssueBody(row.Body) + out.Issues = append(out.Issues, api.RepoIssue{ID: row.ID, Number: row.Number, Title: row.Title, Body: body, URL: row.URL, + State: row.State, CreatedAt: row.CreatedAt, UpdatedAt: row.UpdatedAt, BodyCut: cut}) + } + return out +} + +func repoIssueBody(body string) (string, bool) { + if len(body) <= api.RepoIssueBodyMax { + return body, false + } + end := api.RepoIssueBodyMax + for !utf8.RuneStart(body[end]) { + end-- + } + return body[:end], true +} diff --git a/internal/factory/serve/repo_issues_test.go b/internal/factory/serve/repo_issues_test.go new file mode 100644 index 0000000000..af0fc845e7 --- /dev/null +++ b/internal/factory/serve/repo_issues_test.go @@ -0,0 +1,379 @@ +package serve + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "net/url" + "path/filepath" + "reflect" + "strings" + "testing" + "time" + "unicode/utf8" + + "github.com/Agent-Field/codeaf/internal/factory/api" + "github.com/Agent-Field/codeaf/internal/factory/tokens" +) + +func repoIssuesFloor(t *testing.T) (*Handler, string) { + t.Helper() + h, token, _ := repoIssuesFloorAt(t) + return h, token +} + +// repoIssuesFloorClock is the same floor for a caller who brings their own +// bearer, with the Factory's clock. +func repoIssuesFloorClock(t *testing.T) (*Handler, *time.Time) { + t.Helper() + h, _, clock := repoIssuesFloorAt(t) + return h, clock +} + +func repoIssuesFloorAt(t *testing.T) (*Handler, string, *time.Time) { + t.Helper() + h, clock, g := visibilityFloor(t, map[string]map[string]bool{"reader": {"acme/api": true}}) + token := signViewer(t, h, g, clock, "reader") + // Cache both visibility answers before installing the issues-only transport. + for repo, status := range map[string]int{"api": 200, "web": 404} { + w := identityRequest(h, "GET", "/v1/repos/acme/"+repo+"/facts", "", token, nil) + if w.Code != status { + t.Fatalf("prime %s = %d %s", repo, w.Code, w.Body) + } + } + return h, token, clock +} + +func repoIssuesGitHub(h *Handler, serve func(*http.Request) (int, http.Header, string)) { + h.config.GitHubHTTPClient = &http.Client{Transport: githubTransport(func(r *http.Request) (*http.Response, error) { + status, header, body := serve(r) + return &http.Response{StatusCode: status, Header: header, Body: io.NopCloser(strings.NewReader(body))}, nil + })} +} + +func readRepoIssuesAnswer(t *testing.T, h *Handler, token, query string) (api.RepoIssues, *httptest.ResponseRecorder) { + t.Helper() + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues"+query, "", token, nil) + var got api.RepoIssues + if w.Code != 200 || json.Unmarshal(w.Body.Bytes(), &got) != nil { + t.Fatalf("read = %d %s", w.Code, w.Body) + } + return got, w +} + +// C2, C3, C4: Every issue state and field survives; PRs only count toward +// rows and newest. The caller's cursor and the fixed ordering reach GitHub. +func TestRepoIssuesListsEveryStateAndCountsPullRequests(t *testing.T) { + h, token := repoIssuesFloor(t) + const since = "2026-11-01T02:03:04.123+05:30" + long := strings.Repeat("界", api.RepoIssueBodyMax/3) + "🙂" + "x" + rows := []map[string]any{ + {"id": int64(9007199254740993), "number": 7, "title": "Open retry", "body": long, "html_url": "https://github.com/acme/api/issues/7", "state": "open", "created_at": "2026-10-01T00:00:00Z", "updated_at": "2026-11-01T01:00:00Z"}, + {"id": 82, "number": 8, "title": "Closed retry", "body": "fixed", "html_url": "https://github.com/acme/api/issues/8", "state": "closed", "created_at": "2026-10-02T00:00:00Z", "updated_at": "2026-11-01T02:00:00Z"}, + {"id": 83, "number": 9, "updated_at": "2026-11-01T03:00:00Z", "pull_request": map[string]any{"url": "https://api.github.com/repos/acme/api/pulls/9"}}, + } + raw, err := json.Marshal(rows) + if err != nil { + t.Fatal(err) + } + calls := 0 + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + calls++ + want := url.Values{"state": {"all"}, "sort": {"updated"}, "direction": {"asc"}, "per_page": {"50"}, "since": {since}, "page": {"3"}} + if r.Method != "GET" || r.URL.Host != "api.github.com" || r.URL.Path != "/repos/acme/api/issues" || !reflect.DeepEqual(r.URL.Query(), want) { + t.Errorf("GitHub request = %s %s", r.Method, r.URL) + } + if r.Header.Get("Authorization") != "Bearer gh-reader" || r.Header.Get("Accept") != "application/vnd.github+json" || + r.Header.Get("Content-Type") != "application/json" || r.Header.Get("User-Agent") != "codeaf-factory" { + t.Errorf("GitHub headers = %v", r.Header) + } + return 200, http.Header{}, string(raw) + }) + got, _ := readRepoIssuesAnswer(t, h, token, "?"+url.Values{"since": {since}, "page": {"3"}}.Encode()) + if calls != 1 || got.Repo != "acme/api" || got.Rows != 3 || len(got.Issues) != 2 || got.Newest == nil || got.Newest.Format(time.RFC3339) != "2026-11-01T03:00:00Z" { + t.Fatalf("result = %+v, calls = %d", got, calls) + } + checkRepoIssueFields(t, got.Issues) +} + +func checkRepoIssueFields(t *testing.T, issues []api.RepoIssue) { + t.Helper() + first, second := issues[0], issues[1] + if first.ID != 9007199254740993 || first.Number != 7 || first.Title != "Open retry" || first.State != "open" || + first.URL != "https://github.com/acme/api/issues/7" || first.CreatedAt.Format(time.RFC3339) != "2026-10-01T00:00:00Z" || + first.UpdatedAt.Format(time.RFC3339) != "2026-11-01T01:00:00Z" || !first.BodyCut || !utf8.ValidString(first.Body) || + first.Body != strings.Repeat("界", api.RepoIssueBodyMax/3) { + t.Fatalf("first issue = %+v", first) + } + if second.ID != 82 || second.Number != 8 || second.Title != "Closed retry" || second.State != "closed" || second.Body != "fixed" || second.BodyCut || + second.URL != "https://github.com/acme/api/issues/8" || second.CreatedAt.Format(time.RFC3339) != "2026-10-02T00:00:00Z" || !first.UpdatedAt.Before(second.UpdatedAt) { + t.Fatalf("second issue = %+v", second) + } +} + +// C3, C4: Empty and PR-only results retain the right cursor and row count; +// a full result counts 50 rows and defaults to the first request with no since. +func TestRepoIssuesEmptyPullRequestOnlyAndFullResults(t *testing.T) { + h, token := repoIssuesFloor(t) + pr := `{"updated_at":"2026-11-01T03:00:00Z","pull_request":{}}` + for _, tc := range []struct { + name, body string + rows int + }{ + {"empty", `[]`, 0}, + {"pull requests", `[` + pr + `]`, 1}, + {"full", `[` + strings.Repeat(pr+",", api.RepoIssuesPerPage-1) + pr + `]`, api.RepoIssuesPerPage}, + } { + t.Run(tc.name, func(t *testing.T) { + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + if r.URL.Query().Get("page") != "1" || r.URL.Query().Has("since") { + t.Errorf("default query = %s", r.URL.RawQuery) + } + return 200, http.Header{}, tc.body + }) + got, w := readRepoIssuesAnswer(t, h, token, "") + if got.Rows != tc.rows || got.Issues == nil || len(got.Issues) != 0 || (got.Newest == nil) != (tc.rows == 0) || + !strings.Contains(w.Body.String(), `"issues":[]`) || (tc.rows == 0 && strings.Contains(w.Body.String(), `"newest"`)) { + t.Fatalf("result = %s", w.Body) + } + }) + } +} + +// C4, C5, C8: Invalid queries, hidden repositories and writes never call GitHub. +func TestRepoIssuesRefusesQueriesHiddenRepositoriesAndWrites(t *testing.T) { + h, token := repoIssuesFloor(t) + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + t.Errorf("unexpected GitHub request: %s %s", r.Method, r.URL) + return 500, http.Header{}, "" + }) + for _, query := range []string{"?other=x", "?state=all", "?since=a&since=b", "?page=1&page=2", "?page=0", "?page=11", "?page=x", "?page=", "?since=yesterday", "?since=2026-11-01", "?bad=%zz"} { + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues"+query, "", token, nil) + if w.Code != 422 { + t.Errorf("%s = %d %s", query, w.Code, w.Body) + } + } + w := identityRequest(h, "GET", "/v1/repos/acme/web/issues", "", token, nil) + if w.Code != 404 || !strings.Contains(w.Body.String(), `"error":"repository not found"`) { + t.Fatalf("hidden repository = %d %s", w.Code, w.Body) + } + for _, method := range []string{"POST", "PUT", "PATCH", "DELETE", "HEAD"} { + w := identityRequest(h, method, "/v1/repos/acme/api/issues", "", token, nil) + if w.Code != 405 || w.Header().Get("Allow") != "GET" { + t.Errorf("%s = %d, Allow %q", method, w.Code, w.Header().Get("Allow")) + } + } +} + +// C6: Both GitHub limit statuses carry the reset, with retry-after as fallback. +func TestRepoIssuesRateLimitsCarryRetryAt(t *testing.T) { + h, token := repoIssuesFloor(t) + now := time.Date(2026, 11, 1, 0, 0, 0, 0, time.UTC) + h.config.Now = func() time.Time { return now } + for _, tc := range []struct { + status int + header http.Header + want time.Time + }{ + {403, http.Header{"X-Ratelimit-Remaining": {"0"}, "X-Ratelimit-Reset": {"1793494800"}}, now.Add(time.Hour)}, + {429, http.Header{"X-Ratelimit-Remaining": {"0"}, "X-Ratelimit-Reset": {"1793494800"}, "Retry-After": {"30"}}, now.Add(time.Hour)}, + {403, http.Header{"Retry-After": {"30"}}, now.Add(30 * time.Second)}, + {429, http.Header{"Retry-After": {now.Add(time.Minute).Format(http.TimeFormat)}}, now.Add(time.Minute)}, + } { + repoIssuesGitHub(h, func(*http.Request) (int, http.Header, string) { return tc.status, tc.header, `{}` }) + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil) + var retry api.RepoIssuesRetry + if w.Code != 429 || json.Unmarshal(w.Body.Bytes(), &retry) != nil || !retry.RetryAt.Equal(tc.want) { + t.Errorf("GitHub %d = %d %s, want %s", tc.status, w.Code, w.Body, tc.want) + } + } +} + +// C8: The route itself stores nothing and appends no activity; the Factory's +// ordinary per-request housekeeping runs as it does for every authenticated request. +// This fixture has no job store and supplies a fixed snapshot, so it covers +// only the route's own effects. +func TestRepoIssuesRouteStoresNothingAndAppendsNoActivity(t *testing.T) { + h, token := repoIssuesFloor(t) + before, err := h.config.Record.Since(context.Background(), time.Time{}) + if err != nil { + t.Fatal(err) + } + snapshot, err := h.seam.Load() + if err != nil { + t.Fatal(err) + } + repoIssuesGitHub(h, func(*http.Request) (int, http.Header, string) { return 200, http.Header{}, `[]` }) + readRepoIssuesAnswer(t, h, token, "") + after, err := h.config.Record.Since(context.Background(), time.Time{}) + if err != nil { + t.Fatal(err) + } + next, err := h.seam.Load() + if err != nil || !reflect.DeepEqual(before, after) || !reflect.DeepEqual(snapshot, next) { + t.Fatalf("route changed the fixture's snapshot or activity: %v", err) + } +} + +// C2: Bodies retain the first 8,192 bytes on a rune boundary, including matching +// words after character 4,000. Only bodies beyond the byte limit are marked cut. +func TestRepoIssuesBodyCutBoundary(t *testing.T) { + h, token := repoIssuesFloor(t) + ascii := strings.Repeat("a", 8192) + emoji := strings.Repeat("🙂", 2048) + han := strings.Repeat("界", 2730) + matching := strings.Repeat("a", 4001) + " relatedness " + strings.Repeat("z", 5000) + for _, tc := range []struct { + name, body, want string + cut bool + }{ + {"empty", "", "", false}, + {"ASCII below limit", ascii[:8191], ascii[:8191], false}, + {"ASCII at limit", ascii, ascii, false}, + {"ASCII beyond limit", ascii + "a", ascii, true}, + {"matching word after character 4000", matching, matching[:8192], true}, + {"multibyte below limit", han, han, false}, + {"multibyte at limit", emoji, emoji, false}, + {"multibyte crossing limit", han + "🙂", han, true}, + {"multibyte at last byte", ascii[:8191] + "🙂", ascii[:8191], true}, + {"multibyte beyond limit", emoji + "🙂", emoji, true}, + } { + t.Run(tc.name, func(t *testing.T) { + raw, err := json.Marshal([]map[string]any{{"body": tc.body, "updated_at": "2026-11-01T00:00:00Z"}}) + if err != nil { + t.Fatal(err) + } + repoIssuesGitHub(h, func(*http.Request) (int, http.Header, string) { return 200, http.Header{}, string(raw) }) + got, _ := readRepoIssuesAnswer(t, h, token, "") + if len(got.Issues) != 1 { + t.Fatalf("issues = %+v", got.Issues) + } + issue := got.Issues[0] + if issue.Body != tc.want || issue.BodyCut != tc.cut || len(issue.Body) > 8192 || !utf8.ValidString(issue.Body) { + t.Fatalf("body = %q (%d bytes), cut = %t; want %q, cut = %t", issue.Body, len(issue.Body), issue.BodyCut, tc.want, tc.cut) + } + }) + } +} + +// C5 and the sign-in contract: GitHub's own 404 stays not found, and a +// rejected GitHub sign-in is github_expired rather than a missing repository. +func TestRepoIssuesMapsGitHubRepositoryAndSignInFailures(t *testing.T) { + for _, tc := range []struct{ github, status int }{{404, 404}, {403, 502}, {401, 401}} { + h, token := repoIssuesFloor(t) + repoIssuesGitHub(h, func(*http.Request) (int, http.Header, string) { return tc.github, http.Header{}, `{}` }) + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil) + if w.Code != tc.status { + t.Fatalf("GitHub %d = %d %s", tc.github, w.Code, w.Body) + } + if tc.github == 401 && !strings.Contains(w.Body.String(), `"reason":"github_expired"`) { + t.Fatalf("GitHub sign-in = %s", w.Body) + } + } +} + +// The credential contract permits the local profile only on a solo Factory. +func TestRepoIssuesOnlySoloMayUseTheFactoryProfile(t *testing.T) { + for _, mode := range []string{api.FactoryTeam, api.FactorySolo} { + t.Run(mode, func(t *testing.T) { + h, _ := repoIssuesFloor(t) + h.config.Mode = mode + h.config.GitHubToken = func(context.Context) string { return "gh-profile" } + calls := 0 + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + calls++ + if mode != api.FactorySolo || r.Header.Get("Authorization") != "Bearer gh-profile" { + t.Errorf("%s used profile: %v", mode, r.Header) + } + return 200, http.Header{}, `[]` + }) + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", "floor-secret", nil) + wantStatus, wantCalls := 401, 0 + if mode == api.FactorySolo { + wantStatus, wantCalls = 200, 1 + } + if w.Code != wantStatus || calls != wantCalls { + t.Fatalf("%s = %d %s, GitHub calls %d", mode, w.Code, w.Body, calls) + } + }) + } +} + +// A sign-in the Factory accepts but that carries no GitHub credential this +// route may use is told to reconnect GitHub with github_expired. That covers a +// team Factory reached with the host's access token or a minted access-token +// row, and a solo Factory whose host has no GitHub profile. The sign-in itself +// is not refused, so there is no bearer challenge and the same bearer still +// reads the queue; an app must not forget it. +func TestRepoIssuesWithoutAGitHubCredentialAsksToReconnectGitHub(t *testing.T) { + for _, tc := range []struct{ name, mode, bearer string }{ + {"team host token", api.FactoryTeam, "floor-secret"}, + {"team minted token", api.FactoryTeam, "minted"}, + {"solo without a GitHub profile", api.FactorySolo, "floor-secret"}, + } { + t.Run(tc.name, func(t *testing.T) { + h, clock := repoIssuesFloorClock(t) + h.config.Mode = tc.mode + h.config.GitHubToken = func(context.Context) string { return "" } + bearer := tc.bearer + if bearer == "minted" { + row, err := tokens.Mint() + if err != nil { + t.Fatal(err) + } + row.Created, row.Expires = *clock, clock.Add(time.Hour) + bearer = row.Token + h.config.TokensPath = filepath.Join(t.TempDir(), "tokens.json") + if err := tokens.Save(h.config.TokensPath, []tokens.Token{row}); err != nil { + t.Fatal(err) + } + } + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + t.Errorf("unexpected GitHub request: %s %s", r.Method, r.URL) + return 500, http.Header{}, `{}` + }) + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", bearer, nil) + var got struct{ Error, Reason string } + if w.Code != 401 || json.Unmarshal(w.Body.Bytes(), &got) != nil || got.Reason != api.ReasonGitHubExpired || + got.Error != "Reconnect GitHub to read repository issues" || w.Header().Get("WWW-Authenticate") != "" { + t.Fatalf("issues without GitHub = %d %s, WWW-Authenticate %q", w.Code, w.Body, w.Header().Get("WWW-Authenticate")) + } + if q := identityRequest(h, "GET", "/v1/queue", "", bearer, nil); q.Code != 200 { + t.Fatalf("the same sign-in no longer reads the queue: %d %s", q.Code, q.Body) + } + }) + } +} + +// C2, C3: Oversized or invalid upstream results are refused within the +// bounded body read rather than reported as a valid partial result. +func TestRepoIssuesRefusesInvalidGitHubResults(t *testing.T) { + h, token := repoIssuesFloor(t) + for _, body := range []string{`{}`, `[] {}`, `[` + strings.Repeat(`{},`, api.RepoIssuesPerPage) + `{}` + `]`} { + repoIssuesGitHub(h, func(*http.Request) (int, http.Header, string) { return 200, http.Header{}, body }) + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil) + if w.Code != 502 { + t.Fatalf("invalid GitHub result = %d %s", w.Code, w.Body) + } + } + spaces := &repoIssuesSpaces{} + h.config.GitHubHTTPClient = &http.Client{Transport: githubTransport(func(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: 200, Header: http.Header{}, Body: io.NopCloser(io.LimitReader(spaces, repoIssuesGitHubMax+2))}, nil + })} + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil) + if w.Code != 502 || spaces.read != repoIssuesGitHubMax+1 { + t.Fatalf("oversized GitHub result = %d %s, read %d bytes", w.Code, w.Body, spaces.read) + } +} + +type repoIssuesSpaces struct{ read int } + +func (s *repoIssuesSpaces) Read(p []byte) (int, error) { + for i := range p { + p[i] = ' ' + } + s.read += len(p) + return len(p), nil +} diff --git a/internal/factory/serve/repos_v2.go b/internal/factory/serve/repos_v2.go index c570dd6f8d..28c2e49a0b 100644 --- a/internal/factory/serve/repos_v2.go +++ b/internal/factory/serve/repos_v2.go @@ -29,6 +29,9 @@ func (h *Handler) repoPower(w http.ResponseWriter, r *http.Request) { return } if !h.repoVisible(r, repo) { + if h.githubVisibilityLimited(w, r) { + return + } h.fail(w, http.StatusNotFound, "repository not found") return } diff --git a/internal/factory/serve/serve.go b/internal/factory/serve/serve.go index 66c3846119..a1f403584d 100644 --- a/internal/factory/serve/serve.go +++ b/internal/factory/serve/serve.go @@ -271,6 +271,7 @@ func New(seam factory.Seam, config Config) *Handler { h.read("/v1/repos/{owner}/{repo}/pipeline", h.pipeline) h.read("/v1/repos/{owner}/{repo}/pipeline/edit-source", h.pipelineEditSource) h.read("/v1/repos/{owner}/{repo}/read", h.repoRead) + h.read("/v1/repos/{owner}/{repo}/issues", h.repoIssues) h.read("/v1/repos/{owner}/{repo}/pipeline/prs", h.pipelinePRs) h.read("/v1/repos/{owner}/{repo}/dryrun", h.dryrun) h.read("/v1/repos/{owner}/{repo}/estimate", h.estimateNew) @@ -799,6 +800,9 @@ func (h *Handler) splitRepo(w http.ResponseWriter, r *http.Request) (owner, repo return "", "", false } if !visible { + if h.githubVisibilityLimited(w, r) { + return "", "", false + } h.fail(w, http.StatusNotFound, "repository not found") return "", "", false } diff --git a/internal/factory/serve/session_refresh.go b/internal/factory/serve/session_refresh.go index dc9595a85d..de2fd8cf58 100644 --- a/internal/factory/serve/session_refresh.go +++ b/internal/factory/serve/session_refresh.go @@ -18,8 +18,9 @@ var errSessionEnded = errors.New("factory session has ended; sign in again") type authErrorKey struct{} type authErrorState struct { - err error - hadSession bool + err error + hadSession bool + githubRetryAt time.Time } func (h *Handler) setAuthError(r *http.Request, err error) { diff --git a/internal/factory/serve/verbs.go b/internal/factory/serve/verbs.go index 213b7fac07..82936333bd 100644 --- a/internal/factory/serve/verbs.go +++ b/internal/factory/serve/verbs.go @@ -856,6 +856,9 @@ func (h *Handler) newJob(w http.ResponseWriter, r *http.Request) { return } if !visible { + if h.githubVisibilityLimited(w, r) { + return + } h.fail(w, http.StatusNotFound, "repository not found") return } diff --git a/internal/factory/serve/visibility.go b/internal/factory/serve/visibility.go index b9fbbd975a..2e224064fc 100644 --- a/internal/factory/serve/visibility.go +++ b/internal/factory/serve/visibility.go @@ -28,6 +28,9 @@ type viewerState struct { people map[string]memberMemory grants map[string]repoGrant github map[string]string + // limits holds GitHub's spent rate limit per token hash. It is kept apart + // from grants: a limit says nothing about any one repository. + limits map[string]repoLimit } type memberMemory struct { @@ -41,11 +44,19 @@ type repoGrant struct { until time.Time } +// repoLimit is GitHub's own retry time for a token, used until the earlier of +// that time and one visibility TTL, so a far-off retry time is checked again. +type repoLimit struct { + retry time.Time + until time.Time +} + func newViewerState() *viewerState { return &viewerState{ people: map[string]memberMemory{}, grants: map[string]repoGrant{}, github: map[string]string{}, + limits: map[string]repoLimit{}, } } @@ -188,18 +199,30 @@ func (h *Handler) repoVisible(r *http.Request, name string) bool { func (h *Handler) repoAllowed(r *http.Request, token, key string) bool { sum := sha256.Sum256([]byte(token)) - cacheKey := hex.EncodeToString(sum[:]) + "\n" + key + who := hex.EncodeToString(sum[:]) + cacheKey := who + "\n" + key now := h.now() if h.viewer != nil { h.viewer.mu.Lock() grant, ok := h.viewer.grants[cacheKey] + limit := h.viewer.limits[who] h.viewer.mu.Unlock() if ok && now.Before(grant.until) { return grant.allow } + // GitHub said this token's limit is spent: until its retry time every + // uncached repository answers that, without asking GitHub again. + if now.Before(limit.until) { + h.githubLimitMet(r, limit.retry) + return false + } } - allow, cache := h.askRepo(r, token, key) - if cache && h.viewer != nil { + allow, cache, retry := h.askRepo(r, token, key) + switch { + case !retry.IsZero(): + h.githubLimitMet(r, retry) + h.viewer.limit(who, retry, h.now()) + case cache && h.viewer != nil: h.viewer.mu.Lock() h.viewer.grants[cacheKey] = repoGrant{allow: allow, until: h.now().Add(repoVisibilityTTL)} h.viewer.mu.Unlock() @@ -207,24 +230,55 @@ func (h *Handler) repoAllowed(r *http.Request, token, key string) bool { return allow } -// askRepo is GitHub's own answer for this token. A 5xx or a transport failure -// is not cached: the next read tries again. 200 is readable; anything else -// definitive is not. -func (h *Handler) askRepo(r *http.Request, token, key string) (allow, cache bool) { +// limit keeps GitHub's retry time for a token for at most one visibility TTL, +// and drops limits that have ended. +func (v *viewerState) limit(who string, retry, now time.Time) { + if v == nil { + return + } + until := now.Add(repoVisibilityTTL) + if retry.Before(until) { + until = retry + } + v.mu.Lock() + defer v.mu.Unlock() + for hash, old := range v.limits { + if !now.Before(old.until) { + delete(v.limits, hash) + } + } + v.limits[who] = repoLimit{retry: retry, until: until} +} + +// githubLimitMet tells this request's repository route to answer retry_at. +func (h *Handler) githubLimitMet(r *http.Request, retry time.Time) { + if state, ok := r.Context().Value(authErrorKey{}).(*authErrorState); ok { + state.githubRetryAt = retry + } +} + +// askRepo is GitHub's own answer for this token. A 5xx or transport failure +// is not cached: the next read tries again. A spent limit returns GitHub's +// retry time instead of an answer. 200 is readable; anything else definitive +// is not. +func (h *Handler) askRepo(r *http.Request, token, key string) (allow, cache bool, retry time.Time) { owner, name, ok := strings.Cut(key, "/") if !ok || owner == "" || name == "" { - return false, true + return false, true, time.Time{} } endpoint := "https://api.github.com/repos/" + url.PathEscape(owner) + "/" + url.PathEscape(name) - status, err := h.githubStatus(r, http.MethodGet, endpoint, token) + status, header, err := h.githubVisibilityStatus(r, endpoint, token) if status == http.StatusUnauthorized { h.endGitHub(r, token) - return false, false + return false, false, time.Time{} } if err != nil || status >= 500 { - return false, false + return false, false, time.Time{} + } + if githubLimitSpent(status, header) { + return false, false, h.repoIssuesRetryAt(header) } - return status == http.StatusOK, true + return status == http.StatusOK, true, time.Time{} } // forViewer narrows a floor to the repositories this caller can read. diff --git a/internal/factory/serve/visibility_bare429_test.go b/internal/factory/serve/visibility_bare429_test.go new file mode 100644 index 0000000000..d5715c4a7d --- /dev/null +++ b/internal/factory/serve/visibility_bare429_test.go @@ -0,0 +1,165 @@ +package serve + +import ( + "net/http" + "strconv" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/factory/api" + "github.com/Agent-Field/codeaf/internal/factory/store" +) + +// D1: A bare 429 on visibility answers the three-field retry body at every +// repository entry point, before issue reads, job creation or repo changes. +func TestBare429VisibilityAnswersRetryNotMissing(t *testing.T) { + for _, route := range []struct{ name, method, path, body string }{ + {"issues", "GET", "/v1/repos/acme/api/issues", ""}, + {"power off", "PUT", "/v1/repos/acme/api", `{"on":false}`}, + {"power on", "PUT", "/v1/repos/acme/api", `{"on":true}`}, + {"create", "POST", "/v1/jobs", `{"repo":"acme/api","title":"retry"}`}, + {"search", "GET", "/v1/issues?repo=acme/api", ""}, + } { + t.Run(route.name, func(t *testing.T) { + h, token, clock := visibilityLimitFloor(t) + if route.method == "PUT" { + var err error + h.config.Store, err = store.Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + h.config.Mode, h.config.OperatorLogin = api.FactorySolo, "reader" + h.seam.SetRepos = func([]string) error { t.Error("changed repositories"); return nil } + } + h.seam.New = func(string, string) (int, error) { t.Error("created a job"); return 9, nil } + probes := 0 + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + probes++ + if r.Method != "GET" || r.URL.String() != "https://api.github.com/repos/acme/api" { + t.Errorf("unexpected GitHub request: %s %s", r.Method, r.URL) + } + return 429, http.Header{}, `{}` + }) + w := identityRequest(h, route.method, route.path, route.body, token, nil) + checkVisibilityRetry(t, h, w, clock.Add(time.Minute)) + if probes != 1 { + t.Fatalf("GitHub calls = %d, want only the visibility probe", probes) + } + }) + } +} + +// D2: A bare 429 is not a cached denial. Nothing asks GitHub again before its +// one-minute retry_at; the first read after it, well within the denial TTL, +// does, and the recovered grant is cached normally. +func TestBare429VisibilityIsNotCached(t *testing.T) { + h, token, clock := visibilityLimitFloor(t) + probes, issues := 0, 0 + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + switch r.URL.Path { + case "/repos/acme/api": + probes++ + if probes == 1 { + return 429, http.Header{}, `{}` + } + return 200, http.Header{}, `{}` + case "/repos/acme/api/issues": + issues++ + return 200, http.Header{}, `[]` + default: + t.Errorf("unexpected GitHub request: %s", r.URL) + return 500, http.Header{}, `{}` + } + }) + retry := clock.Add(time.Minute) + for range 2 { + checkVisibilityRetry(t, h, identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil), retry) + } + if probes != 1 || issues != 0 { + t.Fatalf("limited read: probes=%d issues=%d", probes, issues) + } + *clock = retry + for range 2 { + readRepoIssuesAnswer(t, h, token, "") + } + if probes != 2 || issues != 2 { + t.Fatalf("recovered reads: probes=%d issues=%d", probes, issues) + } +} + +// D3, D4: The issues read treats a bare 429 as busy with the Factory clock's +// default, while known resets take precedence and retry hints keep their times. +func TestRepoIssuesRateLimitsIncludeBare429(t *testing.T) { + h, token := repoIssuesFloor(t) + now := time.Date(2026, 11, 1, 0, 0, 0, 0, time.UTC) + h.config.Now = func() time.Time { return now } + reset := strconv.FormatInt(now.Add(time.Hour).Unix(), 10) + for _, tc := range []struct { + name string + status int + header http.Header + want time.Time + }{ + {"bare 429", 429, http.Header{}, now.Add(time.Minute)}, + {"429 reset only", 429, http.Header{"X-Ratelimit-Reset": {reset}}, now.Add(time.Hour)}, + {"429 reset precedence", 429, http.Header{"X-Ratelimit-Reset": {reset}, "Retry-After": {"30"}}, now.Add(time.Hour)}, + {"403 reset precedence", 403, http.Header{"X-Ratelimit-Remaining": {"0"}, "X-Ratelimit-Reset": {reset}, "Retry-After": {"30"}}, now.Add(time.Hour)}, + {"429 retry seconds", 429, http.Header{"Retry-After": {"30"}}, now.Add(30 * time.Second)}, + {"403 retry seconds", 403, http.Header{"Retry-After": {"30"}}, now.Add(30 * time.Second)}, + {"429 retry date", 429, http.Header{"Retry-After": {now.Add(2 * time.Minute).Format(http.TimeFormat)}}, now.Add(2 * time.Minute)}, + {"403 retry date", 403, http.Header{"Retry-After": {now.Add(2 * time.Minute).Format(http.TimeFormat)}}, now.Add(2 * time.Minute)}, + {"403 no reset", 403, http.Header{"X-Ratelimit-Remaining": {"0"}}, now.Add(time.Minute)}, + } { + t.Run(tc.name, func(t *testing.T) { + calls := 0 + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + calls++ + if r.URL.Path != "/repos/acme/api/issues" { + t.Errorf("unexpected GitHub request: %s", r.URL) + } + return tc.status, tc.header, `{}` + }) + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil) + checkVisibilityRetry(t, h, w, tc.want) + if calls != 1 { + t.Fatalf("GitHub issue reads = %d, want 1", calls) + } + }) + } +} + +// A secondary limit leaves primary requests remaining, and GitHub sends the +// primary window's X-RateLimit-Reset on every response. That reset is not when +// this limit ends: Retry-After is, or one minute without it. With nothing +// remaining the reset still holds, even when a Retry-After is sooner. The +// same rule answers the issues request and the visibility check before it. +func TestSecondaryRateLimitRetriesAfterItsHintNotThePrimaryReset(t *testing.T) { + now := time.Date(2026, 11, 1, 0, 0, 0, 0, time.UTC) + reset := strconv.FormatInt(now.Add(55*time.Minute).Unix(), 10) + for _, tc := range []struct { + name string + status int + header http.Header + want time.Time + }{ + {"403 retry seconds", 403, http.Header{"X-Ratelimit-Remaining": {"4200"}, "X-Ratelimit-Reset": {reset}, "Retry-After": {"60"}}, now.Add(time.Minute)}, + {"429 retry seconds", 429, http.Header{"X-Ratelimit-Remaining": {"4200"}, "X-Ratelimit-Reset": {reset}, "Retry-After": {"60"}}, now.Add(time.Minute)}, + {"403 retry date", 403, http.Header{"X-Ratelimit-Remaining": {"1"}, "X-Ratelimit-Reset": {reset}, "Retry-After": {now.Add(2 * time.Minute).Format(http.TimeFormat)}}, now.Add(2 * time.Minute)}, + {"429 no hint", 429, http.Header{"X-Ratelimit-Remaining": {"4200"}, "X-Ratelimit-Reset": {reset}}, now.Add(time.Minute)}, + {"primary spent, sooner retry", 429, http.Header{"X-Ratelimit-Remaining": {"0"}, "X-Ratelimit-Reset": {reset}, "Retry-After": {"30"}}, now.Add(55 * time.Minute)}, + } { + for _, step := range []string{"issues", "visibility"} { + t.Run(tc.name+"/"+step, func(t *testing.T) { + h, token, clock := visibilityLimitFloor(t) + *clock = now + if step == "issues" { + // Cache the visibility grant first, so only the issues request is limited. + repoIssuesGitHub(h, func(*http.Request) (int, http.Header, string) { return 200, http.Header{}, `[]` }) + readRepoIssuesAnswer(t, h, token, "") + } + repoIssuesGitHub(h, func(*http.Request) (int, http.Header, string) { return tc.status, tc.header, `{}` }) + checkVisibilityRetry(t, h, identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil), tc.want) + }) + } + } +} diff --git a/internal/factory/serve/visibility_limit.go b/internal/factory/serve/visibility_limit.go new file mode 100644 index 0000000000..a3a53aca36 --- /dev/null +++ b/internal/factory/serve/visibility_limit.go @@ -0,0 +1,50 @@ +package serve + +import ( + "io" + "net/http" + + "github.com/Agent-Field/codeaf/internal/factory/api" +) + +// githubVisibilityStatus keeps the headers that distinguish a spent limit +// from a repository denial, without retaining GitHub's response body. +func (h *Handler) githubVisibilityStatus(r *http.Request, endpoint, token string) (int, http.Header, error) { + req, err := http.NewRequestWithContext(r.Context(), http.MethodGet, endpoint, nil) + if err != nil { + return 0, nil, err + } + req.Header.Set("Accept", "application/json") + req.Header.Set("User-Agent", "codeaf-factory") + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + client := h.githubClient() + response, err := client.Do(req) + if err != nil { + return 0, nil, err + } + defer response.Body.Close() + _, _ = io.Copy(io.Discard, io.LimitReader(response.Body, 1<<20)) + return response.StatusCode, response.Header, nil +} + +func githubLimitSpent(status int, header http.Header) bool { + return status == http.StatusTooManyRequests || status == http.StatusForbidden && + (header.Get("X-RateLimit-Remaining") == "0" || header.Get("Retry-After") != "") +} + +// githubVisibilityLimited is used only when the caller named the repository. Job +// lookups must keep the same answer for hidden and missing jobs. +func (h *Handler) githubVisibilityLimited(w http.ResponseWriter, r *http.Request) bool { + state, ok := r.Context().Value(authErrorKey{}).(*authErrorState) + if !ok || state.githubRetryAt.IsZero() { + return false + } + h.write(w, http.StatusTooManyRequests, struct { + Factory api.Factory `json:"factory"` + Error string `json:"error"` + api.RepoIssuesRetry + }{h.factory, "GitHub's rate limit is spent; retry after the reset", api.RepoIssuesRetry{RetryAt: state.githubRetryAt}}) + return true +} diff --git a/internal/factory/serve/visibility_limit_test.go b/internal/factory/serve/visibility_limit_test.go new file mode 100644 index 0000000000..89460b00d8 --- /dev/null +++ b/internal/factory/serve/visibility_limit_test.go @@ -0,0 +1,319 @@ +package serve + +import ( + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strconv" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/factory/api" + "github.com/Agent-Field/codeaf/internal/factory/store" +) + +func visibilityLimitFloor(t *testing.T) (*Handler, string, *time.Time) { + t.Helper() + h, clock, g := visibilityFloor(t, map[string]map[string]bool{"reader": {"acme/api": true}}) + token := signViewer(t, h, g, clock, "reader") + h.config.Now = func() time.Time { return *clock } + return h, token, clock +} + +func checkVisibilityRetry(t *testing.T, h *Handler, w *httptest.ResponseRecorder, retry time.Time) { + t.Helper() + var got struct { + Factory api.Factory `json:"factory"` + Error string `json:"error"` + api.RepoIssuesRetry + } + if w.Code != http.StatusTooManyRequests || json.Unmarshal(w.Body.Bytes(), &got) != nil || + got.Factory != h.factory || got.Error != "GitHub's rate limit is spent; retry after the reset" || !got.RetryAt.Equal(retry) { + t.Fatalf("visibility refusal = %d %s, want retry at %s", w.Code, w.Body, retry) + } + var fields map[string]json.RawMessage + if err := json.Unmarshal(w.Body.Bytes(), &fields); err != nil || len(fields) != 3 { + t.Fatalf("retry body shape = %s, err = %v", w.Body, err) + } +} + +// B1, B6: Only caller-named repository checks answer a spent limit with +// retry detail, before any issues read, job creation or repository change. +func TestARateLimitedVisibilityCheckAnswersRetryNotMissing(t *testing.T) { + for _, limit := range []string{"403 remaining", "429 retry seconds", "403 retry date", "403 no reset"} { + for _, route := range []struct{ name, method, path, body string }{ + {"issues", "GET", "/v1/repos/acme/api/issues", ""}, + {"power", "PUT", "/v1/repos/acme/api", `{"on":false}`}, + {"create", "POST", "/v1/jobs", `{"repo":"acme/api","title":"retry"}`}, + {"search", "GET", "/v1/issues?repo=acme/api", ""}, + } { + t.Run(limit+"/"+route.name, func(t *testing.T) { + h, token, clock := visibilityLimitFloor(t) + if route.name == "power" { + var err error + h.config.Store, err = store.Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + h.config.Mode, h.config.OperatorLogin = api.FactorySolo, "reader" + h.seam.SetRepos = func([]string) error { t.Error("changed repositories"); return nil } + } + h.seam.New = func(string, string) (int, error) { t.Error("created a job"); return 9, nil } + status, header, retry := 403, http.Header{}, clock.Add(time.Minute) + switch limit { + case "403 remaining": + header.Set("X-RateLimit-Remaining", "0") + header.Set("X-RateLimit-Reset", strconv.FormatInt(retry.Unix(), 10)) + header.Set("Retry-After", "5") + case "429 retry seconds": + status = 429 + header.Set("Retry-After", "60") + case "403 retry date": + header.Set("Retry-After", retry.Format(http.TimeFormat)) + case "403 no reset": + header.Set("X-RateLimit-Remaining", "0") + } + probes := 0 + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + if r.Method != "GET" || r.URL.String() != "https://api.github.com/repos/acme/api" || + r.Header.Get("Authorization") != "Bearer gh-reader" || r.Header.Get("Accept") != "application/json" || + r.Header.Get("User-Agent") != "codeaf-factory" { + t.Errorf("unexpected GitHub request: %s %s, headers %v", r.Method, r.URL, r.Header) + } + probes++ + return status, header, `{}` + }) + w := identityRequest(h, route.method, route.path, route.body, token, nil) + checkVisibilityRetry(t, h, w, retry) + if probes != 1 { + t.Fatalf("GitHub calls = %d, want only the visibility probe", probes) + } + }) + } + } +} + +// B2: A spent limit never becomes a cached denial; a read after reset probes +// again within the denial TTL, and its readable answer is cached normally. +func TestARateLimitedVisibilityCheckIsNotCached(t *testing.T) { + for _, status := range []int{403, 429} { + t.Run(strconv.Itoa(status), func(t *testing.T) { + h, token, clock := visibilityLimitFloor(t) + reset := clock.Add(time.Minute) + probes, issues := 0, 0 + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + switch r.URL.Path { + case "/repos/acme/api": + probes++ + if clock.Before(reset) { + header := http.Header{} + header.Set("X-RateLimit-Remaining", "0") + header.Set("X-RateLimit-Reset", strconv.FormatInt(reset.Unix(), 10)) + return status, header, `{}` + } + return 200, http.Header{}, `{}` + case "/repos/acme/api/issues": + issues++ + return 200, http.Header{}, `[]` + default: + t.Errorf("unexpected GitHub request: %s", r.URL) + return 500, http.Header{}, `{}` + } + }) + checkVisibilityRetry(t, h, identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil), reset) + if probes != 1 || issues != 0 { + t.Fatalf("limited read: probes=%d issues=%d", probes, issues) + } + *clock = reset.Add(time.Second) + for range 2 { + readRepoIssuesAnswer(t, h, token, "") + } + if probes != 2 || issues != 2 { + t.Fatalf("after reset: probes=%d issues=%d", probes, issues) + } + }) + } +} + +// B3: Definitive denials remain repository-not-found answers cached for the +// visibility TTL. +func TestVisibilityDenialsStillReadAsMissing(t *testing.T) { + for _, status := range []int{404, 403} { + t.Run(strconv.Itoa(status), func(t *testing.T) { + h, token, clock := visibilityLimitFloor(t) + probes := 0 + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + probes++ + if r.URL.Path != "/repos/acme/api" { + t.Errorf("unexpected GitHub request: %s", r.URL) + } + return status, http.Header{}, `{}` + }) + for range 2 { + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil) + var got struct{ Error string } + if w.Code != 404 || json.Unmarshal(w.Body.Bytes(), &got) != nil || got.Error != "repository not found" { + t.Fatalf("denied read = %d %s", w.Code, w.Body) + } + } + if probes != 1 { + t.Fatalf("cached denial asked GitHub %d times", probes) + } + *clock = clock.Add(repoVisibilityTTL) + identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil) + if probes != 2 { + t.Fatalf("expired denial did not ask GitHub again: %d", probes) + } + }) + } +} + +// B3: Server and transport failures still avoid the visibility cache, while +// a 401 still ends the GitHub sign-in rather than setting a retry hint. +func TestVisibilityTransientAndSignInFailuresKeepTheirAnswers(t *testing.T) { + for _, failure := range []string{"server", "transport", "sign-in"} { + t.Run(failure, func(t *testing.T) { + h, token, _ := visibilityLimitFloor(t) + probes := 0 + h.config.GitHubHTTPClient = &http.Client{Transport: githubTransport(func(*http.Request) (*http.Response, error) { + probes++ + if failure == "transport" { + return nil, errors.New("GitHub unavailable") + } + status := 503 + if failure == "sign-in" { + status = 401 + } + return &http.Response{StatusCode: status, Header: http.Header{}, Body: http.NoBody}, nil + })} + for range 2 { + w := identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil) + var got struct{ Error, Reason string } + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if failure == "sign-in" { + if w.Code != 401 || got.Reason != api.ReasonGitHubExpired { + t.Fatalf("ended sign-in = %d %s", w.Code, w.Body) + } + } else if w.Code != 404 || got.Error != "repository not found" { + t.Fatalf("transient visibility failure = %d %s", w.Code, w.Body) + } + } + want := 2 + if failure == "sign-in" { + want = 1 + } + if probes != want { + t.Fatalf("probes = %d, want %d", probes, want) + } + }) + } +} + +// B3, B6: A hidden job keeps the missing-job answer; a narrowed queue hides +// limited repositories until the limit's retry time, with no retry detail or +// cached denial. Only the first check asks GitHub inside that window. +func TestRateLimitedVisibilityStillHidesJobsAndNarrowsLists(t *testing.T) { + h, token, clock := visibilityLimitFloor(t) + limited, probes := true, 0 + repoIssuesGitHub(h, func(*http.Request) (int, http.Header, string) { + probes++ + if limited { + return 429, http.Header{"Retry-After": {"60"}}, `{}` + } + return 200, http.Header{}, `{}` + }) + for _, path := range []string{"/v1/jobs/1", "/v1/jobs/999"} { + w := identityRequest(h, "GET", path, "", token, nil) + var got struct { + Error string `json:"error"` + RetryAt json.RawMessage `json:"retry_at"` + } + if w.Code != 404 || json.Unmarshal(w.Body.Bytes(), &got) != nil || got.Error != "job not found" || got.RetryAt != nil { + t.Fatalf("hidden or missing job = %d %s", w.Code, w.Body) + } + } + if queue := queueAs(t, h, token, false); len(queue.Jobs) != 0 || len(queue.Repos) != 0 { + t.Fatalf("limited queue exposed jobs or repositories: %+v", queue) + } + if probes != 1 { + t.Fatalf("hidden-job and queue probes = %d, want 1", probes) + } + limited = false + *clock = clock.Add(time.Minute) + if queue := queueAs(t, h, token, false); len(queue.Jobs) != 3 || len(queue.Repos) != 2 || probes != 3 { + t.Fatalf("next queue retained a denial: jobs=%d repos=%d probes=%d", len(queue.Jobs), len(queue.Repos), probes) + } +} + +// B2: GitHub said this token's limit is spent until retry_at, so until then the +// Factory asks it nothing more for that token: a second issues read, a queue +// paint over every repository and a job lookup answer from that alone, with +// the same retry_at. The first check after retry_at asks again, and its +// readable answer is cached normally. The limit is never a cached denial. +func TestARateLimitedVisibilityCheckWaitsForRetryAt(t *testing.T) { + h, token, clock := visibilityLimitFloor(t) + retry := clock.Add(time.Minute) + limited, probes := true, 0 + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + switch r.URL.Path { + case "/repos/acme/api", "/repos/acme/web": + probes++ + if limited { + return 403, http.Header{"X-Ratelimit-Remaining": {"4200"}, "Retry-After": {"60"}}, `{}` + } + return 200, http.Header{}, `{}` + case "/repos/acme/api/issues": + return 200, http.Header{}, `[]` + default: + t.Errorf("unexpected GitHub request: %s", r.URL) + return 500, http.Header{}, `{}` + } + }) + checkVisibilityRetry(t, h, identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil), retry) + *clock = clock.Add(30 * time.Second) + checkVisibilityRetry(t, h, identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil), retry) + if queue := queueAs(t, h, token, false); len(queue.Jobs) != 0 || len(queue.Repos) != 0 { + t.Fatalf("limited queue exposed jobs or repositories: %+v", queue) + } + if w := identityRequest(h, "GET", "/v1/jobs/1", "", token, nil); w.Code != 404 { + t.Fatalf("limited job lookup = %d %s", w.Code, w.Body) + } + if probes != 1 { + t.Fatalf("GitHub was asked %d times inside its limit window, want 1", probes) + } + limited = false + *clock = retry + for range 2 { + readRepoIssuesAnswer(t, h, token, "") + } + if probes != 2 { + t.Fatalf("after retry_at: probes = %d, want 2", probes) + } +} + +// B2: A retry time far off is relied on for one visibility TTL at most, still +// answering GitHub's own retry_at, and then GitHub is asked again. +func TestAFarRateLimitRetryIsCheckedAgainAfterTheVisibilityTTL(t *testing.T) { + h, token, clock := visibilityLimitFloor(t) + reset := clock.Add(55 * time.Minute) + probes := 0 + repoIssuesGitHub(h, func(r *http.Request) (int, http.Header, string) { + probes++ + return 403, http.Header{"X-Ratelimit-Remaining": {"0"}, "X-Ratelimit-Reset": {strconv.FormatInt(reset.Unix(), 10)}}, `{}` + }) + start := *clock + checkVisibilityRetry(t, h, identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil), reset) + *clock = start.Add(repoVisibilityTTL - time.Second) + checkVisibilityRetry(t, h, identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil), reset) + if probes != 1 { + t.Fatalf("inside the visibility TTL GitHub was asked %d times, want 1", probes) + } + *clock = start.Add(repoVisibilityTTL) + checkVisibilityRetry(t, h, identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil), reset) + if probes != 2 { + t.Fatalf("after the visibility TTL GitHub was asked %d times, want 2", probes) + } +}