Merge pull request #388 from yowainwright/update-retry

fix: fixes retry

Author: yowainwright

bun.lock

@@ -1,5 +1,6 @@
 {
   "lockfileVersion": 1,
+  "configVersion": 0,
   "workspaces": {
     "": {
       "name": "pastoralist",

src/cli/index.ts

@@ -109,28 +109,39 @@ export const runSecurityCheck = async (
     )
     .start();
 
-  const securityChecker = new deps.SecurityChecker({
-    provider: mergedOptions.securityProvider,
-    forceRefactor: mergedOptions.forceSecurityRefactor,
-    interactive: mergedOptions.interactive,
-    token: mergedOptions.securityProviderToken,
-    debug: isLogging,
-    isIRLFix: mergedOptions.isIRLFix,
-    isIRLCatch: mergedOptions.isIRLCatch,
-  });
-
-  const scanPaths = deps.determineSecurityScanPaths(config, mergedOptions, log);
-  const {
-    alerts,
-    overrides: securityOverrides,
-    updates,
-  } = await securityChecker.checkSecurity(config, {
-    ...mergedOptions,
-    depPaths: scanPaths,
-    root: mergedOptions.root || "./",
-  });
-
-  return { spinner, securityChecker, alerts, securityOverrides, updates };
+  try {
+    const securityChecker = new deps.SecurityChecker({
+      provider: mergedOptions.securityProvider,
+      forceRefactor: mergedOptions.forceSecurityRefactor,
+      interactive: mergedOptions.interactive,
+      token: mergedOptions.securityProviderToken,
+      debug: isLogging,
+      isIRLFix: mergedOptions.isIRLFix,
+      isIRLCatch: mergedOptions.isIRLCatch,
+    });
+
+    const scanPaths = deps.determineSecurityScanPaths(
+      config,
+      mergedOptions,
+      log,
+    );
+    const {
+      alerts,
+      overrides: securityOverrides,
+      updates,
+    } = await securityChecker.checkSecurity(config, {
+      ...mergedOptions,
+      depPaths: scanPaths,
+      root: mergedOptions.root || "./",
+    });
+
+    return { spinner, securityChecker, alerts, securityOverrides, updates };
+  } catch (error) {
+    spinner.fail(
+      `🔒 ${deps.green(`pastoralist`)} security check failed: ${error instanceof Error ? error.message : String(error)}`,
+    );
+    throw error;
+  }
 };
 
 export const handleSecurityResults = (

src/core/security/providers/github.ts

@@ -15,6 +15,9 @@ import {
 
 const execFileAsync = promisify(execFile);
 
+const DEFAULT_FETCH_TIMEOUT = 30000;
+const DEFAULT_CLI_TIMEOUT = 60000;
+
 export class GitHubSecurityProvider {
   private owner: string;
   private repo: string;
@@ -208,7 +211,9 @@ export class GitHubSecurityProvider {
       "executeGhCli",
     );
 
-    const { stdout } = await execFileAsync("gh", args);
+    const { stdout } = await execFileAsync("gh", args, {
+      timeout: DEFAULT_CLI_TIMEOUT,
+    });
     this.log.debug(`gh CLI stdout length: ${stdout.length}`, "executeGhCli");
     return stdout;
   }
@@ -246,23 +251,33 @@ export class GitHubSecurityProvider {
 
   private async fetchFromGitHubAPI(): Promise<DependabotAlert[]> {
     const url = `https://api.github.com/repos/${this.owner}/${this.repo}/dependabot/alerts`;
+    const controller = new AbortController();
+    const timeoutId = setTimeout(
+      () => controller.abort(),
+      DEFAULT_FETCH_TIMEOUT,
+    );
 
-    const response = await fetch(url, {
-      headers: {
-        Authorization: `Bearer ${this.token}`,
-        Accept: "application/vnd.github.v3+json",
-      },
-    });
+    try {
+      const response = await fetch(url, {
+        headers: {
+          Authorization: `Bearer ${this.token}`,
+          Accept: "application/vnd.github.v3+json",
+        },
+        signal: controller.signal,
+      });
 
-    if (!response.ok) {
-      const error: GithubApiError = await response.json();
-      throw new Error(
-        `GitHub API error: ${error.message || response.statusText}`,
-      );
-    }
+      if (!response.ok) {
+        const error: GithubApiError = await response.json();
+        throw new Error(
+          `GitHub API error: ${error.message || response.statusText}`,
+        );
+      }
 
-    const alerts = await response.json();
-    return Array.isArray(alerts) ? alerts : [];
+      const alerts = await response.json();
+      return Array.isArray(alerts) ? alerts : [];
+    } finally {
+      clearTimeout(timeoutId);
+    }
   }
 
   private async fetchAlertsWithApi(): Promise<DependabotAlert[]> {

src/core/update/utils.ts

@@ -131,8 +131,8 @@ export const hasOverrides = (
 
   return Boolean(
     (optionsOverrides && Object.keys(optionsOverrides).length > 0) ||
-      (configOverrides && Object.keys(configOverrides).length > 0) ||
-      (configResolutions && Object.keys(configResolutions).length > 0) ||
-      (configPnpmOverrides && Object.keys(configPnpmOverrides).length > 0),
+    (configOverrides && Object.keys(configOverrides).length > 0) ||
+    (configResolutions && Object.keys(configResolutions).length > 0) ||
+    (configPnpmOverrides && Object.keys(configPnpmOverrides).length > 0),
   );
 };

src/utils/retry.ts

@@ -1,6 +1,8 @@
 import type { RetryOptions, RetryError } from "./types";
 
-const DEFAULT_OPTIONS: Required<Omit<RetryOptions, "onFailedAttempt">> = {
+const DEFAULT_OPTIONS: Required<
+  Omit<RetryOptions, "onFailedAttempt" | "onRetry">
+> = {
   retries: 3,
   factor: 2,
   minTimeout: 1000,
@@ -42,6 +44,7 @@ export const retry = async <T>(
     minTimeout = DEFAULT_OPTIONS.minTimeout,
     maxTimeout = DEFAULT_OPTIONS.maxTimeout,
     onFailedAttempt,
+    onRetry,
   } = options;
 
   let attemptNumber = 0;
@@ -69,6 +72,10 @@ export const retry = async <T>(
         await onFailedAttempt(retryError);
       }
 
+      if (onRetry) {
+        onRetry(attemptNumber, clampedRetriesLeft);
+      }
+
       const delay = calculateDelay(
         attemptNumber,
         factor,

src/utils/spinner.ts

@@ -120,6 +120,12 @@ export const warn = (state: SpinnerState, text?: string): Spinner => {
   return createSpinnerMethods(state);
 };
 
+export const update = (state: SpinnerState, text: string): Spinner => {
+  const newState = updateStateText(state, text);
+  Object.assign(state, newState);
+  return createSpinnerMethods(state);
+};
+
 export const createSpinnerMethods = (state: SpinnerState): Spinner => {
   return {
     start: () => start(state),
@@ -128,6 +134,7 @@ export const createSpinnerMethods = (state: SpinnerState): Spinner => {
     fail: (text?: string) => fail(state, text),
     info: (text?: string) => info(state, text),
     warn: (text?: string) => warn(state, text),
+    update: (text: string) => update(state, text),
   };
 };
 

src/utils/types.ts

@@ -12,6 +12,7 @@ export type Spinner = {
   fail: (text?: string) => Spinner;
   info: (text?: string) => Spinner;
   warn: (text?: string) => Spinner;
+  update: (text: string) => Spinner;
 };
 
 export type RGB = {
@@ -49,6 +50,7 @@ export interface RetryOptions {
   minTimeout?: number;
   maxTimeout?: number;
   onFailedAttempt?: (error: RetryError) => void | Promise<void>;
+  onRetry?: (attemptNumber: number, retriesLeft: number) => void;
 }
 
 export interface RetryError extends Error {

tests/unit/cli/index.test.ts

@@ -1860,3 +1860,83 @@ test("determineSecurityScanPaths - handles workspace with hasWorkspaceSecurityCh
 
   expect(result).toEqual([]);
 });
+
+test("runSecurityCheck - handles error and calls spinner.fail", async () => {
+  const { runSecurityCheck } = require("../../../src/cli/index");
+
+  const config: PastoralistJSON = {
+    name: "test",
+    version: "1.0.0",
+  };
+
+  const mergedOptions: Options = {
+    checkSecurity: true,
+    securityProvider: "osv",
+  };
+
+  const mockSpinner = {
+    start: mock(() => mockSpinner),
+    fail: mock(),
+  };
+
+  const testError = new Error("Security check failed");
+
+  const mockSecurityChecker = {
+    checkSecurity: mock(() => Promise.reject(testError)),
+  };
+
+  const deps = {
+    createSpinner: mock(() => mockSpinner),
+    SecurityChecker: mock(() => mockSecurityChecker),
+    determineSecurityScanPaths: mock(() => []),
+    green: mock((text: string) => text),
+  };
+
+  await expect(
+    runSecurityCheck(config, mergedOptions, false, log, deps),
+  ).rejects.toThrow("Security check failed");
+
+  expect(mockSpinner.fail).toHaveBeenCalled();
+  const failCall = mockSpinner.fail.mock.calls[0][0];
+  expect(failCall).toContain("security check failed");
+  expect(failCall).toContain("Security check failed");
+});
+
+test("runSecurityCheck - handles non-Error throws and calls spinner.fail", async () => {
+  const { runSecurityCheck } = require("../../../src/cli/index");
+
+  const config: PastoralistJSON = {
+    name: "test",
+    version: "1.0.0",
+  };
+
+  const mergedOptions: Options = {
+    checkSecurity: true,
+    securityProvider: "osv",
+  };
+
+  const mockSpinner = {
+    start: mock(() => mockSpinner),
+    fail: mock(),
+  };
+
+  const mockSecurityChecker = {
+    checkSecurity: mock(() => Promise.reject("String error")),
+  };
+
+  const deps = {
+    createSpinner: mock(() => mockSpinner),
+    SecurityChecker: mock(() => mockSecurityChecker),
+    determineSecurityScanPaths: mock(() => []),
+    green: mock((text: string) => text),
+  };
+
+  await expect(
+    runSecurityCheck(config, mergedOptions, false, log, deps),
+  ).rejects.toBe("String error");
+
+  expect(mockSpinner.fail).toHaveBeenCalled();
+  const failCall = mockSpinner.fail.mock.calls[0][0];
+  expect(failCall).toContain("security check failed");
+  expect(failCall).toContain("String error");
+});