Uh oh!
There was an error while loading. Please reload this page.
fix(backend): recover entitlements after license sync - #1454
Conversation
This comment has been minimized.
This comment has been minimized.
WalkthroughGitHub App initialization is now asynchronous, shared across concurrent callers, and retryable after failure. Authentication errors propagate explicitly. Permission sync schedulers remain active without entitlements and delay jobs for later retries. ChangesEntitlement-aware services
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant GithubAppManager
participant Entitlements
participant GitHub
Request->>GithubAppManager: ensureInitialized()
Request->>Entitlements: check github-app
Request->>GithubAppManager: getInstallationToken()
GithubAppManager->>GitHub: retrieve installation token
GitHub-->>Request: authenticated Octokit client
sequenceDiagram
participant Scheduler
participant PermissionSyncWorker
participant Entitlements
participant BullMQ
Scheduler->>Entitlements: check permission-syncing
PermissionSyncWorker->>Entitlements: re-check entitlement
PermissionSyncWorker->>BullMQ: move job to delayed state
BullMQ-->>PermissionSyncWorker: DelayedError
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/backend/src/utils.ts (1)
135-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEntitlement-gated token flow duplicates
github.ts::getOctokitWithGithubApp.Same ensureInitialized → appsConfigured → hasEntitlement('github-app') → getInstallationToken sequence as in
packages/backend/src/github.ts(Lines 123-151). See consolidated comment for a shared-helper suggestion.🤖 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/utils.ts` around lines 135 - 163, Refactor the GitHub App authentication branch in the repository token flow to reuse the existing shared logic from github.ts::getOctokitWithGithubApp, preserving entitlement validation, initialization, app configuration checks, and installation-token acquisition. Remove the duplicated ensureInitialized/appsConfigured/hasEntitlement/getInstallationToken sequence while retaining the current repository-specific URL and clone-token construction.packages/backend/src/github.ts (1)
123-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEntitlement-gated token flow is duplicated in
utils.ts::getAuthCredentialsForRepo.The
ensureInitialized()→appsConfigured()→hasEntitlement('github-app')→getInstallationToken()sequence here is re-implemented almost verbatim inpackages/backend/src/utils.ts(Lines 135-152). Worth extracting into a shared helper to avoid the two call sites drifting (e.g., diverging error messages or entitlement logic) as this code evolves.🤖 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/github.ts` around lines 123 - 151, Extract the shared GitHub App initialization, configuration, entitlement validation, and installation-token retrieval logic from getOctokitWithGithubApp into a reusable helper, then update both getOctokitWithGithubApp and utils.ts::getAuthCredentialsForRepo to use it. Preserve the existing behavior, context-aware entitlement checks, hostname handling, and error propagation while removing the duplicated sequence.packages/backend/src/ee/githubAppManager.test.ts (1)
28-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
getInstallationToken()/assertInitialized().The suite covers
ensureInitialized()single-flight and retry semantics well, but the companion guard change —getInstallationToken()now throwing viaassertInitialized()when called before a successfulensureInitialized()— has no direct test here. Worth adding a case asserting it throws pre-init and resolves correctly post-init, since this guard is the other half of the entitlement-recovery contract this PR introduces.🤖 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/githubAppManager.test.ts` around lines 28 - 57, Add a test in the GithubAppManager.ensureInitialized suite covering getInstallationToken and assertInitialized: verify getInstallationToken throws before ensureInitialized succeeds, then initialize the manager and verify it returns the expected token afterward. Reuse the existing mocks and manager setup, preserving the current concurrency and retry tests.
🤖 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/githubAppManager.test.ts`:
- Around line 28-57: Add a test in the GithubAppManager.ensureInitialized suite
covering getInstallationToken and assertInitialized: verify getInstallationToken
throws before ensureInitialized succeeds, then initialize the manager and verify
it returns the expected token afterward. Reuse the existing mocks and manager
setup, preserving the current concurrency and retry tests.
In `@packages/backend/src/github.ts`:
- Around line 123-151: Extract the shared GitHub App initialization,
configuration, entitlement validation, and installation-token retrieval logic
from getOctokitWithGithubApp into a reusable helper, then update both
getOctokitWithGithubApp and utils.ts::getAuthCredentialsForRepo to use it.
Preserve the existing behavior, context-aware entitlement checks, hostname
handling, and error propagation while removing the duplicated sequence.
In `@packages/backend/src/utils.ts`:
- Around line 135-163: Refactor the GitHub App authentication branch in the
repository token flow to reuse the existing shared logic from
github.ts::getOctokitWithGithubApp, preserving entitlement validation,
initialization, app configuration checks, and installation-token acquisition.
Remove the duplicated
ensureInitialized/appsConfigured/hasEntitlement/getInstallationToken sequence
while retaining the current repository-specific URL and clone-token
construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13ddd89f-3e76-4df4-9a1b-e8a438a0bbac
📒 Files selected for processing (9)
CHANGELOG.mdpackages/backend/src/ee/accountPermissionSyncer.tspackages/backend/src/ee/githubAppManager.test.tspackages/backend/src/ee/githubAppManager.tspackages/backend/src/ee/repoPermissionSyncer.tspackages/backend/src/github.tspackages/backend/src/githubAppAuth.test.tspackages/backend/src/index.tspackages/backend/src/utils.ts
Fixes SOU-1511
Summary
Testing
Note
Medium Risk
Changes GitHub connection sync and repo auth to error instead of PAT fallback when GitHub Apps are configured, and shifts permission-sync scheduling—both affect EE auth and repository visibility after license changes.
Overview
Fixes worker behavior when EE entitlements arrive after startup (e.g. online license refresh) by removing one-time entitlement gates at boot and re-checking at use time.
GitHub App: Startup no longer calls
GithubAppManager.init()only whengithub-appis licensed at launch.ensureInitialized()loads config on first need, dedupes concurrent init, and retries after transient failures. When apps are configured in config,getOctokitWithGithubAppandgetAuthCredentialsForReporequire thegithub-appentitlement and throw instead of silently falling back to PATs—so connection sync cannot treat a partial repo list as complete. Token/installation errors propagate the same way.Permission sync: Schedulers always start when
PERMISSION_SYNC_ENABLEDis true; polling skips enqueueing without the entitlement, and in-flight jobs delay 30s and retry via BullMQDelayedErrorifpermission-syncingis missing mid-run.Tests cover concurrent/retry init and GitHub App auth entitlement and no-fallback behavior.
Reviewed by Cursor Bugbot for commit a19d611. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit