diff --git a/node-src/git/git.test.ts b/node-src/git/git.test.ts index e2bf8bd92..37b63297f 100644 --- a/node-src/git/git.test.ts +++ b/node-src/git/git.test.ts @@ -13,6 +13,7 @@ import { findFilesFromRepositoryRoot, getBranch, getChangedFiles, + getChangedFilesWithStatus, getCommit, getCommittedFileCount, getNumberOfCommitters, @@ -203,6 +204,63 @@ describe('getChangedFiles', () => { }); }); +describe('getChangedFilesWithStatus', () => { + it('parses single-path rows (added, modified, deleted) into status objects', async () => { + execGitCommand.mockResolvedValue( + ['A\tpackage.json', 'M\tsrc/index.ts', 'D\told.ts'].join('\n') + ); + + const result = await getChangedFilesWithStatus(ctx, 'abc123', 'HEAD'); + + expect(result).toEqual([ + { status: 'added', path: 'package.json' }, + { status: 'modified', path: 'src/index.ts' }, + { status: 'deleted', path: 'old.ts' }, + ]); + expect(execGitCommand).toHaveBeenCalledWith( + ctx, + 'git diff --name-status --find-renames=20% abc123 HEAD' + ); + }); + + it('parses a rename row into a renamed status carrying both paths', async () => { + execGitCommand.mockResolvedValue('R097\tlibs/old/package.json\tlibs/app/package.json\n'); + + const result = await getChangedFilesWithStatus(ctx, 'abc123', 'HEAD'); + + expect(result).toEqual([ + { status: 'renamed', fromPath: 'libs/old/package.json', path: 'libs/app/package.json' }, + ]); + }); + + it('maps an unrecognized status code to unknown', async () => { + execGitCommand.mockResolvedValue('X\tweird.txt\n'); + + const result = await getChangedFilesWithStatus(ctx, 'abc123', 'HEAD'); + + expect(result).toEqual([{ status: 'unknown', path: 'weird.txt' }]); + }); + + it('returns an empty array when there are no changes', async () => { + execGitCommand.mockResolvedValue(''); + + const result = await getChangedFilesWithStatus(ctx, 'abc123', 'HEAD'); + + expect(result).toEqual([]); + }); + + it('appends a quoted pathspec when one is provided', async () => { + execGitCommand.mockResolvedValue(''); + + await getChangedFilesWithStatus(ctx, 'abc123', 'HEAD', ':(glob,top)**/pnpm-lock.yaml'); + + expect(execGitCommand).toHaveBeenCalledWith( + ctx, + 'git diff --name-status --find-renames=20% abc123 HEAD -- ":(glob,top)**/pnpm-lock.yaml"' + ); + }); +}); + describe('findFilesFromRepositoryRoot', () => { it('finds files relative to the repository root', async () => { const filesFound = ['package.json', 'another/package/package.json']; diff --git a/node-src/git/git.ts b/node-src/git/git.ts index 7a733a276..a44461ad8 100644 --- a/node-src/git/git.ts +++ b/node-src/git/git.ts @@ -221,6 +221,73 @@ export async function getChangedFiles(deps: GitDeps, baseCommit: string, headCom } } +/** The kind of change Git reported for a file in a `--name-status` diff. */ +export type ChangedFileStatus = + | 'added' + | 'deleted' + | 'modified' + | 'renamed' + | 'typeChanged' + | 'unknown'; + +/** A single row of a `--name-status` diff, parsed into a usable shape. */ +export interface ChangedFileWithStatus { + /** The kind of change Git reported. */ + status: ChangedFileStatus; + /** The file's current path (the new path for renames). */ + path: string; + /** The original path for renames; otherwise undefined. */ + fromPath?: string; +} + +// Maps the leading character of a `--name-status` row to a readable status. Renames carry a +// similarity score suffix (e.g. `R097`), so we only key off the first character. +const NAME_STATUS_CODES: Record = { + A: 'added', + D: 'deleted', + M: 'modified', + R: 'renamed', + T: 'typeChanged', +}; + +/** + * Get the name-status diff of a single commit or between two, with rename detection, parsed into + * one object per changed file. + * + * @param deps Function dependencies. + * @param baseCommit The base commit to diff from. + * @param headCommit The head commit to diff to (empty includes uncommitted changes). + * @param pathspec Optional pathspec limiting the diff (e.g. a `:(glob)` magic pathspec). + * See: https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-pathspec for details on + * magic pathspecs. + * + * @returns One {@link ChangedFileWithStatus} per changed file. + */ +export async function getChangedFilesWithStatus( + deps: GitDeps, + baseCommit: string, + headCommit = '', + pathspec = '' +): Promise { + const pathspecArgument = pathspec ? ` -- "${pathspec}"` : ''; + const output = await execGitCommand( + deps, + `git diff --name-status --find-renames=20% ${baseCommit} ${headCommit}${pathspecArgument}` + ); + + return output + .split(newline) + .filter(Boolean) + .map((line) => { + const [code, ...paths] = line.split('\t'); + const status = NAME_STATUS_CODES[code[0]] ?? 'unknown'; + // Rename rows carry the old path first, then the new path. + return status === 'renamed' + ? { status, fromPath: paths[0], path: paths.at(-1) as string } + : { status, path: paths[0] }; + }); +} + /** * Returns a boolean indicating whether the workspace is up-to-date (neither ahead nor behind) with * the remote. Returns true on error, assuming the workspace is up-to-date. diff --git a/node-src/lib/turbosnap/captureBailException.ts b/node-src/lib/turbosnap/captureBailException.ts index 30e12d60d..0b2833fc2 100644 --- a/node-src/lib/turbosnap/captureBailException.ts +++ b/node-src/lib/turbosnap/captureBailException.ts @@ -11,15 +11,28 @@ import * as Sentry from '@sentry/node'; * @param options Bail context. * @param options.bailSubreason The classified subreason, or `undefined` to use default Sentry grouping. * @param options.bailPath A stable identifier for the bail site (used as the `bail_path` tag). + * @param options.additionalTags Extra tags to attach, e.g. a refined root-cause classification. * * @returns The Sentry event ID for the captured exception. */ export function captureBailException( error: unknown, - { bailSubreason, bailPath }: { bailSubreason: string | undefined; bailPath: string } + { + bailSubreason, + bailPath, + additionalTags, + }: { + bailSubreason: string | undefined; + bailPath: string; + additionalTags?: Record; + } ): string { return Sentry.captureException(error, { - tags: { bail_path: bailPath, ...(bailSubreason && { bail_detail: bailSubreason }) }, + tags: { + bail_path: bailPath, + ...(bailSubreason && { bail_detail: bailSubreason }), + ...additionalTags, + }, ...(bailSubreason && { fingerprint: [bailSubreason] }), }); } diff --git a/node-src/lib/turbosnap/classifyBailRootCause.test.ts b/node-src/lib/turbosnap/classifyBailRootCause.test.ts new file mode 100644 index 000000000..8573846ca --- /dev/null +++ b/node-src/lib/turbosnap/classifyBailRootCause.test.ts @@ -0,0 +1,102 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { getChangedFilesWithStatus as getChangedFilesWithStatusDep } from '../../git/git'; +import TestLogger from '../testLogger'; +import { classifyTagsFromError } from './classifyBailRootCause'; +import { BaselineCheckoutFailedError } from './errors'; + +vi.mock('../../git/git'); +const getChangedFilesWithStatus = vi.mocked(getChangedFilesWithStatusDep); + +const deps = { log: new TestLogger(), options: {} } as any; + +describe('classifyTagsFromError', () => { + it('returns undefined for an error that is not classified yet', async () => { + const result = await classifyTagsFromError( + deps, + new Error("some random error which won't have special handling") + ); + + expect(result).toBeUndefined(); + expect(getChangedFilesWithStatus).not.toHaveBeenCalled(); + }); + + describe('BaselineCheckoutFailedError', () => { + it('classifies a file rename as "baselineManifestMoved"', async () => { + getChangedFilesWithStatus.mockResolvedValue([ + { status: 'renamed', fromPath: 'libs/old/package.json', path: 'libs/app/package.json' }, + ]); + + const result = await classifyTagsFromError( + deps, + new BaselineCheckoutFailedError('abc123:libs/app/package.json') + ); + + expect(result).toEqual({ baseline_failure_kind: 'baselineManifestMoved' }); + }); + + it('classifies a file add as "baselineManifestAdded"', async () => { + getChangedFilesWithStatus.mockResolvedValue([ + { status: 'added', path: 'libs/app/package.json' }, + ]); + + const result = await classifyTagsFromError( + deps, + new BaselineCheckoutFailedError('abc123:libs/app/package.json') + ); + + expect(result).toEqual({ baseline_failure_kind: 'baselineManifestAdded' }); + }); + + it('classifies a file rename as "baselineManifestMoved" over "baselineManifestAdded" when the diff has both rows', async () => { + getChangedFilesWithStatus.mockResolvedValue([ + { status: 'added', path: 'libs/app/package.json' }, + { status: 'renamed', fromPath: 'libs/old/package.json', path: 'libs/app/package.json' }, + ]); + + const result = await classifyTagsFromError( + deps, + new BaselineCheckoutFailedError('abc123:libs/app/package.json') + ); + + expect(result).toEqual({ baseline_failure_kind: 'baselineManifestMoved' }); + }); + + it('returns "unknownBaselineCheckoutFailure" when we cannot classify the change', async () => { + getChangedFilesWithStatus.mockResolvedValue([ + { status: 'modified', path: 'some/other/package.json' }, + ]); + + const result = await classifyTagsFromError( + deps, + new BaselineCheckoutFailedError('abc123:libs/app/package.json') + ); + + expect(result).toEqual({ baseline_failure_kind: 'unknownBaselineCheckoutFailure' }); + }); + + it('returns "unknownBaselineCheckoutFailure" when a git command throws unexpectedly', async () => { + getChangedFilesWithStatus.mockRejectedValue(new Error('git diff blew up')); + + const result = await classifyTagsFromError( + deps, + new BaselineCheckoutFailedError('abc123:libs/app/package.json') + ); + + expect(result).toEqual({ baseline_failure_kind: 'unknownBaselineCheckoutFailure' }); + }); + + it('splits the pathspec on the first colon so a filename containing colons still matches', async () => { + getChangedFilesWithStatus.mockResolvedValue([ + { status: 'renamed', fromPath: 'libs/old/a:b.json', path: 'libs/app/a:b.json' }, + ]); + + const result = await classifyTagsFromError( + deps, + new BaselineCheckoutFailedError('abc123:libs/app/a:b.json') + ); + + expect(result).toEqual({ baseline_failure_kind: 'baselineManifestMoved' }); + }); + }); +}); diff --git a/node-src/lib/turbosnap/classifyBailRootCause.ts b/node-src/lib/turbosnap/classifyBailRootCause.ts new file mode 100644 index 000000000..2f6fc18ed --- /dev/null +++ b/node-src/lib/turbosnap/classifyBailRootCause.ts @@ -0,0 +1,119 @@ +import * as Sentry from '@sentry/node'; +import path from 'path'; + +import { GitDeps } from '../../git/execGit'; +import { ChangedFileWithStatus, getChangedFilesWithStatus } from '../../git/git'; +import { BaselineCheckoutFailedError } from './errors'; + +/** + * Classify an error captured at the TurboSnap bail site into additional Sentry tags describing its + * root cause. + * + * @param deps Function dependencies. + * @param error The error captured at the bail site. + * + * @returns A map of Sentry tags, or `undefined` when the error type is not recognized. + */ +export async function classifyTagsFromError( + deps: GitDeps, + error: unknown +): Promise | undefined> { + if (error instanceof BaselineCheckoutFailedError) { + return classifyBaselineCheckoutFailureTags(deps, error); + } + + return undefined; +} + +/** + * Refined root-cause kinds for a BaselineCheckoutFailedError. + * + * These exist for Sentry ONLY and are not sent to the backend. + */ +export type BaselineCheckoutFailureKind = + | 'baselineManifestMoved' + | 'baselineManifestAdded' + | 'unknownBaselineCheckoutFailure'; + +/** + * Check git to determine the root cause of a BaselineCheckoutFailedError. + * + * @param deps Function dependencies. + * @param error The error captured at the bail site. + * + * @returns The refined failure kind. + */ +async function classifyBaselineCheckoutFailureTags( + deps: GitDeps, + error: BaselineCheckoutFailedError +): Promise> { + const { reference, fileName } = parsePathspec(error.pathspec); + + try { + // Determine how the file changed between the baseline and HEAD (such as if it was added, moved, + // renamed, etc). + const basename = path.basename(fileName); + const changes = await getChangedFilesWithStatus( + deps, + reference, + 'HEAD', + // See https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-pathspec for details + // + // `glob` allows us to pass in a glob pattern + // `top` anchors the response to the root of the repository (so we can catch cross-directory changes) + `:(glob,top)**/${basename}` + ); + return { baseline_failure_kind: classifyManifestChange(changes, fileName) }; + } catch (err) { + // We capture the exception higher in the stack so we can simply attach the error context here. + Sentry.addBreadcrumb({ + level: 'error', + category: 'classifyBaselineCheckoutFailure', + message: 'Error classifying baseline checkout failure', + data: { + error: err, + }, + }); + + return { baseline_failure_kind: 'unknownBaselineCheckoutFailure' }; + } +} + +/** + * Determine if the file was moved or added or something else. + * + * @param changes The parsed changed files from the diff. + * @param fileName The failed file path to match against. + * + * @returns The matching manifest failure kind. + */ +function classifyManifestChange( + changes: ChangedFileWithStatus[], + fileName: string +): BaselineCheckoutFailureKind { + // Check renames first so a moved manifest is never mislabeled as added. + if (changes.some((change) => change.status === 'renamed' && change.path === fileName)) { + return 'baselineManifestMoved'; + } + if (changes.some((change) => change.status === 'added' && change.path === fileName)) { + return 'baselineManifestAdded'; + } + return 'unknownBaselineCheckoutFailure'; +} + +/** + * Split a pathspec of the form `:` into its parts. + * + * @param pathspec The pathspec carried by a `BaselineCheckoutFailedError`. + * + * @returns The baseline reference (SHA) and the file path. + */ +function parsePathspec(pathspec: string): { reference: string; fileName: string } { + // Find the first colon which separates the reference from the file path. We can't just split on + // colon and return the first and second parts since files can contain colons. + const separatorIndex = pathspec.indexOf(':'); + return { + reference: pathspec.slice(0, separatorIndex), + fileName: pathspec.slice(separatorIndex + 1), + }; +} diff --git a/node-src/lib/turbosnap/index.test.ts b/node-src/lib/turbosnap/index.test.ts index 69359bde7..e239b66a2 100644 --- a/node-src/lib/turbosnap/index.test.ts +++ b/node-src/lib/turbosnap/index.test.ts @@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { exitCodes } from '../setExitCode'; import TestLogger from '../testLogger'; import { traceChangedFiles } from '.'; +import { classifyTagsFromError as classifyTagsFromErrorDep } from './classifyBailRootCause'; import { BaselineCheckoutFailedError, LockFileParseFailedError, @@ -27,6 +28,7 @@ vi.mock('./findChangedDependencies', async (importOriginal) => { }); vi.mock('./findChangedPackageFiles'); vi.mock('./getDependentStoryFiles'); +vi.mock('./classifyBailRootCause'); vi.mock('../../tasks/readStatsFile', () => ({ readStatsFile: () => Promise.resolve({ @@ -42,6 +44,7 @@ vi.mock('../../tasks/readStatsFile', () => ({ const getDependentStoryFiles = vi.mocked(getDependentStoryFilesDep); const findChangedPackageFiles = vi.mocked(findChangedPackageFilesDep); const findChangedDependencies = vi.mocked(findChangedDependenciesDep); +const classifyTagsFromError = vi.mocked(classifyTagsFromErrorDep); const accessMock = vi.mocked(access); const captureException = vi.mocked(Sentry.captureException); @@ -116,6 +119,7 @@ describe('traceChangedFiles', () => { sentryEventId: 'sentry-event-id', }, expectedKey: 'baselineCheckoutFailed', + expectedFailureKind: 'baselineManifestMoved', expectFingerprint: true, }, { @@ -135,11 +139,17 @@ describe('traceChangedFiles', () => { error, expectedBailReason, expectedKey, + expectedFailureKind, expectFingerprint, } of findChangedDependenciesFailureCases) { it(`bails on package.json changes and tags bailReason as ${name}`, async () => { findChangedDependencies.mockRejectedValue(error); findChangedPackageFiles.mockResolvedValue(['./package.json']); + classifyTagsFromError.mockImplementation(async (_deps, err) => + err instanceof BaselineCheckoutFailedError + ? { baseline_failure_kind: 'baselineManifestMoved' } + : undefined + ); const packageMetadataChanges = [{ changedFiles: ['./package.json'], commit: 'abcdef' }]; const ctx = { @@ -160,6 +170,7 @@ describe('traceChangedFiles', () => { tags: { bail_path: 'findChangedDependencies', ...(expectedKey && { bail_detail: expectedKey }), + ...(expectedFailureKind && { baseline_failure_kind: expectedFailureKind }), }, ...(expectFingerprint && { fingerprint: [expectedKey] }), }); diff --git a/node-src/lib/turbosnap/index.ts b/node-src/lib/turbosnap/index.ts index f7ab642ef..8c8bc222c 100644 --- a/node-src/lib/turbosnap/index.ts +++ b/node-src/lib/turbosnap/index.ts @@ -7,6 +7,7 @@ import bailFile from '../../ui/messages/warnings/bailFile'; import { checkStorybookBaseDirectory } from '../checkStorybookBaseDirectory'; import { captureBailException } from './captureBailException'; import { classifyChangedPackageFilesDetail } from './classifyBailDetail'; +import { classifyTagsFromError } from './classifyBailRootCause'; import { findChangedDependencies } from './findChangedDependencies'; import { findChangedPackageFiles } from './findChangedPackageFiles'; import { getDependentStoryFiles } from './getDependentStoryFiles'; @@ -61,6 +62,7 @@ export const traceChangedFiles = async (ctx: Context) => { pendingPatch.sentryEventId = captureBailException(pendingError, { bailSubreason: pendingPatch.bailSubreason, bailPath: 'findChangedDependencies', + additionalTags: await classifyTagsFromError(ctx, pendingError), }); }