fix: classify 429 as rate limit and scope 403 permission failures per repo

Signed-off-by: Joana Maia <jmaia@contractor.linuxfoundation.org>

Author: joanagmaia

services/apps/security_best_practices_worker/src/activities/index.ts

@@ -183,9 +183,10 @@ export async function saveOSPSBaselineInsightsToRedis(
   await redisCache.set(key, JSON.stringify(insights), 60 * 60 * 24) // 1 day
 }
 
-// GitHub returns 403 for both rate-limit exhaustion AND persistent permission/SAML failures.
-// Only rate-limit responses should trigger the 1h token-rotation cooldown; permission failures
-// must be marked as invalid so the token isn't retried in a loop.
+// GitHub returns 403 for rate-limit exhaustion AND persistent permission/SAML failures, and
+// 429 for primary rate-limit hits. Only 401 proves the token is globally invalid — SAML/repo
+// permissions vary per org, so a permission 403 on one repo doesn't mean the token is unusable
+// elsewhere.
 function classifyTokenError(output: string, source: string): void {
   // Wall-clock timestamp of the failure. Passed via ApplicationFailure.details so the
   // workflow can persist an accurate rateLimitedAt — workflow code can't call Date.now()
@@ -201,25 +202,30 @@ function classifyTokenError(output: string, source: string): void {
       details: [failedAtMs],
     })
   }
-  if (!output.includes('403')) return
 
-  // Rate-limit 403s from GitHub carry a distinctive body: "API rate limit exceeded" or
-  // "secondary rate limit". Anything else (SAML enforcement, missing scopes, resource
-  // not accessible) is a permission problem and the token won't recover.
-  const isRateLimit = /rate limit|rate_limit|secondary rate/i.test(output)
+  const has403 = output.includes('403')
+  const has429 = output.includes('429')
+  if (!has403 && !has429) return
+
+  // Rate-limit responses carry a distinctive body: "API rate limit exceeded" or
+  // "secondary rate limit". 429 is always a rate-limit signal. 403 without those markers
+  // is a permission problem (SAML enforcement, missing scopes, resource not accessible).
+  const isRateLimit = has429 || /rate limit|rate_limit|secondary rate/i.test(output)
   if (isRateLimit) {
-    svc.log.warn(`Detected 403 rate-limit in ${source} - token rate-limited!`)
+    svc.log.warn(`Detected rate-limit in ${source} - token rate-limited!`)
     throw ApplicationFailure.create({
       message: 'GitHub token rate-limited',
       type: 'Token403Error',
       nonRetryable: true,
       details: [failedAtMs],
     })
   }
-  svc.log.warn(`Detected 403 permission error in ${source} - token lacks required access!`)
+  // Permission 403: this token can't access THIS repo, but may work for others. Signal the
+  // workflow to try a different token without marking this one globally invalid.
+  svc.log.warn(`Detected 403 permission error in ${source} - token lacks access to this repo!`)
   throw ApplicationFailure.create({
-    message: 'GitHub token lacks required permissions',
-    type: 'TokenAuthError',
+    message: 'GitHub token lacks required permissions for this repo',
+    type: 'TokenRepoAccessError',
     nonRetryable: true,
     details: [failedAtMs],
   })

services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts

@@ -89,7 +89,7 @@ export async function triggerSecurityInsightsCheckForRepos(
             initialInterval: 2000,
             backoffCoefficient: 2,
             maximumInterval: 30000,
-            nonRetryableErrorTypes: ['Token403Error', 'TokenAuthError'],
+            nonRetryableErrorTypes: ['Token403Error', 'TokenAuthError', 'TokenRepoAccessError'],
           },
           args: [
             {
@@ -120,6 +120,12 @@ export async function triggerSecurityInsightsCheckForRepos(
           tokenInfo.isInvalid = true
           attempts++
           continue // retry with a different token
+        } else if (appFailure?.type === 'TokenRepoAccessError') {
+          // Token can't access this repo (SAML/perms) but may work elsewhere — don't taint
+          // the token's global state, just try the next one for this repo.
+          console.warn(`Token lacks access to repo ${repo.repoUrl}, trying different token`)
+          attempts++
+          continue
         } else {
           console.error(`Failed to process repo ${repo.repoUrl}:`, error)
           // we retried this error using the retry policy (because it's not a token non-retryable error)