From d69c03aec037bc5f8c7b9aeabc96c98e7c0fde4a Mon Sep 17 00:00:00 2001 From: Yuncan050115 <64594860+Yuncan050115@users.noreply.github.com> Date: Wed, 4 Nov 2026 21:04:41 +0800 Subject: [PATCH] fix(backend): treat a plain 403 as missing evidence, not a collection abort hydrateTopRepoEvidence compared readmeErr/languageErr with `!=` against ErrGitHubUnavailable, but restGet wraps that sentinel with %w, so the comparison never matched and one unreadable README aborted the whole candidate loop. finishCollectedScan discards the returned error, so the scan still reported complete_public while later repositories silently lost their README, languages and root listing. Use errors.Is, which is already this package's idiom for wrapped sentinels. ErrGitHubRateLimited and the unwrapped 404 path are untouched, so a drained token pool still aborts and a missing README still means "no README". Adds three httptest cases; the first one fails on the pre-fix code. Fixes #621 --- internal/backend/github_enrichment.go | 5 +- internal/backend/github_enrichment_test.go | 174 +++++++++++++++++++++ 2 files changed, 177 insertions(+), 2 deletions(-) create mode 100644 internal/backend/github_enrichment_test.go diff --git a/internal/backend/github_enrichment.go b/internal/backend/github_enrichment.go index d135ee6b..a105a865 100644 --- a/internal/backend/github_enrichment.go +++ b/internal/backend/github_enrichment.go @@ -3,6 +3,7 @@ package backend import ( "context" "encoding/base64" + "errors" "fmt" "math" "regexp" @@ -320,11 +321,11 @@ func (c *GitHubClient) hydrateTopRepoEvidence(ctx context.Context, repos []TopRe for index := 0; index < limit; index++ { owner := valueOrString(repos[index].OwnerLogin, fallbackOwner) readme, readmeErr := c.fetchReadmeDocument(ctx, owner, repos[index].Name) - if readmeErr != nil && readmeErr != ErrGitHubUnavailable { + if readmeErr != nil && !errors.Is(readmeErr, ErrGitHubUnavailable) { return readmeErr } languages, languageErr := c.fetchRepoLanguages(ctx, owner, repos[index].Name) - if languageErr != nil && languageErr != ErrGitHubUnavailable { + if languageErr != nil && !errors.Is(languageErr, ErrGitHubUnavailable) { return languageErr } if readme != nil { diff --git a/internal/backend/github_enrichment_test.go b/internal/backend/github_enrichment_test.go new file mode 100644 index 00000000..982420ae --- /dev/null +++ b/internal/backend/github_enrichment_test.go @@ -0,0 +1,174 @@ +package backend + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" +) + +// A plain 403 (no Retry-After, X-Ratelimit-Remaining != 0) wraps +// ErrGitHubUnavailable. hydrateTopRepoEvidence used direct comparison, which +// cannot match that sentinel and aborted the remaining candidate repositories. + +func newEvidenceTestClient(t *testing.T, handler http.HandlerFunc) *GitHubClient { + t.Helper() + server := httptest.NewServer(handler) + t.Cleanup(server.Close) + return NewGitHubClient("token").withBaseURL(server.URL).withHTTPClient(server.Client()) +} + +// base64 of a README carrying install/usage/api/testing signals, so the hydrated +// repository has real parsed features rather than an empty document. +const evidenceReadmeBase64 = "IyBFeGFtcGxlCgpBIHNtYWxsIGxpYnJhcnkuCgojIyBJbnN0YWxsCgpgYGBiYXNoCm5wbSBpIGV4YW1wbGUKYGBgCgojIyBVc2FnZQoKU2VlIHRoZSBkZW1vLgoKIyMgQVBJCgphcGkgc3VyZmFjZQoKIyMgVGVzdGluZwoKcnVuIHRlc3RzCg==" + +func TestHydrateTopRepoEvidenceContinuesAfterPlainForbidden(t *testing.T) { + requested := map[string]int{} + client := newEvidenceTestClient(t, func(w http.ResponseWriter, r *http.Request) { + requested[r.URL.Path]++ + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Ratelimit-Remaining", "4998") + switch { + // The first repository's README is forbidden on every attempt. + case r.URL.Path == "/repos/alice/first/readme": + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"Forbidden","documentation_url":"https://docs.github.com"}`)) + case r.URL.Path == "/repos/alice/first/languages": + _, _ = w.Write([]byte(`{"Go":1200}`)) + case r.URL.Path == "/repos/alice/first/contents": + _, _ = w.Write([]byte(`[{"name":"main.go"}]`)) + case r.URL.Path == "/repos/alice/second/readme": + _, _ = w.Write([]byte(`{"path":"README.md","encoding":"base64","size":120,"content":"` + evidenceReadmeBase64 + `"}`)) + case r.URL.Path == "/repos/alice/second/languages": + _, _ = w.Write([]byte(`{"TypeScript":800}`)) + case r.URL.Path == "/repos/alice/second/contents": + _, _ = w.Write([]byte(`[{"name":"src"}]`)) + default: + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"Not Found"}`)) + } + }) + + repos := []TopRepo{ + {Name: "first", OwnerLogin: stringPointer("alice"), Stars: 40}, + {Name: "second", OwnerLogin: stringPointer("alice"), Stars: 20}, + } + if err := client.hydrateTopRepoEvidence(context.Background(), repos, "alice", 6); err != nil { + t.Fatalf("hydrateTopRepoEvidence returned error: %v", err) + } + + // The forbidden README must not stop the loop: the second repository still gets + // its README, languages and root listing. + if requested["/repos/alice/second/readme"] == 0 { + t.Errorf("second repository README was never requested; paths seen = %v", requested) + } + if repos[1].Readme == nil { + t.Fatalf("second repository lost its README despite only the first being forbidden") + } + if len(repos[1].Languages) != 1 || repos[1].Languages[0].Name != "TypeScript" { + t.Errorf("second repository languages = %#v", repos[1].Languages) + } + if len(repos[1].RootPaths) != 1 || repos[1].RootPaths[0] != "src" { + t.Errorf("second repository root paths = %#v", repos[1].RootPaths) + } + // The degraded repository keeps its other evidence and simply has no README. + if repos[0].Readme != nil { + t.Errorf("first repository should have no README, got %#v", repos[0].Readme) + } + if len(repos[0].Languages) != 1 { + t.Errorf("first repository languages = %#v, want the languages call to have succeeded", repos[0].Languages) + } +} + +func TestHydrateTopRepoEvidenceContinuesAfterForbiddenLanguages(t *testing.T) { + client := newEvidenceTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Ratelimit-Remaining", "4998") + switch r.URL.Path { + case "/repos/alice/first/readme", "/repos/alice/second/readme": + _, _ = w.Write([]byte(`{"path":"README.md","encoding":"base64","size":120,"content":"` + evidenceReadmeBase64 + `"}`)) + case "/repos/alice/first/languages": + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"Forbidden"}`)) + case "/repos/alice/second/languages": + _, _ = w.Write([]byte(`{"TypeScript":800}`)) + default: + _, _ = w.Write([]byte(`[]`)) + } + }) + + repos := []TopRepo{{Name: "first", OwnerLogin: stringPointer("alice")}, {Name: "second", OwnerLogin: stringPointer("alice")}} + if err := client.hydrateTopRepoEvidence(context.Background(), repos, "alice", 6); err != nil { + t.Fatalf("a plain languages 403 must not abort later evidence: %v", err) + } + if repos[1].Readme == nil || len(repos[1].Languages) != 1 { + t.Fatalf("second repository was not hydrated: %#v", repos[1]) + } + if len(repos[0].Languages) != 0 || repos[0].Readme == nil { + t.Fatalf("forbidden languages should retain the README and omit languages: %#v", repos[0]) + } +} + +func TestHydrateTopRepoEvidenceStillAbortsOnRateLimit(t *testing.T) { + requested := map[string]int{} + client := newEvidenceTestClient(t, func(w http.ResponseWriter, r *http.Request) { + requested[r.URL.Path]++ + w.Header().Set("Content-Type", "application/json") + if r.URL.Path == "/repos/alice/first/readme" { + // A real rate-limit signal: remaining == 0. This is ErrGitHubRateLimited, + // not ErrGitHubUnavailable, and must still stop collection. + w.Header().Set("X-Ratelimit-Remaining", "0") + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"API rate limit exceeded"}`)) + return + } + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"Not Found"}`)) + }) + + repos := []TopRepo{ + {Name: "first", OwnerLogin: stringPointer("alice"), Stars: 40}, + {Name: "second", OwnerLogin: stringPointer("alice"), Stars: 20}, + } + err := client.hydrateTopRepoEvidence(context.Background(), repos, "alice", 6) + if !errors.Is(err, ErrGitHubRateLimited) { + t.Fatalf("expected ErrGitHubRateLimited after retries, got %v", err) + } + if requested["/repos/alice/first/readme"] != 2 { + t.Errorf("primary rate-limit response should be retried once, got %d requests", requested["/repos/alice/first/readme"]) + } + if requested["/repos/alice/second/readme"] != 0 { + t.Errorf("rate limited pool should abort, but later repos were requested: %v", requested) + } +} + +func TestHydrateTopRepoEvidenceTreatsMissingReadmeAsAbsent(t *testing.T) { + // 404 is returned unwrapped as ErrGitHubAccountNotFound and means "no README". + // It must stay non-fatal after the errors.Is change. + client := newEvidenceTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/repos/alice/first/readme": + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"Not Found"}`)) + case "/repos/alice/first/languages": + _, _ = w.Write([]byte(`{"Go":10}`)) + case "/repos/alice/first/contents": + _, _ = w.Write([]byte(`[]`)) + default: + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"Not Found"}`)) + } + }) + repos := []TopRepo{{Name: "first", OwnerLogin: stringPointer("alice"), Stars: 1}} + if err := client.hydrateTopRepoEvidence(context.Background(), repos, "alice", 6); err != nil { + t.Fatalf("a missing README must not abort collection: %v", err) + } + if repos[0].Readme != nil { + t.Errorf("expected no README, got %#v", repos[0].Readme) + } + if valueOrString(repos[0].ReadmeExcerpt, "") != "" { + t.Errorf("expected empty README excerpt, got %q", valueOrString(repos[0].ReadmeExcerpt, "")) + } +}