Merge pull request #4719 from Shopify/allowlist-of-environmental-errors

Allow specific error messages to be flagged as environmental and not bugs

packages/cli-kit/src/public/node/error-handler.ts

@@ -6,7 +6,7 @@ import {
   AbortSilentError,
   CancelExecution,
   errorMapper,
-  shouldReportError,
+  shouldReportErrorAsUnexpected,
   handler,
   cleanSingleStackTracePath,
 } from './error.js'
@@ -46,7 +46,7 @@ export async function errorHandler(
 const reportError = async (error: unknown, config?: Interfaces.Config): Promise<void> => {
   // categorise the error first
   let exitMode: CommandExitMode = 'expected_error'
-  if (shouldReportError(error)) exitMode = 'unexpected_error'
+  if (shouldReportErrorAsUnexpected(error)) exitMode = 'unexpected_error'
 
   if (config !== undefined) {
     // Log an analytics event when there's an error

packages/cli-kit/src/public/node/error.test.ts

@@ -1,4 +1,4 @@
-import {AbortError, BugError, handler, cleanSingleStackTracePath} from './error.js'
+import {AbortError, BugError, handler, cleanSingleStackTracePath, shouldReportErrorAsUnexpected} from './error.js'
 import {renderFatalError} from './ui.js'
 import {describe, expect, test, vi} from 'vitest'
 
@@ -53,3 +53,21 @@ describe('stack file path helpers', () => {
     expect(cleanSingleStackTracePath(path)).toEqual('/something/there.js')
   })
 })
+
+describe('shouldReportErrorAsUnexpected helper', () => {
+  test('returns true for normal errors', () => {
+    expect(shouldReportErrorAsUnexpected(new Error('test'))).toBe(true)
+  })
+
+  test('returns false for AbortError', () => {
+    expect(shouldReportErrorAsUnexpected(new AbortError('test'))).toBe(false)
+  })
+
+  test('returns true for BugError', () => {
+    expect(shouldReportErrorAsUnexpected(new BugError('test'))).toBe(true)
+  })
+
+  test('returns false for errors that imply environment issues', () => {
+    expect(shouldReportErrorAsUnexpected(new Error('EPERM: operation not permitted, scandir'))).toBe(false)
+  })
+})

packages/cli-kit/src/public/node/error.ts

@@ -181,13 +181,18 @@ function isFatal(error: unknown): error is FatalError {
 }
 
 /**
- * A function that checks if an error should be reported as unhandled.
+ * A function that checks if an error should be reported as unexpected.
  *
  * @param error - Error to be checked.
- * @returns A boolean indicating if the error should be reported.
+ * @returns A boolean indicating if the error should be reported as unexpected.
  */
-export function shouldReportError(error: unknown): boolean {
+export function shouldReportErrorAsUnexpected(error: unknown): boolean {
   if (!isFatal(error)) {
+    // this means its not one of the CLI wrapped errors
+    if (error instanceof Error) {
+      const message = error.message
+      return !errorMessageImpliesEnvironmentIssue(message)
+    }
     return true
   }
   if (error.type === FatalErrorType.Bug) {
@@ -207,3 +212,25 @@ export function cleanSingleStackTracePath(filePath: string): string {
     .replace('file:/', '/')
     .replace(/^\/?[A-Z]:/, '')
 }
+
+/**
+ * There are certain errors that we know are not due to a CLI bug, but are environmental/user error.
+ *
+ * @param message - The error message to check.
+ * @returns A boolean indicating if the error message implies an environment issue.
+ */
+function errorMessageImpliesEnvironmentIssue(message: string): boolean {
+  const environmentIssueMessages = [
+    'EPERM: operation not permitted, scandir',
+    'EACCES: permission denied',
+    'EPERM: operation not permitted, symlink',
+    'This version of npm supports the following node versions',
+    'EBUSY: resource busy or locked, rmdir',
+    'getaddrinfo ENOTFOUND',
+    'Client network socket disconnected before secure TLS connection was established',
+    'spawn EPERM',
+    'socket hang up',
+  ]
+  const anyMatches = environmentIssueMessages.some((issueMessage) => message.includes(issueMessage))
+  return anyMatches
+}