Merge pull request #7130 from Shopify/ryan/surface-discovery-errors

Add errors to TomlFile and Project for TOML parse failure tracking

Author: ryancbahan

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

@@ -2,48 +2,74 @@ import Validate from './validate.js'
 import {linkedAppContext} from '../../../services/app-context.js'
 import {validateApp} from '../../../services/validate.js'
 import {testAppLinked} from '../../../models/app/app.test-data.js'
+import {Project} from '../../../models/project/project.js'
+import {selectActiveConfig} from '../../../models/project/active-config.js'
+import {errorsForConfig} from '../../../models/project/config-selection.js'
+import {outputResult} from '@shopify/cli-kit/node/output'
+import {TomlFile} from '@shopify/cli-kit/node/toml/toml-file'
 import {describe, expect, test, vi} from 'vitest'
 
 vi.mock('../../../services/app-context.js')
 vi.mock('../../../services/validate.js')
+vi.mock('../../../models/project/project.js')
+vi.mock('../../../models/project/active-config.js')
+vi.mock('../../../models/project/config-selection.js')
+vi.mock('@shopify/cli-kit/node/output', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('@shopify/cli-kit/node/output')>()
+  return {...actual, outputResult: vi.fn()}
+})
+vi.mock('@shopify/cli-kit/node/ui')
+
+function mockHealthyProject() {
+  vi.mocked(Project.load).mockResolvedValue({errors: []} as unknown as Project)
+  vi.mocked(selectActiveConfig).mockResolvedValue({file: new TomlFile('shopify.app.toml', {})} as any)
+  vi.mocked(errorsForConfig).mockReturnValue([])
+}
 
 describe('app config validate command', () => {
   test('calls validateApp with json: false by default', async () => {
-    // Given
     const app = testAppLinked()
+    mockHealthyProject()
     vi.mocked(linkedAppContext).mockResolvedValue({app} as Awaited<ReturnType<typeof linkedAppContext>>)
     vi.mocked(validateApp).mockResolvedValue()
 
-    // When
     await Validate.run([], import.meta.url)
 
-    // Then
     expect(validateApp).toHaveBeenCalledWith(app, {json: false})
   })
 
   test('calls validateApp with json: true when --json flag is passed', async () => {
-    // Given
     const app = testAppLinked()
+    mockHealthyProject()
     vi.mocked(linkedAppContext).mockResolvedValue({app} as Awaited<ReturnType<typeof linkedAppContext>>)
     vi.mocked(validateApp).mockResolvedValue()
 
-    // When
     await Validate.run(['--json'], import.meta.url)
 
-    // Then
     expect(validateApp).toHaveBeenCalledWith(app, {json: true})
   })
 
   test('calls validateApp with json: true when -j flag is passed', async () => {
-    // Given
     const app = testAppLinked()
+    mockHealthyProject()
     vi.mocked(linkedAppContext).mockResolvedValue({app} as Awaited<ReturnType<typeof linkedAppContext>>)
     vi.mocked(validateApp).mockResolvedValue()
 
-    // When
     await Validate.run(['-j'], import.meta.url)
 
-    // Then
     expect(validateApp).toHaveBeenCalledWith(app, {json: true})
   })
+
+  test('outputs JSON issues when active config has TOML parse errors', async () => {
+    vi.mocked(Project.load).mockResolvedValue({errors: []} as unknown as Project)
+    vi.mocked(selectActiveConfig).mockResolvedValue({file: new TomlFile('shopify.app.toml', {})} as any)
+    vi.mocked(errorsForConfig).mockReturnValue([
+      {path: '/app/shopify.app.toml', message: 'Unexpected character at row 1, col 5'} as any,
+    ])
+
+    await expect(Validate.run(['--json'], import.meta.url)).rejects.toThrow()
+
+    expect(outputResult).toHaveBeenCalledWith(expect.stringContaining('"valid": false'))
+    expect(linkedAppContext).not.toHaveBeenCalled()
+  })
 })

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

