Skip to content

fix(github): classify backfill permission 403s as errors, not rate limits#1747

Open
joaovictor91123 wants to merge 1 commit into
JSONbored:mainfrom
joaovictor91123:fix/backfill-permission-403-not-rate-limit
Open

fix(github): classify backfill permission 403s as errors, not rate limits#1747
joaovictor91123 wants to merge 1 commit into
JSONbored:mainfrom
joaovictor91123:fix/backfill-permission-403-not-rate-limit

Conversation

@joaovictor91123

Copy link
Copy Markdown
Contributor

Summary

Fixes #1746.

GitHubApiError in src/github/backfill.ts flagged every failure as rate-limited with statusCode === 403 || statusCode === 429 || remainingHeader === "0", so a genuine permission 403Resource not accessible by integration, a missing scope, or branch protection — with x-ratelimit-remaining still above 0 and no Retry-After was misclassified as a rate limit. That pushed it into the rate-limit branches: backfillRepositorySegment (:1246) set the segment waiting_rate_limit and applied backoff, and the metadata path (:1692) set repo status rate_limited / dataQuality.rateLimited: true, instead of surfacing the permission error to the operator.

src/github/app.ts already solved this for the live-review path with isRateLimitedResponse (a 403/429 is a rate limit only with a Retry-After, an exhausted x-ratelimit-remaining, or a secondary-limit/abuse body), but the backfill REST/GraphQL path uses raw fetch and its own error class and was never aligned. This adds a shared isRateLimitedGitHubFailure predicate mirroring that logic and threads the retry-after header and response body into GitHubApiError.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥97% coverage of the lines AND branches you changed (aim for 98%+ on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

Ran the full npm run test:ci (green) plus npm audit --audit-level=moderate (0 vulnerabilities). Added unit tests for isRateLimitedGitHubFailure (permission 403, exhausted-remaining, Retry-After on 403 and 429, secondary-limit body, bare 429, non-403/429) and a backfill regression test asserting a permission 403 yields an error segment with dataQuality.rateLimited: false, not rate_limited.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with screenshots.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

Backend-only error-classification change; no auth/CORS/session/OpenAPI/UI surface is touched (ui:openapi:check passes), so no UI evidence applies. The new behavior is exercised by the negative-path predicate tests and the backfill regression test.

Notes

  • The predicate is intentionally a local mirror of app.ts's isRateLimitedResponse rather than a refactor of app.ts, keeping this PR scoped to the backfill error path.
  • A bare 429 now requires a rate-limit signal (matching isRateLimitedResponse); GitHub 429s carry Retry-After, so live behavior is unchanged for real secondary limits.

…mits (JSONbored#1746)

GitHubApiError flagged every 403/429 as rate-limited, so a genuine permission
403 — "Resource not accessible by integration", a missing scope, branch
protection — with x-ratelimit-remaining still above 0 and no Retry-After was
recorded as a rate limit: backfillRepositorySegment set the segment
waiting_rate_limit and backed off, and the metadata path set repo status
rate_limited, instead of surfacing the error. Mirror app.ts's
isRateLimitedResponse with a shared isRateLimitedGitHubFailure predicate
(403/429 plus Retry-After / exhausted x-ratelimit-remaining / secondary-limit
body), threading the retry-after header and response body into GitHubApiError.
Covered by predicate unit tests and a backfill regression test.

Co-authored-by: Cursor <[email protected]>
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jun 29, 2026
@gittensory-orb

gittensory-orb Bot commented Jun 29, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-06-29 17:36:14 UTC

2 files · 1 AI reviewer · no blockers · readiness 55/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change aligns the backfill REST/GraphQL error classification with the live GitHub path: bare permission-style 403s now surface as segment errors instead of being recorded as rate-limit waits, while Retry-After, exhausted remaining budget, and secondary-limit bodies still classify as rate limits. The touched call sites pass the new response context into GitHubApiError, and the added integration-style test exercises the real backfill segment path that previously misclassified the 403. The most notable maintainability cost is that the rate-limit predicate is now duplicated from src/github/app.ts rather than shared, which can drift as GitHub handling evolves.

Nits — 6 non-blocking
  • nit: src/github/backfill.ts:2700 duplicates the live-path rate-limit predicate from src/github/app.ts; consider extracting a shared helper so future GitHub response-classification changes land in one place.
  • nit: test/unit/backfill.test.ts:3226 covers the exported predicate well, but it does not explicitly exercise the `api rate limit exceeded` body variant that the regex accepts.
  • Move the common `isRateLimitedResponse`/`isRateLimitedGitHubFailure` status-header-body logic into a small shared GitHub rate-limit helper and have both src/github/app.ts and src/github/backfill.ts call it.
  • Add one focused unit assertion in test/unit/backfill.test.ts for `body: "API rate limit exceeded"` so every documented accepted body phrase has a regression test.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #1746
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (size label size:S; 1 linked issue).
Validation posture ❌ 5/25 Preflight is holding this PR; address the blocker before review.
Contributor workload ✅ 10/10 Author activity: 35 registered-repo PR(s), 17 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor joaovictor91123; Gittensor profile; 35 PR(s), 0 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
Contributor next steps
  • Review top overlaps.
  • Add a concise scope and risk note.
  • Fix the blocker.
  • Triage stale or unlinked PRs.
  • Refresh registry data or choose a registered active repo.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added gittensor Gittensor contributor context gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. labels Jun 29, 2026
@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.59%. Comparing base (ce1d354) to head (8ac89c7).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1747   +/-   ##
=======================================
  Coverage   95.59%   95.59%           
=======================================
  Files         204      204           
  Lines       22316    22320    +4     
  Branches     8067     8069    +2     
=======================================
+ Hits        21332    21336    +4     
  Misses        408      408           
  Partials      576      576           
Files with missing lines Coverage Δ
src/github/backfill.ts 92.98% <100.00%> (+0.03%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. gittensor Gittensor contributor context size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: GitHub backfill misclassifies every 403 (including permission errors) as a rate limit

1 participant