diff --git a/packages/cli/src/commands/scan/create-scan-from-github.mts b/packages/cli/src/commands/scan/create-scan-from-github.mts index 9da610346..f02556bd5 100644 --- a/packages/cli/src/commands/scan/create-scan-from-github.mts +++ b/packages/cli/src/commands/scan/create-scan-from-github.mts @@ -13,7 +13,14 @@ import { REPORT_LEVEL_ERROR } from '../../constants/reporting.mjs' import { formatErrorWithDetail } from '../../utils/error/errors.mjs' import { socketHttpRequest } from '../../utils/socket/api.mjs' import { isReportSupportedFile } from '../../utils/fs/glob.mts' -import { getOctokit, withGitHubRetry } from '../../utils/git/github.mts' +import { + GITHUB_ERR_ABUSE_DETECTION, + GITHUB_ERR_AUTH_FAILED, + GITHUB_ERR_GRAPHQL_RATE_LIMIT, + GITHUB_ERR_RATE_LIMIT, + getOctokit, + withGitHubRetry, +} from '../../utils/git/github.mts' import { fetchListAllRepos } from '../repository/fetch-list-all-repos.mts' import type { CResult, OutputKind } from '../../types.mts' @@ -92,7 +99,17 @@ export async function createScanFromGithub({ } let scansCreated = 0 + let reposScanned = 0 + // Track a blocking error (rate limit / auth) so we can surface it + // instead of reporting silent success with "0 manifests". Without + // this, a rate-limited GitHub token made every repo fail its tree + // fetch, the outer loop swallowed each error, and the final summary + // ("N repos / 0 manifests") misled users into thinking the scan + // worked. + let blockingError: CResult | undefined + const perRepoFailures: Array<{ repo: string; message: string }> = [] for (const repoSlug of targetRepos) { + reposScanned += 1 // eslint-disable-next-line no-await-in-loop const scanCResult = await scanRepo(repoSlug, { githubApiUrl, @@ -107,12 +124,52 @@ export async function createScanFromGithub({ if (scanCreated) { scansCreated += 1 } + continue } + perRepoFailures.push({ + repo: repoSlug, + message: scanCResult.message, + }) + // Stop on rate-limit / auth failures: every subsequent repo will + // fail for the same reason and continuing only burns more quota + // while delaying the real error. + if ( + scanCResult.message === GITHUB_ERR_ABUSE_DETECTION || + scanCResult.message === GITHUB_ERR_AUTH_FAILED || + scanCResult.message === GITHUB_ERR_GRAPHQL_RATE_LIMIT || + scanCResult.message === GITHUB_ERR_RATE_LIMIT + ) { + blockingError = { + ok: false, + message: scanCResult.message, + cause: scanCResult.cause, + } + break + } + } + + if (blockingError) { + logger.fail(blockingError.message) + return blockingError } - logger.success(targetRepos.length, 'GitHub repos detected') + logger.success(reposScanned, 'GitHub repos processed') logger.success(scansCreated, 'with supported Manifest files') + // If every repo failed but not for a known-blocking reason, treat + // the run as an error so scripts know something went wrong instead + // of inferring success from an ok: true with 0 scans. + if (reposScanned > 0 && scansCreated === 0 && perRepoFailures.length === reposScanned) { + const firstFailure = perRepoFailures[0]! + return { + ok: false, + message: 'All repos failed to scan', + cause: + `All ${reposScanned} repos failed to scan. First failure for ${firstFailure.repo}: ${firstFailure.message}. ` + + 'Check the log above for per-repo details.', + } + } + return { ok: true, data: undefined, diff --git a/packages/cli/src/utils/git/github.mts b/packages/cli/src/utils/git/github.mts index 3f1f5b815..9b20f9c53 100644 --- a/packages/cli/src/utils/git/github.mts +++ b/packages/cli/src/utils/git/github.mts @@ -54,6 +54,15 @@ import type { SpawnOptions } from '@socketsecurity/lib/spawn' export type Pr = components['schemas']['pull-request'] +// Canonical `message` values returned by `handleGitHubApiError` / +// `handleGraphqlError`. Exported so callers can short-circuit on +// blocking conditions without matching free-form strings. +export const GITHUB_ERR_ABUSE_DETECTION = 'GitHub abuse detection triggered' +export const GITHUB_ERR_AUTH_FAILED = 'GitHub authentication failed' +export const GITHUB_ERR_GRAPHQL_RATE_LIMIT = + 'GitHub GraphQL rate limit exceeded' +export const GITHUB_ERR_RATE_LIMIT = 'GitHub rate limit exceeded' + interface CacheEntry { timestamp: number data: JsonContent @@ -398,7 +407,7 @@ export function handleGraphqlError( if (isGraphqlRateLimitError(e)) { return { ok: false, - message: 'GitHub GraphQL rate limit exceeded', + message: GITHUB_ERR_GRAPHQL_RATE_LIMIT, cause: `GitHub GraphQL rate limit exceeded while ${context}. ` + 'Try again in a few minutes.\n\n' + @@ -440,7 +449,7 @@ export function handleGitHubApiError( if (status === 403 && e.message.includes('secondary rate limit')) { return { ok: false, - message: 'GitHub abuse detection triggered', + message: GITHUB_ERR_ABUSE_DETECTION, cause: `GitHub abuse detection triggered while ${context}. ` + 'This happens when making too many requests in a short period. ' + @@ -474,7 +483,7 @@ export function handleGitHubApiError( return { ok: false, - message: 'GitHub rate limit exceeded', + message: GITHUB_ERR_RATE_LIMIT, cause: `GitHub API rate limit exceeded while ${context}. ` + (waitTime @@ -492,7 +501,7 @@ export function handleGitHubApiError( if (status === 401) { return { ok: false, - message: 'GitHub authentication failed', + message: GITHUB_ERR_AUTH_FAILED, cause: `GitHub authentication failed while ${context}. ` + 'Your token may be invalid, expired, or missing required permissions.\n\n' + diff --git a/packages/cli/test/unit/commands/scan/create-scan-from-github.test.mts b/packages/cli/test/unit/commands/scan/create-scan-from-github.test.mts index 654a4fd49..ca8550f9a 100644 --- a/packages/cli/test/unit/commands/scan/create-scan-from-github.test.mts +++ b/packages/cli/test/unit/commands/scan/create-scan-from-github.test.mts @@ -51,6 +51,10 @@ const mockWithGitHubRetry = vi.hoisted(() => // Mock dependencies. vi.mock('../../../../src/utils/git/github.mts', () => ({ + GITHUB_ERR_ABUSE_DETECTION: 'GitHub abuse detection triggered', + GITHUB_ERR_AUTH_FAILED: 'GitHub authentication failed', + GITHUB_ERR_GRAPHQL_RATE_LIMIT: 'GitHub GraphQL rate limit exceeded', + GITHUB_ERR_RATE_LIMIT: 'GitHub rate limit exceeded', getOctokit: vi.fn(() => mockOctokit), withGitHubRetry: mockWithGitHubRetry, })) @@ -428,3 +432,164 @@ describe('error message quality', () => { } }) }) + +// Regression tests: the bulk loop in createScanFromGithub used to +// swallow per-repo failures, so a rate-limited token returned ok:true +// with "0 manifests". These drive the full function through mocked +// octokit calls. +describe('createScanFromGithub rate-limit short-circuit', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('returns ok:false and stops the loop on GitHub rate limit', async () => { + // First call (getRepoDetails for repo-a) fails with rate limit. + mockWithGitHubRetry.mockResolvedValueOnce({ + ok: false, + message: 'GitHub rate limit exceeded', + cause: 'GitHub API rate limit exceeded.', + }) + + const { createScanFromGithub } = await import( + '../../../../src/commands/scan/create-scan-from-github.mts' + ) + + const result = await createScanFromGithub({ + all: false, + githubApiUrl: '', + githubToken: '', + interactive: false, + orgGithub: 'org', + orgSlug: 'org', + outputKind: 'text', + repos: 'repo-a,repo-b,repo-c', + }) + + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.message).toBe('GitHub rate limit exceeded') + } + // Short-circuit: only the first repo's getRepoDetails should have run. + expect(mockWithGitHubRetry).toHaveBeenCalledTimes(1) + }) + + it('returns ok:false and stops on GitHub GraphQL rate limit', async () => { + mockWithGitHubRetry.mockResolvedValueOnce({ + ok: false, + message: 'GitHub GraphQL rate limit exceeded', + cause: 'GraphQL rate limit hit.', + }) + + const { createScanFromGithub } = await import( + '../../../../src/commands/scan/create-scan-from-github.mts' + ) + + const result = await createScanFromGithub({ + all: false, + githubApiUrl: '', + githubToken: '', + interactive: false, + orgGithub: 'org', + orgSlug: 'org', + outputKind: 'text', + repos: 'repo-a,repo-b,repo-c', + }) + + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.message).toBe('GitHub GraphQL rate limit exceeded') + } + expect(mockWithGitHubRetry).toHaveBeenCalledTimes(1) + }) + + it('returns ok:false and stops on GitHub abuse detection', async () => { + mockWithGitHubRetry.mockResolvedValueOnce({ + ok: false, + message: 'GitHub abuse detection triggered', + cause: 'Secondary rate limit hit.', + }) + + const { createScanFromGithub } = await import( + '../../../../src/commands/scan/create-scan-from-github.mts' + ) + + const result = await createScanFromGithub({ + all: false, + githubApiUrl: '', + githubToken: '', + interactive: false, + orgGithub: 'org', + orgSlug: 'org', + outputKind: 'text', + repos: 'repo-a,repo-b,repo-c', + }) + + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.message).toBe('GitHub abuse detection triggered') + } + expect(mockWithGitHubRetry).toHaveBeenCalledTimes(1) + }) + + it('returns ok:false and stops on GitHub auth failure', async () => { + mockWithGitHubRetry.mockResolvedValueOnce({ + ok: false, + message: 'GitHub authentication failed', + cause: 'Bad credentials.', + }) + + const { createScanFromGithub } = await import( + '../../../../src/commands/scan/create-scan-from-github.mts' + ) + + const result = await createScanFromGithub({ + all: false, + githubApiUrl: '', + githubToken: '', + interactive: false, + orgGithub: 'org', + orgSlug: 'org', + outputKind: 'text', + repos: 'repo-a,repo-b', + }) + + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.message).toBe('GitHub authentication failed') + } + expect(mockWithGitHubRetry).toHaveBeenCalledTimes(1) + }) + + it('returns "All repos failed to scan" when every repo errors with a non-blocking reason', async () => { + // Each repo's getRepoDetails fails with a non-rate-limit error; + // the loop should finish all repos and return the catch-all error. + mockWithGitHubRetry.mockResolvedValue({ + ok: false, + message: 'GitHub resource not found', + cause: 'Not found.', + }) + + const { createScanFromGithub } = await import( + '../../../../src/commands/scan/create-scan-from-github.mts' + ) + + const result = await createScanFromGithub({ + all: false, + githubApiUrl: '', + githubToken: '', + interactive: false, + orgGithub: 'org', + orgSlug: 'org', + outputKind: 'text', + repos: 'repo-a,repo-b', + }) + + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.message).toBe('All repos failed to scan') + expect(result.cause).toContain('repo-a') + } + // Both repos should have been attempted (no short-circuit for 404). + expect(mockWithGitHubRetry).toHaveBeenCalledTimes(2) + }) +})