@@ -2,9 +2,13 @@ import {appFlags} from '../../../flags.js'
 import {validateApp} from '../../../services/validate.js'
 import AppLinkedCommand, {AppLinkedCommandOutput} from '../../../utilities/app-linked-command.js'
 import {linkedAppContext} from '../../../services/app-context.js'
+import {selectActiveConfig} from '../../../models/project/active-config.js'
+import {errorsForConfig} from '../../../models/project/config-selection.js'
+import {Project} from '../../../models/project/project.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'
+import {renderError} from '@shopify/cli-kit/node/ui'
 
 export default class Validate extends AppLinkedCommand {
   static summary = 'Validate your app configuration and extensions.'
@@ -22,6 +26,47 @@ export default class Validate extends AppLinkedCommand {
   public async run(): Promise<AppLinkedCommandOutput> {
     const {flags} = await this.parse(Validate)
 
+    // Stage 1: Load project
+    let project: Project
+    try {
+      project = await Project.load(flags.path)
+    } catch (err) {
+      if (err instanceof AbortError && flags.json) {
+        const message = unstyled(stringifyMessage(err.message)).trim()
+        outputResult(JSON.stringify({valid: false, issues: [{message}]}, null, 2))
+        throw new AbortSilentError()
+      }
+      throw err
+    }
+
+    // Stage 2: Select active config and check for TOML parse errors scoped to it
+    let activeConfig
+    try {
+      activeConfig = await selectActiveConfig(project, flags.config)
+    } catch (err) {
+      if (err instanceof AbortError && flags.json) {
+        const message = unstyled(stringifyMessage(err.message)).trim()
+        outputResult(JSON.stringify({valid: false, issues: [{message}]}, null, 2))
+        throw new AbortSilentError()
+      }
+      throw err
+    }
+
+    const configErrors = errorsForConfig(project, activeConfig.file)
+    if (configErrors.length > 0) {
+      const issues = configErrors.map((err) => ({file: err.path, message: err.message}))
+      if (flags.json) {
+        outputResult(JSON.stringify({valid: false, issues}, null, 2))
+        throw new AbortSilentError()
+      }
+      renderError({
+        headline: 'Validation errors found.',
+        body: issues.map((issue) => `• ${issue.message}`).join('\n'),
+      })
+      throw new AbortSilentError()
+    }
+
+    // Stage 3: Load app (link + remote fetch + schema validation)
     let app
     try {
       const context = await linkedAppContext({
@@ -33,9 +78,12 @@ export default class Validate extends AppLinkedCommand {
       })
       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))
+      // Only catch config validation errors for JSON output. Auth/linking/remote
+      // failures should propagate normally — they aren't validation results.
+      const message = err instanceof AbortError ? unstyled(stringifyMessage(err.message)).trim() : ''
+      const isValidationError = message.startsWith('Validation errors in ')
+      if (isValidationError && flags.json) {
+        outputResult(JSON.stringify({valid: false, issues: [{message}]}, null, 2))
         throw new AbortSilentError()
       }
       throw err

packages/app/src/cli/models/app/loader.ts

@@ -38,7 +38,7 @@ import {
 } from '../project/config-selection.js'
 import {showMultipleCLIWarningIfNeeded} from '@shopify/cli-kit/node/multiple-installation-warning'
 import {fileExists, readFile, fileExistsSync} from '@shopify/cli-kit/node/fs'
-import {TomlFile, TomlFileNotFoundError, TomlParseError} from '@shopify/cli-kit/node/toml/toml-file'
+import {TomlFile, TomlFileError} from '@shopify/cli-kit/node/toml/toml-file'
 import {zod} from '@shopify/cli-kit/node/schema'
 import {PackageManager} from '@shopify/cli-kit/node/node-package-manager'
 import {resolveFramework} from '@shopify/cli-kit/node/framework'
@@ -96,10 +96,7 @@ export async function parseConfigurationFile<TSchema extends zod.ZodType>(
       const file = await TomlFile.read(filepath)
       content = file.content
     } catch (err) {
-      if (err instanceof TomlFileNotFoundError) {
-        return {errors: [{file: filepath, message: `Couldn't find the configuration file at ${filepath}`}]}
-      }
-      if (err instanceof TomlParseError) {
+      if (err instanceof TomlFileError) {
         return {errors: [{file: filepath, message: err.message}]}
       }
       throw err

packages/app/src/cli/models/project/active-config.test.ts

@@ -164,25 +164,29 @@ describe('selectActiveConfig', () => {
     })
   })
 
-  test('throws when the only app config is malformed (no valid configs to fall back to)', async () => {
+  test('loads with errors when the only app config is malformed', async () => {
     await inTemporaryDirectory(async (dir) => {
-      // The only config is broken TOML — Project.load skips it and finds 0 valid configs
+      // The only config is broken TOML — Project.load includes it with errors
       await writeFile(joinPath(dir, 'shopify.app.toml'), '{{invalid toml')
 
-      await expect(Project.load(dir)).rejects.toThrow(/Could not find/)
+      const project = await Project.load(dir)
+      expect(project.appConfigFiles).toHaveLength(1)
+      expect(project.appConfigFiles[0]!.errors).toHaveLength(1)
+      expect(project.errors).toHaveLength(1)
+      expect(project.errors[0]!.path).toContain('shopify.app.toml')
     })
   })
 
-  test('surfaces parse error when selecting a broken config while a valid one exists', async () => {
+  test('selects a broken config and exposes its errors', async () => {
     await inTemporaryDirectory(async (dir) => {
-      // Two configs: one good, one broken. Selecting the broken one by name should
-      // surface the real parse error via the fallback re-read, not a generic "not found".
+      // Two configs: one good, one broken. Selecting the broken one succeeds
+      // but the selected file has errors.
       await writeFile(joinPath(dir, 'shopify.app.toml'), 'client_id = "good"')
       await writeFile(joinPath(dir, 'shopify.app.broken.toml'), '{{invalid toml')
 
       const project = await Project.load(dir)
-
-      await expect(selectActiveConfig(project, 'shopify.app.broken.toml')).rejects.toThrow()
+      const activeConfig = await selectActiveConfig(project, 'broken')
+      expect(activeConfig.file.errors).toHaveLength(1)
     })
   })
 

packages/app/src/cli/models/project/config-selection.ts

@@ -4,7 +4,7 @@ import {getAppConfigurationShorthand} from '../app/loader.js'
 import {dotEnvFileNames} from '../../constants.js'
 import {patchAppHiddenConfigFile} from '../../services/app/patch-app-configuration-file.js'
 import {getOrCreateAppConfigHiddenPath} from '../../utilities/app/config/hidden-app-config.js'
-import {TomlFile} from '@shopify/cli-kit/node/toml/toml-file'
+import {TomlFile, TomlFileError} from '@shopify/cli-kit/node/toml/toml-file'
 import {DotEnvFile} from '@shopify/cli-kit/node/dot-env'
 import {matchGlob} from '@shopify/cli-kit/node/fs'
 import {relativePath} from '@shopify/cli-kit/node/path'
@@ -112,3 +112,29 @@ export function webFilesForConfig(project: Project, activeConfig: TomlFile): Tom
     return globPatterns.some((pattern) => matchGlob(relPath, pattern))
   })
 }
+
+/**
+ * Filter project.errors to only those relevant to the active config:
+ * the active config file itself, plus extension/web TOMLs in its directories.
+ * @public
+ */
+export function errorsForConfig(project: Project, activeConfig: TomlFile): TomlFileError[] {
+  const extDirs = activeConfig.content.extension_directories
+  const extPatterns = (Array.isArray(extDirs) && extDirs.length > 0 ? (extDirs as string[]) : ['extensions/*']).map(
+    (dir) => `${dir}/*.extension.toml`,
+  )
+
+  const webDirs = activeConfig.content.web_directories
+  const webPatterns =
+    Array.isArray(webDirs) && webDirs.length > 0
+      ? (webDirs as string[]).map((dir) => `${dir}/shopify.web.toml`)
+      : ['**/shopify.web.toml']
+
+  const allPatterns = [...extPatterns, ...webPatterns]
+
+  return project.errors.filter((err) => {
+    if (err.path === activeConfig.path) return true
+    const relPath = relativePath(project.directory, err.path).replace(/\\/g, '/')
+    return allPatterns.some((pattern) => matchGlob(relPath, pattern))
+  })
+}

packages/app/src/cli/models/project/project.test.ts

@@ -159,44 +159,54 @@ describe('Project', () => {
       })
     })
 
-    test('skips malformed inactive app config without blocking active config', async () => {
+    test('includes malformed inactive app config with errors without blocking active config', async () => {
       await inTemporaryDirectory(async (dir) => {
         await writeAppToml(dir, 'client_id = "good"')
         await writeAppToml(dir, '{{invalid toml content', 'shopify.app.broken.toml')
 
         const project = await Project.load(dir)
 
-        // The broken config is skipped, but the valid one is loaded
-        expect(project.appConfigFiles).toHaveLength(1)
-        expect(project.appConfigFiles[0]!.content.client_id).toBe('good')
+        expect(project.appConfigFiles).toHaveLength(2)
+        const good = project.appConfigFiles.find((file) => file.content.client_id === 'good')
+        const broken = project.appConfigFiles.find((file) => file.errors.length > 0)
+        expect(good).toBeDefined()
+        expect(broken).toBeDefined()
+        expect(broken!.path).toContain('shopify.app.broken.toml')
+        expect(project.errors).toHaveLength(1)
       })
     })
 
-    test('skips malformed extension TOML without blocking project load', async () => {
+    test('includes malformed extension TOML with errors without blocking project load', async () => {
       await inTemporaryDirectory(async (dir) => {
         await writeAppToml(dir, 'client_id = "abc"')
         await writeExtensionToml(dir, 'good-ext', 'type = "function"\nname = "good"')
         await writeExtensionToml(dir, 'bad-ext', '{{broken toml')
 
         const project = await Project.load(dir)
 
-        // Only the valid extension is loaded
-        expect(project.extensionConfigFiles).toHaveLength(1)
-        expect(project.extensionConfigFiles[0]!.content.name).toBe('good')
+        expect(project.extensionConfigFiles).toHaveLength(2)
+        const good = project.extensionConfigFiles.find((file) => file.content.name === 'good')
+        const broken = project.extensionConfigFiles.find((file) => file.errors.length > 0)
+        expect(good).toBeDefined()
+        expect(broken).toBeDefined()
+        expect(project.errors).toHaveLength(1)
       })
     })
 
-    test('skips malformed web TOML without blocking project load', async () => {
+    test('includes malformed web TOML with errors without blocking project load', async () => {
       await inTemporaryDirectory(async (dir) => {
         await writeAppToml(dir, 'client_id = "abc"')
         await writeWebToml(dir, 'good-web', 'name = "good"\nroles = ["backend"]')
         await writeWebToml(dir, 'bad-web', '{{broken toml')
 
         const project = await Project.load(dir)
 
-        // Only the valid web config is loaded
-        expect(project.webConfigFiles).toHaveLength(1)
-        expect(project.webConfigFiles[0]!.content.name).toBe('good')
+        expect(project.webConfigFiles).toHaveLength(2)
+        const good = project.webConfigFiles.find((file) => file.content.name === 'good')
+        const broken = project.webConfigFiles.find((file) => file.errors.length > 0)
+        expect(good).toBeDefined()
+        expect(broken).toBeDefined()
+        expect(project.errors).toHaveLength(1)
       })
     })