From 52af2a594766b84f5cd345fcc977a4dc83986c96 Mon Sep 17 00:00:00 2001 From: Ryan Bahan Date: Mon, 6 Apr 2026 09:44:02 -0600 Subject: [PATCH] add monorail recording --- .../cli/commands/app/config/validate.test.ts | 95 +++++++++++++++++++ .../src/cli/commands/app/config/validate.ts | 18 ++++ .../app/src/cli/services/validate.test.ts | 37 ++++++++ packages/app/src/cli/services/validate.ts | 14 +++ .../cli-kit/src/private/node/analytics.ts | 6 ++ packages/cli-kit/src/public/node/monorail.ts | 4 + 6 files changed, 174 insertions(+) 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 784c89a1031..98fdd12ce00 100644 --- a/packages/app/src/cli/commands/app/config/validate.test.ts +++ b/packages/app/src/cli/commands/app/config/validate.test.ts @@ -5,7 +5,9 @@ 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 metadata from '../../../metadata.js' import {outputResult} from '@shopify/cli-kit/node/output' +import {AbortError} from '@shopify/cli-kit/node/error' import {TomlFile} from '@shopify/cli-kit/node/toml/toml-file' import {describe, expect, test, vi} from 'vitest' @@ -14,12 +16,19 @@ 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('../../../metadata.js', () => ({default: {addPublicMetadata: vi.fn()}})) vi.mock('@shopify/cli-kit/node/output', async (importOriginal) => { const actual = await importOriginal() return {...actual, outputResult: vi.fn()} }) vi.mock('@shopify/cli-kit/node/ui') +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) +} + function mockHealthyProject() { vi.mocked(Project.load).mockResolvedValue({errors: []} as unknown as Project) vi.mocked(selectActiveConfig).mockResolvedValue({file: new TomlFile('shopify.app.toml', {})} as any) @@ -36,6 +45,7 @@ describe('app config validate command', () => { await Validate.run([], import.meta.url) 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 () => { @@ -47,6 +57,7 @@ describe('app config validate command', () => { await Validate.run(['--json'], import.meta.url) 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 () => { @@ -58,6 +69,7 @@ describe('app config validate command', () => { await Validate.run(['-j'], import.meta.url) expect(validateApp).toHaveBeenCalledWith(app, {json: true}) + await expectValidationMetadataCalls({cmd_app_validate_json: true}) }) test('outputs JSON issues when active config has TOML parse errors', async () => { @@ -71,5 +83,88 @@ describe('app config validate command', () => { expect(outputResult).toHaveBeenCalledWith(expect.stringContaining('"valid": false')) expect(linkedAppContext).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('records failure metadata for config errors in non-json mode', 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: 'Missing required field'} as any, + {path: '/app/shopify.app.toml', message: 'Invalid value'} as any, + ]) + + await expect(Validate.run([], import.meta.url)).rejects.toThrow() + + expect(linkedAppContext).not.toHaveBeenCalled() + await expectValidationMetadataCalls( + {cmd_app_validate_json: false}, + { + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 2, + cmd_app_validate_file_count: 1, + }, + ) + }) + + test('records failure metadata when Project.load fails with --json', async () => { + vi.mocked(Project.load).mockRejectedValue(new AbortError('Could not find app configuration')) + + await expect(Validate.run(['--json'], import.meta.url)).rejects.toThrow() + + expect(outputResult).toHaveBeenCalledWith(expect.stringContaining('"valid": false')) + expect(selectActiveConfig).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('records failure metadata when selectActiveConfig fails with --json', async () => { + vi.mocked(Project.load).mockResolvedValue({errors: []} as unknown as Project) + vi.mocked(selectActiveConfig).mockRejectedValue(new AbortError('No config found')) + + await expect(Validate.run(['--json'], import.meta.url)).rejects.toThrow() + + expect(outputResult).toHaveBeenCalledWith(expect.stringContaining('"valid": false')) + expect(linkedAppContext).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('records failure metadata when linkedAppContext throws a validation error with --json', 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([]) + vi.mocked(linkedAppContext).mockRejectedValue(new AbortError('Validation errors in /app/shopify.app.toml')) + + await expect(Validate.run(['--json'], import.meta.url)).rejects.toThrow() + + expect(outputResult).toHaveBeenCalledWith(expect.stringContaining('"valid": false')) + await expectValidationMetadataCalls( + {cmd_app_validate_json: true}, + { + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 1, + cmd_app_validate_file_count: 1, + }, + ) }) }) diff --git a/packages/app/src/cli/commands/app/config/validate.ts b/packages/app/src/cli/commands/app/config/validate.ts index dd9efc4958a..4da9ddc48ba 100644 --- a/packages/app/src/cli/commands/app/config/validate.ts +++ b/packages/app/src/cli/commands/app/config/validate.ts @@ -5,11 +5,20 @@ 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 metadata from '../../../metadata.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' +async function recordValidationFailure(issueCount: number, fileCount: number) { + await metadata.addPublicMetadata(() => ({ + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: issueCount, + cmd_app_validate_file_count: fileCount, + })) +} + export default class Validate extends AppLinkedCommand { static summary = 'Validate your app configuration and extensions.' @@ -26,12 +35,17 @@ 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, + })) + // Stage 1: Load project let project: Project try { project = await Project.load(flags.path) } catch (err) { if (err instanceof AbortError && flags.json) { + await recordValidationFailure(1, 1) const message = unstyled(stringifyMessage(err.message)).trim() outputResult(JSON.stringify({valid: false, issues: [{message}]}, null, 2)) throw new AbortSilentError() @@ -45,6 +59,7 @@ export default class Validate extends AppLinkedCommand { activeConfig = await selectActiveConfig(project, flags.config) } catch (err) { if (err instanceof AbortError && flags.json) { + await recordValidationFailure(1, 1) const message = unstyled(stringifyMessage(err.message)).trim() outputResult(JSON.stringify({valid: false, issues: [{message}]}, null, 2)) throw new AbortSilentError() @@ -55,6 +70,8 @@ export default class Validate extends AppLinkedCommand { const configErrors = errorsForConfig(project, activeConfig.file) if (configErrors.length > 0) { const issues = configErrors.map((err) => ({file: err.path, message: err.message})) + const fileCount = new Set(configErrors.map((err) => err.path)).size + await recordValidationFailure(issues.length, fileCount) if (flags.json) { outputResult(JSON.stringify({valid: false, issues}, null, 2)) throw new AbortSilentError() @@ -83,6 +100,7 @@ export default class Validate extends AppLinkedCommand { const message = err instanceof AbortError ? unstyled(stringifyMessage(err.message)).trim() : '' const isValidationError = message.startsWith('Validation errors in ') if (isValidationError && flags.json) { + await recordValidationFailure(1, 1) outputResult(JSON.stringify({valid: false, issues: [{message}]}, null, 2)) throw new AbortSilentError() } diff --git a/packages/app/src/cli/services/validate.test.ts b/packages/app/src/cli/services/validate.test.ts index f6c16446640..f22f543286f 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 {AppErrors, formatConfigurationError} from '../models/app/loader.js' +import metadata from '../metadata.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('formatConfigurationError', () => { test('returns plain message when no path', () => { expect(formatConfigurationError({file: 'foo.toml', message: 'something broke'})).toBe('something broke') @@ -34,6 +46,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 () => { @@ -42,6 +59,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 structured json issues when --json is enabled and there are validation errors', async () => { @@ -83,6 +110,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('renders success when errors object exists but is empty', async () => { @@ -111,5 +143,10 @@ describe('validateApp', () => { 2, ), ) + await expectLastValidationMetadata({ + cmd_app_validate_valid: false, + cmd_app_validate_issue_count: 1, + cmd_app_validate_file_count: 1, + }) }) }) diff --git a/packages/app/src/cli/services/validate.ts b/packages/app/src/cli/services/validate.ts index 00b634c90af..7655c369a5b 100644 --- a/packages/app/src/cli/services/validate.ts +++ b/packages/app/src/cli/services/validate.ts @@ -1,5 +1,6 @@ import {AppLinkedInterface} from '../models/app/app.js' import {formatConfigurationError} from '../models/app/loader.js' +import metadata from '../metadata.js' 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' @@ -8,10 +9,22 @@ interface ValidateAppOptions { json: boolean } +async function recordValidationMetadata(valid: boolean, errors: {file: string}[]) { + const fileCount = new Set(errors.map((error) => error.file)).size + + await metadata.addPublicMetadata(() => ({ + cmd_app_validate_valid: valid, + cmd_app_validate_issue_count: errors.length, + cmd_app_validate_file_count: fileCount, + })) +} + export async function validateApp(app: AppLinkedInterface, options: ValidateAppOptions = {json: false}): Promise { const appErrors = app.errors if (!appErrors || appErrors.isEmpty()) { + await recordValidationMetadata(true, []) + if (options.json) { outputResult(JSON.stringify({valid: true, issues: []}, null, 2)) return @@ -22,6 +35,7 @@ export async function validateApp(app: AppLinkedInterface, options: ValidateAppO } const errors = appErrors.getErrors() + await recordValidationMetadata(false, errors) if (options.json) { const issues = errors.map(({file, message, path, code}) => ({file, message, path, code})) diff --git a/packages/cli-kit/src/private/node/analytics.ts b/packages/cli-kit/src/private/node/analytics.ts index 3034d8c1eff..0306936343c 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 b687ec9fee4..dc6130bc372 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