Merge pull request #7126 from Shopify/ryan/structured-app-errors

Refactor app validation errors: result-based parsing, structured error data

Author: ryancbahan

packages/app/src/cli/commands/app/config/validate.ts

@@ -3,6 +3,8 @@ import {validateApp} from '../../../services/validate.js'
 import AppLinkedCommand, {AppLinkedCommandOutput} from '../../../utilities/app-linked-command.js'
 import {linkedAppContext} from '../../../services/app-context.js'
 import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli'
+import {AbortError, AbortSilentError} from '@shopify/cli-kit/node/error'
+import {outputResult, stringifyMessage, unstyled} from '@shopify/cli-kit/node/output'
 
 export default class Validate extends AppLinkedCommand {
   static summary = 'Validate your app configuration and extensions.'
@@ -20,13 +22,24 @@ export default class Validate extends AppLinkedCommand {
   public async run(): Promise<AppLinkedCommandOutput> {
     const {flags} = await this.parse(Validate)
 
-    const {app} = await linkedAppContext({
-      directory: flags.path,
-      clientId: flags['client-id'],
-      forceRelink: flags.reset,
-      userProvidedConfigName: flags.config,
-      unsafeTolerateErrors: true,
-    })
+    let app
+    try {
+      const context = await linkedAppContext({
+        directory: flags.path,
+        clientId: flags['client-id'],
+        forceRelink: flags.reset,
+        userProvidedConfigName: flags.config,
+        unsafeTolerateErrors: true,
+      })
+      app = context.app
+    } catch (err) {
+      if (err instanceof AbortError && flags.json) {
+        const message = unstyled(stringifyMessage(err.message)).trim()
+        outputResult(JSON.stringify({valid: false, errors: [{message}]}, null, 2))
+        throw new AbortSilentError()
+      }
+      throw err
+    }
 
     await validateApp(app, {json: flags.json})
 

packages/app/src/cli/models/app/error-parsing.test.ts

@@ -1,237 +1,91 @@
-import {parseHumanReadableError} from './error-parsing.js'
+import {parseStructuredErrors} from './error-parsing.js'
 import {describe, expect, test} from 'vitest'
 
-describe('parseHumanReadableError', () => {
-  test('formats union errors with smart variant detection', () => {
-    const unionErrorObject = [
-      {
-        code: 'invalid_union',
-        unionErrors: [
-          {
-            issues: [
-              {
-                code: 'invalid_type',
-                expected: 'array',
-                received: 'number',
-                path: ['web', 'roles'],
-                message: 'Expected array, received number',
-              },
-              {
-                code: 'invalid_type',
-                expected: 'string',
-                received: 'undefined',
-                path: ['web', 'commands', 'build'],
-                message: 'Required',
-              },
-            ],
-            name: 'ZodError',
-          },
-          {
-            issues: [
-              {
-                code: 'invalid_literal',
-                expected: "'frontend'",
-                received: 'number',
-                path: ['web', 'type'],
-                message: "Invalid literal value, expected 'frontend'",
-              },
-            ],
-            name: 'ZodError',
-          },
-        ],
-        path: ['web'],
-        message: 'Invalid input',
-      },
+describe('parseStructuredErrors', () => {
+  test('converts regular issues to structured format', () => {
+    const issues = [
+      {path: ['name'], message: 'Required', code: 'invalid_type'},
+      {path: ['version'], message: 'Must be a string', code: 'invalid_type'},
     ]
 
-    const result = parseHumanReadableError(unionErrorObject)
-
-    // Verify the enhanced format shows only the best matching variant's errors
-    // (Option 1 has both missing field + type error, so it's likely the intended one)
-    expect(result).toContain('[web.roles]: Expected array, received number')
-    expect(result).toContain('[web.commands.build]: Required')
-
-    // Should NOT show confusing union variant breakdown
-    expect(result).not.toContain('Union validation failed')
-    expect(result).not.toContain('Option 1:')
-    expect(result).not.toContain('Option 2:')
+    const result = parseStructuredErrors(issues)
 
-    // Should NOT show errors from the less likely option 2
-    expect(result).not.toContain("[web.type]: Invalid literal value, expected 'frontend'")
+    expect(result).toEqual([
+      {path: ['name'], message: 'Required', code: 'invalid_type'},
+      {path: ['version'], message: 'Must be a string', code: 'invalid_type'},
+    ])
   })
 
-  test('handles regular non-union errors', () => {
-    const regularErrorObject = [
-      {
-        path: ['name'],
-        message: 'Required field is missing',
-      },
-      {
-        path: ['version'],
-        message: 'Must be a valid semver string',
-      },
-    ]
-
-    const result = parseHumanReadableError(regularErrorObject)
-
-    // Verify regular errors still work as expected
-    expect(result).toBe('• [name]: Required field is missing\n• [version]: Must be a valid semver string\n')
-    expect(result).not.toContain('Union validation failed')
-  })
-
-  test('handles edge cases for union error detection', () => {
-    // Test case 1: Union error with no unionErrors array
-    const noUnionErrors = [
-      {
-        code: 'invalid_union',
-        path: ['root'],
-        message: 'Invalid input',
-      },
-    ]
-
-    const result1 = parseHumanReadableError(noUnionErrors)
-    expect(result1).toBe('• [root]: Invalid input\n')
-
-    // Test case 2: Union error with empty unionErrors array - should fall back to showing full union error
-    const emptyUnionErrors = [
-      {
-        code: 'invalid_union',
-        unionErrors: [],
-        path: ['root'],
-        message: 'Invalid input',
-      },
-    ]
-
-    const result2 = parseHumanReadableError(emptyUnionErrors)
-    expect(result2).toContain("Configuration doesn't match any expected format:")
-
-    // Test case 3: Union errors with variants that have no issues - results in empty string
-    const noIssuesVariants = [
-      {
-        code: 'invalid_union',
-        unionErrors: [
-          {issues: [], name: 'ZodError'},
-          {issues: [], name: 'ZodError'},
-        ],
-        path: ['root'],
-        message: 'Invalid input',
-      },
-    ]
-
-    const result3 = parseHumanReadableError(noIssuesVariants)
-    // When all variants have no issues, the best variant selection returns null issues
-    // resulting in no output, which falls back to the union error display
-    expect(result3).toContain("Configuration doesn't match any expected format:")
-  })
-
-  test('findBestMatchingVariant scoring logic works correctly', () => {
-    // Test various scoring scenarios by creating mock union errors
-    const scenarioWithMissingFields = [
+  test('selects best union variant based on scoring', () => {
+    const issues = [
       {
         code: 'invalid_union',
         unionErrors: [
           {
-            // Variant with missing fields - should score highest
             issues: [
               {path: ['supports_moto'], message: 'Required'},
               {path: ['merchant_label'], message: 'Required'},
             ],
             name: 'ZodError',
           },
           {
-            // Variant with only type errors - should score lower
             issues: [
               {path: ['type'], message: 'Expected string, received number'},
               {path: ['id'], message: 'Expected number, received string'},
             ],
             name: 'ZodError',
           },
-          {
-            // Variant with other errors - should score lowest
-            issues: [{path: ['url'], message: 'Invalid URL format'}],
-            name: 'ZodError',
-          },
         ],
         path: [],
         message: 'Invalid input',
       },
     ]
 
-    const result = parseHumanReadableError(scenarioWithMissingFields)
+    const result = parseStructuredErrors(issues)
 
-    // Should show only the variant with missing fields (highest score)
-    expect(result).toContain('[supports_moto]: Required')
-    expect(result).toContain('[merchant_label]: Required')
-
-    // Should NOT show errors from other variants
-    expect(result).not.toContain('Expected string, received number')
-    expect(result).not.toContain('Invalid URL format')
-    expect(result).not.toContain('Union validation failed')
+    expect(result).toEqual([
+      {path: ['supports_moto'], message: 'Required', code: 'invalid_union'},
+      {path: ['merchant_label'], message: 'Required', code: 'invalid_union'},
+    ])
   })
 
-  test('handles undefined messages gracefully', () => {
-    const undefinedMessageError = [
-      {
-        path: ['field'],
-        message: undefined,
-      },
+  test('falls back when all union variants have empty issues', () => {
+    const issues = [
       {
-        path: [],
-        // message is completely missing
+        code: 'invalid_union',
+        unionErrors: [
+          {issues: [], name: 'ZodError'},
+          {issues: [], name: 'ZodError'},
+        ],
+        path: ['root'],
+        message: 'Invalid input',
       },
     ]
 
-    const result = parseHumanReadableError(undefinedMessageError)
+    const result = parseStructuredErrors(issues)
 
-    expect(result).toBe('• [field]: Unknown error\n• [root]: Unknown error\n')
+    expect(result).toEqual([{path: ['root'], message: 'Invalid input', code: 'invalid_union'}])
   })
 
-  test('handles mixed scoring scenarios', () => {
-    // Test scenario where we need to pick between variants with different error combinations
-    const mixedScenario = [
+  test('falls back when union has no unionErrors array', () => {
+    const issues = [
       {
         code: 'invalid_union',
-        unionErrors: [
-          {
-            // Mix of missing fields and type errors - this should win due to missing fields
-            issues: [
-              {path: ['required_field'], message: 'Required'},
-              {path: ['wrong_type'], message: 'Expected string, received number'},
-            ],
-            name: 'ZodError',
-          },
-          {
-            // Only type errors - should lose
-            issues: [
-              {path: ['field1'], message: 'Expected boolean, received string'},
-              {path: ['field2'], message: 'Expected array, received object'},
-              {path: ['field3'], message: 'Expected number, received string'},
-            ],
-            name: 'ZodError',
-          },
-          {
-            // Only other validation errors - should score lowest
-            issues: [
-              {path: ['url'], message: 'Must be valid URL'},
-              {path: ['email'], message: 'Must be valid email'},
-            ],
-            name: 'ZodError',
-          },
-        ],
-        path: [],
+        path: ['field'],
         message: 'Invalid input',
       },
     ]
 
-    const result = parseHumanReadableError(mixedScenario)
+    const result = parseStructuredErrors(issues)
+
+    expect(result).toEqual([{path: ['field'], message: 'Invalid input', code: 'invalid_union'}])
+  })
+
+  test('handles missing message with fallback', () => {
+    const issues = [{path: ['x'], code: 'custom'}]
 
-    // Should pick the variant with missing field (even though it has fewer total errors)
-    expect(result).toContain('[required_field]: Required')
-    expect(result).toContain('[wrong_type]: Expected string, received number')
+    const result = parseStructuredErrors(issues)
 
-    // Should not show errors from other variants
-    expect(result).not.toContain('Expected boolean, received string')
-    expect(result).not.toContain('Must be valid URL')
-    expect(result).not.toContain('Union validation failed')
+    expect(result).toEqual([{path: ['x'], message: 'Unknown error', code: 'custom'}])
   })
 })

packages/app/src/cli/models/app/error-parsing.ts

@@ -1,3 +1,9 @@
+interface ParsedIssue {
+  path?: (string | number)[]
+  message: string
+  code?: string
+}
+
 interface UnionError {
   issues?: {path?: (string | number)[]; message: string}[]
   name: string
@@ -56,44 +62,25 @@ function findBestMatchingVariant(unionErrors: UnionError[]): UnionError | null {
   return bestVariant
 }
 
-/**
- * Formats an issue into a human-readable error line
- */
-function formatErrorLine(issue: {path?: (string | number)[]; message?: string}, indent = '') {
-  const path = issue.path && issue.path.length > 0 ? issue.path.map(String).join('.') : 'root'
-  const message = issue.message ?? 'Unknown error'
-  return `${indent}• [${path}]: ${message}\n`
-}
-
-export function parseHumanReadableError(issues: ExtendedZodIssue[]) {
-  let humanReadableError = ''
-
-  issues.forEach((issue) => {
-    // Handle union errors with smart variant detection
+export function parseStructuredErrors(issues: ExtendedZodIssue[]): ParsedIssue[] {
+  const result: ParsedIssue[] = []
+  for (const issue of issues) {
     if (issue.code === 'invalid_union' && issue.unionErrors) {
-      // Find the variant that's most likely the intended one
       const bestVariant = findBestMatchingVariant(issue.unionErrors)
-
       if (bestVariant?.issues?.length) {
-        // Show errors only for the best matching variant
-        bestVariant.issues.forEach((nestedIssue) => {
-          humanReadableError += formatErrorLine(nestedIssue)
-        })
+        for (const nested of bestVariant.issues) {
+          result.push({path: nested.path, message: nested.message, code: issue.code})
+        }
       } else {
-        // Fallback: show all variants if we can't determine the best one
-        humanReadableError += `• Configuration doesn't match any expected format:\n`
-        issue.unionErrors.forEach((unionError, index: number) => {
-          humanReadableError += `  Option ${index + 1}:\n`
-          unionError.issues?.forEach((nestedIssue) => {
-            humanReadableError += formatErrorLine(nestedIssue, '    ')
-          })
+        result.push({
+          path: issue.path,
+          message: issue.message ?? "Configuration doesn't match any expected format",
+          code: issue.code,
         })
       }
     } else {
-      // Handle regular issues
-      humanReadableError += formatErrorLine(issue)
+      result.push({path: issue.path, message: issue.message ?? 'Unknown error', code: issue.code})
     }
-  })
-
-  return humanReadableError
+  }
+  return result
 }