Skip to content

Split CLI error analytics grouping #7894

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
stephanie-shopify wants to merge 2 commits into from
Closed

Split CLI error analytics grouping #7894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73c1243
Split CLI error analytics grouping
stephanie-shopify Jun 22, 2026
6a8a4e2
Fix lint for Bugsnag callback rejection
stephanie-shopify Jun 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/split-observe-error-groups.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/cli-kit': patch
---

Split CLI error analytics grouping by command slice and normalized error signature.
105 changes: 95 additions & 10 deletions packages/cli-kit/src/public/node/error-handler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {beforeEach, describe, expect, test, vi} from 'vitest'

const onNotify = vi.fn()
const capturedEventHandler = vi.fn()
let lastBugsnagEvent: {addMetadata: ReturnType<typeof vi.fn>} | undefined
let lastBugsnagEvent: {addMetadata: ReturnType<typeof vi.fn>; groupingHash?: string} | undefined

vi.mock('process')
vi.mock('@bugsnag/js', () => {
Expand Down Expand Up @@ -174,15 +174,6 @@ describe('skips sending errors to Bugsnag', () => {
expect(onNotify).not.toHaveBeenCalled()
expect(mockOutput.debug()).toMatch('Skipping Bugsnag report')
})

test('when error is expected', async () => {
const mockOutput = mockAndCaptureOutput()
const res = await sendErrorToBugsnag(new error.AbortError('In test'), 'expected_error')
expect(res.reported).toEqual(false)
expect(res.unhandled).toBeUndefined()
expect(onNotify).not.toHaveBeenCalled()
expect(mockOutput.debug()).toMatch('Skipping Bugsnag report for expected error')
})
})

