From fa87d81b016e3a14975acf4f21bdb370b7f84f4f Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 12:17:47 -0500 Subject: [PATCH 1/6] factory.repo_issues: read a repository's issues through the Factory's GitHub connection Carried from #2419 on workstreams/main (closed integration PR #2306). Co-Authored-By: Claude Mythos 5.2 --- cmd/codeaf/appfactory.go | 1 + cmd/codeaf/appfactory_repo_issues.go | 87 +++++ cmd/codeaf/appfactory_repo_issues_test.go | 165 +++++++++ .../unreleased/2419-factory-repo-issues.md | 27 ++ docs/design/app-v3/PROTOCOL.md | 31 ++ internal/appwire/factory.go | 2 + internal/appwire/factory_repo_issues.go | 83 +++++ internal/appwire/factory_repo_issues_test.go | 138 ++++++++ internal/appwire/factory_too_old.go | 5 + internal/appwire/server.go | 1 + internal/factory/api/repo_issues.go | 54 +++ internal/factory/api/repo_issues_test.go | 27 ++ internal/factory/serve/repo_issues.go | 188 +++++++++++ internal/factory/serve/repo_issues_test.go | 317 ++++++++++++++++++ internal/factory/serve/serve.go | 1 + 15 files changed, 1127 insertions(+) create mode 100644 cmd/codeaf/appfactory_repo_issues.go create mode 100644 cmd/codeaf/appfactory_repo_issues_test.go create mode 100644 docs/changes/unreleased/2419-factory-repo-issues.md create mode 100644 internal/appwire/factory_repo_issues.go create mode 100644 internal/appwire/factory_repo_issues_test.go create mode 100644 internal/appwire/factory_too_old.go create mode 100644 internal/factory/api/repo_issues.go create mode 100644 internal/factory/api/repo_issues_test.go create mode 100644 internal/factory/serve/repo_issues.go create mode 100644 internal/factory/serve/repo_issues_test.go 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_repo_issues.go b/cmd/codeaf/appfactory_repo_issues.go new file mode 100644 index 0000000000..b6db998afa --- /dev/null +++ b/cmd/codeaf/appfactory_repo_issues.go @@ -0,0 +1,87 @@ +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 +} + +func repoIssuesFailure(r appFactoryReply) error { + 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..8ce927ffd7 --- /dev/null +++ b/cmd/codeaf/appfactory_repo_issues_test.go @@ -0,0 +1,165 @@ +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" +) + +type repoIssuesFactory struct { + mu sync.Mutex + asks []string +} + +func attachRepoIssuesFactory(t *testing.T, status int, 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() + 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 existing Factory authentication reasons remain the wire's sign-in codes. +func TestAppFactoryRepoIssuesKeepsSignInRefusals(t *testing.T) { + for _, tc := range []struct{ body, code string }{ + {`{"error":"sign in"}`, appwire.CodeSignedOut}, + {`{"error":"reconnect","reason":"github_expired"}`, appwire.CodeGitHubExpired}, + } { + t.Run(tc.code, func(t *testing.T) { + wire, id, _ := attachRepoIssuesFactory(t, 401, tc.body) + wire.mustFail("factory.repo_issues", map[string]any{"factory": id, "repo": "acme/api"}, tc.code) + }) + } +} diff --git a/docs/changes/unreleased/2419-factory-repo-issues.md b/docs/changes/unreleased/2419-factory-repo-issues.md new file mode 100644 index 0000000000..87377168d4 --- /dev/null +++ b/docs/changes/unreleased/2419-factory-repo-issues.md @@ -0,0 +1,27 @@ +--- +kind: added +title: App clients can read a repository's issues through the Factory's GitHub connection +pr: 2419 +surface: [engine, factory] +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." +--- + +`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`. This replaces +the earlier 4,000-character limit, preserving matching words later in that prefix. + +Pull requests are excluded from `issues` while still contributing to `rows` +and `newest`, so a client can advance its cursor through them. GitHub's rate +limit on the issues request answers `busy` with `detail.retry_at`; a rate +limit during repository visibility checking currently answers `not_found`, +as on every repository route. A missing Factory route answers +`conflict` with reason `factory_too_old`, distinct from a repository's +`not_found`. 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 app wire read adds no chat or resident surface, so neither shipped manual +corpus changes. diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index 897d253df7..f042090395 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,36 @@ 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 issues request is `busy` with `detail.retry_at` in RFC 3339, +using GitHub's reset time when supplied, otherwise its `retry-after` hint. +A visibility check refused by GitHub's rate limit currently reads as +"repository not found" (`not_found`), as on every repository route. +Sign-in failures retain `signed_out` or `github_expired`. 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/repo_issues.go b/internal/factory/serve/repo_issues.go new file mode 100644 index 0000000000..ccd33115f6 --- /dev/null +++ b/internal/factory/serve/repo_issues.go @@ -0,0 +1,188 @@ +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. +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.fail(w, http.StatusUnauthorized, "Reconnect GitHub to read repository issues") + } + 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 (status == http.StatusForbidden || status == http.StatusTooManyRequests) && + (header.Get("X-RateLimit-Remaining") == "0" || header.Get("Retry-After") != ""): + 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 +} + +func (h *Handler) repoIssuesRetryAt(header http.Header) time.Time { + if reset, err := strconv.ParseInt(header.Get("X-RateLimit-Reset"), 10, 64); err == nil && reset > 0 { + return time.Unix(reset, 0).UTC() + } + if seconds, err := strconv.ParseInt(header.Get("Retry-After"), 10, 32); err == nil && seconds >= 0 { + return h.clock().Add(time.Duration(seconds) * time.Second).UTC() + } + if retry, err := http.ParseTime(header.Get("Retry-After")); err == nil { + return retry.UTC() + } + // A spent limit without either clock hint still needs a usable retry time. + return h.clock().Add(time.Minute).UTC() +} + +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..0f3a223e1b --- /dev/null +++ b/internal/factory/serve/repo_issues_test.go @@ -0,0 +1,317 @@ +package serve + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "net/url" + "reflect" + "strings" + "testing" + "time" + "unicode/utf8" + + "github.com/Agent-Field/codeaf/internal/factory/api" +) + +func repoIssuesFloor(t *testing.T) (*Handler, string) { + 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 +} + +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) + } + }) + } +} + +// 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/serve.go b/internal/factory/serve/serve.go index 66c3846119..0af0e85b0e 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) From b28d7e906858ef5d2d8bac5db6d11f838433aae0 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 12:19:19 -0500 Subject: [PATCH 2/6] Factory: a GitHub rate limit on a repository check answers retry, not "repository not found" Carried from #2439 and #2460 on workstreams/main (closed integration PR #2306). A spent limit on the visibility check (any 429, or 403 with no remaining quota or a Retry-After) is never cached as a denial; the repository routes answer 429 with retry_at in the issues read's shape, and the issues read does the same for a bare 429. Co-Authored-By: Claude Mythos 5.2 --- docs/design/app-v3/PROTOCOL.md | 12 +- internal/factory/serve/job_screens.go | 2 +- internal/factory/serve/repo_issues.go | 3 +- internal/factory/serve/repos_v2.go | 3 + internal/factory/serve/serve.go | 3 + internal/factory/serve/session_refresh.go | 5 +- internal/factory/serve/verbs.go | 3 + internal/factory/serve/visibility.go | 14 +- .../factory/serve/visibility_bare429_test.go | 124 +++++++++ internal/factory/serve/visibility_limit.go | 50 ++++ .../factory/serve/visibility_limit_test.go | 247 ++++++++++++++++++ 11 files changed, 453 insertions(+), 13 deletions(-) create mode 100644 internal/factory/serve/visibility_bare429_test.go create mode 100644 internal/factory/serve/visibility_limit.go create mode 100644 internal/factory/serve/visibility_limit_test.go diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index f042090395..3ed180afa4 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -917,10 +917,14 @@ 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 issues request is `busy` with `detail.retry_at` in RFC 3339, -using GitHub's reset time when supplied, otherwise its `retry-after` hint. -A visibility check refused by GitHub's rate limit currently reads as -"repository not found" (`not_found`), as on every repository route. +limit on the visibility check or issues request is `busy` with +`detail.retry_at` in RFC 3339, using GitHub's reset time when supplied, +otherwise its `retry-after` hint. +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; +the next read checks GitHub again. Without a usable reset or retry hint, +`detail.retry_at` is one minute after the Factory's clock. Sign-in failures retain `signed_out` or `github_expired`. 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 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 index ccd33115f6..4b44c478b4 100644 --- a/internal/factory/serve/repo_issues.go +++ b/internal/factory/serve/repo_issues.go @@ -127,8 +127,7 @@ func (h *Handler) repoIssuesStatus(w http.ResponseWriter, r *http.Request, respo switch { case status == http.StatusOK: return true - case (status == http.StatusForbidden || status == http.StatusTooManyRequests) && - (header.Get("X-RateLimit-Remaining") == "0" || header.Get("Retry-After") != ""): + case githubLimitSpent(status, header): h.write(w, http.StatusTooManyRequests, struct { Factory api.Factory `json:"factory"` Error string `json:"error"` 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 0af0e85b0e..a1f403584d 100644 --- a/internal/factory/serve/serve.go +++ b/internal/factory/serve/serve.go @@ -800,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..1122c3e09f 100644 --- a/internal/factory/serve/visibility.go +++ b/internal/factory/serve/visibility.go @@ -207,16 +207,16 @@ 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. +// askRepo is GitHub's own answer for this token. A spent limit, 5xx or +// 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) { owner, name, ok := strings.Cut(key, "/") if !ok || owner == "" || name == "" { return false, true } 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 @@ -224,6 +224,12 @@ func (h *Handler) askRepo(r *http.Request, token, key string) (allow, cache bool if err != nil || status >= 500 { return false, false } + if githubLimitSpent(status, header) { + if state, ok := r.Context().Value(authErrorKey{}).(*authErrorState); ok { + state.githubRetryAt = h.repoIssuesRetryAt(header) + } + return false, false + } return status == http.StatusOK, true } diff --git a/internal/factory/serve/visibility_bare429_test.go b/internal/factory/serve/visibility_bare429_test.go new file mode 100644 index 0000000000..84550dae13 --- /dev/null +++ b/internal/factory/serve/visibility_bare429_test.go @@ -0,0 +1,124 @@ +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: The next request retries a bare 429 within the denial TTL, without +// waiting for retry_at; 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{}, `{}` + } + }) + checkVisibilityRetry(t, h, identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil), clock.Add(time.Minute)) + if probes != 1 || issues != 0 { + t.Fatalf("limited read: probes=%d issues=%d", probes, issues) + } + 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) + } + }) + } +} 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..6d0fb304c6 --- /dev/null +++ b/internal/factory/serve/visibility_limit_test.go @@ -0,0 +1,247 @@ +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 for that read, with no retry detail or cached denial. +func TestRateLimitedVisibilityStillHidesJobsAndNarrowsLists(t *testing.T) { + h, token, _ := 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 != 3 { + t.Fatalf("hidden-job and queue probes = %d, want 3", probes) + } + limited = false + if queue := queueAs(t, h, token, false); len(queue.Jobs) != 3 || len(queue.Repos) != 2 || probes != 5 { + t.Fatalf("next queue retained a denial: jobs=%d repos=%d probes=%d", len(queue.Jobs), len(queue.Repos), probes) + } +} From f8d8994cf326c382e754887520fdd6144a926da5 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 12:22:16 -0500 Subject: [PATCH 3/6] changes: entry for this pull request Co-Authored-By: Claude Mythos 5.2 --- .../unreleased/2419-factory-repo-issues.md | 27 ---------------- .../unreleased/2618-factory-repo-issues.md | 32 +++++++++++++++++++ 2 files changed, 32 insertions(+), 27 deletions(-) delete mode 100644 docs/changes/unreleased/2419-factory-repo-issues.md create mode 100644 docs/changes/unreleased/2618-factory-repo-issues.md diff --git a/docs/changes/unreleased/2419-factory-repo-issues.md b/docs/changes/unreleased/2419-factory-repo-issues.md deleted file mode 100644 index 87377168d4..0000000000 --- a/docs/changes/unreleased/2419-factory-repo-issues.md +++ /dev/null @@ -1,27 +0,0 @@ ---- -kind: added -title: App clients can read a repository's issues through the Factory's GitHub connection -pr: 2419 -surface: [engine, factory] -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." ---- - -`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`. This replaces -the earlier 4,000-character limit, preserving matching words later in that prefix. - -Pull requests are excluded from `issues` while still contributing to `rows` -and `newest`, so a client can advance its cursor through them. GitHub's rate -limit on the issues request answers `busy` with `detail.retry_at`; a rate -limit during repository visibility checking currently answers `not_found`, -as on every repository route. A missing Factory route answers -`conflict` with reason `factory_too_old`, distinct from a repository's -`not_found`. 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 app wire read adds no chat or resident surface, so neither shipped manual -corpus changes. 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..76e24d3eac --- /dev/null +++ b/docs/changes/unreleased/2618-factory-repo-issues.md @@ -0,0 +1,32 @@ +--- +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 the next check asks GitHub again." + - "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 the next read checks GitHub again." +--- + +`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 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 reset or `Retry-After` when given, otherwise one minute +after the Factory's clock) and is never cached as a denial. 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. From 6438ec3d3f62c92d1da11351a194763ccccae6cf Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 18:23:44 -0500 Subject: [PATCH 4/6] factory.repo_issues: a sign-in without GitHub is github_expired and is kept A team Factory attached with a pasted access token (the host's access token or a `codeaf factory token add` row) and no GitHub sign-in answered the issues route with a reasonless 401. The engine read that as signed_out and called refused, so the desk's background issue poll deleted a restored Factory sign-in the Factory still accepted for every other route. The Factory now names github_expired for the route's missing GitHub credential and sends no bearer challenge. The engine keeps signed_out, and the forgetting of a restored sign-in, for the Factory's sign-in gate, which always challenges; any unchallenged 401 from this route is github_expired, including from a Factory that sent the step without a reason. The desk already falls back to the public issue source on github_expired. Co-Authored-By: Claude Opus 5.5 --- cmd/codeaf/appfactory_admin_test.go | 6 +- cmd/codeaf/appfactory_repo_issues.go | 9 ++ cmd/codeaf/appfactory_repo_issues_test.go | 139 +++++++++++++++++- .../unreleased/2618-factory-repo-issues.md | 8 + docs/design/app-v3/PROTOCOL.md | 9 +- internal/factory/serve/repo_issues.go | 7 +- internal/factory/serve/repo_issues_test.go | 64 +++++++- 7 files changed, 231 insertions(+), 11 deletions(-) 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 index b6db998afa..e3ecede25d 100644 --- a/cmd/codeaf/appfactory_repo_issues.go +++ b/cmd/codeaf/appfactory_repo_issues.go @@ -67,7 +67,16 @@ func (h *appFactoryHub) readRepoIssues(ctx context.Context, id, path string) (ap 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 diff --git a/cmd/codeaf/appfactory_repo_issues_test.go b/cmd/codeaf/appfactory_repo_issues_test.go index 8ce927ffd7..f398144101 100644 --- a/cmd/codeaf/appfactory_repo_issues_test.go +++ b/cmd/codeaf/appfactory_repo_issues_test.go @@ -13,6 +13,7 @@ import ( "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 { @@ -21,6 +22,11 @@ type repoIssuesFactory struct { } 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) { @@ -35,6 +41,9 @@ func attachRepoIssuesFactory(t *testing.T, status int, body string) (*factoryWir 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: @@ -151,15 +160,131 @@ func TestAppFactoryRepoIssuesDistinguishesAnOlderFactoryFromAMissingRepository(t } } -// The existing Factory authentication reasons remain the wire's sign-in codes. +// 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) { - for _, tc := range []struct{ body, code string }{ - {`{"error":"sign in"}`, appwire.CodeSignedOut}, - {`{"error":"reconnect","reason":"github_expired"}`, appwire.CodeGitHubExpired}, + 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.code, func(t *testing.T) { - wire, id, _ := attachRepoIssuesFactory(t, 401, tc.body) - wire.mustFail("factory.repo_issues", map[string]any{"factory": id, "repo": "acme/api"}, tc.code) + 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 index 76e24d3eac..36fc520390 100644 --- a/docs/changes/unreleased/2618-factory-repo-issues.md +++ b/docs/changes/unreleased/2618-factory-repo-issues.md @@ -20,6 +20,14 @@ 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 diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index 3ed180afa4..e671d06633 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -925,7 +925,14 @@ rate limit only with `X-RateLimit-Remaining: 0` or `Retry-After`. A rate-limited visibility check is not cached as a repository denial; the next read checks GitHub again. Without a usable reset or retry hint, `detail.retry_at` is one minute after the Factory's clock. -Sign-in failures retain `signed_out` or `github_expired`. A Factory missing +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`. diff --git a/internal/factory/serve/repo_issues.go b/internal/factory/serve/repo_issues.go index 4b44c478b4..bcce5de977 100644 --- a/internal/factory/serve/repo_issues.go +++ b/internal/factory/serve/repo_issues.go @@ -79,6 +79,11 @@ func repoIssuesQuery(raw string) (url.Values, error) { // 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 @@ -89,7 +94,7 @@ func (h *Handler) repoIssuesToken(w http.ResponseWriter, r *http.Request) string return "" } if token == "" { - h.fail(w, http.StatusUnauthorized, "Reconnect GitHub to read repository issues") + h.refuse(w, http.StatusUnauthorized, "Reconnect GitHub to read repository issues", api.ReasonGitHubExpired, "", nil, "") } return token } diff --git a/internal/factory/serve/repo_issues_test.go b/internal/factory/serve/repo_issues_test.go index 0f3a223e1b..af0fc845e7 100644 --- a/internal/factory/serve/repo_issues_test.go +++ b/internal/factory/serve/repo_issues_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "path/filepath" "reflect" "strings" "testing" @@ -14,9 +15,24 @@ import ( "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") @@ -27,7 +43,7 @@ func repoIssuesFloor(t *testing.T) (*Handler, string) { t.Fatalf("prime %s = %d %s", repo, w.Code, w.Body) } } - return h, token + return h, token, clock } func repoIssuesGitHub(h *Handler, serve func(*http.Request) (int, http.Header, string)) { @@ -285,6 +301,52 @@ func TestRepoIssuesOnlySoloMayUseTheFactoryProfile(t *testing.T) { } } +// 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) { From 897364cb2a1ae299659db89659dd5f5434d4ce57 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 18:24:56 -0500 Subject: [PATCH 5/6] Factory: a secondary GitHub limit retries after its own hint, not the primary reset GitHub sends X-RateLimit-Reset on every response, so preferring it made a secondary limit (requests still remaining, Retry-After: 60) answer retry_at at the primary window's reset, up to an hour away. The desk then paused private-repository issue wakes for that repository for the hour. retry_at is now Retry-After when given, and the reset only when X-RateLimit-Remaining is 0 or absent; when both apply the later wins, since a request before either is refused again. Without either it stays one minute after the Factory's clock. The visibility check and the issues read share the rule. Co-Authored-By: Claude Opus 5.5 --- .../unreleased/2618-factory-repo-issues.md | 7 ++-- docs/design/app-v3/PROTOCOL.md | 6 ++-- internal/factory/serve/repo_issues.go | 28 ++++++++++----- .../factory/serve/visibility_bare429_test.go | 36 +++++++++++++++++++ 4 files changed, 65 insertions(+), 12 deletions(-) diff --git a/docs/changes/unreleased/2618-factory-repo-issues.md b/docs/changes/unreleased/2618-factory-repo-issues.md index 36fc520390..835299fbb8 100644 --- a/docs/changes/unreleased/2618-factory-repo-issues.md +++ b/docs/changes/unreleased/2618-factory-repo-issues.md @@ -31,8 +31,11 @@ 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 reset or `Retry-After` when given, otherwise one minute -after the Factory's clock) and is never cached as a denial. Job lookups behind +`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. 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 diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index e671d06633..62f854253f 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -918,8 +918,10 @@ 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, using GitHub's reset time when supplied, -otherwise its `retry-after` hint. +`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; diff --git a/internal/factory/serve/repo_issues.go b/internal/factory/serve/repo_issues.go index bcce5de977..3ff883e507 100644 --- a/internal/factory/serve/repo_issues.go +++ b/internal/factory/serve/repo_issues.go @@ -149,18 +149,30 @@ func (h *Handler) repoIssuesStatus(w http.ResponseWriter, r *http.Request, respo 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 { - if reset, err := strconv.ParseInt(header.Get("X-RateLimit-Reset"), 10, 64); err == nil && reset > 0 { - return time.Unix(reset, 0).UTC() - } + var at time.Time if seconds, err := strconv.ParseInt(header.Get("Retry-After"), 10, 32); err == nil && seconds >= 0 { - return h.clock().Add(time.Duration(seconds) * time.Second).UTC() + 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 retry, err := http.ParseTime(header.Get("Retry-After")); err == nil { - return retry.UTC() + if at.IsZero() { + // A spent limit without either clock hint still needs a usable retry time. + return h.clock().Add(time.Minute).UTC() } - // 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 { diff --git a/internal/factory/serve/visibility_bare429_test.go b/internal/factory/serve/visibility_bare429_test.go index 84550dae13..c1e57562e6 100644 --- a/internal/factory/serve/visibility_bare429_test.go +++ b/internal/factory/serve/visibility_bare429_test.go @@ -122,3 +122,39 @@ func TestRepoIssuesRateLimitsIncludeBare429(t *testing.T) { }) } } + +// 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) + }) + } + } +} From a3a458ca6037b7e593519fcdfefd7a4ed5e7d58a Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 18:29:40 -0500 Subject: [PATCH 6/6] Factory: a spent GitHub limit on the visibility check waits for retry_at A rate-limited visibility check was never remembered, so until the reset every queue paint, event tick and issue poll asked GitHub again for each uncached repository with the person's token, while GitHub had said to stop. The Factory now keeps GitHub's retry time per token hash, apart from the allow/deny grants, and answers that token's uncached repository checks with the same retry_at without asking GitHub, for at most one visibility TTL so a far-off retry time is checked again. It is still never a cached denial: the first check after retry_at asks GitHub again. TestBare429VisibilityIsNotCached and TestRateLimitedVisibilityStillHidesJobsAndNarrowsLists asserted that the next read asks GitHub again before retry_at; they now advance the clock to retry_at first and assert that nothing asks GitHub before it. Co-Authored-By: Claude Opus 5.5 --- .../unreleased/2618-factory-repo-issues.md | 15 ++-- docs/design/app-v3/PROTOCOL.md | 13 +-- internal/factory/serve/visibility.go | 78 ++++++++++++++---- .../factory/serve/visibility_bare429_test.go | 11 ++- .../factory/serve/visibility_limit_test.go | 82 +++++++++++++++++-- 5 files changed, 165 insertions(+), 34 deletions(-) diff --git a/docs/changes/unreleased/2618-factory-repo-issues.md b/docs/changes/unreleased/2618-factory-repo-issues.md index 835299fbb8..9634f1943b 100644 --- a/docs/changes/unreleased/2618-factory-repo-issues.md +++ b/docs/changes/unreleased/2618-factory-repo-issues.md @@ -5,8 +5,8 @@ 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 the next check asks GitHub again." - - "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 the next read checks GitHub again." + - "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 @@ -33,10 +33,13 @@ 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. Job lookups behind -visibility still answer a hidden job the same as a missing one. +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 diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index 62f854253f..9b6a8439c5 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -924,9 +924,12 @@ 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; -the next read checks GitHub again. Without a usable reset or retry hint, -`detail.retry_at` is one minute after the Factory's clock. +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 @@ -934,8 +937,8 @@ 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 +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 diff --git a/internal/factory/serve/visibility.go b/internal/factory/serve/visibility.go index 1122c3e09f..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,30 +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 spent limit, 5xx or -// 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, 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) { - if state, ok := r.Context().Value(authErrorKey{}).(*authErrorState); ok { - state.githubRetryAt = h.repoIssuesRetryAt(header) - } - return false, false + 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 index c1e57562e6..d5715c4a7d 100644 --- a/internal/factory/serve/visibility_bare429_test.go +++ b/internal/factory/serve/visibility_bare429_test.go @@ -49,8 +49,9 @@ func TestBare429VisibilityAnswersRetryNotMissing(t *testing.T) { } } -// D2: The next request retries a bare 429 within the denial TTL, without -// waiting for retry_at; the recovered grant is cached normally. +// 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 @@ -70,10 +71,14 @@ func TestBare429VisibilityIsNotCached(t *testing.T) { return 500, http.Header{}, `{}` } }) - checkVisibilityRetry(t, h, identityRequest(h, "GET", "/v1/repos/acme/api/issues", "", token, nil), clock.Add(time.Minute)) + 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, "") } diff --git a/internal/factory/serve/visibility_limit_test.go b/internal/factory/serve/visibility_limit_test.go index 6d0fb304c6..89460b00d8 100644 --- a/internal/factory/serve/visibility_limit_test.go +++ b/internal/factory/serve/visibility_limit_test.go @@ -213,9 +213,10 @@ func TestVisibilityTransientAndSignInFailuresKeepTheirAnswers(t *testing.T) { } // B3, B6: A hidden job keeps the missing-job answer; a narrowed queue hides -// limited repositories for that read, with no retry detail or cached denial. +// 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, _ := visibilityLimitFloor(t) + h, token, clock := visibilityLimitFloor(t) limited, probes := true, 0 repoIssuesGitHub(h, func(*http.Request) (int, http.Header, string) { probes++ @@ -237,11 +238,82 @@ func TestRateLimitedVisibilityStillHidesJobsAndNarrowsLists(t *testing.T) { 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 != 3 { - t.Fatalf("hidden-job and queue probes = %d, want 3", probes) + if probes != 1 { + t.Fatalf("hidden-job and queue probes = %d, want 1", probes) } limited = false - if queue := queueAs(t, h, token, false); len(queue.Jobs) != 3 || len(queue.Repos) != 2 || probes != 5 { + *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) + } +}