From 7f7f6491d34bb2690f7b011ebba63d180ed9ec9a Mon Sep 17 00:00:00 2001 From: jdalton Date: Fri, 17 Apr 2026 21:08:30 -0400 Subject: [PATCH 1/4] 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 --- .../commands/scan/create-scan-from-github.mts | 54 ++++++++- .../scan/create-scan-from-github.test.mts | 103 ++++++++++++++++++ 2 files changed, 156 insertions(+), 1 deletion(-) 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..0726af029 100644 --- a/packages/cli/src/commands/scan/create-scan-from-github.mts +++ b/packages/cli/src/commands/scan/create-scan-from-github.mts @@ -92,7 +92,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. See ASK-167. + 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 +117,54 @@ export async function createScanFromGithub({ if (scanCreated) { scansCreated += 1 } + continue + } + perRepoFailures.push({ + repo: repoSlug, + message: scanCResult.message, + }) + // Stop the loop if we hit a rate limit or auth failure — every + // subsequent repo will fail for the same reason and continuing + // only burns more quota while delaying the real error. Strings + // here match `handleGitHubApiError` / `handleGraphqlError` in + // utils/git/github.mts. + if ( + scanCResult.message === 'GitHub rate limit exceeded' || + scanCResult.message === 'GitHub GraphQL rate limit exceeded' || + scanCResult.message === 'GitHub abuse detection triggered' || + scanCResult.message === 'GitHub authentication failed' + ) { + blockingError = { + ok: false, + message: scanCResult.message, + cause: scanCResult.cause, + } + break } } - logger.success(targetRepos.length, 'GitHub repos detected') + logger.success(reposScanned, 'GitHub repos processed') logger.success(scansCreated, 'with supported Manifest files') + if (blockingError) { + logger.fail(blockingError.message) + return blockingError + } + + // 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/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..a65ba8220 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 @@ -428,3 +428,106 @@ describe('error message quality', () => { } }) }) + +// Regression tests for ASK-167: 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 (ASK-167)', () => { + 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 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) + }) +}) From 6886f666786ef88a4a94cc939fc370feea6d0ea0 Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 20 Apr 2026 14:30:12 -0400 Subject: [PATCH 2/4] refactor(github): extract canonical error-message constants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Export `GITHUB_ERR_*` constants from utils/git/github.mts so callers short-circuit on blocking conditions via constant reference instead of matching literal strings. Previously the create-scan-from-github loop hard-coded four copies of the same message text — quietly drifting if the handler's wording ever changed would re-introduce the "ok:true / 0 manifests" silent-failure this PR was opened to fix. --- .../commands/scan/create-scan-from-github.mts | 25 +++++++++++-------- packages/cli/src/utils/git/github.mts | 17 ++++++++++--- .../scan/create-scan-from-github.test.mts | 4 +++ 3 files changed, 32 insertions(+), 14 deletions(-) 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 0726af029..476b4cea5 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' @@ -123,16 +130,14 @@ export async function createScanFromGithub({ repo: repoSlug, message: scanCResult.message, }) - // Stop the loop if we hit a rate limit or auth failure — every - // subsequent repo will fail for the same reason and continuing - // only burns more quota while delaying the real error. Strings - // here match `handleGitHubApiError` / `handleGraphqlError` in - // utils/git/github.mts. + // 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 rate limit exceeded' || - scanCResult.message === 'GitHub GraphQL rate limit exceeded' || - scanCResult.message === 'GitHub abuse detection triggered' || - scanCResult.message === 'GitHub authentication failed' + scanCResult.message === GITHUB_ERR_RATE_LIMIT || + scanCResult.message === GITHUB_ERR_GRAPHQL_RATE_LIMIT || + scanCResult.message === GITHUB_ERR_ABUSE_DETECTION || + scanCResult.message === GITHUB_ERR_AUTH_FAILED ) { blockingError = { ok: false, 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 a65ba8220..463149b6f 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, })) From 627ffc091b5121057ffa117fc80f4c8b680c896a Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 20 Apr 2026 14:49:12 -0400 Subject: [PATCH 3/4] chore(scan github): sort blocking-error checks alphabetically --- packages/cli/src/commands/scan/create-scan-from-github.mts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 476b4cea5..48023f6aa 100644 --- a/packages/cli/src/commands/scan/create-scan-from-github.mts +++ b/packages/cli/src/commands/scan/create-scan-from-github.mts @@ -134,10 +134,10 @@ export async function createScanFromGithub({ // fail for the same reason and continuing only burns more quota // while delaying the real error. if ( - scanCResult.message === GITHUB_ERR_RATE_LIMIT || - scanCResult.message === GITHUB_ERR_GRAPHQL_RATE_LIMIT || scanCResult.message === GITHUB_ERR_ABUSE_DETECTION || - scanCResult.message === GITHUB_ERR_AUTH_FAILED + scanCResult.message === GITHUB_ERR_AUTH_FAILED || + scanCResult.message === GITHUB_ERR_GRAPHQL_RATE_LIMIT || + scanCResult.message === GITHUB_ERR_RATE_LIMIT ) { blockingError = { ok: false, From 5a26e9d4027a207b1395f034e6f11e30e3bbfdbd Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 20 Apr 2026 15:09:42 -0400 Subject: [PATCH 4/4] fix(scan github): don't print success counts when returning blocking error Before: on rate-limit / auth abort, the function emitted "N GitHub repos processed / 0 with supported Manifest files" followed by logger.fail(...). The success lines implied partial progress even though we're about to return ok:false, which is confusing output. Move the success counters past the blocking-error return so they only print on actual completion. --- packages/cli/src/commands/scan/create-scan-from-github.mts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 48023f6aa..21427644a 100644 --- a/packages/cli/src/commands/scan/create-scan-from-github.mts +++ b/packages/cli/src/commands/scan/create-scan-from-github.mts @@ -148,14 +148,14 @@ export async function createScanFromGithub({ } } - logger.success(reposScanned, 'GitHub repos processed') - logger.success(scansCreated, 'with supported Manifest files') - if (blockingError) { logger.fail(blockingError.message) return blockingError } + 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.