From cae63b4735470cc8194d65983ba2ab3e23f32550 Mon Sep 17 00:00:00 2001 From: Donald Merand Date: Tue, 24 Mar 2026 13:10:18 -0400 Subject: [PATCH 1/3] Introduce AppConfigurationAbortError for app validate json --- packages/app/src/cli/services/validation-result.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/app/src/cli/services/validation-result.ts b/packages/app/src/cli/services/validation-result.ts index 23dc56bc2f..79bca2d793 100644 --- a/packages/app/src/cli/services/validation-result.ts +++ b/packages/app/src/cli/services/validation-result.ts @@ -1,6 +1,6 @@ import type {AppValidationIssue} from '../models/app/error-parsing.js' -interface AppValidationResult { +export interface AppValidationResult { valid: boolean issues: AppValidationIssue[] } From 283b883d50997c55ed42d12ce293062e4097a1d0 Mon Sep 17 00:00:00 2001 From: Donald Merand Date: Wed, 25 Mar 2026 16:35:41 -0400 Subject: [PATCH 2/3] Surface malformed discovered configs in validate json --- .../app/src/cli/models/project/project.ts | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/packages/app/src/cli/models/project/project.ts b/packages/app/src/cli/models/project/project.ts index 7afceff606..5dde3d50d5 100644 --- a/packages/app/src/cli/models/project/project.ts +++ b/packages/app/src/cli/models/project/project.ts @@ -303,24 +303,18 @@ async function discoverDotEnvFiles(directory: string): Promise() - for (const entry of entries) { - if (entry) result.set(entry[0], entry[1]) - } - return result + return new Map(entries.filter(Boolean) as [string, DotEnvFile][]) } -/** Load the raw .shopify/project.json as JsonMapType */ +/** Read raw .shopify/project.json if present */ async function loadRawHiddenConfig(directory: string): Promise { - const hiddenPath = joinPath(directory, configurationFileNames.hiddenFolder, configurationFileNames.hiddenConfig) + const path = joinPath(directory, '.shopify', 'project.json') + if (!(await fileExists(path))) return {} + try { - if (await fileExists(hiddenPath)) { - const raw = await readFile(hiddenPath) - return JSON.parse(raw) as JsonMapType - } + return JSON.parse(await readFile(path)) as JsonMapType // eslint-disable-next-line no-catch-all/no-catch-all } catch { - // Parse errors are not fatal + return {} } - return {} } From 92db345b9325722ae502bcafb1cbf327f24129c0 Mon Sep 17 00:00:00 2001 From: Donald Merand Date: Thu, 26 Mar 2026 16:24:33 -0400 Subject: [PATCH 3/3] Add monorail telemetry for app config validate --- .../cli/commands/app/config/validate.test.ts | 46 +++++++++++++ .../src/cli/commands/app/config/validate.ts | 24 ++++++- .../app/src/cli/models/project/project.ts | 20 ++++-- .../app/src/cli/services/validate.test.ts | 62 +++++++++++++++++ packages/app/src/cli/services/validate.ts | 69 +++++++++++++++++-- .../app/src/cli/services/validation-result.ts | 2 +- .../cli-kit/src/private/node/analytics.ts | 6 ++ packages/cli-kit/src/public/node/monorail.ts | 4 ++ 8 files changed, 216 insertions(+), 17 deletions(-) diff --git a/packages/app/src/cli/commands/app/config/validate.test.ts b/packages/app/src/cli/commands/app/config/validate.test.ts index 16fc36676e..9fd4ca23aa 100644 --- a/packages/app/src/cli/commands/app/config/validate.test.ts +++ b/packages/app/src/cli/commands/app/config/validate.test.ts @@ -1,6 +1,7 @@ import Validate from './validate.js' import {linkedAppContext} from '../../../services/app-context.js' import {validateApp} from '../../../services/validate.js' +import metadata from '../../../metadata.js' import {testAppLinked} from '../../../models/app/app.test-data.js' import {LocalConfigError} from '../../../models/app/local-config-error.js' import {describe, expect, test, vi} from 'vitest' @@ -9,6 +10,7 @@ import {outputResult} from '@shopify/cli-kit/node/output' vi.mock('../../../services/app-context.js') vi.mock('../../../services/validate.js') +vi.mock('../../../metadata.js', () => ({default: {addPublicMetadata: vi.fn()}})) vi.mock('@shopify/cli-kit/node/output', async (importOriginal) => { const actual = await importOriginal() return { @@ -17,6 +19,13 @@ vi.mock('@shopify/cli-kit/node/output', async (importOriginal) => { } }) +async function expectValidationMetadataCalls(...expectedMetadata: Record[]) { + const metadataCalls = vi.mocked(metadata.addPublicMetadata).mock.calls.map(([getMetadata]) => getMetadata) + expect(metadataCalls).toHaveLength(expectedMetadata.length) + + await expect(Promise.all(metadataCalls.map((getMetadata) => getMetadata()))).resolves.toEqual(expectedMetadata) +} + describe('app config validate command', () => { test('calls validateApp with json: false by default', async () => { // Given @@ -29,6 +38,7 @@ describe('app config validate command', () => { // Then expect(validateApp).toHaveBeenCalledWith(app, {json: false}) + await expectValidationMetadataCalls({cmd_app_validate_json: false}) }) test('calls validateApp with json: true when --json flag is passed', async () => { @@ -42,6 +52,7 @@ describe('app config validate command', () => { // Then expect(validateApp).toHaveBeenCalledWith(app, {json: true}) + await expectValidationMetadataCalls({cmd_app_validate_json: true}) }) test('calls validateApp with json: true when -j flag is passed', async () => { @@ -55,6 +66,7 @@ describe('app config validate command', () => { // Then expect(validateApp).toHaveBeenCalledWith(app, {json: true}) + await expectValidationMetadataCalls({cmd_app_validate_json: true}) }) test('rethrows LocalConfigError in non-json mode without emitting json', async () => { @@ -67,6 +79,14 @@ describe('app config validate command', () => { await expect(Validate.run(['--path=/tmp/app'], import.meta.url)).rejects.toThrow() expect(outputResult).not.toHaveBeenCalled() expect(validateApp).not.toHaveBeenCalled() + await expectValidationMetadataCalls( + {cmd_app_validate_json: false}, + { + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 1, + cmd_app_validate_file_count: 1, + }, + ) }) test('outputs structured configuration issues from app loading before validateApp runs', async () => { @@ -109,6 +129,14 @@ describe('app config validate command', () => { ), ) expect(validateApp).not.toHaveBeenCalled() + await expectValidationMetadataCalls( + {cmd_app_validate_json: true}, + { + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 1, + cmd_app_validate_file_count: 1, + }, + ) }) test('outputs a root json issue when app loading fails without structured issues', async () => { @@ -140,6 +168,14 @@ describe('app config validate command', () => { ), ) expect(validateApp).not.toHaveBeenCalled() + await expectValidationMetadataCalls( + {cmd_app_validate_json: true}, + { + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 1, + cmd_app_validate_file_count: 1, + }, + ) }) test('outputs json when validateApp throws a structured configuration abort', async () => { @@ -181,6 +217,14 @@ describe('app config validate command', () => { 2, ), ) + await expectValidationMetadataCalls( + {cmd_app_validate_json: true}, + { + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 1, + cmd_app_validate_file_count: 1, + }, + ) }) test('rethrows non-configuration errors from validateApp in json mode without converting them to validation json', async () => { @@ -192,6 +236,7 @@ describe('app config validate command', () => { // When / Then await expect(Validate.run(['--json'], import.meta.url)).rejects.toThrow() expect(outputResult).not.toHaveBeenCalled() + await expectValidationMetadataCalls({cmd_app_validate_json: true}) }) test('rethrows unrelated abort errors in json mode without converting them to validation json', async () => { @@ -202,5 +247,6 @@ describe('app config validate command', () => { await expect(Validate.run(['--json', '--path=/tmp/app'], import.meta.url)).rejects.toThrow() expect(outputResult).not.toHaveBeenCalled() expect(validateApp).not.toHaveBeenCalled() + await expectValidationMetadataCalls({cmd_app_validate_json: true}) }) }) diff --git a/packages/app/src/cli/commands/app/config/validate.ts b/packages/app/src/cli/commands/app/config/validate.ts index 4ad584bff4..79fc72f7c5 100644 --- a/packages/app/src/cli/commands/app/config/validate.ts +++ b/packages/app/src/cli/commands/app/config/validate.ts @@ -2,6 +2,7 @@ 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 metadata from '../../../metadata.js' import {toRootValidationIssue} from '../../../models/app/error-parsing.js' import {LocalConfigError} from '../../../models/app/local-config-error.js' import {invalidAppValidationResult, stringifyAppValidationResult} from '../../../services/validation-result.js' @@ -9,6 +10,10 @@ import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' import {AbortSilentError} from '@shopify/cli-kit/node/error' import {outputResult, stringifyMessage} from '@shopify/cli-kit/node/output' +function getValidationIssueFileCount(issues: {filePath: string}[]) { + return new Set(issues.map((issue) => issue.filePath)).size +} + export default class Validate extends AppLinkedCommand { static summary = 'Validate your app configuration and extensions.' @@ -25,6 +30,10 @@ export default class Validate extends AppLinkedCommand { public async run(): Promise { const {flags} = await this.parse(Validate) + await metadata.addPublicMetadata(() => ({ + cmd_app_validate_json: flags.json, + })) + try { const {app} = await linkedAppContext({ directory: flags.path, @@ -38,13 +47,22 @@ export default class Validate extends AppLinkedCommand { return {app} } catch (error) { - if (flags.json && error instanceof LocalConfigError) { + if (error instanceof LocalConfigError) { const issues = error.issues.length > 0 ? error.issues : [toRootValidationIssue(error.configurationPath, stringifyMessage(error.message).trim())] - outputResult(stringifyAppValidationResult(invalidAppValidationResult(issues))) - throw new AbortSilentError() + + await metadata.addPublicMetadata(() => ({ + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: issues.length, + cmd_app_validate_file_count: getValidationIssueFileCount(issues), + })) + + if (flags.json) { + outputResult(stringifyAppValidationResult(invalidAppValidationResult(issues))) + throw new AbortSilentError() + } } throw error diff --git a/packages/app/src/cli/models/project/project.ts b/packages/app/src/cli/models/project/project.ts index 5dde3d50d5..7afceff606 100644 --- a/packages/app/src/cli/models/project/project.ts +++ b/packages/app/src/cli/models/project/project.ts @@ -303,18 +303,24 @@ async function discoverDotEnvFiles(directory: string): Promise() + for (const entry of entries) { + if (entry) result.set(entry[0], entry[1]) + } + return result } -/** Read raw .shopify/project.json if present */ +/** Load the raw .shopify/project.json as JsonMapType */ async function loadRawHiddenConfig(directory: string): Promise { - const path = joinPath(directory, '.shopify', 'project.json') - if (!(await fileExists(path))) return {} - + const hiddenPath = joinPath(directory, configurationFileNames.hiddenFolder, configurationFileNames.hiddenConfig) try { - return JSON.parse(await readFile(path)) as JsonMapType + if (await fileExists(hiddenPath)) { + const raw = await readFile(hiddenPath) + return JSON.parse(raw) as JsonMapType + } // eslint-disable-next-line no-catch-all/no-catch-all } catch { - return {} + // Parse errors are not fatal } + return {} } diff --git a/packages/app/src/cli/services/validate.test.ts b/packages/app/src/cli/services/validate.test.ts index 7ba5d2b2ac..4b9e20799d 100644 --- a/packages/app/src/cli/services/validate.test.ts +++ b/packages/app/src/cli/services/validate.test.ts @@ -1,11 +1,13 @@ import {validateApp} from './validate.js' import {testAppLinked} from '../models/app/app.test-data.js' +import metadata from '../metadata.js' import {AppErrors} from '../models/app/loader.js' import {describe, expect, test, vi} from 'vitest' import {outputResult} from '@shopify/cli-kit/node/output' import {renderError, renderSuccess} from '@shopify/cli-kit/node/ui' import {AbortSilentError} from '@shopify/cli-kit/node/error' +vi.mock('../metadata.js', () => ({default: {addPublicMetadata: vi.fn()}})) vi.mock('@shopify/cli-kit/node/output', async (importOriginal) => { const actual = await importOriginal() return { @@ -15,6 +17,16 @@ vi.mock('@shopify/cli-kit/node/output', async (importOriginal) => { }) vi.mock('@shopify/cli-kit/node/ui') +async function expectLastValidationMetadata(expected: { + cmd_app_validate_valid: boolean + cmd_app_validate_issue_count: number + cmd_app_validate_file_count: number +}) { + const getMetadata = vi.mocked(metadata.addPublicMetadata).mock.calls.at(-1)?.[0] + expect(getMetadata).toBeDefined() + await expect(Promise.resolve(getMetadata!())).resolves.toEqual(expected) +} + describe('validateApp', () => { test('renders success when there are no errors', async () => { // Given @@ -27,6 +39,11 @@ describe('validateApp', () => { expect(renderSuccess).toHaveBeenCalledWith({headline: 'App configuration is valid.'}) expect(renderError).not.toHaveBeenCalled() expect(outputResult).not.toHaveBeenCalled() + await expectLastValidationMetadata({ + cmd_app_validate_valid: true, + cmd_app_validate_issue_count: 0, + cmd_app_validate_file_count: 0, + }) }) test('outputs json success when --json is enabled and there are no errors', async () => { @@ -40,6 +57,11 @@ describe('validateApp', () => { expect(outputResult).toHaveBeenCalledWith(JSON.stringify({valid: true, issues: []}, null, 2)) expect(renderSuccess).not.toHaveBeenCalled() expect(renderError).not.toHaveBeenCalled() + await expectLastValidationMetadata({ + cmd_app_validate_valid: true, + cmd_app_validate_issue_count: 0, + cmd_app_validate_file_count: 0, + }) }) test('renders errors and throws when there are validation errors', async () => { @@ -58,6 +80,11 @@ describe('validateApp', () => { }) expect(renderSuccess).not.toHaveBeenCalled() expect(outputResult).not.toHaveBeenCalled() + await expectLastValidationMetadata({ + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 2, + cmd_app_validate_file_count: 2, + }) }) test('outputs json errors and throws when --json is enabled and there are validation errors', async () => { @@ -95,6 +122,11 @@ describe('validateApp', () => { ) expect(renderError).not.toHaveBeenCalled() expect(renderSuccess).not.toHaveBeenCalled() + await expectLastValidationMetadata({ + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 2, + cmd_app_validate_file_count: 2, + }) }) test('outputs only structured issues when the rendered message matches them exactly', async () => { @@ -221,6 +253,36 @@ describe('validateApp', () => { 2, ), ) + await expectLastValidationMetadata({ + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 1, + cmd_app_validate_file_count: 1, + }) + }) + + test('counts a distinct root issue when the rendered message changes to a new same-file error', async () => { + // Given + const errors = new AppErrors() + errors.addError('/path/to/shopify.app.toml', '• [client_id]: Required', [ + { + filePath: '/path/to/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ]) + errors.addError('/path/to/shopify.app.toml', 'Unsupported section(s) in app configuration: webhooks') + const app = testAppLinked() + app.errors = errors + + // When / Then + await expect(validateApp(app, {json: true})).rejects.toThrow(AbortSilentError) + await expectLastValidationMetadata({ + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 2, + cmd_app_validate_file_count: 1, + }) }) test('renders success when errors object exists but is empty', async () => { diff --git a/packages/app/src/cli/services/validate.ts b/packages/app/src/cli/services/validate.ts index 3377983b33..66bea0d726 100644 --- a/packages/app/src/cli/services/validate.ts +++ b/packages/app/src/cli/services/validate.ts @@ -4,6 +4,7 @@ import { validAppValidationResult, } from './validation-result.js' import {AppLinkedInterface} from '../models/app/app.js' +import metadata from '../metadata.js' import {AbortSilentError} from '@shopify/cli-kit/node/error' import {outputResult, stringifyMessage} from '@shopify/cli-kit/node/output' import {renderError, renderSuccess} from '@shopify/cli-kit/node/ui' @@ -22,18 +23,26 @@ function toRootIssue(filePath: string, message: string): AppValidationIssue { } } +function normalizeStructuredMessage(message: string) { + return message.trim().replace(/^App configuration is not valid\n/, '') +} + +function getStructuredIssueLines(issues: AppValidationIssue[], startIndex: number) { + return issues + .slice(startIndex) + .map((issue) => `• [${issue.pathString}]: ${issue.message}`) + .join('\n') +} + // Some loader/config messages are just a rendered wrapper around the same // structured issues. Detect that case so JSON mode doesn't duplicate them. function isStructuredMessageEquivalent(message: string, issues: AppValidationIssue[]): boolean { if (issues.length === 0) return false - const normalizedMessage = message.trim().replace(/^App configuration is not valid\n/, '') + const normalizedMessage = normalizeStructuredMessage(message) for (let startIndex = 0; startIndex < issues.length; startIndex++) { - const issueLines = issues - .slice(startIndex) - .map((issue) => `• [${issue.pathString}]: ${issue.message}`) - .join('\n') + const issueLines = getStructuredIssueLines(issues, startIndex) if (normalizedMessage === issueLines) { return true @@ -48,6 +57,27 @@ function isStructuredMessageEquivalent(message: string, issues: AppValidationIss return false } +function hasStructuredIssueListing(message: string, issues: AppValidationIssue[]): boolean { + if (issues.length === 0) return false + + const normalizedMessage = normalizeStructuredMessage(message) + + for (let startIndex = 0; startIndex < issues.length; startIndex++) { + const issueLines = getStructuredIssueLines(issues, startIndex) + + if (normalizedMessage === issueLines) { + return true + } + + const isValidationWrapper = normalizedMessage.startsWith('Validation errors in ') + if (isValidationWrapper && normalizedMessage.includes(`\n\n${issueLines}`)) { + return true + } + } + + return false +} + function toPublicIssues(app: AppLinkedInterface): AppValidationIssue[] { const structuredErrors = app.errors?.toStructuredJSON() ?? [] @@ -59,10 +89,33 @@ function toPublicIssues(app: AppLinkedInterface): AppValidationIssue[] { }) } +function toTelemetryIssues(app: AppLinkedInterface): AppValidationIssue[] { + const structuredErrors = app.errors?.toStructuredJSON() ?? [] + + return structuredErrors.flatMap(({filePath, message, issues}) => { + const renderedMessage = stringifyMessage(message).trim() + if (issues.length === 0) return [toRootIssue(filePath, renderedMessage)] + if (hasStructuredIssueListing(renderedMessage, issues)) return issues + return [...issues, toRootIssue(filePath, renderedMessage)] + }) +} + +async function recordValidationMetadata(valid: boolean, issues: AppValidationIssue[]) { + const distinctFileCount = new Set(issues.map((issue) => issue.filePath)).size + + await metadata.addPublicMetadata(() => ({ + cmd_app_validate_valid: valid, + cmd_app_validate_issue_count: issues.length, + cmd_app_validate_file_count: distinctFileCount, + })) +} + export async function validateApp(app: AppLinkedInterface, options: ValidateAppOptions = {json: false}): Promise { const errors = app.errors if (!errors || errors.isEmpty()) { + await recordValidationMetadata(true, []) + if (options.json) { outputResult(stringifyAppValidationResult(validAppValidationResult())) return @@ -72,10 +125,14 @@ export async function validateApp(app: AppLinkedInterface, options: ValidateAppO return } + const publicIssues = toPublicIssues(app) + const telemetryIssues = toTelemetryIssues(app) const errorMessages = errors.toJSON().map((error) => stringifyMessage(error).trim()) + await recordValidationMetadata(false, telemetryIssues) + if (options.json) { - outputResult(stringifyAppValidationResult(invalidAppValidationResult(toPublicIssues(app)))) + outputResult(stringifyAppValidationResult(invalidAppValidationResult(publicIssues))) throw new AbortSilentError() } diff --git a/packages/app/src/cli/services/validation-result.ts b/packages/app/src/cli/services/validation-result.ts index 79bca2d793..23dc56bc2f 100644 --- a/packages/app/src/cli/services/validation-result.ts +++ b/packages/app/src/cli/services/validation-result.ts @@ -1,6 +1,6 @@ import type {AppValidationIssue} from '../models/app/error-parsing.js' -export interface AppValidationResult { +interface AppValidationResult { valid: boolean issues: AppValidationIssue[] } diff --git a/packages/cli-kit/src/private/node/analytics.ts b/packages/cli-kit/src/private/node/analytics.ts index 3034d8c1ef..0306936343 100644 --- a/packages/cli-kit/src/private/node/analytics.ts +++ b/packages/cli-kit/src/private/node/analytics.ts @@ -103,6 +103,12 @@ export async function getSensitiveEnvironmentData(config: Interfaces.Config) { } function getShopifyEnvironmentVariables() { + // Agent callers can identify themselves today via SHOPIFY_* environment + // variables. The current contract is intentionally lightweight and is kept in + // the sensitive payload until we prove which dimensions deserve first-class + // Monorail fields, e.g. SHOPIFY_CLI_AGENT, SHOPIFY_CLI_AGENT_VERSION, + // SHOPIFY_CLI_AGENT_RUN_ID, SHOPIFY_CLI_AGENT_SESSION_ID, and + // SHOPIFY_CLI_AGENT_PROVIDER. return Object.fromEntries(Object.entries(process.env).filter(([key]) => key.startsWith('SHOPIFY_'))) } diff --git a/packages/cli-kit/src/public/node/monorail.ts b/packages/cli-kit/src/public/node/monorail.ts index b687ec9fee..dc6130bc37 100644 --- a/packages/cli-kit/src/public/node/monorail.ts +++ b/packages/cli-kit/src/public/node/monorail.ts @@ -88,6 +88,10 @@ export interface Schemas { cmd_app_linked_config_uses_cli_managed_urls?: Optional cmd_app_warning_api_key_deprecation_displayed?: Optional cmd_app_deployment_mode?: Optional + cmd_app_validate_json?: Optional + cmd_app_validate_valid?: Optional + cmd_app_validate_issue_count?: Optional + cmd_app_validate_file_count?: Optional // Dev related commands cmd_dev_tunnel_type?: Optional