diff --git a/.changeset/autocomplete-instant-local-search.md b/.changeset/autocomplete-instant-local-search.md deleted file mode 100644 index d515d7b298f..00000000000 --- a/.changeset/autocomplete-instant-local-search.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@shopify/cli-kit': patch ---- - -Make the autocomplete prompt feel instant when filtering against in-memory choices. diff --git a/.changeset/guard-bundle-upload-size-and-paths.md b/.changeset/guard-bundle-upload-size-and-paths.md deleted file mode 100644 index 4508c32ff75..00000000000 --- a/.changeset/guard-bundle-upload-size-and-paths.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@shopify/app': patch ---- - -Guard app bundle uploads against oversized bundles and asset paths resolving outside the app directory diff --git a/.jules/refactor.md b/.jules/refactor.md deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts b/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts index fd627c21b24..52e20a2dd47 100644 --- a/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts +++ b/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts @@ -22,8 +22,6 @@ export type Scalars = { ActionAuditID: { input: any; output: any; } /** The ID for a Address. */ AddressID: { input: any; output: any; } - /** The ID for a Attestation. */ - AttestationID: { input: any; output: any; } /** The ID for a BulkDataOperation. */ BulkDataOperationID: { input: any; output: any; } /** The ID for a BusinessUser. */ diff --git a/packages/app/src/cli/commands/app/generate/extension.ts b/packages/app/src/cli/commands/app/generate/extension.ts index b0ea709fb9a..3d16eab7984 100644 --- a/packages/app/src/cli/commands/app/generate/extension.ts +++ b/packages/app/src/cli/commands/app/generate/extension.ts @@ -47,6 +47,10 @@ export default class AppGenerateExtension extends AppLinkedCommand { }), } + public static analyticsNameOverride(): string | undefined { + return 'app scaffold extension' + } + public async run(): Promise { const {flags} = await this.parse(AppGenerateExtension) diff --git a/packages/app/src/cli/services/build/bundle-size.test.ts b/packages/app/src/cli/services/build/bundle-size.test.ts index 28fefa4ca6d..af06b225854 100644 --- a/packages/app/src/cli/services/build/bundle-size.test.ts +++ b/packages/app/src/cli/services/build/bundle-size.test.ts @@ -1,85 +1,77 @@ import {getBundleSize, formatBundleSize} from './bundle-size.js' -import {describe, expect, test} from 'vitest' -import {inTemporaryDirectory, writeFile} from '@shopify/cli-kit/node/fs' -import {joinPath} from '@shopify/cli-kit/node/path' +import {describe, expect, test, vi} from 'vitest' +import {readFile} from '@shopify/cli-kit/node/fs' import {deflate} from 'node:zlib' import {promisify} from 'node:util' const deflateAsync = promisify(deflate) +vi.mock('@shopify/cli-kit/node/fs') + describe('getBundleSize', () => { test('returns raw and compressed sizes', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given - const content = 'a'.repeat(10000) - const filePath = joinPath(tmpDir, 'bundle.js') - await writeFile(filePath, content) - - // When - const result = await getBundleSize(filePath) - - // Then - expect(result.rawBytes).toBe(10000) - expect(result.compressedBytes).toBe((await deflateAsync(Buffer.from(content))).byteLength) - expect(result.compressedBytes).toBeLessThan(result.rawBytes) - }) + // Given + const content = 'a'.repeat(10000) + vi.mocked(readFile).mockResolvedValue(content as any) + + // When + const result = await getBundleSize('/some/path.js') + + // Then + expect(result.rawBytes).toBe(10000) + expect(result.compressedBytes).toBe((await deflateAsync(Buffer.from(content))).byteLength) + expect(result.compressedBytes).toBeLessThan(result.rawBytes) }) test('compressed size uses deflate to match the backend (Ruby Zlib::Deflate.deflate)', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given - const content = JSON.stringify({key: 'value', nested: {array: [1, 2, 3]}}) - const filePath = joinPath(tmpDir, 'bundle.js') - await writeFile(filePath, content) - - // When - const result = await getBundleSize(filePath) - - // Then - const expectedCompressed = (await deflateAsync(Buffer.from(content))).byteLength - expect(result.compressedBytes).toBe(expectedCompressed) - }) + // Given + const content = JSON.stringify({key: 'value', nested: {array: [1, 2, 3]}}) + vi.mocked(readFile).mockResolvedValue(content as any) + + // When + const result = await getBundleSize('/some/path.js') + + // Then + const expectedCompressed = (await deflateAsync(Buffer.from(content))).byteLength + expect(result.compressedBytes).toBe(expectedCompressed) }) }) describe('formatBundleSize', () => { test('returns formatted size string with raw and compressed sizes', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given - const content = 'x'.repeat(50000) - const filePath = joinPath(tmpDir, 'bundle.js') - await writeFile(filePath, content) - const compressedSize = (await deflateAsync(Buffer.from(content))).byteLength - - // When - const result = await formatBundleSize(filePath) - - // Then - const expectedRaw = (50000 / 1024).toFixed(1) - const expectedCompressed = (compressedSize / 1024).toFixed(1) - expect(result).toBe(` (${expectedRaw} KB original, ~${expectedCompressed} KB compressed)`) - }) + // Given + const content = 'x'.repeat(50000) + const compressedSize = (await deflateAsync(Buffer.from(content))).byteLength + vi.mocked(readFile).mockResolvedValue(content as any) + + // When + const result = await formatBundleSize('/some/path.js') + + // Then + const expectedRaw = (50000 / 1024).toFixed(1) + const expectedCompressed = (compressedSize / 1024).toFixed(1) + expect(result).toBe(` (${expectedRaw} KB original, ~${expectedCompressed} KB compressed)`) }) test('formats MB for large files', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given - const content = 'a'.repeat(2 * 1024 * 1024) - const filePath = joinPath(tmpDir, 'bundle.js') - await writeFile(filePath, content) - const compressedSize = (await deflateAsync(Buffer.from(content))).byteLength - - // When - const result = await formatBundleSize(filePath) - - // Then - const expectedRaw = (Buffer.byteLength(content) / (1024 * 1024)).toFixed(2) - const expectedCompressed = (compressedSize / 1024).toFixed(1) - expect(result).toBe(` (${expectedRaw} MB original, ~${expectedCompressed} KB compressed)`) - }) + // Given + const content = 'a'.repeat(2 * 1024 * 1024) + const compressedSize = (await deflateAsync(Buffer.from(content))).byteLength + vi.mocked(readFile).mockResolvedValue(content as any) + + // When + const result = await formatBundleSize('/some/path.js') + + // Then + const expectedRaw = (Buffer.byteLength(content) / (1024 * 1024)).toFixed(2) + const expectedCompressed = (compressedSize / 1024).toFixed(1) + expect(result).toBe(` (${expectedRaw} MB original, ~${expectedCompressed} KB compressed)`) }) test('returns empty string on error', async () => { + // Given + vi.mocked(readFile).mockRejectedValue(new Error('file not found')) + // When const result = await formatBundleSize('/missing/path.js') diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts index e493f046643..e9decd6b02d 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts @@ -25,7 +25,7 @@ describe('executeIncludeAssetsStep', () => { options: { stdout: mockStdout, stderr: mockStderr, - app: {directory: '/test'} as any, + app: {} as any, environment: 'production', }, stepResults: new Map(), @@ -552,11 +552,6 @@ describe('executeIncludeAssetsStep', () => { }) describe('pattern entries', () => { - beforeEach(() => { - // copyByPattern now short-circuits if sourceDir doesn't exist; default true here. - vi.mocked(fs.fileExists).mockResolvedValue(true) - }) - test('copies files matching include patterns', async () => { // Given vi.mocked(fs.glob).mockResolvedValue(['/test/extension/public/logo.png', '/test/extension/public/style.css']) @@ -948,13 +943,11 @@ describe('executeIncludeAssetsStep', () => { }) test('writes manifest.json with files array when generatesAssetsManifest is true and only pattern inclusions exist', async () => { - // Given — pattern entries contribute output paths to the manifest "files" array. - // sourceDir must exist for copyByPattern's pre-glob fileExists check to pass; - // everything else can read false (the parent beforeEach default). + // Given — pattern entries contribute output paths to the manifest "files" array vi.mocked(fs.glob).mockResolvedValue(['/test/extension/public/logo.png']) vi.mocked(fs.copyFile).mockResolvedValue() vi.mocked(fs.mkdir).mockResolvedValue() - vi.mocked(fs.fileExists).mockImplementation(async (path) => String(path) === '/test/extension/public') + vi.mocked(fs.fileExists).mockResolvedValue(false) vi.mocked(fs.writeFile).mockResolvedValue() const step: LifecycleStep = { diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.ts b/packages/app/src/cli/services/build/steps/include-assets-step.ts index dc7bdba614f..1aba68d5d5d 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.ts @@ -125,7 +125,6 @@ export async function executeIncludeAssetsStep( const config = IncludeAssetsConfigSchema.parse(step.config) const {extension, options} = context const outputDir = resolveOutputDir(extension.outputPath) - const appDirectory = options.app.directory const aggregatedPathMap = new Map() // Track basenames written across all configKey entries in this build. In @@ -148,7 +147,6 @@ export async function executeIncludeAssetsStep( baseDir: extension.directory, outputDir, context, - appDirectory, destination: sanitizedDest, usedBasenames, preserveFilePaths: entry.preserveFilePaths, @@ -174,8 +172,6 @@ export async function executeIncludeAssetsStep( outputDir: destinationDir, patterns: entry.include, ignore: entry.ignore ?? [], - appDirectory, - sourceDirConfigValue: entry.baseDir ?? '.', }, options, ) @@ -193,7 +189,6 @@ export async function executeIncludeAssetsStep( destination: sanitizedDest, baseDir: extension.directory, outputDir, - appDirectory, }, options, ) diff --git a/packages/app/src/cli/services/build/steps/include-assets/assert-path-within-app.test.ts b/packages/app/src/cli/services/build/steps/include-assets/assert-path-within-app.test.ts deleted file mode 100644 index c6f2634c5c8..00000000000 --- a/packages/app/src/cli/services/build/steps/include-assets/assert-path-within-app.test.ts +++ /dev/null @@ -1,101 +0,0 @@ -import {assertPathWithinAppDir} from './assert-path-within-app.js' -import {AbortError} from '@shopify/cli-kit/node/error' -import {inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs' -import {joinPath} from '@shopify/cli-kit/node/path' -import {describe, expect, test} from 'vitest' -import {symlink} from 'fs/promises' - -describe('assertPathWithinAppDir', () => { - test('allows a path inside the app directory', async () => { - await inTemporaryDirectory(async (appDir) => { - const inside = joinPath(appDir, 'extensions', 'ext-a') - await mkdir(inside) - await writeFile(joinPath(inside, 'icon.png'), 'x') - await expect( - assertPathWithinAppDir(joinPath(inside, 'icon.png'), appDir, 'extensions/ext-a/icon.png'), - ).resolves.toBeUndefined() - }) - }) - - test('allows the app directory itself', async () => { - await inTemporaryDirectory(async (appDir) => { - await expect(assertPathWithinAppDir(appDir, appDir, '.')).resolves.toBeUndefined() - }) - }) - - test('rejects a path that resolves outside the app directory via ..', async () => { - await inTemporaryDirectory(async (parent) => { - const appDir = joinPath(parent, 'app') - await mkdir(appDir) - const outside = joinPath(parent, 'outside.json') - await writeFile(outside, '{}') - await expect(assertPathWithinAppDir(outside, appDir, '../outside.json')).rejects.toThrow(AbortError) - await expect(assertPathWithinAppDir(outside, appDir, '../outside.json')).rejects.toThrow( - /resolves outside the app directory/, - ) - }) - }) - - test('rejects a symlink whose target is outside the app directory', async () => { - await inTemporaryDirectory(async (parent) => { - const appDir = joinPath(parent, 'app') - await mkdir(appDir) - const outsideDir = joinPath(parent, 'home', 'big-folder') - await mkdir(outsideDir) - await writeFile(joinPath(outsideDir, 'huge.bin'), 'x') - - // Inside the app dir, but the symlink points outside. - const symlinkInApp = joinPath(appDir, 'assets') - await symlink(outsideDir, symlinkInApp) - - await expect(assertPathWithinAppDir(symlinkInApp, appDir, 'assets')).rejects.toThrow(AbortError) - }) - }) - - test('allows an in-tree symlink (e.g. pnpm-style links staying inside the app)', async () => { - await inTemporaryDirectory(async (appDir) => { - const realTarget = joinPath(appDir, 'shared') - await mkdir(realTarget) - const linkPath = joinPath(appDir, 'extensions', 'ext-a-assets') - await mkdir(joinPath(appDir, 'extensions')) - await symlink(realTarget, linkPath) - - await expect(assertPathWithinAppDir(linkPath, appDir, 'extensions/ext-a-assets')).resolves.toBeUndefined() - }) - }) - - test('does not false-positive on macOS-style symlinked temp dirs (both sides realpath’d)', async () => { - // inTemporaryDirectory on macOS returns a /var/folders/... path whose - // realpath is /private/var/folders/.... If only the source were realpath’d - // the check would treat the temp dir as outside itself. - await inTemporaryDirectory(async (appDir) => { - const inside = joinPath(appDir, 'src') - await mkdir(inside) - await writeFile(joinPath(inside, 'schema.json'), '{}') - await expect( - assertPathWithinAppDir(joinPath(inside, 'schema.json'), appDir, 'src/schema.json'), - ).resolves.toBeUndefined() - }) - }) - - test('allows a sibling whose name starts with two dots (e.g. ..cache)', async () => { - await inTemporaryDirectory(async (appDir) => { - const dotdotDir = joinPath(appDir, '..cache') - await mkdir(dotdotDir) - await writeFile(joinPath(dotdotDir, 'file.txt'), 'x') - await expect( - assertPathWithinAppDir(joinPath(dotdotDir, 'file.txt'), appDir, '..cache/file.txt'), - ).resolves.toBeUndefined() - }) - }) - - test('includes the original config value in the error for debuggability', async () => { - await inTemporaryDirectory(async (parent) => { - const appDir = joinPath(parent, 'app') - await mkdir(appDir) - const outside = joinPath(parent, 'leak') - await writeFile(outside, '') - await expect(assertPathWithinAppDir(outside, appDir, '~/anywhere')).rejects.toThrow(/Asset path '~\/anywhere'/) - }) - }) -}) diff --git a/packages/app/src/cli/services/build/steps/include-assets/assert-path-within-app.ts b/packages/app/src/cli/services/build/steps/include-assets/assert-path-within-app.ts deleted file mode 100644 index f875e599ff3..00000000000 --- a/packages/app/src/cli/services/build/steps/include-assets/assert-path-within-app.ts +++ /dev/null @@ -1,37 +0,0 @@ -import {relativePath, isAbsolutePath} from '@shopify/cli-kit/node/path' -import {fileRealPath} from '@shopify/cli-kit/node/fs' -import {AbortError} from '@shopify/cli-kit/node/error' - -/** - * Throws if `resolvedPath` is not inside `appDirectory` after symlink resolution. - * - * Guards against accidental misuse where an extension config points an asset - * source outside the app folder — either via `..`/absolute paths or by - * symlinking an in-app directory to somewhere else (e.g. a home directory). - * - * Both sides are realpath'd before comparison so macOS temp paths - * (`/var/folders` → `/private/var/folders`) and pnpm-style in-tree symlinks - * don't trip a false positive. - * - * `configValue` is the raw value from configuration used in the error message - * so the user can locate the offending entry. Caller must ensure `resolvedPath` - * exists on disk — `fileRealPath` throws on missing paths. - */ -export async function assertPathWithinAppDir( - resolvedPath: string, - appDirectory: string, - configValue: string, -): Promise { - const [realSource, realAppDir] = await Promise.all([fileRealPath(resolvedPath), fileRealPath(appDirectory)]) - const relative = relativePath(realAppDir, realSource) - // Check `..` as a path segment, not just a prefix. A sibling-style name like - // `..cache` is a valid in-app entry whose relative path begins with `..` but - // does not escape. - const firstSegment = relative.split(/[/\\]/, 1)[0] - if (firstSegment === '..' || isAbsolutePath(relative)) { - throw new AbortError( - `Asset path '${configValue}' resolves outside the app directory.`, - `Asset sources must be inside the app folder. Resolved to: ${realSource}`, - ) - } -} diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts index 73a713f9ba2..ea4cf855116 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts @@ -1,7 +1,8 @@ import {copyByPattern} from './copy-by-pattern.js' import {describe, expect, test, vi, beforeEach} from 'vitest' -import {inTemporaryDirectory, writeFile, mkdir, fileExistsSync} from '@shopify/cli-kit/node/fs' -import {joinPath} from '@shopify/cli-kit/node/path' +import * as fs from '@shopify/cli-kit/node/fs' + +vi.mock('@shopify/cli-kit/node/fs') describe('copyByPattern', () => { let mockStdout: any @@ -11,179 +12,141 @@ describe('copyByPattern', () => { }) test('copies matched files preserving relative paths', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given - const sourceDir = joinPath(tmpDir, 'src') - const outputDir = joinPath(tmpDir, 'out') - await mkdir(sourceDir) - await mkdir(joinPath(sourceDir, 'components')) - await mkdir(joinPath(sourceDir, 'utils')) - await writeFile(joinPath(sourceDir, 'components/Button.tsx'), 'content') - await writeFile(joinPath(sourceDir, 'utils/helpers.ts'), 'content') - - // When - const result = await copyByPattern( - { - sourceDir, - outputDir, - patterns: ['**/*.ts', '**/*.tsx'], - ignore: [], - appDirectory: sourceDir, - sourceDirConfigValue: '.', - }, - {stdout: mockStdout}, - ) - - // Then - expect(fileExistsSync(joinPath(outputDir, 'components/Button.tsx'))).toBe(true) - expect(fileExistsSync(joinPath(outputDir, 'utils/helpers.ts'))).toBe(true) - expect(result.filesCopied).toBe(2) - expect(result.outputPaths).toEqual(expect.arrayContaining(['components/Button.tsx', 'utils/helpers.ts'])) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included 2 file(s)')) - }) + // Given + vi.mocked(fs.glob).mockResolvedValue(['/src/components/Button.tsx', '/src/utils/helpers.ts']) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copyByPattern( + { + sourceDir: '/src', + outputDir: '/out', + patterns: ['**/*.ts', '**/*.tsx'], + ignore: [], + }, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyFile).toHaveBeenCalledWith('/src/components/Button.tsx', '/out/components/Button.tsx') + expect(fs.copyFile).toHaveBeenCalledWith('/src/utils/helpers.ts', '/out/utils/helpers.ts') + expect(result.filesCopied).toBe(2) + expect(result.outputPaths).toEqual(expect.arrayContaining(['components/Button.tsx', 'utils/helpers.ts'])) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included 2 file(s)')) }) test('returns 0 when no files match patterns', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given - const sourceDir = joinPath(tmpDir, 'src') - const outputDir = joinPath(tmpDir, 'out') - await mkdir(sourceDir) - await writeFile(joinPath(sourceDir, 'style.css'), 'content') - - // When - const result = await copyByPattern( - { - sourceDir, - outputDir, - patterns: ['**/*.png'], - ignore: [], - appDirectory: sourceDir, - sourceDirConfigValue: '.', - }, - {stdout: mockStdout}, - ) - - // Then - expect(result.filesCopied).toBe(0) - expect(result.outputPaths).toEqual([]) - expect(fileExistsSync(outputDir)).toBe(false) - }) + // Given + vi.mocked(fs.glob).mockResolvedValue([]) + + // When + const result = await copyByPattern( + { + sourceDir: '/src', + outputDir: '/out', + patterns: ['**/*.png'], + ignore: [], + }, + {stdout: mockStdout}, + ) + + // Then + expect(result.filesCopied).toBe(0) + expect(result.outputPaths).toEqual([]) + expect(fs.mkdir).not.toHaveBeenCalled() + expect(fs.copyFile).not.toHaveBeenCalled() }) test('skips file and warns when resolved destination escapes the output directory', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given — an absolute pattern that points outside sourceDir, so the - // computed destination ends up outside outputDir. - const sourceDir = joinPath(tmpDir, 'src') - const outputDir = joinPath(tmpDir, 'out') - const otherDir = joinPath(tmpDir, 'other') - await mkdir(sourceDir) - await mkdir(otherDir) - await writeFile(joinPath(otherDir, 'evil.js'), 'content') - - // When - const result = await copyByPattern( - { - sourceDir, - outputDir, - patterns: [joinPath(otherDir, 'evil.js')], - ignore: [], - appDirectory: sourceDir, - sourceDirConfigValue: '.', - }, - {stdout: mockStdout}, - ) - - // Then - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('skipping')) - expect(result.filesCopied).toBe(0) - expect(result.outputPaths).toEqual([]) - expect(fileExistsSync(joinPath(outputDir, 'evil.js'))).toBe(false) - }) + // Given — sourceDir is /out/sub, so a file from /out/sub/../../evil resolves outside /out/sub + // Simulate by providing a glob result whose relative path traverses upward + vi.mocked(fs.glob).mockResolvedValue(['/out/sub/../../evil.js']) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copyByPattern( + { + sourceDir: '/out/sub', + outputDir: '/out/sub', + patterns: ['**/*'], + ignore: [], + }, + {stdout: mockStdout}, + ) + + // Then + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('skipping')) + expect(fs.copyFile).not.toHaveBeenCalled() + expect(result.filesCopied).toBe(0) + expect(result.outputPaths).toEqual([]) }) test('returns 0 without copying when filepath equals computed destPath', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given — file already lives at the exact destination path - const sourceDir = tmpDir - const outputDir = tmpDir - await writeFile(joinPath(tmpDir, 'logo.png'), 'content') - - // When — sourceDir==outputDir so relPath='logo.png' and destPath==filepath - const result = await copyByPattern( - { - sourceDir, - outputDir, - patterns: ['*.png'], - ignore: [], - appDirectory: sourceDir, - sourceDirConfigValue: '.', - }, - {stdout: mockStdout}, - ) - - // Then - expect(result.filesCopied).toBe(0) - expect(result.outputPaths).toEqual([]) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included 0 file(s)')) - }) + // Given — file already lives at the exact destination path + vi.mocked(fs.glob).mockResolvedValue(['/out/logo.png']) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When — sourceDir is /out so relPath=relativePath('/out','/out/logo.png')='logo.png', destPath='/out/logo.png'==filepath + const result = await copyByPattern( + { + sourceDir: '/out', + outputDir: '/out', + patterns: ['*.png'], + ignore: [], + }, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyFile).not.toHaveBeenCalled() + expect(result.filesCopied).toBe(0) + expect(result.outputPaths).toEqual([]) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included 0 file(s)')) }) test('calls mkdir(outputDir) before copying when files are matched', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given - const sourceDir = joinPath(tmpDir, 'src') - const outputDir = joinPath(tmpDir, 'out/dist') - await mkdir(sourceDir) - await writeFile(joinPath(sourceDir, 'app.js'), 'content') - - // When - await copyByPattern( - { - sourceDir, - outputDir, - patterns: ['*.js'], - ignore: [], - appDirectory: sourceDir, - sourceDirConfigValue: '.', - }, - {stdout: mockStdout}, - ) - - // Then - expect(fileExistsSync(outputDir)).toBe(true) - expect(fileExistsSync(joinPath(outputDir, 'app.js'))).toBe(true) - }) + // Given + vi.mocked(fs.glob).mockResolvedValue(['/src/app.js']) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + await copyByPattern( + { + sourceDir: '/src', + outputDir: '/out/dist', + patterns: ['*.js'], + ignore: [], + }, + {stdout: mockStdout}, + ) + + // Then — outputDir created before copying + expect(fs.mkdir).toHaveBeenCalledWith('/out/dist') }) test('passes ignore patterns to glob', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given - const sourceDir = joinPath(tmpDir, 'src') - const outputDir = joinPath(tmpDir, 'out') - await mkdir(sourceDir) - await writeFile(joinPath(sourceDir, 'app.js'), 'content') - await writeFile(joinPath(sourceDir, 'app.test.js'), 'content') - - // When - const result = await copyByPattern( - { - sourceDir, - outputDir, - patterns: ['**/*'], - ignore: ['**/*.test.js'], - appDirectory: sourceDir, - sourceDirConfigValue: '.', - }, - {stdout: mockStdout}, - ) - - // Then - expect(result.filesCopied).toBe(1) - expect(result.outputPaths).toEqual(['app.js']) - expect(fileExistsSync(joinPath(outputDir, 'app.js'))).toBe(true) - expect(fileExistsSync(joinPath(outputDir, 'app.test.js'))).toBe(false) - }) + // Given + vi.mocked(fs.glob).mockResolvedValue([]) + + // When + await copyByPattern( + { + sourceDir: '/src', + outputDir: '/out', + patterns: ['**/*'], + ignore: ['**/*.test.ts', 'node_modules/**'], + }, + {stdout: mockStdout}, + ) + + // Then + expect(fs.glob).toHaveBeenCalledWith( + ['**/*'], + expect.objectContaining({ignore: ['**/*.test.ts', 'node_modules/**']}), + ) }) }) diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts index 0493973a56f..c1716d7d2da 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts @@ -1,6 +1,5 @@ -import {assertPathWithinAppDir} from './assert-path-within-app.js' import {joinPath, dirname, relativePath} from '@shopify/cli-kit/node/path' -import {glob, copyFile, mkdir, fileExists} from '@shopify/cli-kit/node/fs' +import {glob, copyFile, mkdir} from '@shopify/cli-kit/node/fs' /** * Pattern strategy: glob-based file selection. @@ -11,21 +10,10 @@ export async function copyByPattern( outputDir: string patterns: string[] ignore: string[] - appDirectory: string - sourceDirConfigValue: string }, options: {stdout: NodeJS.WritableStream}, ): Promise<{filesCopied: number; outputPaths: string[]}> { - const {sourceDir, outputDir, patterns, ignore, appDirectory, sourceDirConfigValue} = config - - // Validate the boundary up front, before touching the filesystem. Realpath - // would throw on a missing sourceDir, so preserve the existing "missing dir - // = no files" behavior by short-circuiting first. - if (!(await fileExists(sourceDir))) { - return {filesCopied: 0, outputPaths: []} - } - await assertPathWithinAppDir(sourceDir, appDirectory, sourceDirConfigValue) - + const {sourceDir, outputDir, patterns, ignore} = config const files = await glob(patterns, { absolute: true, cwd: sourceDir, diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts index 9e6cddf87b9..70a12187d21 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts @@ -28,13 +28,7 @@ describe('copyConfigKeyEntry', () => { await mkdir(outDir) const context = makeContext({static_root: 'public'}, mockStdout) - const result = await copyConfigKeyEntry({ - key: 'static_root', - baseDir: tmpDir, - outputDir: outDir, - context, - appDirectory: tmpDir, - }) + const result = await copyConfigKeyEntry({key: 'static_root', baseDir: tmpDir, outputDir: outDir, context}) expect(result.filesCopied).toBe(2) await expect(fileExists(joinPath(outDir, 'index.html'))).resolves.toBe(true) @@ -54,13 +48,7 @@ describe('copyConfigKeyEntry', () => { await mkdir(outDir) const context = makeContext({schema_path: 'src/schema.json'}, mockStdout) - const result = await copyConfigKeyEntry({ - key: 'schema_path', - baseDir: tmpDir, - outputDir: outDir, - context, - appDirectory: tmpDir, - }) + const result = await copyConfigKeyEntry({key: 'schema_path', baseDir: tmpDir, outputDir: outDir, context}) expect(result.filesCopied).toBe(1) await expect(fileExists(joinPath(outDir, 'schema.json'))).resolves.toBe(true) @@ -84,13 +72,7 @@ describe('copyConfigKeyEntry', () => { await mkdir(outDir) const context = makeContext({files: ['a/schema.json', 'b/schema.json']}, mockStdout) - const result = await copyConfigKeyEntry({ - key: 'files', - baseDir: tmpDir, - outputDir: outDir, - context, - appDirectory: tmpDir, - }) + const result = await copyConfigKeyEntry({key: 'files', baseDir: tmpDir, outputDir: outDir, context}) expect(result.filesCopied).toBe(2) // Both have basename schema.json — second one gets renamed @@ -105,13 +87,7 @@ describe('copyConfigKeyEntry', () => { await mkdir(outDir) const context = makeContext({}, mockStdout) - const result = await copyConfigKeyEntry({ - key: 'static_root', - baseDir: tmpDir, - outputDir: outDir, - context, - appDirectory: tmpDir, - }) + const result = await copyConfigKeyEntry({key: 'static_root', baseDir: tmpDir, outputDir: outDir, context}) expect(result.filesCopied).toBe(0) expect(result.pathMap.size).toBe(0) @@ -125,7 +101,7 @@ describe('copyConfigKeyEntry', () => { const context = makeContext({assets_dir: 'nonexistent'}, mockStdout) await expect( - copyConfigKeyEntry({key: 'assets_dir', baseDir: tmpDir, outputDir: outDir, context, appDirectory: tmpDir}), + copyConfigKeyEntry({key: 'assets_dir', baseDir: tmpDir, outputDir: outDir, context}), ).rejects.toThrow(`Couldn't find`) }) }) @@ -145,13 +121,7 @@ describe('copyConfigKeyEntry', () => { await mkdir(outDir) const context = makeContext({roots: ['public', 'assets']}, mockStdout) - const result = await copyConfigKeyEntry({ - key: 'roots', - baseDir: tmpDir, - outputDir: outDir, - context, - appDirectory: tmpDir, - }) + const result = await copyConfigKeyEntry({key: 'roots', baseDir: tmpDir, outputDir: outDir, context}) // Promise.all runs copies sequentially; glob on the shared outDir may see files // from the other copy, so the total count is at least 3 (one per real file). @@ -177,7 +147,6 @@ describe('copyConfigKeyEntry', () => { baseDir: tmpDir, outputDir: outDir, context, - appDirectory: tmpDir, destination: 'static/icons', }) @@ -205,7 +174,6 @@ describe('copyConfigKeyEntry', () => { baseDir: tmpDir, outputDir: outDir, context, - appDirectory: tmpDir, }) expect(result.filesCopied).toBe(3) @@ -232,7 +200,6 @@ describe('copyConfigKeyEntry', () => { baseDir: tmpDir, outputDir: outDir, context, - appDirectory: tmpDir, }) expect(result.filesCopied).toBe(0) @@ -260,7 +227,6 @@ describe('copyConfigKeyEntry', () => { baseDir: tmpDir, outputDir: outDir, context: contextA, - appDirectory: tmpDir, usedBasenames, preserveFilePaths: true, }) @@ -271,7 +237,6 @@ describe('copyConfigKeyEntry', () => { baseDir: tmpDir, outputDir: outDir, context: contextB, - appDirectory: tmpDir, usedBasenames, preserveFilePaths: true, }) @@ -298,7 +263,6 @@ describe('copyConfigKeyEntry', () => { baseDir: tmpDir, outputDir: outDir, context, - appDirectory: tmpDir, preserveFilePaths: true, }) await expect(promise).rejects.toThrow(AbortError) @@ -321,7 +285,6 @@ describe('copyConfigKeyEntry', () => { baseDir: tmpDir, outputDir: outDir, context, - appDirectory: tmpDir, preserveFilePaths: true, }) @@ -347,64 +310,10 @@ describe('copyConfigKeyEntry', () => { baseDir: tmpDir, outputDir: outDir, context, - appDirectory: tmpDir, }) expect(result.filesCopied).toBe(1) await expect(fileExists(joinPath(outDir, 'tools.json'))).resolves.toBe(true) }) }) - - describe('value guard', () => { - test('throws when value is an empty string', async () => { - await inTemporaryDirectory(async (tmpDir) => { - const outDir = joinPath(tmpDir, 'out') - await mkdir(outDir) - const context = makeContext({assets: ''}) - const promise = copyConfigKeyEntry({ - key: 'assets', - baseDir: tmpDir, - outputDir: outDir, - context, - appDirectory: tmpDir, - }) - await expect(promise).rejects.toThrow(AbortError) - await expect(promise).rejects.toThrow(`'assets' can't be empty.`) - }) - }) - - test('throws when value is whitespace-only', async () => { - await inTemporaryDirectory(async (tmpDir) => { - const outDir = joinPath(tmpDir, 'out') - await mkdir(outDir) - const context = makeContext({assets: ' '}) - const promise = copyConfigKeyEntry({ - key: 'assets', - baseDir: tmpDir, - outputDir: outDir, - context, - appDirectory: tmpDir, - }) - await expect(promise).rejects.toThrow(AbortError) - await expect(promise).rejects.toThrow(`'assets' can't be empty.`) - }) - }) - - test('throws with the full configKey when the key is nested', async () => { - await inTemporaryDirectory(async (tmpDir) => { - const outDir = joinPath(tmpDir, 'out') - await mkdir(outDir) - const context = makeContext({extension_points: [{assets: ''}]}) - const promise = copyConfigKeyEntry({ - key: 'extension_points[].assets', - baseDir: tmpDir, - outputDir: outDir, - context, - appDirectory: tmpDir, - }) - await expect(promise).rejects.toThrow(AbortError) - await expect(promise).rejects.toThrow(`'extension_points[].assets' can't be empty.`) - }) - }) - }) }) diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts index c952cd8abf1..edb9b9e6b38 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts @@ -1,4 +1,3 @@ -import {assertPathWithinAppDir} from './assert-path-within-app.js' import {joinPath, basename, relativePath, extname} from '@shopify/cli-kit/node/path' import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs' import {outputContent, outputDebug, outputToken} from '@shopify/cli-kit/node/output' @@ -25,7 +24,6 @@ export async function copyConfigKeyEntry(config: { baseDir: string outputDir: string context: BuildContext - appDirectory: string destination?: string usedBasenames?: Set preserveFilePaths?: boolean @@ -35,7 +33,6 @@ export async function copyConfigKeyEntry(config: { baseDir, outputDir, context, - appDirectory, destination, usedBasenames = new Set(), preserveFilePaths = false, @@ -51,12 +48,6 @@ export async function copyConfigKeyEntry(config: { paths = [] } - for (const sourcePath of paths) { - if (sourcePath.trim() === '') { - throw new AbortError(`'${key}' can't be empty.`) - } - } - if (paths.length === 0) { outputDebug(`No value for configKey '${key}', skipping\n`, stdout) return {filesCopied: 0, pathMap: new Map()} @@ -82,7 +73,6 @@ export async function copyConfigKeyEntry(config: { .value, ) } - await assertPathWithinAppDir(fullPath, appDirectory, sourcePath) const sourceIsDir = await isDirectory(fullPath) diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts index 5c1436fc2d2..1a98fdb70a8 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts @@ -23,7 +23,6 @@ describe('copySourceEntry', () => { destination: undefined, baseDir: '/ext', outputDir: '/out', - appDirectory: '/ext', }, {stdout: mockStdout}, ), @@ -44,7 +43,6 @@ describe('copySourceEntry', () => { destination: 'assets/icon.png', baseDir: '/ext', outputDir: '/out', - appDirectory: '/ext', }, {stdout: mockStdout}, ) @@ -65,7 +63,7 @@ describe('copySourceEntry', () => { // When const result = await copySourceEntry( - {source: 'dist', destination: undefined, baseDir: '/ext', outputDir: '/out', appDirectory: '/ext'}, + {source: 'dist', destination: undefined, baseDir: '/ext', outputDir: '/out'}, {stdout: mockStdout}, ) @@ -85,7 +83,7 @@ describe('copySourceEntry', () => { // When const result = await copySourceEntry( - {source: 'README.md', destination: undefined, baseDir: '/ext', outputDir: '/out', appDirectory: '/ext'}, + {source: 'README.md', destination: undefined, baseDir: '/ext', outputDir: '/out'}, {stdout: mockStdout}, ) @@ -105,7 +103,7 @@ describe('copySourceEntry', () => { // When const result = await copySourceEntry( - {source: 'dist', destination: 'vendor/dist', baseDir: '/ext', outputDir: '/out', appDirectory: '/ext'}, + {source: 'dist', destination: 'vendor/dist', baseDir: '/ext', outputDir: '/out'}, {stdout: mockStdout}, ) @@ -126,7 +124,7 @@ describe('copySourceEntry', () => { // When const result = await copySourceEntry( - {source: 'theme', destination: undefined, baseDir: '/ext', outputDir: '/out', appDirectory: '/ext'}, + {source: 'theme', destination: undefined, baseDir: '/ext', outputDir: '/out'}, {stdout: mockStdout}, ) @@ -149,7 +147,6 @@ describe('copySourceEntry', () => { destination: 'assets/icons/icon.png', baseDir: '/ext', outputDir: '/out', - appDirectory: '/ext', }, {stdout: mockStdout}, ) diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts index 6aa75a9823b..f573338b74b 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts @@ -1,4 +1,3 @@ -import {assertPathWithinAppDir} from './assert-path-within-app.js' import {joinPath, dirname, basename, relativePath} from '@shopify/cli-kit/node/path' import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs' @@ -14,16 +13,14 @@ export async function copySourceEntry( destination: string | undefined baseDir: string outputDir: string - appDirectory: string }, options: {stdout: NodeJS.WritableStream}, ): Promise<{filesCopied: number; outputPaths: string[]}> { - const {source, destination, baseDir, outputDir, appDirectory} = config + const {source, destination, baseDir, outputDir} = config const sourcePath = joinPath(baseDir, source) if (!(await fileExists(sourcePath))) { throw new Error(`Source does not exist: ${sourcePath}`) } - await assertPathWithinAppDir(sourcePath, appDirectory, source) const sourceIsDir = await isDirectory(sourcePath) diff --git a/packages/app/src/cli/services/bulk-operations/execute-bulk-operation.test.ts b/packages/app/src/cli/services/bulk-operations/execute-bulk-operation.test.ts index 0e866066d6c..815d934fd34 100644 --- a/packages/app/src/cli/services/bulk-operations/execute-bulk-operation.test.ts +++ b/packages/app/src/cli/services/bulk-operations/execute-bulk-operation.test.ts @@ -10,7 +10,7 @@ import {BulkOperationRunMutationMutation} from '../../api/graphql/bulk-operation import {OrganizationApp, OrganizationSource, OrganizationStore} from '../../models/organization.js' import {renderSuccess, renderWarning, renderError, renderInfo} from '@shopify/cli-kit/node/ui' import {ensureAuthenticatedAdminAsApp} from '@shopify/cli-kit/node/session' -import {inTemporaryDirectory, writeFile, readFile} from '@shopify/cli-kit/node/fs' +import {inTemporaryDirectory, writeFile} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' import {describe, test, expect, vi, beforeEach, afterEach} from 'vitest' @@ -39,6 +39,7 @@ vi.mock('@shopify/cli-kit/node/ui', async () => { renderInfo: vi.fn(), } }) +vi.mock('@shopify/cli-kit/node/fs') vi.mock('@shopify/cli-kit/node/session', async () => { const actual = await vi.importActual('@shopify/cli-kit/node/session') return { @@ -275,7 +276,6 @@ describe('executeBulkOperation', () => { adminSession: mockAdminSession, query: mutation, variablesJsonl: variables.join('\n'), - version: BULK_OPERATIONS_MIN_API_VERSION, }) }) }) @@ -552,39 +552,36 @@ describe('executeBulkOperation', () => { ) test('writes results to file when --output-file flag is provided', async () => { - await inTemporaryDirectory(async (tmpDir) => { - const query = '{ products { edges { node { id } } } }' - const outputFile = joinPath(tmpDir, 'results.jsonl') - const resultsContent = - '{"data":{"productCreate":{"product":{"id":"gid://shopify/Product/123"},"userErrors":[]}},"__lineNumber":0}\n{"data":{"productCreate":{"product":{"id":"gid://shopify/Product/456"},"userErrors":[]}},"__lineNumber":1}' - - const initialResponse: BulkOperationRunQueryMutation['bulkOperationRunQuery'] = { - bulkOperation: createdBulkOperation, - userErrors: [], - } - const completedOperation = { - ...createdBulkOperation, - status: 'COMPLETED' as const, - url: 'https://example.com/download', - objectCount: '2', - } + const query = '{ products { edges { node { id } } } }' + const outputFile = '/tmp/results.jsonl' + const resultsContent = + '{"data":{"productCreate":{"product":{"id":"gid://shopify/Product/123"},"userErrors":[]}},"__lineNumber":0}\n{"data":{"productCreate":{"product":{"id":"gid://shopify/Product/456"},"userErrors":[]}},"__lineNumber":1}' - vi.mocked(runBulkOperationQuery).mockResolvedValue(initialResponse) - vi.mocked(watchBulkOperation).mockResolvedValue(completedOperation) - vi.mocked(downloadBulkOperationResults).mockResolvedValue(resultsContent) + const initialResponse: BulkOperationRunQueryMutation['bulkOperationRunQuery'] = { + bulkOperation: createdBulkOperation, + userErrors: [], + } + const completedOperation = { + ...createdBulkOperation, + status: 'COMPLETED' as const, + url: 'https://example.com/download', + objectCount: '2', + } - await executeBulkOperation({ - organization: mockOrganization, - remoteApp: mockRemoteApp, - store: mockStore, - query, - watch: true, - outputFile, - }) + vi.mocked(runBulkOperationQuery).mockResolvedValue(initialResponse) + vi.mocked(watchBulkOperation).mockResolvedValue(completedOperation) + vi.mocked(downloadBulkOperationResults).mockResolvedValue(resultsContent) - const content = await readFile(outputFile) - expect(content).toBe(resultsContent) + await executeBulkOperation({ + organization: mockOrganization, + remoteApp: mockRemoteApp, + store: mockStore, + query, + watch: true, + outputFile, }) + + expect(writeFile).toHaveBeenCalledWith(outputFile, resultsContent) }) test('writes results to stdout when --output-file flag is not provided', async () => { @@ -618,6 +615,7 @@ describe('executeBulkOperation', () => { }) expect(mockOutput.info()).toContain(resultsContent) + expect(writeFile).not.toHaveBeenCalled() }) test.each(['FAILED', 'CANCELED', 'EXPIRED'] as const)( @@ -750,45 +748,41 @@ describe('executeBulkOperation', () => { }) test('renders warning when results written to file contain userErrors', async () => { - await inTemporaryDirectory(async (tmpDir) => { - const query = '{ products { edges { node { id } } } }' - const outputFile = joinPath(tmpDir, 'results.jsonl') - const resultsWithErrors = - '{"data":{"productUpdate":{"userErrors":[{"message":"invalid input"}]}},"__lineNumber":0}' - - const initialResponse: BulkOperationRunQueryMutation['bulkOperationRunQuery'] = { - bulkOperation: createdBulkOperation, - userErrors: [], - } - const completedOperation = { - ...createdBulkOperation, - status: 'COMPLETED' as const, - url: 'https://example.com/download', - objectCount: '1', - } + const query = '{ products { edges { node { id } } } }' + const outputFile = '/tmp/results.jsonl' + const resultsWithErrors = '{"data":{"productUpdate":{"userErrors":[{"message":"invalid input"}]}},"__lineNumber":0}' - vi.mocked(runBulkOperationQuery).mockResolvedValue(initialResponse) - vi.mocked(watchBulkOperation).mockResolvedValue(completedOperation) - vi.mocked(downloadBulkOperationResults).mockResolvedValue(resultsWithErrors) + const initialResponse: BulkOperationRunQueryMutation['bulkOperationRunQuery'] = { + bulkOperation: createdBulkOperation, + userErrors: [], + } + const completedOperation = { + ...createdBulkOperation, + status: 'COMPLETED' as const, + url: 'https://example.com/download', + objectCount: '1', + } - await executeBulkOperation({ - organization: mockOrganization, - remoteApp: mockRemoteApp, - store: mockStore, - query, - watch: true, - outputFile, - }) + vi.mocked(runBulkOperationQuery).mockResolvedValue(initialResponse) + vi.mocked(watchBulkOperation).mockResolvedValue(completedOperation) + vi.mocked(downloadBulkOperationResults).mockResolvedValue(resultsWithErrors) - const content = await readFile(outputFile) - expect(content).toBe(resultsWithErrors) - expect(renderWarning).toHaveBeenCalledWith( - expect.objectContaining({ - headline: 'Bulk operation completed with errors.', - body: `Results written to ${outputFile}. Check file for error details.`, - }), - ) + await executeBulkOperation({ + organization: mockOrganization, + remoteApp: mockRemoteApp, + store: mockStore, + query, + watch: true, + outputFile, }) + + expect(writeFile).toHaveBeenCalledWith(outputFile, resultsWithErrors) + expect(renderWarning).toHaveBeenCalledWith( + expect.objectContaining({ + headline: 'Bulk operation completed with errors.', + body: `Results written to ${outputFile}. Check file for error details.`, + }), + ) }) test('calls resolveApiVersion with minimum API version constant', async () => { diff --git a/packages/app/src/cli/services/bundle.test.ts b/packages/app/src/cli/services/bundle.test.ts index 35fc7679335..0adf8cbe87a 100644 --- a/packages/app/src/cli/services/bundle.test.ts +++ b/packages/app/src/cli/services/bundle.test.ts @@ -1,11 +1,9 @@ -import {writeManifestToBundle, compressBundle, uploadToGCS, BUNDLE_EXCLUSION_PATTERNS} from './bundle.js' +import {writeManifestToBundle, compressBundle, BUNDLE_EXCLUSION_PATTERNS} from './bundle.js' import {AppInterface} from '../models/app/app.js' import {describe, test, expect, vi} from 'vitest' import {joinPath} from '@shopify/cli-kit/node/path' -import {inTemporaryDirectory, mkdir, writeFile, readFile, fileSize} from '@shopify/cli-kit/node/fs' +import {inTemporaryDirectory, mkdir, writeFile, readFile} from '@shopify/cli-kit/node/fs' import {brotliCompress, zip} from '@shopify/cli-kit/node/archiver' -import {AbortError} from '@shopify/cli-kit/node/error' -import {fetch} from '@shopify/cli-kit/node/http' vi.mock('@shopify/cli-kit/node/archiver', () => { return { @@ -14,16 +12,6 @@ vi.mock('@shopify/cli-kit/node/archiver', () => { } }) -vi.mock('@shopify/cli-kit/node/http', async (importActual) => { - const actual: any = await importActual() - return {...actual, fetch: vi.fn()} -}) - -vi.mock('@shopify/cli-kit/node/fs', async (importActual) => { - const actual: any = await importActual() - return {...actual, fileSize: vi.fn(actual.fileSize)} -}) - describe('writeManifestToBundle', () => { test('writes manifest.json to the specified directory', async () => { await inTemporaryDirectory(async (tmpDir) => { @@ -204,42 +192,3 @@ describe('compressBundle', () => { ) }) }) - -describe('uploadToGCS', () => { - test('uploads the bundle when it is under the size limit', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given - const bundlePath = joinPath(tmpDir, 'bundle.zip') - await writeFile(bundlePath, 'small contents') - vi.mocked(fetch).mockResolvedValue({} as never) - - // When - await uploadToGCS('https://signed.example/upload', bundlePath) - - // Then - expect(fetch).toHaveBeenCalledWith( - 'https://signed.example/upload', - expect.objectContaining({method: 'put'}), - 'slow-request', - ) - }) - }) - - test('aborts with a helpful error when the bundle exceeds the 100 MB upload limit', async () => { - await inTemporaryDirectory(async (tmpDir) => { - // Given — mock the reported size so CI doesn't have to allocate 101 MB on disk. - const bundlePath = joinPath(tmpDir, 'huge.zip') - await writeFile(bundlePath, 'placeholder') - const oneHundredOneMb = 101 * 1024 * 1024 - vi.mocked(fileSize).mockResolvedValueOnce(oneHundredOneMb).mockResolvedValueOnce(oneHundredOneMb) - vi.mocked(fetch).mockResolvedValue({} as never) - - // When / Then - await expect(uploadToGCS('https://signed.example/upload', bundlePath)).rejects.toThrow(AbortError) - await expect(uploadToGCS('https://signed.example/upload', bundlePath)).rejects.toThrow( - /exceeds the 100 MB upload limit/, - ) - expect(fetch).not.toHaveBeenCalled() - }) - }) -}) diff --git a/packages/app/src/cli/services/bundle.ts b/packages/app/src/cli/services/bundle.ts index bd853c31753..f849da2a77d 100644 --- a/packages/app/src/cli/services/bundle.ts +++ b/packages/app/src/cli/services/bundle.ts @@ -5,14 +5,10 @@ import {MinimalAppIdentifiers} from '../models/organization.js' import {joinPath} from '@shopify/cli-kit/node/path' import {brotliCompress, zip} from '@shopify/cli-kit/node/archiver' import {formData, fetch} from '@shopify/cli-kit/node/http' -import {fileSize, readFileSync} from '@shopify/cli-kit/node/fs' +import {readFileSync} from '@shopify/cli-kit/node/fs' import {AbortError} from '@shopify/cli-kit/node/error' import {writeFile} from 'fs/promises' -const MEGABYTE = 1024 * 1024 -const MAX_BUNDLE_SIZE_MB = 100 -const MAX_BUNDLE_SIZE_BYTES = MAX_BUNDLE_SIZE_MB * MEGABYTE - export async function writeManifestToBundle(appManifest: AppManifest, bundlePath: string) { const manifestPath = joinPath(bundlePath, 'manifest.json') await writeFile(manifestPath, JSON.stringify(appManifest, null, 2)) @@ -35,15 +31,6 @@ export async function compressBundle(inputDirectory: string, outputPath: string, * @param filePath - The path to the file */ export async function uploadToGCS(signedURL: string, filePath: string) { - const size = await fileSize(filePath) - if (size > MAX_BUNDLE_SIZE_BYTES) { - // Round up so a size that barely exceeds the cap never displays as the cap. - const humanSize = `${(Math.ceil((size / MEGABYTE) * 100) / 100).toFixed(2)} MB` - throw new AbortError( - `Your app bundle exceeds the ${MAX_BUNDLE_SIZE_MB} MB upload limit (it is ${humanSize}).`, - `Check the asset paths in your extension configuration — a misconfigured source can pull in much more than intended. Exclude large files or directories from your bundle, then try again.`, - ) - } const form = formData() const buffer = readFileSync(filePath) form.append('my_upload', buffer) diff --git a/packages/app/src/cli/services/dev/extension/server/middlewares.test.ts b/packages/app/src/cli/services/dev/extension/server/middlewares.test.ts index 206d63ed971..2e689898ed0 100644 --- a/packages/app/src/cli/services/dev/extension/server/middlewares.test.ts +++ b/packages/app/src/cli/services/dev/extension/server/middlewares.test.ts @@ -405,7 +405,6 @@ describe('getExtensionAssetMiddleware()', () => { baseDir: extDir, outputDir, context: {extension, options: {stdout: {write: vi.fn()}}} as any, - appDirectory: tmpDir, }) const flattened = buildResult.pathMap.get('../tools.json') as string diff --git a/packages/app/src/cli/services/dev/ui/components/Dev.test.tsx b/packages/app/src/cli/services/dev/ui/components/Dev.test.tsx index 2808a12dbba..013035dfe7b 100644 --- a/packages/app/src/cli/services/dev/ui/components/Dev.test.tsx +++ b/packages/app/src/cli/services/dev/ui/components/Dev.test.tsx @@ -27,13 +27,6 @@ vi.mock('@shopify/cli-kit/node/system', async () => { vi.mock('../../../context.js') vi.mock('../../fetch.js') vi.mock('../../processes/dev-session.js') -vi.mock('@shopify/cli-kit/node/hooks/postrun', async () => { - const actual: any = await vi.importActual('@shopify/cli-kit/node/hooks/postrun') - return { - ...actual, - waitForPostRunHookAndExit: vi.fn(), - } -}) const developerPlatformClient = testDeveloperPlatformClient() diff --git a/packages/app/src/cli/services/dev/ui/components/Dev.tsx b/packages/app/src/cli/services/dev/ui/components/Dev.tsx index 32f7f163666..eb93e7f529a 100644 --- a/packages/app/src/cli/services/dev/ui/components/Dev.tsx +++ b/packages/app/src/cli/services/dev/ui/components/Dev.tsx @@ -10,7 +10,8 @@ import {Box, Text, useInput, useStdin} from '@shopify/cli-kit/node/ink' import {handleCtrlC} from '@shopify/cli-kit/node/ui' import {openURL} from '@shopify/cli-kit/node/system' import figures from '@shopify/cli-kit/node/figures' -import {waitForPostRunHookAndExit} from '@shopify/cli-kit/node/hooks/postrun' +import {isUnitTest} from '@shopify/cli-kit/node/context/local' +import {treeKill} from '@shopify/cli-kit/node/tree-kill' import {Writable} from 'stream' export interface DeveloperPreviewController { @@ -73,7 +74,12 @@ const Dev: FunctionComponent = ({ setIsShuttingDownMessage('Shutting down dev because of an error ...') } else { setIsShuttingDownMessage('Shutting down dev ...') - waitForPostRunHookAndExit() + setTimeout(() => { + if (isUnitTest()) return + treeKill(process.pid, 'SIGINT', false, () => { + process.exit(0) + }) + }, 2000) } clearInterval(pollingInterval.current) await developerPreview.disable() diff --git a/packages/app/src/cli/services/dev/ui/components/DevSessionUI.test.tsx b/packages/app/src/cli/services/dev/ui/components/DevSessionUI.test.tsx index bf7ecbf24b2..06c85d56802 100644 --- a/packages/app/src/cli/services/dev/ui/components/DevSessionUI.test.tsx +++ b/packages/app/src/cli/services/dev/ui/components/DevSessionUI.test.tsx @@ -24,13 +24,6 @@ vi.mock('@shopify/cli-kit/node/system', async () => { }) vi.mock('@shopify/cli-kit/node/context/local') vi.mock('@shopify/cli-kit/node/tree-kill') -vi.mock('@shopify/cli-kit/node/hooks/postrun', async () => { - const actual: any = await vi.importActual('@shopify/cli-kit/node/hooks/postrun') - return { - ...actual, - waitForPostRunHookAndExit: vi.fn(), - } -}) const mocks = vi.hoisted(() => { return { diff --git a/packages/app/src/cli/services/dev/ui/components/DevSessionUI.tsx b/packages/app/src/cli/services/dev/ui/components/DevSessionUI.tsx index 058d663dace..6f99d6ed7b4 100644 --- a/packages/app/src/cli/services/dev/ui/components/DevSessionUI.tsx +++ b/packages/app/src/cli/services/dev/ui/components/DevSessionUI.tsx @@ -17,7 +17,9 @@ import {Box, Text, useInput, useStdin} from '@shopify/cli-kit/node/ink' import {handleCtrlC} from '@shopify/cli-kit/node/ui' import {openURL, terminalSupportsHyperlinks} from '@shopify/cli-kit/node/system' import figures from '@shopify/cli-kit/node/figures' -import {waitForPostRunHookAndExit} from '@shopify/cli-kit/node/hooks/postrun' +import {isUnitTest} from '@shopify/cli-kit/node/context/local' +import {treeKill} from '@shopify/cli-kit/node/tree-kill' +import {postRunHookHasCompleted} from '@shopify/cli-kit/node/hooks/postrun' import {Writable} from 'stream' interface DevStatusShortcut extends TabShortcut { @@ -68,7 +70,18 @@ const DevSessionUI: FunctionComponent = ({ setIsShuttingDownMessage('Shutting down dev ...') await onAbort() } - waitForPostRunHookAndExit() + if (isUnitTest()) return + + // Wait for the post run hook to complete or timeout after 5 seconds. + let totalTime = 0 + setInterval(() => { + if (postRunHookHasCompleted() || totalTime > 5000) { + treeKill(process.pid, 'SIGINT', false, () => { + process.exit(0) + }) + } + totalTime += 100 + }, 100) }) const errorHandledProcesses = useMemo(() => { diff --git a/packages/app/src/cli/services/execute-operation.test.ts b/packages/app/src/cli/services/execute-operation.test.ts index 4bc426cac54..03af22d7d2e 100644 --- a/packages/app/src/cli/services/execute-operation.test.ts +++ b/packages/app/src/cli/services/execute-operation.test.ts @@ -4,7 +4,7 @@ import {OrganizationApp, OrganizationSource, OrganizationStore} from '../models/ import {renderSuccess, renderError, renderSingleTask} from '@shopify/cli-kit/node/ui' import {adminRequestDoc} from '@shopify/cli-kit/node/api/admin' import {ClientError} from 'graphql-request' -import {inTemporaryDirectory, writeFile, readFile} from '@shopify/cli-kit/node/fs' +import {inTemporaryDirectory, writeFile} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' import {describe, test, expect, vi, beforeEach, afterEach} from 'vitest' @@ -12,6 +12,7 @@ import {describe, test, expect, vi, beforeEach, afterEach} from 'vitest' vi.mock('./graphql/common.js') vi.mock('@shopify/cli-kit/node/ui') vi.mock('@shopify/cli-kit/node/api/admin') +vi.mock('@shopify/cli-kit/node/fs') describe('executeOperation', () => { const mockOrganization = { @@ -218,6 +219,7 @@ describe('executeOperation', () => { const expectedOutput = JSON.stringify(mockResult, null, 2) expect(mockOutput.info()).toContain(expectedOutput) + expect(writeFile).not.toHaveBeenCalled() }) test('writes results to file when outputFile is provided', async () => { @@ -236,7 +238,7 @@ describe('executeOperation', () => { }) const expectedContent = JSON.stringify(mockResult, null, 2) - await expect(readFile(outputFile)).resolves.toBe(expectedContent) + expect(writeFile).toHaveBeenCalledWith(outputFile, expectedContent) expect(renderSuccess).toHaveBeenCalledWith( expect.objectContaining({ body: expect.stringContaining(outputFile), diff --git a/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.test.tsx b/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.test.tsx index b909d64b7e3..7c449a9d710 100644 --- a/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.test.tsx +++ b/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.test.tsx @@ -620,34 +620,6 @@ describe('AutocompletePrompt', async () => { `) }) - test('searchDebounceMs: 0 invokes search on every keystroke without throttling', async () => { - const search = vi.fn(async (term: string) => ({ - data: DATABASE.filter((item) => item.label.includes(term)), - })) - - const renderInstance = render( - {}} - search={search} - searchDebounceMs={0} - />, - ) - - await waitForInputsToBeReady() - await sendInputAndWaitForChange(renderInstance, 'f') - await sendInputAndWaitForChange(renderInstance, 'i') - await sendInputAndWaitForChange(renderInstance, 'r') - - // With the default 400ms throttle, three rapid keystrokes coalesce to ~2 calls - // (leading + trailing edge). With searchDebounceMs=0, each keystroke fires. - expect(search).toHaveBeenCalledTimes(3) - expect(search).toHaveBeenNthCalledWith(1, 'f') - expect(search).toHaveBeenNthCalledWith(2, 'fi') - expect(search).toHaveBeenNthCalledWith(3, 'fir') - }) - test('displays an error message if the search fails', async () => { const search = (_term: string) => { return Promise.reject(new Error('Something went wrong')) diff --git a/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.tsx b/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.tsx index 7214cec2b11..01f277dfe66 100644 --- a/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.tsx +++ b/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.tsx @@ -28,17 +28,9 @@ export interface AutocompletePromptProps { abortSignal?: AbortSignal infoMessage?: InfoMessageProps['message'] groupOrder?: string[] - /** - * Throttle window in milliseconds applied to the search callback. Defaults to 400ms, - * which is appropriate for remote/paginated backends. In-memory consumers (where the - * search callback resolves synchronously) can pass 0 for instant filtering on every - * keystroke. - */ - searchDebounceMs?: number } const MIN_NUMBER_OF_ITEMS_FOR_SEARCH = 5 -const DEFAULT_SEARCH_DEBOUNCE_MS = 400 function AutocompletePrompt({ message, @@ -50,7 +42,6 @@ function AutocompletePrompt({ abortSignal, infoMessage, groupOrder, - searchDebounceMs = DEFAULT_SEARCH_DEBOUNCE_MS, }: React.PropsWithChildren>): ReactElement | null { const complete = useComplete() const [searchTerm, setSearchTerm] = useState('') @@ -130,10 +121,10 @@ function AutocompletePrompt({ clearTimeout(setLoadingWhenSlow.current) }) }, - searchDebounceMs, + 400, {leading: true, trailing: true}, ), - [paginatedSearch, setPromptState, searchDebounceMs], + [paginatedSearch, setPromptState], ) return ( diff --git a/packages/cli-kit/src/public/common/string.ts b/packages/cli-kit/src/public/common/string.ts index 9daa97d6101..0e3e295df4d 100644 --- a/packages/cli-kit/src/public/common/string.ts +++ b/packages/cli-kit/src/public/common/string.ts @@ -161,17 +161,6 @@ const SAFE_RANDOM_CREATIVE_NOUNS = [ export type RandomNameFamily = 'business' | 'creative' -const NAME_FAMILIES: Record = { - business: { - adjectives: SAFE_RANDOM_BUSINESS_ADJECTIVES, - nouns: SAFE_RANDOM_BUSINESS_NOUNS, - }, - creative: { - adjectives: SAFE_RANDOM_CREATIVE_ADJECTIVES, - nouns: SAFE_RANDOM_CREATIVE_NOUNS, - }, -} - /** * Generates a random name by combining an adjective and noun. * @@ -179,7 +168,17 @@ const NAME_FAMILIES: Record { - const tag = await getLatestTag(directory) + const stdout = await gitCommand(['describe', '--tags', '--abbrev=0'], directory) + const tag = stdout.trim() if (!tag) { throw new AbortError(`Couldn't obtain the most recent tag of the repository ${repoUrl}`) diff --git a/packages/cli-kit/src/public/node/hooks/postrun.ts b/packages/cli-kit/src/public/node/hooks/postrun.ts index 09146d0cd8c..52d600cdcbc 100644 --- a/packages/cli-kit/src/public/node/hooks/postrun.ts +++ b/packages/cli-kit/src/public/node/hooks/postrun.ts @@ -2,7 +2,6 @@ * Postrun hook — uses dynamic imports to avoid loading heavy modules (base-command, analytics) * at module evaluation time. These are only needed after the command has already finished. */ -import {treeKill} from '../tree-kill.js' import {Command, Hook} from '@oclif/core' let postRunHookCompleted = false @@ -16,42 +15,6 @@ export function postRunHookHasCompleted(): boolean { return postRunHookCompleted } -/** - * Wait for the postrun hook to finish (so auto-upgrade has a chance to run) and then - * tree-kill the current process tree before exiting. - * - * Long-running interactive commands like `app dev` need this when the user terminates - * the command via `q` or Ctrl+C. The dev sub-processes such as servers and watchers keep - * the event loop alive, so even after oclif's postrun hook completes the node process - * won't exit on its own and we have to `treeKill` the process tree. We must not do that - * before the postrun hook has actually finished running auto-upgrade, otherwise we would - * kill the upgrade mid-way while `npm install` is still running. - * - * The flag `postRunHookCompleted` is flipped at the very end of the hook after - * `autoUpgradeIfNeeded` resolves, so polling it here is safe. - */ -export function waitForPostRunHookAndExit(): void { - const pollIntervalMs = 100 - // Auto-upgrade can take a while (npm/pnpm/yarn install). Cap the wait generously so - // a stuck upgrade still terminates the process eventually. - const maxWaitMs = 120000 - - let elapsed = 0 - let terminating = false - const handle = setInterval(() => { - if (terminating) return - if (postRunHookHasCompleted() || elapsed >= maxWaitMs) { - terminating = true - clearInterval(handle) - treeKill(process.pid, 'SIGINT', false, () => { - process.exit(0) - }) - return - } - elapsed += pollIntervalMs - }, pollIntervalMs) -} - // This hook is called after each successful command run. More info: https://oclif.io/docs/hooks export const hook: Hook.Postrun = async ({config, Command}) => { await detectStopCommand(Command as unknown as typeof Command) @@ -65,9 +28,9 @@ export const hook: Hook.Postrun = async ({config, Command}) => { const {outputDebug} = await import('../output.js') const command = Command.id.replace(/:/g, ' ') outputDebug(`Completed command ${command}`) + postRunHookCompleted = true if (!command.includes('notifications') && !command.includes('upgrade')) await autoUpgradeIfNeeded() - postRunHookCompleted = true } /** diff --git a/packages/cli-kit/src/public/node/is-global.test.ts b/packages/cli-kit/src/public/node/is-global.test.ts index 258315cdc55..4e2e0b91e9e 100644 --- a/packages/cli-kit/src/public/node/is-global.test.ts +++ b/packages/cli-kit/src/public/node/is-global.test.ts @@ -81,23 +81,6 @@ describe('currentProcessIsGlobal', () => { // Then expect(got).toBeFalsy() }) - - test('returns false on Windows when argv uses backslashes and projectDir uses forward slashes', () => { - // Given - mimic the exact Windows shape: - // projectDir comes from pathe -> normalized forward slashes - // argv[1] is OS-native -> backslash separated - const winProjectDir = 'C:/Users/me/project' - const winArgv1 = 'C:\\Users\\me\\project\\node_modules\\@shopify\\cli\\bin\\run.js' - vi.mocked(findPathUpSync).mockReturnValue(`${winProjectDir}/shopify.app.toml`) - const argv = ['node', winArgv1, 'shopify'] - - // When - const got = currentProcessIsGlobal(argv) - - // Then - regression test for the path-separator bug that misclassified - // Windows local installs as global, triggering an unwanted `npm install -g`. - expect(got).toBeFalsy() - }) }) describe('inferPackageManagerForGlobalCLI', () => { @@ -246,25 +229,6 @@ describe('inferPackageManagerForGlobalCLI', () => { expect(got).toBe('homebrew') }) - test('returns bun when symlink under ~/.bun/bin resolves out of the bun install dir', async () => { - // Given: `bun add -g @shopify/cli` creates ~/.bun/bin/shopify as a symlink to - // ../../node_modules/@shopify/cli/bin/run.js, which on most setups resolves to - // /node_modules/@shopify/cli/bin/run.js — a path that does NOT contain "bun". - // Without inspecting the original symlink path we'd fall through to npm and the - // autoupgrade flow would shell out to `npm install -g` instead of `bun add -g`. - const symlinkPath = '/users/fonso/.bun/bin/shopify' - const realBunPath = '/users/fonso/node_modules/@shopify/cli/bin/run.js' - const argv = ['node', symlinkPath, 'shopify'] - - vi.mocked(realpathSync).mockImplementationOnce(() => realBunPath) - - // When - const got = inferPackageManagerForGlobalCLI(argv) - - // Then - expect(got).toBe('bun') - }) - test('defaults to npm if realpath fails and no other indicator is present', async () => { // Given: A path that realpathSync cannot resolve const nonExistentPath = '/opt/homebrew/bin/shopify' diff --git a/packages/cli-kit/src/public/node/is-global.ts b/packages/cli-kit/src/public/node/is-global.ts index 2a60d6745ba..ac0a1e68958 100644 --- a/packages/cli-kit/src/public/node/is-global.ts +++ b/packages/cli-kit/src/public/node/is-global.ts @@ -1,4 +1,4 @@ -import {cwd, dirname, isSubpath, joinPath, sniffForPath} from './path.js' +import {cwd, dirname, joinPath, sniffForPath} from './path.js' import {isUnitTest} from './context/local.js' import {findPathUpSync, globSync} from './fs.js' import {realpathSync} from 'fs' @@ -27,17 +27,9 @@ export function currentProcessIsGlobal(argv = process.argv): boolean { // From node docs: "The second element [of the array] will be the path to the JavaScript file being executed" const binDir = argv[1] ?? '' - if (!binDir) { - return true - } - // If binDir lives inside projectDir, we are running a local CLI. - // Use isSubpath (pathe.relative under the hood) instead of a raw - // string startsWith: projectDir flows through normalizePath and is - // forward-slash on every platform, while argv[1] is OS-native, so on - // Windows it arrives backslash-separated and a naive startsWith would - // misclassify a local install as global. - const isLocal = isSubpath(projectDir.trim(), binDir) + // If binDir starts with projectDir, then we are running a local CLI + const isLocal = binDir.startsWith(projectDir.trim()) _isGlobal = !isLocal return _isGlobal @@ -105,10 +97,9 @@ export function inferPackageManagerForGlobalCLI(argv = process.argv, env = proce } const processArgv = argv[1] ?? '' - const symlinkPath = processArgv.toLowerCase() // Resolve symlinks to get the real path of the binary. - let realPath = symlinkPath + let realPath = processArgv.toLowerCase() try { realPath = realpathSync(processArgv).toLowerCase() // eslint-disable-next-line no-catch-all/no-catch-all @@ -116,16 +107,9 @@ export function inferPackageManagerForGlobalCLI(argv = process.argv, env = proce // fall back to using the original path for detection } - // Inspect both the (unresolved) symlink path and the resolved real path. Some - // package managers — notably bun (`~/.bun/bin/`) — install global binaries - // as symlinks pointing into a generic `node_modules` directory whose real path - // no longer contains the package manager name. The original symlink under the - // PM's bin dir is the most reliable signal in that case. - const matches = (needle: string) => realPath.includes(needle) || symlinkPath.includes(needle) - - if (matches('yarn')) return 'yarn' - if (matches('pnpm')) return 'pnpm' - if (matches('bun')) return 'bun' + if (realPath.includes('yarn')) return 'yarn' + if (realPath.includes('pnpm')) return 'pnpm' + if (realPath.includes('bun')) return 'bun' // Check for Homebrew via Cellar path (resolved symlink) if (realPath.includes('/cellar/')) return 'homebrew' diff --git a/packages/cli-kit/src/public/node/path.test.ts b/packages/cli-kit/src/public/node/path.test.ts index 4972b3ac276..1cb801b5a9e 100644 --- a/packages/cli-kit/src/public/node/path.test.ts +++ b/packages/cli-kit/src/public/node/path.test.ts @@ -1,5 +1,5 @@ -import {relativizePath, normalizePath, cwd, sniffForPath, commonParentDirectory} from './path.js' -import {describe, test, expect} from 'vitest' +import {relativizePath, normalizePath, cwd, sniffForPath, commonParentDirectory, sanitizeRelativePath} from './path.js' +import {describe, test, expect, vi} from 'vitest' describe('relativize', () => { test('relativizes the path', () => { @@ -93,3 +93,27 @@ describe('sniffForPath', () => { expect(path).toStrictEqual('/path/to/project') }) }) + +describe('sanitizeRelativePath', () => { + test('returns the path if it is relative and has no traversal', () => { + // Given + const path = 'some/path' + const warn = vi.fn() + + // When + const got = sanitizeRelativePath(path, warn) + + // Then + expect(got).toBe('some/path') + expect(warn).not.toHaveBeenCalled() + }) + + test('strips traversal and absolute path segments and warns', () => { + const warn = vi.fn() + expect(sanitizeRelativePath('some/../path', warn)).toBe('path') + expect(sanitizeRelativePath('/etc/passwd', warn)).toBe('etc/passwd') + expect(sanitizeRelativePath('\\some\\path', warn)).toBe('some/path') + expect(sanitizeRelativePath('C:\\Windows', warn)).toBe('Windows') + expect(warn).toHaveBeenCalledTimes(4) + }) +}) diff --git a/packages/cli-kit/src/public/node/path.ts b/packages/cli-kit/src/public/node/path.ts index f3721780110..1b8748013f4 100644 --- a/packages/cli-kit/src/public/node/path.ts +++ b/packages/cli-kit/src/public/node/path.ts @@ -217,20 +217,27 @@ export function sniffForJson(argv = process.argv): boolean { * @returns The sanitized path (may be an empty string if all segments were traversal). */ export function sanitizeRelativePath(input: string, warn: (msg: string) => void): string { - const segments = input.replace(/\\/g, '/').split('/') + const normalized = input.replace(/\\/g, '/') + const segments = normalized.split('/') const stack: string[] = [] let stripped = false + + if (normalized.startsWith('/') || /^[a-zA-Z]:/.test(normalized)) { + stripped = true + } + for (const seg of segments) { if (seg === '..') { stripped = true stack.pop() - } else if (seg !== '.') { + } else if (seg !== '.' && seg !== '' && !/^[a-zA-Z]:$/.test(seg)) { stack.push(seg) } } const result = stack.join('/') if (stripped) { - warn(`Warning: path '${input}' contains '..' traversal — sanitized to '${result || '.'}'\n`) + const sanitizedPath = result || '(empty)' + warn(`Warning: path '${input}' is insecure (contains '..' or is absolute) — sanitized to '${sanitizedPath}'\n`) } return result } diff --git a/packages/cli-kit/src/public/node/ui.tsx b/packages/cli-kit/src/public/node/ui.tsx index 4147ee6247d..35c6bab6a7a 100644 --- a/packages/cli-kit/src/public/node/ui.tsx +++ b/packages/cli-kit/src/public/node/ui.tsx @@ -417,11 +417,6 @@ export async function renderAutocompletePrompt( ): Promise { throwInNonTTY({message: props.message, stdin: renderOptions?.stdin}, uiDebugOptions) - // The default search filters in-memory choices synchronously, so it doesn't need - // throttling. Skipping the throttle makes the keystroke-to-result latency feel - // instant. Callers that supply their own (typically remote/paginated) search keep - // the component's default throttle unless they opt out via `searchDebounceMs`. - const usingDefaultSearch = props.search === undefined const newProps = { search(term: string) { const lowerTerm = term.toLowerCase() @@ -431,7 +426,6 @@ export async function renderAutocompletePrompt( }), }) }, - ...(usingDefaultSearch ? {searchDebounceMs: 0} : {}), ...props, } diff --git a/packages/cli-kit/src/public/node/upgrade.ts b/packages/cli-kit/src/public/node/upgrade.ts index ba9eb541cf5..4db1da0995b 100644 --- a/packages/cli-kit/src/public/node/upgrade.ts +++ b/packages/cli-kit/src/public/node/upgrade.ts @@ -12,7 +12,6 @@ import { getPackageManager, } from './node-package-manager.js' import {outputContent, outputDebug, outputInfo, outputToken, outputWarn} from './output.js' -import {renderSuccess} from './ui.js' import {cwd, moduleDirectory, sniffForPath} from './path.js' import {exec, isCI} from './system.js' import {isPreReleaseVersion} from './version.js' @@ -93,21 +92,8 @@ export async function runCLIUpgrade(options: RunCLIUpgradeOptions = {}): Promise if (!command) { throw new Error('Could not determine the command to run') } - // Cache lookup — already populated by versionToAutoUpgrade() upstream, so this - // is just a synchronous cache read, no extra network call. - const newerVersion = checkForCachedNewVersion('@shopify/cli', CLI_KIT_VERSION) - const headline = newerVersion - ? `✨ New version of Shopify CLI available! (${CLI_KIT_VERSION} → ${newerVersion})` - : '✨ New version of Shopify CLI available!' - outputInfo( - outputContent`${headline} - Now upgrading by running: ${outputToken.genericShellCommand(installCommand)}...`, - ) + outputInfo(outputContent`Upgrading Shopify CLI by running: ${outputToken.genericShellCommand(installCommand)}...`) await exec(command, args, {stdio: 'inherit'}) - renderSuccess({ - headline: 'Shopify CLI upgraded.', - body: newerVersion ? `You're now on version ${newerVersion}.` : "You're now on the latest version.", - }) } else if (projectDir) { await upgradeLocalShopify(projectDir, CLI_KIT_VERSION) } else { diff --git a/packages/plugin-cloudflare/src/tunnel.test.ts b/packages/plugin-cloudflare/src/tunnel.test.ts index 57baee395e4..73b46304406 100644 --- a/packages/plugin-cloudflare/src/tunnel.test.ts +++ b/packages/plugin-cloudflare/src/tunnel.test.ts @@ -119,31 +119,26 @@ describe('hookStart', () => { test('cleans errors coming from the log', async () => { // Given - vi.useFakeTimers() - try { - vi.mocked(exec).mockImplementation(async (command, args, options) => { - const writable = options?.stdout as Writable - writable.write( - Buffer.from( - `2023-10-11T13:32:45Z ERR Failed to serve quic connection error="Application error 0x0 (remote)" connIndex=0 event=0 ip=123.123.123.123`, - ), - ) - }) - // When - const tunnelClient = (await hookStart(port)).valueOrAbort() - await vi.advanceTimersByTimeAsync(250) - const result = tunnelClient.getTunnelStatus() - - // Then - expect(result).toEqual({ - status: 'error', - message: - 'Could not start Cloudflare tunnel: Failed to serve quic connection error="Application error 0x0 (remote)" ', - tryMessage: expect.anything(), - }) - } finally { - vi.useRealTimers() - } + vi.mocked(exec).mockImplementation(async (command, args, options) => { + const writable = options?.stdout as Writable + writable.write( + Buffer.from( + `2023-10-11T13:32:45Z ERR Failed to serve quic connection error="Application error 0x0 (remote)" connIndex=0 event=0 ip=123.123.123.123`, + ), + ) + }) + // When + const tunnelClient = (await hookStart(port)).valueOrAbort() + await new Promise((resolve) => setTimeout(resolve, 250)) + const result = tunnelClient.getTunnelStatus() + + // Then + expect(result).toEqual({ + status: 'error', + message: + 'Could not start Cloudflare tunnel: Failed to serve quic connection error="Application error 0x0 (remote)" ', + tryMessage: expect.anything(), + }) }) test('returns error if it fails to install cloudflared', async () => {