describe('sends errors to Bugsnag', () => {
Expand All @@ -209,6 +200,13 @@ describe('sends errors to Bugsnag', () => {
expect(onNotify).toHaveBeenCalledWith(res.error)
})

test('processes AbortErrors as handled', async () => {
const res = await sendErrorToBugsnag(new error.AbortError('In test'), 'expected_error')
expect(res.reported).toEqual(true)
expect(res.unhandled).toEqual(false)
expect(onNotify).toHaveBeenCalledWith(res.error)
})

test.each([null, undefined, {}, {message: 'nope'}])('deals with strange things to throw %s', async (throwable) => {
const res = await sendErrorToBugsnag(throwable, 'unexpected_error')
expect(res.reported).toEqual(false)
Expand Down Expand Up @@ -303,4 +301,91 @@ describe('sends errors to Bugsnag', () => {
expect(lastBugsnagEvent).toBeDefined()
expect(lastBugsnagEvent!.addMetadata).toHaveBeenCalledWith('custom', {slice_name: 'cli'})
})

test('sets groupingHash using the command slice and analytics error taxonomy', async () => {
await metadata.addSensitiveMetadata(() => ({
commandStartOptions: {startTime: Date.now(), startCommand: 'app deploy', startArgs: []},
}))

await sendErrorToBugsnag(
new Error(
'The App Management GraphQL API responded unsuccessfully with the HTTP status 403 and errors: Unauthorized',
),
'unexpected_error',
)

expect(lastBugsnagEvent).toBeDefined()
expect(lastBugsnagEvent!.groupingHash).toMatch(/^app:authentication:/)
})

test('normalizes GraphQL ClientError JSON dumps before setting groupingHash', async () => {
await metadata.addSensitiveMetadata(() => ({
commandStartOptions: {startTime: Date.now(), startCommand: 'theme dev', startArgs: []},
}))
const payload = {
response: {
status: 200,
errors: [
{
message: 'Access denied for themes field. Required access: read_themes access scope.',
extensions: {code: 'ACCESS_DENIED'},
},
],
},
request: {query: 'query Theme { themes { nodes { id } } }'},
}

await sendErrorToBugsnag(new Error(`GraphQL Error (Code: 200): ${JSON.stringify(payload)}`), 'unexpected_error')

expect(lastBugsnagEvent).toBeDefined()
expect(lastBugsnagEvent!.groupingHash).toMatch(/^theme:permission:/)
expect(lastBugsnagEvent!.groupingHash).not.toContain(':network:')
})

test('groups GraphQL authentication failures separately from permission failures', async () => {
await metadata.addSensitiveMetadata(() => ({
commandStartOptions: {startTime: Date.now(), startCommand: 'store info', startArgs: []},
}))
const payload = {
response: {
status: 401,
errors: [{message: 'Service is not valid for authentication'}],
},
request: {query: 'query Shop { shop { id } }'},
}

await sendErrorToBugsnag(new Error(`GraphQL Error (Code: 401): ${JSON.stringify(payload)}`), 'unexpected_error')

expect(lastBugsnagEvent).toBeDefined()
expect(lastBugsnagEvent!.groupingHash).toMatch(/^store:authentication:/)
})

test('groups GraphQL throttling failures as rate limits', async () => {
await metadata.addSensitiveMetadata(() => ({
commandStartOptions: {startTime: Date.now(), startCommand: 'theme pull', startArgs: []},
}))
const payload = {
response: {
status: 200,
errors: [{message: 'Too many requests', extensions: {code: 'THROTTLED'}}],
},
request: {query: 'query ThemeFiles { theme { files { nodes { filename } } } }'},
}

await sendErrorToBugsnag(new Error(`GraphQL Error (Code: 200): ${JSON.stringify(payload)}`), 'unexpected_error')

expect(lastBugsnagEvent).toBeDefined()
expect(lastBugsnagEvent!.groupingHash).toMatch(/^theme:rate_limit:/)
})

test('sets groupingHash with cli slice when startCommand is missing', async () => {
await metadata.addSensitiveMetadata(() => ({
commandStartOptions: {startTime: Date.now(), startCommand: undefined as unknown as string, startArgs: []},
}))

await sendErrorToBugsnag(new Error('boom'), 'unexpected_error')

expect(lastBugsnagEvent).toBeDefined()
expect(lastBugsnagEvent!.groupingHash).toBe('cli:unknown:boom')
})
})
78 changes: 70 additions & 8 deletions packages/cli-kit/src/public/node/error-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
} from './error.js'
import {outputDebug, outputInfo} from './output.js'
import {getEnvironmentData} from '../../private/node/analytics.js'
import {categorizeError, formatErrorMessage} from '../../private/node/analytics/error-categorizer.js'
import {isLocalEnvironment} from '../../private/node/context/service.js'
import {bugsnagApiKey, reportingRateLimit} from '../../private/node/constants.js'
import {CLI_KIT_VERSION} from '../common/version.js'
Expand All @@ -28,6 +29,72 @@ import {realpath} from 'fs/promises'
// Hardcoded list per product slices to keep analytics consistent.
const ALLOWED_SLICE_NAMES = new Set<string>(['app', 'theme', 'hydrogen', 'store'])

function sliceNameFromStartCommand(startCommand: string | undefined): string {
if (!startCommand) return 'cli'

const firstWord = startCommand.trim().split(/\s+/)[0] ?? 'cli'
return ALLOWED_SLICE_NAMES.has(firstWord) ? firstWord : 'cli'
}

interface GraphQLErrorPayload {
response?: {
status?: number
errors?: {
message?: string
extensions?: {
code?: string
}
}[]
}
}

function errorForGrouping(error: Error): Error {
const graphQLError = graphQLErrorForGrouping(error.message)
if (graphQLError) return graphQLError

const messageWithoutJsonDump = error.message.split(': {')[0] ?? error.message

@stephanie-shopify stephanie-shopify Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 String.prototype.split always returns a non-empty array, so [0] is never undefined and the ?? error.message fallback is unreachable. Same dead fallback on the return new Error(... ?? message) line below. Harmless, just drop for clarity.

return new Error(messageWithoutJsonDump)
}

function graphQLErrorForGrouping(message: string): Error | undefined {
const jsonStart = message.indexOf('{')
if (jsonStart === -1) return undefined

let payload: GraphQLErrorPayload
try {
payload = JSON.parse(message.slice(jsonStart)) as GraphQLErrorPayload
// eslint-disable-next-line no-catch-all/no-catch-all
} catch {
return undefined
}

const {response} = payload
if (!response) return undefined

const firstError = response.errors?.[0]
const code = firstError?.extensions?.code
const firstErrorMessage = firstError?.message?.replace(/too many requests/gi, 'rate limit exceeded')

@stephanie-shopify stephanie-shopify Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 'too many requests' is already a RateLimit term, so this rewrite doesn't change categorization — both forms categorize as rate_limit. If the goal is just a tidier signature, a one-line comment would help; otherwise it's removable.


const parts: string[] = []
if (response.status === 401 && !firstErrorMessage?.match(/auth|credential|token|unauthorized/i)) {
parts.push('Unauthorized')
}
if (response.status === 403 && !firstErrorMessage?.match(/access|denied|forbidden|permission|unauthorized/i)) {

@stephanie-shopify stephanie-shopify Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ The injected 'Forbidden' is misleading given the taxonomy: 'forbidden' is an Authentication term, not a Permission term (['unauthorized','forbidden','auth','token','credential'] vs ['permission','denied','access','insufficient']). So a 403 with a generic message lands in :authentication:, even though 403 = permission semantically — and the issue explicitly frames App-Management 403 as a permission problem distinct from 401 auth.

Decide explicitly: if 403 should be permission, push 'access denied' instead of 'Forbidden'. If everything 4xx→authentication is fine, drop the misleading word and add a comment. Either way there's no test for the 403-with-JSON-payload path, so current behavior is unverified.

@stephanie-shopify stephanie-shopify Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

403 can be permission instead of forbidden

parts.push('Forbidden')
}
if (response.status) parts.push(`HTTP ${response.status}`)
if (code) parts.push(code)
if (firstErrorMessage) parts.push(firstErrorMessage)

return new Error(parts.length > 0 ? parts.join(' ') : (message.split(': {')[0] ?? message))
}

function bugsnagGroupingHash(error: Error, sliceName: string): string {
const groupingError = errorForGrouping(error)
const category = categorizeError(groupingError)
return `${sliceName}:${category.toLowerCase()}:${formatErrorMessage(groupingError, category)}`
}

export async function errorHandler(
error: Error & {exitCode?: number | undefined},
config?: Interfaces.Config,
Expand Down Expand Up @@ -76,11 +143,6 @@ export async function sendErrorToBugsnag(
return {reported: false, error, unhandled: undefined}
}

if (exitMode === 'expected_error') {
outputDebug(`Skipping Bugsnag report for expected error`)
return {reported: false, error, unhandled: undefined}
}

// If the error was unexpected, we flag it as "unhandled" in Bugsnag. This is a helpful distinction.
const unhandled = exitMode === 'unexpected_error'

@stephanie-shopify stephanie-shopify Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Critical / not in the summary: the diff removes the if (exitMode === 'expected_error') { return {reported:false…} } guard that used to sit right above this line, and flips the test from 'when error is expected' (not reported) to 'processes AbortErrors as handled' (reported: true).

Net effect: every AbortError now flows to Bugsnag as a handled event — these were deliberately suppressed before. This is unrelated to grouping, increases Bugsnag volume/cost by an unquantified amount, and runs opposite to the linked issue's own recommendation ("stop reporting known transient/user errors … so they leave crash reporting entirely").

Please either (a) revert this removal and keep the PR purely about grouping, or (b) split it out, justify it with projected event volume, and document it in both the summary and the changeset (which currently only mentions grouping). As written it's the riskiest line in the diff and the least visible.


Expand Down Expand Up @@ -147,9 +209,9 @@ export async function sendErrorToBugsnag(
// Attach command metadata so we know which CLI command triggered the error
const {commandStartOptions} = metadata.getAllSensitiveMetadata()
const {startCommand} = commandStartOptions ?? {}
const sliceName = sliceNameFromStartCommand(startCommand)
event.groupingHash = bugsnagGroupingHash(reportableError, sliceName)

@stephanie-shopify stephanie-shopify Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting groupingHash unconditionally re-buckets all CLI errors (known, acknowledged). The sharp edge is the Unknown category: formatGenericError keys on the first 50 slugified chars, so two genuinely distinct crashes in the same slice that share an opening message fragment will now merge into one bucket, discarding the stack-trace grouping that keeps them apart today.

Consider skipping the override when category === Unknown (let Bugsnag's stack grouping stand), or folding a short stack fingerprint into the hash for that case. That keeps the win for categorized buckets without coarsening the truly-unknown ones.

if (startCommand) {
const firstWord = startCommand.trim().split(/\s+/)[0] ?? 'cli'
const sliceName = ALLOWED_SLICE_NAMES.has(firstWord) ? firstWord : 'cli'
event.addMetadata('custom', {slice_name: sliceName})
}
}
Expand Down Expand Up @@ -192,7 +254,7 @@ export function cleanStackFrameFilePath({
? currentFilePath
: path.joinPath(projectRoot, currentFilePath)

const matchingPluginPath = pluginLocations.find(({pluginPath}) => fullLocation.startsWith(pluginPath))
const matchingPluginPath = pluginLocations.filter(({pluginPath}) => fullLocation.startsWith(pluginPath))[0]

@stephanie-shopify stephanie-shopify Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Unrelated change, and strictly worse: .find(pred) short-circuits on first match; .filter(pred)[0] walks the whole array and allocates a throwaway array for an identical result. Recommend reverting to .find(...) unless there's a reason I'm missing.


if (matchingPluginPath !== undefined) {
// the plugin name (e.g. @shopify/cli-kit), plus the relative path of the error line from within the plugin's code (e.g. dist/something.js )
Expand Down
Loading