Skip to content

Commit 7f7f649

Browse files
committed
fix(scan): surface GitHub rate-limit errors in bulk repo scan
The bulk-scan loop in `createScanFromGithub` silently swallowed per-repo failures. A rate-limited token made every repo fail, yet the outer function returned ok:true with "N repos / 0 manifests", misleading users into thinking the scan worked. - Track per-repo failures and short-circuit the loop on known blocking errors (rate limit, GraphQL rate limit, abuse detection, auth failure) so subsequent repos don't burn more quota for the same error - Return an error CResult when all repos fail, so scripts can tell the run did not succeed - Add regression tests covering the short-circuit and the all-repos-failed fallback
1 parent 64a14c5 commit 7f7f649

2 files changed

Lines changed: 156 additions & 1 deletion

File tree

packages/cli/src/commands/scan/create-scan-from-github.mts

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,17 @@ export async function createScanFromGithub({
9292
}
9393

9494
let scansCreated = 0
95+
let reposScanned = 0
96+
// Track a blocking error (rate limit / auth) so we can surface it
97+
// instead of reporting silent success with "0 manifests". Without
98+
// this, a rate-limited GitHub token made every repo fail its tree
99+
// fetch, the outer loop swallowed each error, and the final summary
100+
// ("N repos / 0 manifests") misled users into thinking the scan
101+
// worked. See ASK-167.
102+
let blockingError: CResult<undefined> | undefined
103+
const perRepoFailures: Array<{ repo: string; message: string }> = []
95104
for (const repoSlug of targetRepos) {
105+
reposScanned += 1
96106
// eslint-disable-next-line no-await-in-loop
97107
const scanCResult = await scanRepo(repoSlug, {
98108
githubApiUrl,
@@ -107,12 +117,54 @@ export async function createScanFromGithub({
107117
if (scanCreated) {
108118
scansCreated += 1
109119
}
120+
continue
121+
}
122+
perRepoFailures.push({
123+
repo: repoSlug,
124+
message: scanCResult.message,
125+
})
126+
// Stop the loop if we hit a rate limit or auth failure — every
127+
// subsequent repo will fail for the same reason and continuing
128+
// only burns more quota while delaying the real error. Strings
129+
// here match `handleGitHubApiError` / `handleGraphqlError` in
130+
// utils/git/github.mts.
131+
if (
132+
scanCResult.message === 'GitHub rate limit exceeded' ||
133+
scanCResult.message === 'GitHub GraphQL rate limit exceeded' ||
134+
scanCResult.message === 'GitHub abuse detection triggered' ||
135+
scanCResult.message === 'GitHub authentication failed'
136+
) {
137+
blockingError = {
138+
ok: false,
139+
message: scanCResult.message,
140+
cause: scanCResult.cause,
141+
}
142+
break
110143
}
111144
}
112145

113-
logger.success(targetRepos.length, 'GitHub repos detected')
146+
logger.success(reposScanned, 'GitHub repos processed')
114147
logger.success(scansCreated, 'with supported Manifest files')
115148

149+
if (blockingError) {
150+
logger.fail(blockingError.message)
151+
return blockingError
152+
}
153+
154+
// If every repo failed but not for a known-blocking reason, treat
155+
// the run as an error so scripts know something went wrong instead
156+
// of inferring success from an ok: true with 0 scans.
157+
if (reposScanned > 0 && scansCreated === 0 && perRepoFailures.length === reposScanned) {
158+
const firstFailure = perRepoFailures[0]!
159+
return {
160+
ok: false,
161+
message: 'All repos failed to scan',
162+
cause:
163+
`All ${reposScanned} repos failed to scan. First failure for ${firstFailure.repo}: ${firstFailure.message}. ` +
164+
'Check the log above for per-repo details.',
165+
}
166+
}
167+
116168
return {
117169
ok: true,
118170
data: undefined,

packages/cli/test/unit/commands/scan/create-scan-from-github.test.mts

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,3 +428,106 @@ describe('error message quality', () => {
428428
}
429429
})
430430
})
431+
432+
// Regression tests for ASK-167: the bulk loop in createScanFromGithub
433+
// used to swallow per-repo failures, so a rate-limited token returned
434+
// ok:true with "0 manifests". These drive the full function through
435+
// mocked octokit calls.
436+
describe('createScanFromGithub rate-limit short-circuit (ASK-167)', () => {
437+
beforeEach(() => {
438+
vi.clearAllMocks()
439+
})
440+
441+
it('returns ok:false and stops the loop on GitHub rate limit', async () => {
442+
// First call (getRepoDetails for repo-a) fails with rate limit.
443+
mockWithGitHubRetry.mockResolvedValueOnce({
444+
ok: false,
445+
message: 'GitHub rate limit exceeded',
446+
cause: 'GitHub API rate limit exceeded.',
447+
})
448+
449+
const { createScanFromGithub } = await import(
450+
'../../../../src/commands/scan/create-scan-from-github.mts'
451+
)
452+
453+
const result = await createScanFromGithub({
454+
all: false,
455+
githubApiUrl: '',
456+
githubToken: '',
457+
interactive: false,
458+
orgGithub: 'org',
459+
orgSlug: 'org',
460+
outputKind: 'text',
461+
repos: 'repo-a,repo-b,repo-c',
462+
})
463+
464+
expect(result.ok).toBe(false)
465+
if (!result.ok) {
466+
expect(result.message).toBe('GitHub rate limit exceeded')
467+
}
468+
// Short-circuit: only the first repo's getRepoDetails should have run.
469+
expect(mockWithGitHubRetry).toHaveBeenCalledTimes(1)
470+
})
471+
472+
it('returns ok:false and stops on GitHub auth failure', async () => {
473+
mockWithGitHubRetry.mockResolvedValueOnce({
474+
ok: false,
475+
message: 'GitHub authentication failed',
476+
cause: 'Bad credentials.',
477+
})
478+
479+
const { createScanFromGithub } = await import(
480+
'../../../../src/commands/scan/create-scan-from-github.mts'
481+
)
482+
483+
const result = await createScanFromGithub({
484+
all: false,
485+
githubApiUrl: '',
486+
githubToken: '',
487+
interactive: false,
488+
orgGithub: 'org',
489+
orgSlug: 'org',
490+
outputKind: 'text',
491+
repos: 'repo-a,repo-b',
492+
})
493+
494+
expect(result.ok).toBe(false)
495+
if (!result.ok) {
496+
expect(result.message).toBe('GitHub authentication failed')
497+
}
498+
expect(mockWithGitHubRetry).toHaveBeenCalledTimes(1)
499+
})
500+
501+
it('returns "All repos failed to scan" when every repo errors with a non-blocking reason', async () => {
502+
// Each repo's getRepoDetails fails with a non-rate-limit error;
503+
// the loop should finish all repos and return the catch-all error.
504+
mockWithGitHubRetry.mockResolvedValue({
505+
ok: false,
506+
message: 'GitHub resource not found',
507+
cause: 'Not found.',
508+
})
509+
510+
const { createScanFromGithub } = await import(
511+
'../../../../src/commands/scan/create-scan-from-github.mts'
512+
)
513+
514+
const result = await createScanFromGithub({
515+
all: false,
516+
githubApiUrl: '',
517+
githubToken: '',
518+
interactive: false,
519+
orgGithub: 'org',
520+
orgSlug: 'org',
521+
outputKind: 'text',
522+
repos: 'repo-a,repo-b',
523+
})
524+
525+
expect(result.ok).toBe(false)
526+
if (!result.ok) {
527+
expect(result.message).toBe('All repos failed to scan')
528+
expect(result.cause).toContain('repo-a')
529+
}
530+
// Both repos should have been attempted (no short-circuit for 404).
531+
expect(mockWithGitHubRetry).toHaveBeenCalledTimes(2)
532+
})
533+
})

0 commit comments

Comments
 (0)