fix(worker): classify permission sync failures by provider context

[brendan-kellam]

Replace generic HTTP status fail-closed handling with typed upstream permission-sync errors so ambiguous forbidden and gone responses preserve cached permissions.

Fixes SOU-1560

Fixes SOU-1177

---

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

---

Note

Medium Risk
Changes when cached repository permissions are wiped during sync failures, which affects authorization visibility until the next successful sync; the new preserve-on-ambiguous-error behavior is safer but is still security-sensitive EE logic.

Overview
Replaces fail-closed cleanup on raw HTTP 401/403/410 during account permission sync with typed upstream errors that record provider and operation before deciding whether to clear cached repo permissions.

Adds PermissionSyncUpstreamError and wraps GitHub, GitLab, and Bitbucket scope checks and repo listing in withPermissionSyncUpstreamError, which classifies responses (e.g. 401 → credential rejected, rate limits via status/headers, 5xx/timeouts → transient). Missing OAuth scopes are raised as insufficient_scope instead of generic errors. classifyPermissionSyncFailure now clears permissions only for oauth_invalid_grant, credential_rejected, and insufficient_scope; 403 forbidden, rate limits, 410, and other unclassified HTTP errors preserve the last good permission set.

errors.ts gains getErrorStatus / getErrorHeader so classification works across Octokit, GitLab, and Bitbucket error shapes.

^{Reviewed by Cursor Bugbot for commit a985fac. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• Bug Fixes
  • Improved permission synchronization failure handling across supported code hosts.
  • Cached repository permissions are now cleared only for confirmed credential rejection or insufficient access scope.
  • Existing permissions are preserved during rate limits, temporary outages, and other uncertain failures.
  • Improved detection of HTTP status codes and rate-limit information from provider errors.
• Documentation
  • Updated the unreleased changelog with the permission synchronization fix.

[coderabbitai]

Walkthrough

Permission synchronization now normalizes HTTP error metadata, classifies upstream failures by provider and operation, wraps provider API calls, and clears cached permissions only for classified credential rejection or insufficient scope failures.

Changes

Permission sync classification

Layer / File(s)	Summary
HTTP error metadata normalization packages/backend/src/errors.ts, packages/backend/src/errors.test.ts	HTTP status and headers are normalized across direct and nested error shapes; existing HTTP predicates use the normalized status.
Upstream error classifier packages/backend/src/ee/permissionSyncError.ts, packages/backend/src/ee/permissionSyncError.test.ts	Adds structured upstream error kinds, provider and operation metadata, rate-limit detection, availability detection, and wrapper behavior.
Permission sync cleanup integration packages/backend/src/ee/accountPermissionSyncer.ts, packages/backend/src/ee/accountPermissionSyncer.test.ts, CHANGELOG.md	Provider scope and repository operations are wrapped for classification; credential rejection and insufficient scope clear permissions, while other classified failures preserve them.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant syncAccountPermissions
  participant withPermissionSyncUpstreamError
  participant classifyPermissionSyncUpstreamError
  participant classifyPermissionSyncFailure
  syncAccountPermissions->>withPermissionSyncUpstreamError: run provider scope or repository operation
  withPermissionSyncUpstreamError->>classifyPermissionSyncUpstreamError: classify thrown upstream error
  classifyPermissionSyncUpstreamError-->>withPermissionSyncUpstreamError: return PermissionSyncUpstreamError
  withPermissionSyncUpstreamError-->>classifyPermissionSyncFailure: propagate classified failure
  classifyPermissionSyncFailure-->>syncAccountPermissions: clear or preserve cached permissions

Loading

Possibly related PRs

• sourcebot-dev/sourcebot#1215: Changes permission-sync failure classification and stale permission cleanup behavior.
• sourcebot-dev/sourcebot#1476: Modifies HTTP status and header extraction around upstream error handling.
• sourcebot-dev/sourcebot#1481: Updates permission-sync cleanup decisions for classified failures.

Suggested reviewers: msukkari

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: permission sync failures are now classified using provider context.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch brendan/classify-permission-sync-errors

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[brendan-kellam]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (1)

packages/backend/src/ee/permissionSyncError.ts (1)

45-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Broad TypeError match risks masking internal bugs as "provider unavailable".

Any TypeError thrown inside a wrapped callback (including unrelated coding bugs, not just fetch network failures) gets classified as upstream_unavailable, producing a misleading "provider is temporarily unavailable" log/kind for what could be an internal defect. Since both upstream_unavailable and unknown currently map to preserve_permissions downstream, there's no behavioral difference today, but it does reduce the accuracy of the error kind for incident triage.

Consider narrowing the check to more specific network-failure signals (e.g., error.cause instanceof Error && error.cause.message.includes('fetch failed'), or checking for ECONNRESET/ETIMEDOUT style codes from undici) rather than any TypeError.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/backend/src/ee/permissionSyncError.ts` around lines 45 - 51, Narrow
isNetworkOrTimeoutError so it does not classify every TypeError as an upstream
outage; rely on specific network or timeout indicators such as fetch-failure
causes or recognized undici connection error codes, while preserving AbortError
and TimeoutError handling.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/backend/src/ee/permissionSyncError.ts`:
- Around line 45-51: Narrow isNetworkOrTimeoutError so it does not classify
every TypeError as an upstream outage; rely on specific network or timeout
indicators such as fetch-failure causes or recognized undici connection error
codes, while preserving AbortError and TimeoutError handling.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d5d503ae-a1b0-4ab5-9c97-e73741dd87df

📥 Commits

Reviewing files that changed from the base of the PR and between 0954bf8 and a985fac.

📒 Files selected for processing (7)

• CHANGELOG.md
• packages/backend/src/ee/accountPermissionSyncer.test.ts
• packages/backend/src/ee/accountPermissionSyncer.ts
• packages/backend/src/ee/permissionSyncError.test.ts
• packages/backend/src/ee/permissionSyncError.ts
• packages/backend/src/errors.test.ts
• packages/backend/src/errors.ts