From 10cf452ada4168878ee8c78dca5225ca3cb51a29 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Tue, 10 Mar 2026 09:32:41 +0100 Subject: [PATCH 01/13] Add client steps implementation to support existing functionalities --- .../models/extensions/extension-instance.ts | 1 - .../build/steps/build-function-step.ts | 12 + .../services/build/steps/build-theme-step.ts | 14 + .../services/build/steps/bundle-theme-step.ts | 30 + .../services/build/steps/bundle-ui-step.ts | 11 + .../build/steps/copy-static-assets-step.ts | 11 + .../build/steps/create-tax-stub-step.ts | 14 + .../build/steps/include-assets-step.test.ts | 606 ++++++++++++++++++ .../build/steps/include_assets_step.ts | 318 +++++++++ .../app/src/cli/services/build/steps/index.ts | 25 +- 10 files changed, 1037 insertions(+), 5 deletions(-) create mode 100644 packages/app/src/cli/services/build/steps/build-function-step.ts create mode 100644 packages/app/src/cli/services/build/steps/build-theme-step.ts create mode 100644 packages/app/src/cli/services/build/steps/bundle-theme-step.ts create mode 100644 packages/app/src/cli/services/build/steps/bundle-ui-step.ts create mode 100644 packages/app/src/cli/services/build/steps/copy-static-assets-step.ts create mode 100644 packages/app/src/cli/services/build/steps/create-tax-stub-step.ts create mode 100644 packages/app/src/cli/services/build/steps/include-assets-step.test.ts create mode 100644 packages/app/src/cli/services/build/steps/include_assets_step.ts diff --git a/packages/app/src/cli/models/extensions/extension-instance.ts b/packages/app/src/cli/models/extensions/extension-instance.ts index ebc742ad74a..05371c5d53f 100644 --- a/packages/app/src/cli/models/extensions/extension-instance.ts +++ b/packages/app/src/cli/models/extensions/extension-instance.ts @@ -46,7 +46,6 @@ export const CONFIG_EXTENSION_IDS: string[] = [ WebhookSubscriptionSpecIdentifier, WebhooksSpecIdentifier, EventsSpecIdentifier, - // Hardcoded identifiers that don't exist locally. 'data', 'admin', diff --git a/packages/app/src/cli/services/build/steps/build-function-step.ts b/packages/app/src/cli/services/build/steps/build-function-step.ts new file mode 100644 index 00000000000..d1e1f16f6eb --- /dev/null +++ b/packages/app/src/cli/services/build/steps/build-function-step.ts @@ -0,0 +1,12 @@ +import {buildFunctionExtension} from '../extension.js' +import type {LifecycleStep, BuildContext} from '../client-steps.js' + +/** + * Executes a build_function build step. + * + * Compiles the function extension (JavaScript or other language) to WASM, + * applying wasm-opt and trampoline as configured. + */ +export async function executeBuildFunctionStep(_step: LifecycleStep, context: BuildContext): Promise { + return buildFunctionExtension(context.extension, context.options) +} diff --git a/packages/app/src/cli/services/build/steps/build-theme-step.ts b/packages/app/src/cli/services/build/steps/build-theme-step.ts new file mode 100644 index 00000000000..5c9c65861a2 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/build-theme-step.ts @@ -0,0 +1,14 @@ +import {runThemeCheck} from '../theme-check.js' +import type {LifecycleStep, BuildContext} from '../client-steps.js' + +/** + * Executes a build_theme build step. + * + * Runs theme check on the extension directory and writes any offenses to stdout. + */ +export async function executeBuildThemeStep(_step: LifecycleStep, context: BuildContext): Promise { + const {extension, options} = context + options.stdout.write(`Running theme check on your Theme app extension...`) + const offenses = await runThemeCheck(extension.directory) + if (offenses) options.stdout.write(offenses) +} diff --git a/packages/app/src/cli/services/build/steps/bundle-theme-step.ts b/packages/app/src/cli/services/build/steps/bundle-theme-step.ts new file mode 100644 index 00000000000..f6fb19d5737 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/bundle-theme-step.ts @@ -0,0 +1,30 @@ +import {themeExtensionFiles} from '../../../utilities/extensions/theme.js' +import {copyFile} from '@shopify/cli-kit/node/fs' +import {relativePath, joinPath} from '@shopify/cli-kit/node/path' +import type {LifecycleStep, BuildContext} from '../client-steps.js' + +/** + * Executes a bundle_theme build step. + * + * Copies theme extension files to the output directory, preserving relative paths. + * Respects the extension's .shopifyignore file and the standard ignore patterns. + */ +export async function executeBundleThemeStep( + _step: LifecycleStep, + context: BuildContext, +): Promise<{filesCopied: number}> { + const {extension, options} = context + options.stdout.write(`Bundling theme extension ${extension.localIdentifier}...`) + const files = await themeExtensionFiles(extension) + + await Promise.all( + files.map(async (filepath) => { + const relativePathName = relativePath(extension.directory, filepath) + const outputFile = joinPath(extension.outputPath, relativePathName) + if (filepath === outputFile) return + await copyFile(filepath, outputFile) + }), + ) + + return {filesCopied: files.length} +} diff --git a/packages/app/src/cli/services/build/steps/bundle-ui-step.ts b/packages/app/src/cli/services/build/steps/bundle-ui-step.ts new file mode 100644 index 00000000000..0e33925e4cb --- /dev/null +++ b/packages/app/src/cli/services/build/steps/bundle-ui-step.ts @@ -0,0 +1,11 @@ +import {buildUIExtension} from '../extension.js' +import type {LifecycleStep, BuildContext} from '../client-steps.js' + +/** + * Executes a bundle_ui build step. + * + * Bundles the UI extension using esbuild, writing output to extension.outputPath. + */ +export async function executeBundleUIStep(_step: LifecycleStep, context: BuildContext): Promise { + return buildUIExtension(context.extension, context.options) +} diff --git a/packages/app/src/cli/services/build/steps/copy-static-assets-step.ts b/packages/app/src/cli/services/build/steps/copy-static-assets-step.ts new file mode 100644 index 00000000000..5bad6cea176 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/copy-static-assets-step.ts @@ -0,0 +1,11 @@ +import type {LifecycleStep, BuildContext} from '../client-steps.js' + +/** + * Executes a copy_static_assets build step. + * + * Copies static assets defined in the extension's build_manifest to the output directory. + * This is a no-op for extensions that do not define static assets. + */ +export async function executeCopyStaticAssetsStep(_step: LifecycleStep, context: BuildContext): Promise { + return context.extension.copyStaticAssets() +} diff --git a/packages/app/src/cli/services/build/steps/create-tax-stub-step.ts b/packages/app/src/cli/services/build/steps/create-tax-stub-step.ts new file mode 100644 index 00000000000..5634f76b867 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/create-tax-stub-step.ts @@ -0,0 +1,14 @@ +import {touchFile, writeFile} from '@shopify/cli-kit/node/fs' +import type {LifecycleStep, BuildContext} from '../client-steps.js' + +/** + * Executes a create_tax_stub build step. + * + * Creates a minimal JavaScript stub file at the extension's output path, + * satisfying the tax calculation extension bundle format. + */ +export async function executeCreateTaxStubStep(_step: LifecycleStep, context: BuildContext): Promise { + const {extension} = context + await touchFile(extension.outputPath) + await writeFile(extension.outputPath, '(()=>{})();') +} 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 new file mode 100644 index 00000000000..e2ed2d24e86 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts @@ -0,0 +1,606 @@ +import {executeIncludeAssetsStep} from './include_assets_step.js' +import {LifecycleStep, BuildContext} from '../client-steps.js' +import {ExtensionInstance} from '../../../models/extensions/extension-instance.js' +import {describe, expect, test, vi, beforeEach} from 'vitest' +import * as fs from '@shopify/cli-kit/node/fs' + +vi.mock('@shopify/cli-kit/node/fs') + +describe('executeIncludeAssetsStep', () => { + let mockExtension: ExtensionInstance + let mockContext: BuildContext + let mockStdout: any + let mockStderr: any + + beforeEach(() => { + mockStdout = {write: vi.fn()} + mockStderr = {write: vi.fn()} + mockExtension = { + directory: '/test/extension', + outputPath: '/test/output/extension.js', + } as ExtensionInstance + + mockContext = { + extension: mockExtension, + options: { + stdout: mockStdout, + stderr: mockStderr, + app: {} as any, + environment: 'production', + }, + stepResults: new Map(), + } + }) + + describe('static entries', () => { + test('copies directory contents to output root when no destination (preserveStructure defaults false)', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['index.html', 'assets/logo.png']) + + const step: LifecycleStep = { + id: 'copy-dist', + name: 'Copy Dist', + type: 'include_assets', + config: { + inclusions: [{type: 'static', source: 'dist'}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, mockContext) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output') + expect(result.filesCopied).toBe(2) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied contents of dist to output root')) + }) + + test('preserves directory name when preserveStructure is true', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['index.html', 'assets/logo.png']) + + const step: LifecycleStep = { + id: 'copy-dist', + name: 'Copy Dist', + type: 'include_assets', + config: { + inclusions: [{type: 'static', source: 'dist', preserveStructure: true}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, mockContext) + + // Then — directory is placed under its own name, not merged into output root + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output/dist') + expect(result.filesCopied).toBe(2) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied dist to dist')) + }) + + test('throws when source directory does not exist', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(false) + + const step: LifecycleStep = { + id: 'copy-dist', + name: 'Copy Dist', + type: 'include_assets', + config: { + inclusions: [{type: 'static', source: 'dist'}], + }, + } + + // When/Then + await expect(executeIncludeAssetsStep(step, mockContext)).rejects.toThrow('Source does not exist') + }) + + test('copies file to explicit destination path', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-icon', + name: 'Copy Icon', + type: 'include_assets', + config: { + inclusions: [{type: 'static', source: 'src/icon.png', destination: 'assets/icon.png'}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, mockContext) + + // Then + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/icon.png', '/test/output/assets/icon.png') + expect(result.filesCopied).toBe(1) + expect(mockStdout.write).toHaveBeenCalledWith('Copied src/icon.png to assets/icon.png\n') + }) + + test('throws when source file does not exist (with destination)', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(false) + + const step: LifecycleStep = { + id: 'copy-icon', + name: 'Copy Icon', + type: 'include_assets', + config: { + inclusions: [{type: 'static', source: 'src/missing.png', destination: 'assets/missing.png'}], + }, + } + + // When/Then + await expect(executeIncludeAssetsStep(step, mockContext)).rejects.toThrow('Source does not exist') + }) + + test('handles multiple static entries in inclusions', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['index.html']) + + const step: LifecycleStep = { + id: 'copy-mixed', + name: 'Copy Mixed', + type: 'include_assets', + config: { + inclusions: [ + {type: 'static', source: 'dist'}, + {type: 'static', source: 'src/icon.png', destination: 'assets/icon.png'}, + ], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, mockContext) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output') + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/icon.png', '/test/output/assets/icon.png') + expect(result.filesCopied).toBe(2) + }) + }) + + describe('configKey entries', () => { + test('copies directory contents for resolved configKey', async () => { + // Given + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {static_root: 'public'}, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['index.html', 'logo.png']) + + const step: LifecycleStep = { + id: 'copy-static', + name: 'Copy Static', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'static_root'}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, contextWithConfig) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/public', '/test/output') + expect(result.filesCopied).toBe(2) + }) + + test('preserves directory name for configKey when preserveStructure is true', async () => { + // Given + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {static_root: 'public'}, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['index.html', 'logo.png']) + + const step: LifecycleStep = { + id: 'copy-static', + name: 'Copy Static', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'static_root', preserveStructure: true}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — directory is placed under its own name + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/public', '/test/output/public') + expect(result.filesCopied).toBe(2) + }) + + test('skips silently when configKey is absent from config', async () => { + // Given — configuration has no static_root + const contextWithoutConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {}, + } as unknown as ExtensionInstance, + } + + const step: LifecycleStep = { + id: 'copy-static', + name: 'Copy Static', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'static_root'}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, contextWithoutConfig) + + // Then — no error, no copies + expect(result.filesCopied).toBe(0) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("No value for configKey 'static_root'")) + }) + + test('skips path that does not exist on disk but logs a warning', async () => { + // Given + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {static_root: 'nonexistent'}, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(false) + + const step: LifecycleStep = { + id: 'copy-static', + name: 'Copy Static', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'static_root'}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — no error, logged warning + expect(result.filesCopied).toBe(0) + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining("Warning: path 'nonexistent' does not exist"), + ) + }) + + test('resolves array config value and copies each path', async () => { + // Given — static_root is an array + const contextWithArrayConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {static_root: ['public', 'assets']}, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['file.html']) + + const step: LifecycleStep = { + id: 'copy-static', + name: 'Copy Static', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'static_root'}], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithArrayConfig) + + // Then — both paths copied + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/public', '/test/output') + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/assets', '/test/output') + }) + + test('resolves nested configKey with [] flatten and collects all leaf values', async () => { + // Given — TOML array-of-tables: extensions[].targeting[].tools + const contextWithNestedConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [ + {targeting: [{tools: 'tools-a.js'}, {tools: 'tools-b.js'}]}, + {targeting: [{tools: 'tools-c.js'}]}, + ], + }, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['file.js']) + + const step: LifecycleStep = { + id: 'copy-tools', + name: 'Copy Tools', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'extensions[].targeting[].tools'}], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithNestedConfig) + + // Then — all three tools paths resolved and copied + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/tools-a.js', '/test/output') + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/tools-b.js', '/test/output') + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/tools-c.js', '/test/output') + }) + + test('skips silently when [] flatten key resolves to a non-array', async () => { + // Given — targeting is a plain object, not an array + const contextWithBadConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {extensions: {targeting: {tools: 'tools.js'}}}, + } as unknown as ExtensionInstance, + } + + const step: LifecycleStep = { + id: 'copy-tools', + name: 'Copy Tools', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'extensions[].targeting[].tools'}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, contextWithBadConfig) + + // Then — contract violated, skipped silently + expect(result.filesCopied).toBe(0) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + }) + + test('handles mixed configKey and source entries in inclusions', async () => { + // Given + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {static_root: 'public'}, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['index.html']) + + const step: LifecycleStep = { + id: 'copy-mixed', + name: 'Copy Mixed', + type: 'include_assets', + config: { + inclusions: [ + {type: 'configKey', key: 'static_root'}, + {type: 'static', source: 'src/icon.png', destination: 'assets/icon.png'}, + ], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, contextWithConfig) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/public', '/test/output') + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/icon.png', '/test/output/assets/icon.png') + expect(result.filesCopied).toBe(2) + }) + }) + + describe('pattern entries', () => { + test('copies files matching include patterns', async () => { + // Given + vi.mocked(fs.glob).mockResolvedValue(['/test/extension/public/logo.png', '/test/extension/public/style.css']) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-public', + name: 'Copy Public', + type: 'include_assets', + config: { + inclusions: [{type: 'pattern', baseDir: 'public', include: ['**/*']}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, mockContext) + + // Then + expect(result.filesCopied).toBe(2) + expect(fs.copyFile).toHaveBeenCalledTimes(2) + }) + + test('uses extension directory as source when source is omitted', async () => { + // Given + vi.mocked(fs.glob).mockResolvedValue(['/test/extension/index.js', '/test/extension/manifest.json']) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-root', + name: 'Copy Root', + type: 'include_assets', + config: { + inclusions: [{type: 'pattern', include: ['*.js', '*.json']}], + }, + } + + // When + await executeIncludeAssetsStep(step, mockContext) + + // Then — glob is called with extension.directory as cwd + expect(fs.glob).toHaveBeenCalledWith(expect.any(Array), expect.objectContaining({cwd: '/test/extension'})) + }) + + test('respects ignore patterns', async () => { + // Given + vi.mocked(fs.glob).mockResolvedValue(['/test/extension/public/style.css']) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-public', + name: 'Copy Public', + type: 'include_assets', + config: { + inclusions: [{type: 'pattern', baseDir: 'public', ignore: ['**/*.png']}], + }, + } + + // When + await executeIncludeAssetsStep(step, mockContext) + + // Then + expect(fs.glob).toHaveBeenCalledWith(expect.any(Array), expect.objectContaining({ignore: ['**/*.png']})) + }) + + test('copies to destination subdirectory when specified', async () => { + // Given + vi.mocked(fs.glob).mockResolvedValue(['/test/extension/public/logo.png']) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-public', + name: 'Copy Public', + type: 'include_assets', + config: { + inclusions: [{type: 'pattern', baseDir: 'public', destination: 'static'}], + }, + } + + // When + await executeIncludeAssetsStep(step, mockContext) + + // Then + expect(fs.glob).toHaveBeenCalledWith(expect.any(Array), expect.objectContaining({cwd: '/test/extension/public'})) + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/public/logo.png', '/test/output/static/logo.png') + }) + + test('flattens files when preserveStructure is false', async () => { + // Given + vi.mocked(fs.glob).mockResolvedValue(['/test/extension/src/components/Button.tsx']) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-source', + name: 'Copy Source', + type: 'include_assets', + config: { + inclusions: [{type: 'pattern', baseDir: 'src', preserveStructure: false}], + }, + } + + // When + await executeIncludeAssetsStep(step, mockContext) + + // Then — filename only, no subdirectory + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/components/Button.tsx', '/test/output/Button.tsx') + }) + + test('returns zero and warns when no files match', async () => { + // Given + vi.mocked(fs.glob).mockResolvedValue([]) + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-public', + name: 'Copy Public', + type: 'include_assets', + config: { + inclusions: [{type: 'pattern', baseDir: 'public'}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, mockContext) + + // Then + expect(result.filesCopied).toBe(0) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('No files matched patterns')) + }) + }) + + describe('mixed inclusions', () => { + test('executes all entry types in parallel and aggregates filesCopied count', async () => { + // Given + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {theme_root: 'theme'}, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + // glob: first call for pattern entry, second for configKey dir listing + vi.mocked(fs.glob) + .mockResolvedValueOnce(['/test/extension/assets/logo.png', '/test/extension/assets/icon.svg']) + .mockResolvedValueOnce(['index.html', 'style.css']) + + const step: LifecycleStep = { + id: 'include-all', + name: 'Include All', + type: 'include_assets', + config: { + inclusions: [ + {type: 'pattern', baseDir: 'assets', include: ['**/*.png', '**/*.svg']}, + {type: 'configKey', key: 'theme_root'}, + {type: 'static', source: 'src/manifest.json', destination: 'manifest.json'}, + ], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, contextWithConfig) + + // Then + // 5 = 2 pattern + 2 configKey dir contents + 1 explicit file + expect(result.filesCopied).toBe(5) + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/manifest.json', '/test/output/manifest.json') + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/theme', '/test/output') + }) + }) +}) 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 new file mode 100644 index 00000000000..2bf11251780 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/include_assets_step.ts @@ -0,0 +1,318 @@ +import {joinPath, dirname, extname, relativePath, basename} from '@shopify/cli-kit/node/path' +import {glob, copyFile, copyDirectoryContents, fileExists, mkdir} from '@shopify/cli-kit/node/fs' +import {z} from 'zod' +import type {LifecycleStep, BuildContext} from '../client-steps.js' + +/** + * Pattern inclusion entry. + * + * Selects files from a source directory using glob patterns. `source` defaults + * to the extension root when omitted. `include` defaults to `['**\/*']`. + * `preserveStructure` defaults to `true` (relative paths preserved). + */ +const PatternEntrySchema = z.object({ + type: z.literal('pattern'), + baseDir: z.string().optional(), + include: z.array(z.string()).default(['**/*']), + ignore: z.array(z.string()).optional(), + destination: z.string().optional(), + preserveStructure: z.boolean().default(true), +}) + +/** + * Static inclusion entry — explicit source path. + * + * - With `destination`: copies the file/directory to that exact path. + * - Without `destination`, `preserveStructure` false (default): merges + * directory contents into the output root. + * - Without `destination`, `preserveStructure` true: places the directory + * under its own name in the output. + */ +const StaticEntrySchema = z.object({ + type: z.literal('static'), + source: z.string(), + destination: z.string().optional(), + preserveStructure: z.boolean().default(false), +}) + +/** + * ConfigKey inclusion entry — config key resolution. + * + * Resolves a path (or array of paths) from the extension configuration and + * copies the directory contents into the output. Silently skipped when the + * key is absent. Respects `preserveStructure` and `destination` the same way + * as the static entry. + */ +const ConfigKeyEntrySchema = z.object({ + type: z.literal('configKey'), + key: z.string(), + destination: z.string().optional(), + preserveStructure: z.boolean().default(false), +}) + +const InclusionEntrySchema = z.discriminatedUnion('type', [PatternEntrySchema, StaticEntrySchema, ConfigKeyEntrySchema]) + +/** + * Configuration schema for include_assets step. + * + * `inclusions` is a flat array of entries, each with a `type` discriminant + * (`'files'` or `'pattern'`). All entries are processed in parallel. + */ +const IncludeAssetsConfigSchema = z.object({ + inclusions: z.array(InclusionEntrySchema), +}) + +/** + * Executes an include_assets build step. + * + * Iterates over `config.inclusions` and dispatches each entry by type: + * + * - `type: 'files'` with `source` — copy a file or directory into the output. + * - `type: 'files'` with `configKey` — resolve a path from the extension's + * config and copy its directory into the output; silently skipped if absent. + * - `type: 'pattern'` — glob-based file selection from a source directory + * (defaults to extension root when `source` is omitted). + */ +export async function executeIncludeAssetsStep( + step: LifecycleStep, + context: BuildContext, +): Promise<{filesCopied: number}> { + const config = IncludeAssetsConfigSchema.parse(step.config) + const {extension, options} = context + // When outputPath is a file (e.g. index.js, index.wasm), the output directory is its + // parent. When outputPath has no extension, it IS the output directory. + const outputDir = extname(extension.outputPath) ? dirname(extension.outputPath) : extension.outputPath + + const counts = await Promise.all( + config.inclusions.map(async (entry) => { + if (entry.type === 'pattern') { + const sourceDir = entry.baseDir ? joinPath(extension.directory, entry.baseDir) : extension.directory + const destinationDir = entry.destination ? joinPath(outputDir, entry.destination) : outputDir + const result = await copyByPattern( + sourceDir, + destinationDir, + entry.include, + entry.ignore ?? [], + entry.preserveStructure, + options, + ) + return result.filesCopied + } + + if (entry.type === 'configKey') { + return copyConfigKeyEntry( + entry.key, + extension.directory, + outputDir, + context, + options, + entry.preserveStructure, + entry.destination, + ) + } + + return copySourceEntry( + entry.source, + entry.destination, + extension.directory, + outputDir, + options, + entry.preserveStructure, + ) + }), + ) + + return {filesCopied: counts.reduce((sum, count) => sum + count, 0)} +} + +/** + * Handles a `{source}` or `{source, destination}` files entry. + * + * - No `destination`, `preserveStructure` false: copy directory contents into the output root. + * - No `destination`, `preserveStructure` true: copy the directory under its own name in the output. + * - With `destination`: copy the file to the explicit destination path (`preserveStructure` is ignored). + */ +async function copySourceEntry( + source: string, + destination: string | undefined, + baseDir: string, + outputDir: string, + options: {stdout: NodeJS.WritableStream}, + preserveStructure: boolean, +): Promise { + const sourcePath = joinPath(baseDir, source) + const exists = await fileExists(sourcePath) + if (!exists) { + throw new Error(`Source does not exist: ${sourcePath}`) + } + + if (destination !== undefined) { + const destPath = joinPath(outputDir, destination) + await mkdir(dirname(destPath)) + await copyFile(sourcePath, destPath) + options.stdout.write(`Copied ${source} to ${destination}\n`) + return 1 + } + + const destDir = preserveStructure ? joinPath(outputDir, basename(sourcePath)) : outputDir + await copyDirectoryContents(sourcePath, destDir) + const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) + const msg = preserveStructure + ? `Copied ${source} to ${basename(sourcePath)}\n` + : `Copied contents of ${source} to output root\n` + options.stdout.write(msg) + return copied.length +} + +/** + * Handles a `{configKey}` files entry. + * + * Resolves the key from the extension's config. String values and string + * arrays are each used as source paths. Unresolved keys and missing paths are + * skipped silently with a log message. When `destination` is given, the + * resolved directory is placed under `outputDir/destination`. + */ +async function copyConfigKeyEntry( + key: string, + baseDir: string, + outputDir: string, + context: BuildContext, + options: {stdout: NodeJS.WritableStream}, + preserveStructure: boolean, + destination?: string, +): Promise { + const value = getNestedValue(context.extension.configuration, key) + let paths: string[] + if (typeof value === 'string') { + paths = [value] + } else if (Array.isArray(value)) { + paths = value.filter((item): item is string => typeof item === 'string') + } else { + paths = [] + } + + if (paths.length === 0) { + options.stdout.write(`No value for configKey '${key}', skipping\n`) + return 0 + } + + const effectiveOutputDir = destination ? joinPath(outputDir, destination) : outputDir + + const counts = await Promise.all( + paths.map(async (sourcePath) => { + const fullPath = joinPath(baseDir, sourcePath) + const exists = await fileExists(fullPath) + if (!exists) { + options.stdout.write(`Warning: path '${sourcePath}' does not exist, skipping\n`) + return 0 + } + const destDir = preserveStructure ? joinPath(effectiveOutputDir, basename(fullPath)) : effectiveOutputDir + await copyDirectoryContents(fullPath, destDir) + const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) + const msg = preserveStructure + ? `Copied '${sourcePath}' to ${basename(fullPath)}\n` + : `Copied contents of '${sourcePath}' to output root\n` + options.stdout.write(msg) + return copied.length + }), + ) + return counts.reduce((sum, count) => sum + count, 0) +} + +/** + * Pattern strategy: glob-based file selection. + */ +async function copyByPattern( + sourceDir: string, + outputDir: string, + patterns: string[], + ignore: string[], + preserveStructure: boolean, + options: {stdout: NodeJS.WritableStream}, +): Promise<{filesCopied: number}> { + const files = await glob(patterns, { + absolute: true, + cwd: sourceDir, + ignore, + }) + + if (files.length === 0) { + options.stdout.write(`Warning: No files matched patterns in ${sourceDir}\n`) + return {filesCopied: 0} + } + + await mkdir(outputDir) + + await Promise.all( + files.map(async (filepath) => { + const relPath = preserveStructure ? relativePath(sourceDir, filepath) : basename(filepath) + const destPath = joinPath(outputDir, relPath) + + if (filepath === destPath) return + + await mkdir(dirname(destPath)) + await copyFile(filepath, destPath) + }), + ) + + options.stdout.write(`Copied ${files.length} file(s) from ${sourceDir} to ${outputDir}\n`) + return {filesCopied: files.length} +} + +/** + * Splits a path into tokens. A token with `flatten: true` (the `[]` suffix) + * signals that an array is expected at that position and the result should be + * flattened one level before continuing. Plain tokens preserve whatever shape + * is already in flight — if the current value is already an array (from a + * prior flatten), the field is plucked from each element automatically. + * + * Examples: + * "tools" → [name:"tools", flatten:false] + * "targeting.tools" → [name:"targeting",...], [name:"tools",...] + * "extensions[].targeting[].schema" → [name:"extensions", flatten:true], ... + */ +function tokenizePath(path: string): {name: string; flatten: boolean}[] { + return path.split('.').map((part) => { + const flatten = part.endsWith('[]') + return {name: flatten ? part.slice(0, -2) : part, flatten} + }) +} + +/** + * Resolves a dot-separated path (with optional `[]` flatten markers) from a + * config object. + * + * - Plain segments (`targeting.tools`): dot-notation access; when the current + * value is already an array (due to a prior flatten), the field is plucked + * from every element automatically. + * - Flatten segments (`extensions[]`): access the field and flatten one level + * of nesting. Returns `undefined` if the value at that point is not an array + * — the `[]` suffix is a contract that an array is expected there. + */ +function getNestedValue(obj: {[key: string]: unknown}, path: string): unknown { + let current: unknown = obj + + for (const {name, flatten} of tokenizePath(path)) { + if (current === null || current === undefined) return undefined + + if (Array.isArray(current)) { + const plucked = current + .map((item) => + item !== null && typeof item === 'object' ? (item as {[key: string]: unknown})[name] : undefined, + ) + .filter((val): val is NonNullable => val !== undefined) + current = plucked.length > 0 ? plucked : undefined + } else if (typeof current === 'object') { + current = (current as {[key: string]: unknown})[name] + } else { + return undefined + } + + if (flatten) { + if (!Array.isArray(current)) return undefined + current = (current as unknown[]).flat(1) + } + } + + return current +} diff --git a/packages/app/src/cli/services/build/steps/index.ts b/packages/app/src/cli/services/build/steps/index.ts index 846a4d4147b..b33787eaccd 100644 --- a/packages/app/src/cli/services/build/steps/index.ts +++ b/packages/app/src/cli/services/build/steps/index.ts @@ -1,3 +1,10 @@ +import {executeIncludeAssetsStep} from './include_assets_step.js' +import {executeBuildThemeStep} from './build-theme-step.js' +import {executeBundleThemeStep} from './bundle-theme-step.js' +import {executeBundleUIStep} from './bundle-ui-step.js' +import {executeCopyStaticAssetsStep} from './copy-static-assets-step.js' +import {executeBuildFunctionStep} from './build-function-step.js' +import {executeCreateTaxStubStep} from './create-tax-stub-step.js' import type {LifecycleStep, BuildContext} from '../client-steps.js' /** @@ -9,18 +16,28 @@ import type {LifecycleStep, BuildContext} from '../client-steps.js' * @returns The output from the step execution * @throws Error if the step type is not implemented or unknown */ -export async function executeStepByType(step: LifecycleStep, _context: BuildContext): Promise { +export async function executeStepByType(step: LifecycleStep, context: BuildContext): Promise { switch (step.type) { - // Future step types (not implemented yet): case 'include_assets': + return executeIncludeAssetsStep(step, context) + case 'build_theme': + return executeBuildThemeStep(step, context) + case 'bundle_theme': + return executeBundleThemeStep(step, context) + case 'bundle_ui': + return executeBundleUIStep(step, context) + case 'copy_static_assets': + return executeCopyStaticAssetsStep(step, context) + case 'build_function': - case 'create_tax_stub': - throw new Error(`Build step type "${step.type}" is not yet implemented.`) + return executeBuildFunctionStep(step, context) + case 'create_tax_stub': + return executeCreateTaxStubStep(step, context) default: throw new Error(`Unknown build step type: ${(step as {type: string}).type}`) } From 08b23f3c8e6d2204c2367a6a38452605f01f9851 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Tue, 17 Mar 2026 13:58:14 +0100 Subject: [PATCH 02/13] Guard include_assets step against path traversal in destinations and pattern copy - Add sanitizeDestination() that strips '..' segments from destination fields and emits a warning when any are removed - Sanitize entry.destination for all three inclusion types (pattern, static, configKey) before it reaches any path join - Add copy-time bounds check in copyByPattern: skip any file whose resolved destPath escapes outputDir and warn Co-Authored-By: Claude Sonnet 4.6 --- .../build/steps/include_assets_step.ts | 42 +++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) 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 2bf11251780..c7d80a97ab5 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 @@ -62,6 +62,31 @@ const IncludeAssetsConfigSchema = z.object({ inclusions: z.array(InclusionEntrySchema), }) +/** + * Removes any '..' traversal segments from a relative destination path and + * emits a warning if any were found. Preserves normal '..' that only navigate + * within the path (e.g. 'foo/../bar' → 'bar') but never allows the result to + * escape the output root. + */ +function sanitizeDestination(input: string, warn: (msg: string) => void): string { + const segments = input.split('/') + const stack: string[] = [] + let stripped = false + for (const seg of segments) { + if (seg === '..') { + stripped = true + stack.pop() + } else if (seg !== '.') { + stack.push(seg) + } + } + const result = stack.join('/') + if (stripped) { + warn(`Warning: destination '${input}' contains '..' path traversal - sanitized to '${result || '.'}'\n`) + } + return result +} + /** * Executes an include_assets build step. * @@ -85,9 +110,13 @@ export async function executeIncludeAssetsStep( const counts = await Promise.all( config.inclusions.map(async (entry) => { + const warn = (msg: string) => options.stdout.write(msg) + const sanitizedDest = + entry.destination !== undefined ? sanitizeDestination(entry.destination, warn) : undefined + if (entry.type === 'pattern') { const sourceDir = entry.baseDir ? joinPath(extension.directory, entry.baseDir) : extension.directory - const destinationDir = entry.destination ? joinPath(outputDir, entry.destination) : outputDir + const destinationDir = sanitizedDest ? joinPath(outputDir, sanitizedDest) : outputDir const result = await copyByPattern( sourceDir, destinationDir, @@ -107,13 +136,13 @@ export async function executeIncludeAssetsStep( context, options, entry.preserveStructure, - entry.destination, + sanitizedDest, ) } return copySourceEntry( entry.source, - entry.destination, + sanitizedDest, extension.directory, outputDir, options, @@ -248,6 +277,13 @@ async function copyByPattern( const relPath = preserveStructure ? relativePath(sourceDir, filepath) : basename(filepath) const destPath = joinPath(outputDir, relPath) + if (relativePath(outputDir, destPath).startsWith('..')) { + options.stdout.write( + `Warning: skipping '${filepath}' - resolved destination is outside the output directory\n`, + ) + return + } + if (filepath === destPath) return await mkdir(dirname(destPath)) From 2923a07be04e3a0fd103efa330530856f2640080 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Wed, 18 Mar 2026 11:41:57 +0100 Subject: [PATCH 03/13] Fix prettier lint errors in include_assets_step.ts Co-Authored-By: Claude Sonnet 4.6 --- .../build/steps/include-assets-step.test.ts | 61 ++++++++++++++- ..._assets_step.ts => include-assets-step.ts} | 78 +++++++++++++++---- .../app/src/cli/services/build/steps/index.ts | 2 +- 3 files changed, 122 insertions(+), 19 deletions(-) rename packages/app/src/cli/services/build/steps/{include_assets_step.ts => include-assets-step.ts} (81%) 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 e2ed2d24e86..47680ab9ed8 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 @@ -1,4 +1,4 @@ -import {executeIncludeAssetsStep} from './include_assets_step.js' +import {executeIncludeAssetsStep} from './include-assets-step.js' import {LifecycleStep, BuildContext} from '../client-steps.js' import {ExtensionInstance} from '../../../models/extensions/extension-instance.js' import {describe, expect, test, vi, beforeEach} from 'vitest' @@ -36,6 +36,7 @@ describe('executeIncludeAssetsStep', () => { test('copies directory contents to output root when no destination (preserveStructure defaults false)', async () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.glob).mockResolvedValue(['index.html', 'assets/logo.png']) @@ -60,6 +61,7 @@ describe('executeIncludeAssetsStep', () => { test('preserves directory name when preserveStructure is true', async () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.glob).mockResolvedValue(['index.html', 'assets/logo.png']) @@ -142,6 +144,7 @@ describe('executeIncludeAssetsStep', () => { test('handles multiple static entries in inclusions', async () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValueOnce(true).mockResolvedValueOnce(false) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() vi.mocked(fs.mkdir).mockResolvedValue() @@ -167,6 +170,56 @@ describe('executeIncludeAssetsStep', () => { expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/icon.png', '/test/output/assets/icon.png') expect(result.filesCopied).toBe(2) }) + + test('copies a file to output root when source is a file and no destination is given', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-readme', + name: 'Copy README', + type: 'include_assets', + config: { + inclusions: [{type: 'static', source: 'README.md'}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, mockContext) + + // Then + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/README.md', '/test/output/README.md') + expect(result.filesCopied).toBe(1) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied README.md to README.md')) + }) + + test('copies a directory to explicit destination path', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-dist', + name: 'Copy Dist', + type: 'include_assets', + config: { + inclusions: [{type: 'static', source: 'dist', destination: 'assets/dist'}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, mockContext) + + // Then + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/dist', '/test/output/assets/dist') + expect(result.filesCopied).toBe(1) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied dist to assets/dist')) + }) }) describe('configKey entries', () => { @@ -181,6 +234,7 @@ describe('executeIncludeAssetsStep', () => { } vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.glob).mockResolvedValue(['index.html', 'logo.png']) @@ -212,6 +266,7 @@ describe('executeIncludeAssetsStep', () => { } vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.glob).mockResolvedValue(['index.html', 'logo.png']) @@ -302,6 +357,7 @@ describe('executeIncludeAssetsStep', () => { } vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.glob).mockResolvedValue(['file.html']) @@ -338,6 +394,7 @@ describe('executeIncludeAssetsStep', () => { } vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.glob).mockResolvedValue(['file.js']) @@ -397,6 +454,7 @@ describe('executeIncludeAssetsStep', () => { } vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() vi.mocked(fs.mkdir).mockResolvedValue() @@ -572,6 +630,7 @@ describe('executeIncludeAssetsStep', () => { } vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() vi.mocked(fs.mkdir).mockResolvedValue() 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 similarity index 81% rename from packages/app/src/cli/services/build/steps/include_assets_step.ts rename to packages/app/src/cli/services/build/steps/include-assets-step.ts index c7d80a97ab5..e1a0f98fa06 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 @@ -1,5 +1,5 @@ import {joinPath, dirname, extname, relativePath, basename} from '@shopify/cli-kit/node/path' -import {glob, copyFile, copyDirectoryContents, fileExists, mkdir} from '@shopify/cli-kit/node/fs' +import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs' import {z} from 'zod' import type {LifecycleStep, BuildContext} from '../client-steps.js' @@ -56,7 +56,7 @@ const InclusionEntrySchema = z.discriminatedUnion('type', [PatternEntrySchema, S * Configuration schema for include_assets step. * * `inclusions` is a flat array of entries, each with a `type` discriminant - * (`'files'` or `'pattern'`). All entries are processed in parallel. + * (`'static'`, `'configKey'`, or `'pattern'`). All entries are processed in parallel. */ const IncludeAssetsConfigSchema = z.object({ inclusions: z.array(InclusionEntrySchema), @@ -69,7 +69,7 @@ const IncludeAssetsConfigSchema = z.object({ * escape the output root. */ function sanitizeDestination(input: string, warn: (msg: string) => void): string { - const segments = input.split('/') + const segments = input.replace(/\\/g, '/').split('/') const stack: string[] = [] let stripped = false for (const seg of segments) { @@ -92,9 +92,9 @@ function sanitizeDestination(input: string, warn: (msg: string) => void): string * * Iterates over `config.inclusions` and dispatches each entry by type: * - * - `type: 'files'` with `source` — copy a file or directory into the output. - * - `type: 'files'` with `configKey` — resolve a path from the extension's - * config and copy its directory into the output; silently skipped if absent. + * - `type: 'static'` — copy a file or directory into the output. + * - `type: 'configKey'` — resolve a path from the extension's + * config and copy into the output; silently skipped if absent. * - `type: 'pattern'` — glob-based file selection from a source directory * (defaults to extension root when `source` is omitted). */ @@ -111,8 +111,7 @@ export async function executeIncludeAssetsStep( const counts = await Promise.all( config.inclusions.map(async (entry) => { const warn = (msg: string) => options.stdout.write(msg) - const sanitizedDest = - entry.destination !== undefined ? sanitizeDestination(entry.destination, warn) : undefined + const sanitizedDest = entry.destination !== undefined ? sanitizeDestination(entry.destination, warn) : undefined if (entry.type === 'pattern') { const sourceDir = entry.baseDir ? joinPath(extension.directory, entry.baseDir) : extension.directory @@ -183,6 +182,14 @@ async function copySourceEntry( return 1 } + if (!(await isDirectory(sourcePath))) { + const destPath = joinPath(outputDir, basename(sourcePath)) + await mkdir(outputDir) + await copyFile(sourcePath, destPath) + options.stdout.write(`Copied ${source} to ${basename(sourcePath)}\n`) + return 1 + } + const destDir = preserveStructure ? joinPath(outputDir, basename(sourcePath)) : outputDir await copyDirectoryContents(sourcePath, destDir) const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) @@ -235,6 +242,13 @@ async function copyConfigKeyEntry( options.stdout.write(`Warning: path '${sourcePath}' does not exist, skipping\n`) return 0 } + if (!(await isDirectory(fullPath))) { + const destPath = joinPath(effectiveOutputDir, basename(fullPath)) + await mkdir(effectiveOutputDir) + await copyFile(fullPath, destPath) + options.stdout.write(`Copied '${sourcePath}' to ${basename(fullPath)}\n`) + return 1 + } const destDir = preserveStructure ? joinPath(effectiveOutputDir, basename(fullPath)) : effectiveOutputDir await copyDirectoryContents(fullPath, destDir) const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) @@ -272,27 +286,57 @@ async function copyByPattern( await mkdir(outputDir) - await Promise.all( - files.map(async (filepath) => { + const duplicates = new Set() + if (!preserveStructure) { + const basenames = files.map((fp) => basename(fp)) + const seen = new Set() + for (const name of basenames) { + if (seen.has(name)) { + duplicates.add(name) + } else { + seen.add(name) + } + } + if (duplicates.size > 0) { + const colliding = files.filter((fp) => duplicates.has(basename(fp))) + options.stdout.write( + `Warning: filename collision detected when flattening — the following files share a basename and will overwrite each other: ${colliding.join(', ')}\n`, + ) + } + } + + // When flattening and collisions exist, deduplicate so last-in-array deterministically wins + const filesToCopy = + !preserveStructure && duplicates.size > 0 + ? files.filter((fp, idx) => { + const name = basename(fp) + if (!duplicates.has(name)) return true + const lastIdx = files.reduce((last, file, ii) => (basename(file) === name ? ii : last), -1) + return lastIdx === idx + }) + : files + + const copyResults = await Promise.all( + filesToCopy.map(async (filepath): Promise => { const relPath = preserveStructure ? relativePath(sourceDir, filepath) : basename(filepath) const destPath = joinPath(outputDir, relPath) if (relativePath(outputDir, destPath).startsWith('..')) { - options.stdout.write( - `Warning: skipping '${filepath}' - resolved destination is outside the output directory\n`, - ) - return + options.stdout.write(`Warning: skipping '${filepath}' - resolved destination is outside the output directory\n`) + return 0 } - if (filepath === destPath) return + if (filepath === destPath) return 0 await mkdir(dirname(destPath)) await copyFile(filepath, destPath) + return 1 }), ) - options.stdout.write(`Copied ${files.length} file(s) from ${sourceDir} to ${outputDir}\n`) - return {filesCopied: files.length} + const copiedCount = copyResults.reduce((sum, count) => sum + count, 0) + options.stdout.write(`Copied ${copiedCount} file(s) from ${sourceDir} to ${outputDir}\n`) + return {filesCopied: copiedCount} } /** diff --git a/packages/app/src/cli/services/build/steps/index.ts b/packages/app/src/cli/services/build/steps/index.ts index b33787eaccd..6ca10e6e041 100644 --- a/packages/app/src/cli/services/build/steps/index.ts +++ b/packages/app/src/cli/services/build/steps/index.ts @@ -1,4 +1,4 @@ -import {executeIncludeAssetsStep} from './include_assets_step.js' +import {executeIncludeAssetsStep} from './include-assets-step.js' import {executeBuildThemeStep} from './build-theme-step.js' import {executeBundleThemeStep} from './bundle-theme-step.js' import {executeBundleUIStep} from './bundle-ui-step.js' From 4a5ddb5a642cd36e7b308d185b046ff4ed5ef16a Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Tue, 24 Mar 2026 19:00:08 +0100 Subject: [PATCH 04/13] Refactor include_assets step helpers and fix directory copy count - Replace positional args with config objects in copyByPattern, copySourceEntry, and copyConfigKeyEntry to prevent argument-order mistakes (e.g. mixing include/ignore patterns) - Make the static dispatch branch explicit with if (entry.type === 'static') - Fix copySourceEntry: when destination is provided and source is a directory, use copyDirectoryContents + glob count instead of copyFile + hardcoded 1 Co-Authored-By: Claude Sonnet 4.6 --- .../build/steps/include-assets-step.test.ts | 12 +-- .../build/steps/include-assets-step.ts | 95 ++++++++++++------- 2 files changed, 65 insertions(+), 42 deletions(-) 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 47680ab9ed8..997ffc5cc57 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 @@ -200,8 +200,8 @@ describe('executeIncludeAssetsStep', () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) vi.mocked(fs.isDirectory).mockResolvedValue(true) - vi.mocked(fs.copyFile).mockResolvedValue() - vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['a.js', 'b.js']) const step: LifecycleStep = { id: 'copy-dist', @@ -216,8 +216,8 @@ describe('executeIncludeAssetsStep', () => { const result = await executeIncludeAssetsStep(step, mockContext) // Then - expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/dist', '/test/output/assets/dist') - expect(result.filesCopied).toBe(1) + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output/assets/dist') + expect(result.filesCopied).toBe(2) expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied dist to assets/dist')) }) }) @@ -454,7 +454,7 @@ describe('executeIncludeAssetsStep', () => { } vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockImplementation((path) => Promise.resolve(!String(path).endsWith('.png'))) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() vi.mocked(fs.mkdir).mockResolvedValue() @@ -630,7 +630,7 @@ describe('executeIncludeAssetsStep', () => { } vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockImplementation((path) => Promise.resolve(!String(path).endsWith('.json'))) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() vi.mocked(fs.mkdir).mockResolvedValue() 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 e1a0f98fa06..bfcd65cbf1a 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 @@ -117,11 +117,13 @@ export async function executeIncludeAssetsStep( const sourceDir = entry.baseDir ? joinPath(extension.directory, entry.baseDir) : extension.directory const destinationDir = sanitizedDest ? joinPath(outputDir, sanitizedDest) : outputDir const result = await copyByPattern( - sourceDir, - destinationDir, - entry.include, - entry.ignore ?? [], - entry.preserveStructure, + { + sourceDir, + outputDir: destinationDir, + patterns: entry.include, + ignore: entry.ignore ?? [], + preserveStructure: entry.preserveStructure, + }, options, ) return result.filesCopied @@ -129,28 +131,34 @@ export async function executeIncludeAssetsStep( if (entry.type === 'configKey') { return copyConfigKeyEntry( - entry.key, - extension.directory, - outputDir, - context, + { + key: entry.key, + baseDir: extension.directory, + outputDir, + context, + preserveStructure: entry.preserveStructure, + destination: sanitizedDest, + }, options, - entry.preserveStructure, - sanitizedDest, ) } - return copySourceEntry( - entry.source, - sanitizedDest, - extension.directory, - outputDir, - options, - entry.preserveStructure, - ) + if (entry.type === 'static') { + return copySourceEntry( + { + source: entry.source, + destination: sanitizedDest, + baseDir: extension.directory, + outputDir, + preserveStructure: entry.preserveStructure, + }, + options, + ) + } }), ) - return {filesCopied: counts.reduce((sum, count) => sum + count, 0)} + return {filesCopied: counts.reduce((sum, count) => sum + (count ?? 0), 0)} } /** @@ -161,13 +169,16 @@ export async function executeIncludeAssetsStep( * - With `destination`: copy the file to the explicit destination path (`preserveStructure` is ignored). */ async function copySourceEntry( - source: string, - destination: string | undefined, - baseDir: string, - outputDir: string, + config: { + source: string + destination: string | undefined + baseDir: string + outputDir: string + preserveStructure: boolean + }, options: {stdout: NodeJS.WritableStream}, - preserveStructure: boolean, ): Promise { + const {source, destination, baseDir, outputDir, preserveStructure} = config const sourcePath = joinPath(baseDir, source) const exists = await fileExists(sourcePath) if (!exists) { @@ -176,6 +187,12 @@ async function copySourceEntry( if (destination !== undefined) { const destPath = joinPath(outputDir, destination) + if (await isDirectory(sourcePath)) { + await copyDirectoryContents(sourcePath, destPath) + const copied = await glob(['**/*'], {cwd: destPath, absolute: false}) + options.stdout.write(`Copied ${source} to ${destination}\n`) + return copied.length + } await mkdir(dirname(destPath)) await copyFile(sourcePath, destPath) options.stdout.write(`Copied ${source} to ${destination}\n`) @@ -209,14 +226,17 @@ async function copySourceEntry( * resolved directory is placed under `outputDir/destination`. */ async function copyConfigKeyEntry( - key: string, - baseDir: string, - outputDir: string, - context: BuildContext, + config: { + key: string + baseDir: string + outputDir: string + context: BuildContext + preserveStructure: boolean + destination?: string + }, options: {stdout: NodeJS.WritableStream}, - preserveStructure: boolean, - destination?: string, ): Promise { + const {key, baseDir, outputDir, context, preserveStructure, destination} = config const value = getNestedValue(context.extension.configuration, key) let paths: string[] if (typeof value === 'string') { @@ -266,13 +286,16 @@ async function copyConfigKeyEntry( * Pattern strategy: glob-based file selection. */ async function copyByPattern( - sourceDir: string, - outputDir: string, - patterns: string[], - ignore: string[], - preserveStructure: boolean, + config: { + sourceDir: string + outputDir: string + patterns: string[] + ignore: string[] + preserveStructure: boolean + }, options: {stdout: NodeJS.WritableStream}, ): Promise<{filesCopied: number}> { + const {sourceDir, outputDir, patterns, ignore, preserveStructure} = config const files = await glob(patterns, { absolute: true, cwd: sourceDir, From ded4712c9674e4ca7e2affcf44b94403603ca292 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Tue, 24 Mar 2026 19:15:13 +0100 Subject: [PATCH 05/13] Fix static preserveStructure default and unwrap copyByPattern return type - Change StaticEntrySchema preserveStructure default from false to true so directory sources preserve their name in the output by default rather than silently merging contents into the output root - Unwrap copyByPattern return from {filesCopied: number} to number, consistent with copySourceEntry and copyConfigKeyEntry Co-Authored-By: Claude Sonnet 4.6 --- .../services/build/steps/include-assets-step.test.ts | 6 +++--- .../cli/services/build/steps/include-assets-step.ts | 11 +++++------ 2 files changed, 8 insertions(+), 9 deletions(-) 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 997ffc5cc57..1c166f0a1fa 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 @@ -33,7 +33,7 @@ describe('executeIncludeAssetsStep', () => { }) describe('static entries', () => { - test('copies directory contents to output root when no destination (preserveStructure defaults false)', async () => { + test('copies directory contents to output root when no destination and preserveStructure is false', async () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) vi.mocked(fs.isDirectory).mockResolvedValue(true) @@ -45,7 +45,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Copy Dist', type: 'include_assets', config: { - inclusions: [{type: 'static', source: 'dist'}], + inclusions: [{type: 'static', source: 'dist', preserveStructure: false}], }, } @@ -166,7 +166,7 @@ describe('executeIncludeAssetsStep', () => { const result = await executeIncludeAssetsStep(step, mockContext) // Then - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output') + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output/dist') expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/icon.png', '/test/output/assets/icon.png') expect(result.filesCopied).toBe(2) }) 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 bfcd65cbf1a..18093b5093b 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 @@ -32,7 +32,7 @@ const StaticEntrySchema = z.object({ type: z.literal('static'), source: z.string(), destination: z.string().optional(), - preserveStructure: z.boolean().default(false), + preserveStructure: z.boolean().default(true), }) /** @@ -116,7 +116,7 @@ export async function executeIncludeAssetsStep( if (entry.type === 'pattern') { const sourceDir = entry.baseDir ? joinPath(extension.directory, entry.baseDir) : extension.directory const destinationDir = sanitizedDest ? joinPath(outputDir, sanitizedDest) : outputDir - const result = await copyByPattern( + return copyByPattern( { sourceDir, outputDir: destinationDir, @@ -126,7 +126,6 @@ export async function executeIncludeAssetsStep( }, options, ) - return result.filesCopied } if (entry.type === 'configKey') { @@ -294,7 +293,7 @@ async function copyByPattern( preserveStructure: boolean }, options: {stdout: NodeJS.WritableStream}, -): Promise<{filesCopied: number}> { +): Promise { const {sourceDir, outputDir, patterns, ignore, preserveStructure} = config const files = await glob(patterns, { absolute: true, @@ -304,7 +303,7 @@ async function copyByPattern( if (files.length === 0) { options.stdout.write(`Warning: No files matched patterns in ${sourceDir}\n`) - return {filesCopied: 0} + return 0 } await mkdir(outputDir) @@ -359,7 +358,7 @@ async function copyByPattern( const copiedCount = copyResults.reduce((sum, count) => sum + count, 0) options.stdout.write(`Copied ${copiedCount} file(s) from ${sourceDir} to ${outputDir}\n`) - return {filesCopied: copiedCount} + return copiedCount } /** From 17065af7cbcf65b05103784d0354a3b7a3f545d2 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Tue, 24 Mar 2026 19:55:53 +0100 Subject: [PATCH 06/13] Move sanitizeRelativePath to cli-kit/path and simplify copySourceEntry - Extract sanitizeDestination into sanitizeRelativePath in @shopify/cli-kit/node/path so it can be shared across the codebase - Simplify copySourceEntry by resolving destPath and logMsg up front then dispatching once on file vs directory, eliminating the duplicated copyDirectoryContents + glob + return count pattern Co-Authored-By: Claude Sonnet 4.6 --- .../build/steps/include-assets-step.ts | 84 +++++++------------ packages/cli-kit/src/public/node/path.ts | 29 +++++++ 2 files changed, 58 insertions(+), 55 deletions(-) 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 18093b5093b..d264d5160d6 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 @@ -1,4 +1,4 @@ -import {joinPath, dirname, extname, relativePath, basename} from '@shopify/cli-kit/node/path' +import {joinPath, dirname, extname, relativePath, basename, sanitizeRelativePath} from '@shopify/cli-kit/node/path' import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs' import {z} from 'zod' import type {LifecycleStep, BuildContext} from '../client-steps.js' @@ -62,31 +62,6 @@ const IncludeAssetsConfigSchema = z.object({ inclusions: z.array(InclusionEntrySchema), }) -/** - * Removes any '..' traversal segments from a relative destination path and - * emits a warning if any were found. Preserves normal '..' that only navigate - * within the path (e.g. 'foo/../bar' → 'bar') but never allows the result to - * escape the output root. - */ -function sanitizeDestination(input: string, warn: (msg: string) => void): string { - const segments = input.replace(/\\/g, '/').split('/') - const stack: string[] = [] - let stripped = false - for (const seg of segments) { - if (seg === '..') { - stripped = true - stack.pop() - } else if (seg !== '.') { - stack.push(seg) - } - } - const result = stack.join('/') - if (stripped) { - warn(`Warning: destination '${input}' contains '..' path traversal - sanitized to '${result || '.'}'\n`) - } - return result -} - /** * Executes an include_assets build step. * @@ -111,7 +86,7 @@ export async function executeIncludeAssetsStep( const counts = await Promise.all( config.inclusions.map(async (entry) => { const warn = (msg: string) => options.stdout.write(msg) - const sanitizedDest = entry.destination !== undefined ? sanitizeDestination(entry.destination, warn) : undefined + const sanitizedDest = entry.destination !== undefined ? sanitizeRelativePath(entry.destination, warn) : undefined if (entry.type === 'pattern') { const sourceDir = entry.baseDir ? joinPath(extension.directory, entry.baseDir) : extension.directory @@ -165,7 +140,7 @@ export async function executeIncludeAssetsStep( * * - No `destination`, `preserveStructure` false: copy directory contents into the output root. * - No `destination`, `preserveStructure` true: copy the directory under its own name in the output. - * - With `destination`: copy the file to the explicit destination path (`preserveStructure` is ignored). + * - With `destination`: copy to that exact path (`preserveStructure` is ignored). */ async function copySourceEntry( config: { @@ -179,41 +154,40 @@ async function copySourceEntry( ): Promise { const {source, destination, baseDir, outputDir, preserveStructure} = config const sourcePath = joinPath(baseDir, source) - const exists = await fileExists(sourcePath) - if (!exists) { + if (!(await fileExists(sourcePath))) { throw new Error(`Source does not exist: ${sourcePath}`) } + const sourceIsDir = await isDirectory(sourcePath) + + // Resolve destination path and log message up front, then dispatch on file vs directory. + let destPath: string + let logMsg: string if (destination !== undefined) { - const destPath = joinPath(outputDir, destination) - if (await isDirectory(sourcePath)) { - await copyDirectoryContents(sourcePath, destPath) - const copied = await glob(['**/*'], {cwd: destPath, absolute: false}) - options.stdout.write(`Copied ${source} to ${destination}\n`) - return copied.length - } - await mkdir(dirname(destPath)) - await copyFile(sourcePath, destPath) - options.stdout.write(`Copied ${source} to ${destination}\n`) - return 1 + destPath = joinPath(outputDir, destination) + logMsg = `Copied ${source} to ${destination}\n` + } else if (sourceIsDir && preserveStructure) { + destPath = joinPath(outputDir, basename(sourcePath)) + logMsg = `Copied ${source} to ${basename(sourcePath)}\n` + } else if (sourceIsDir) { + destPath = outputDir + logMsg = `Copied contents of ${source} to output root\n` + } else { + destPath = joinPath(outputDir, basename(sourcePath)) + logMsg = `Copied ${source} to ${basename(sourcePath)}\n` } - if (!(await isDirectory(sourcePath))) { - const destPath = joinPath(outputDir, basename(sourcePath)) - await mkdir(outputDir) - await copyFile(sourcePath, destPath) - options.stdout.write(`Copied ${source} to ${basename(sourcePath)}\n`) - return 1 + if (sourceIsDir) { + await copyDirectoryContents(sourcePath, destPath) + const copied = await glob(['**/*'], {cwd: destPath, absolute: false}) + options.stdout.write(logMsg) + return copied.length } - const destDir = preserveStructure ? joinPath(outputDir, basename(sourcePath)) : outputDir - await copyDirectoryContents(sourcePath, destDir) - const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) - const msg = preserveStructure - ? `Copied ${source} to ${basename(sourcePath)}\n` - : `Copied contents of ${source} to output root\n` - options.stdout.write(msg) - return copied.length + await mkdir(dirname(destPath)) + await copyFile(sourcePath, destPath) + options.stdout.write(logMsg) + return 1 } /** diff --git a/packages/cli-kit/src/public/node/path.ts b/packages/cli-kit/src/public/node/path.ts index a234622f4f9..b1691d637e3 100644 --- a/packages/cli-kit/src/public/node/path.ts +++ b/packages/cli-kit/src/public/node/path.ts @@ -191,3 +191,32 @@ export function sniffForPath(argv = process.argv): string | undefined { export function sniffForJson(argv = process.argv): boolean { return argv.includes('--json') || argv.includes('-j') } + +/** + * Removes any `..` traversal segments from a relative path and calls `warn` + * if any were stripped. Normal `..` that cancel out within the path (e.g. + * `foo/../bar` → `bar`) are collapsed but never allowed to escape the root. + * Both `/` and `\` are treated as separators for cross-platform safety. + * + * @param input - The relative path to sanitize. + * @param warn - Called with a human-readable warning when traversal segments are removed. + * @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 stack: string[] = [] + let stripped = false + for (const seg of segments) { + if (seg === '..') { + stripped = true + stack.pop() + } else if (seg !== '.') { + stack.push(seg) + } + } + const result = stack.join('/') + if (stripped) { + warn(`Warning: path '${input}' contains '..' traversal — sanitized to '${result || '.'}'\n`) + } + return result +} From 416a40869520bdbc23010820cf5711f7631b36d6 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Tue, 24 Mar 2026 20:45:13 +0100 Subject: [PATCH 07/13] Split include-assets-step into orchestrator + strategy files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move each copy strategy into its own file under include-assets/: - copy-by-pattern.ts — glob-based file selection - copy-source-entry.ts — explicit source path copy - copy-config-key-entry.ts — config key resolution + nested path helpers include-assets-step.ts is now a thin orchestrator holding only schemas and executeIncludeAssetsStep. No behavior change. Co-Authored-By: Claude Sonnet 4.6 --- .../build/steps/include-assets-step.ts | 264 +----------------- .../steps/include-assets/copy-by-pattern.ts | 82 ++++++ .../include-assets/copy-config-key-entry.ts | 126 +++++++++ .../steps/include-assets/copy-source-entry.ts | 57 ++++ 4 files changed, 269 insertions(+), 260 deletions(-) create mode 100644 packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts create mode 100644 packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts create mode 100644 packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts 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 d264d5160d6..8893edabd01 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 @@ -1,5 +1,7 @@ -import {joinPath, dirname, extname, relativePath, basename, sanitizeRelativePath} from '@shopify/cli-kit/node/path' -import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs' +import {copyByPattern} from './include-assets/copy-by-pattern.js' +import {copySourceEntry} from './include-assets/copy-source-entry.js' +import {copyConfigKeyEntry} from './include-assets/copy-config-key-entry.js' +import {joinPath, dirname, extname, sanitizeRelativePath} from '@shopify/cli-kit/node/path' import {z} from 'zod' import type {LifecycleStep, BuildContext} from '../client-steps.js' @@ -134,261 +136,3 @@ export async function executeIncludeAssetsStep( return {filesCopied: counts.reduce((sum, count) => sum + (count ?? 0), 0)} } - -/** - * Handles a `{source}` or `{source, destination}` files entry. - * - * - No `destination`, `preserveStructure` false: copy directory contents into the output root. - * - No `destination`, `preserveStructure` true: copy the directory under its own name in the output. - * - With `destination`: copy to that exact path (`preserveStructure` is ignored). - */ -async function copySourceEntry( - config: { - source: string - destination: string | undefined - baseDir: string - outputDir: string - preserveStructure: boolean - }, - options: {stdout: NodeJS.WritableStream}, -): Promise { - const {source, destination, baseDir, outputDir, preserveStructure} = config - const sourcePath = joinPath(baseDir, source) - if (!(await fileExists(sourcePath))) { - throw new Error(`Source does not exist: ${sourcePath}`) - } - - const sourceIsDir = await isDirectory(sourcePath) - - // Resolve destination path and log message up front, then dispatch on file vs directory. - let destPath: string - let logMsg: string - if (destination !== undefined) { - destPath = joinPath(outputDir, destination) - logMsg = `Copied ${source} to ${destination}\n` - } else if (sourceIsDir && preserveStructure) { - destPath = joinPath(outputDir, basename(sourcePath)) - logMsg = `Copied ${source} to ${basename(sourcePath)}\n` - } else if (sourceIsDir) { - destPath = outputDir - logMsg = `Copied contents of ${source} to output root\n` - } else { - destPath = joinPath(outputDir, basename(sourcePath)) - logMsg = `Copied ${source} to ${basename(sourcePath)}\n` - } - - if (sourceIsDir) { - await copyDirectoryContents(sourcePath, destPath) - const copied = await glob(['**/*'], {cwd: destPath, absolute: false}) - options.stdout.write(logMsg) - return copied.length - } - - await mkdir(dirname(destPath)) - await copyFile(sourcePath, destPath) - options.stdout.write(logMsg) - return 1 -} - -/** - * Handles a `{configKey}` files entry. - * - * Resolves the key from the extension's config. String values and string - * arrays are each used as source paths. Unresolved keys and missing paths are - * skipped silently with a log message. When `destination` is given, the - * resolved directory is placed under `outputDir/destination`. - */ -async function copyConfigKeyEntry( - config: { - key: string - baseDir: string - outputDir: string - context: BuildContext - preserveStructure: boolean - destination?: string - }, - options: {stdout: NodeJS.WritableStream}, -): Promise { - const {key, baseDir, outputDir, context, preserveStructure, destination} = config - const value = getNestedValue(context.extension.configuration, key) - let paths: string[] - if (typeof value === 'string') { - paths = [value] - } else if (Array.isArray(value)) { - paths = value.filter((item): item is string => typeof item === 'string') - } else { - paths = [] - } - - if (paths.length === 0) { - options.stdout.write(`No value for configKey '${key}', skipping\n`) - return 0 - } - - const effectiveOutputDir = destination ? joinPath(outputDir, destination) : outputDir - - const counts = await Promise.all( - paths.map(async (sourcePath) => { - const fullPath = joinPath(baseDir, sourcePath) - const exists = await fileExists(fullPath) - if (!exists) { - options.stdout.write(`Warning: path '${sourcePath}' does not exist, skipping\n`) - return 0 - } - if (!(await isDirectory(fullPath))) { - const destPath = joinPath(effectiveOutputDir, basename(fullPath)) - await mkdir(effectiveOutputDir) - await copyFile(fullPath, destPath) - options.stdout.write(`Copied '${sourcePath}' to ${basename(fullPath)}\n`) - return 1 - } - const destDir = preserveStructure ? joinPath(effectiveOutputDir, basename(fullPath)) : effectiveOutputDir - await copyDirectoryContents(fullPath, destDir) - const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) - const msg = preserveStructure - ? `Copied '${sourcePath}' to ${basename(fullPath)}\n` - : `Copied contents of '${sourcePath}' to output root\n` - options.stdout.write(msg) - return copied.length - }), - ) - return counts.reduce((sum, count) => sum + count, 0) -} - -/** - * Pattern strategy: glob-based file selection. - */ -async function copyByPattern( - config: { - sourceDir: string - outputDir: string - patterns: string[] - ignore: string[] - preserveStructure: boolean - }, - options: {stdout: NodeJS.WritableStream}, -): Promise { - const {sourceDir, outputDir, patterns, ignore, preserveStructure} = config - const files = await glob(patterns, { - absolute: true, - cwd: sourceDir, - ignore, - }) - - if (files.length === 0) { - options.stdout.write(`Warning: No files matched patterns in ${sourceDir}\n`) - return 0 - } - - await mkdir(outputDir) - - const duplicates = new Set() - if (!preserveStructure) { - const basenames = files.map((fp) => basename(fp)) - const seen = new Set() - for (const name of basenames) { - if (seen.has(name)) { - duplicates.add(name) - } else { - seen.add(name) - } - } - if (duplicates.size > 0) { - const colliding = files.filter((fp) => duplicates.has(basename(fp))) - options.stdout.write( - `Warning: filename collision detected when flattening — the following files share a basename and will overwrite each other: ${colliding.join(', ')}\n`, - ) - } - } - - // When flattening and collisions exist, deduplicate so last-in-array deterministically wins - const filesToCopy = - !preserveStructure && duplicates.size > 0 - ? files.filter((fp, idx) => { - const name = basename(fp) - if (!duplicates.has(name)) return true - const lastIdx = files.reduce((last, file, ii) => (basename(file) === name ? ii : last), -1) - return lastIdx === idx - }) - : files - - const copyResults = await Promise.all( - filesToCopy.map(async (filepath): Promise => { - const relPath = preserveStructure ? relativePath(sourceDir, filepath) : basename(filepath) - const destPath = joinPath(outputDir, relPath) - - if (relativePath(outputDir, destPath).startsWith('..')) { - options.stdout.write(`Warning: skipping '${filepath}' - resolved destination is outside the output directory\n`) - return 0 - } - - if (filepath === destPath) return 0 - - await mkdir(dirname(destPath)) - await copyFile(filepath, destPath) - return 1 - }), - ) - - const copiedCount = copyResults.reduce((sum, count) => sum + count, 0) - options.stdout.write(`Copied ${copiedCount} file(s) from ${sourceDir} to ${outputDir}\n`) - return copiedCount -} - -/** - * Splits a path into tokens. A token with `flatten: true` (the `[]` suffix) - * signals that an array is expected at that position and the result should be - * flattened one level before continuing. Plain tokens preserve whatever shape - * is already in flight — if the current value is already an array (from a - * prior flatten), the field is plucked from each element automatically. - * - * Examples: - * "tools" → [name:"tools", flatten:false] - * "targeting.tools" → [name:"targeting",...], [name:"tools",...] - * "extensions[].targeting[].schema" → [name:"extensions", flatten:true], ... - */ -function tokenizePath(path: string): {name: string; flatten: boolean}[] { - return path.split('.').map((part) => { - const flatten = part.endsWith('[]') - return {name: flatten ? part.slice(0, -2) : part, flatten} - }) -} - -/** - * Resolves a dot-separated path (with optional `[]` flatten markers) from a - * config object. - * - * - Plain segments (`targeting.tools`): dot-notation access; when the current - * value is already an array (due to a prior flatten), the field is plucked - * from every element automatically. - * - Flatten segments (`extensions[]`): access the field and flatten one level - * of nesting. Returns `undefined` if the value at that point is not an array - * — the `[]` suffix is a contract that an array is expected there. - */ -function getNestedValue(obj: {[key: string]: unknown}, path: string): unknown { - let current: unknown = obj - - for (const {name, flatten} of tokenizePath(path)) { - if (current === null || current === undefined) return undefined - - if (Array.isArray(current)) { - const plucked = current - .map((item) => - item !== null && typeof item === 'object' ? (item as {[key: string]: unknown})[name] : undefined, - ) - .filter((val): val is NonNullable => val !== undefined) - current = plucked.length > 0 ? plucked : undefined - } else if (typeof current === 'object') { - current = (current as {[key: string]: unknown})[name] - } else { - return undefined - } - - if (flatten) { - if (!Array.isArray(current)) return undefined - current = (current as unknown[]).flat(1) - } - } - - return current -} 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 new file mode 100644 index 00000000000..b0110d6aa5d --- /dev/null +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts @@ -0,0 +1,82 @@ +import {joinPath, dirname, relativePath, basename} from '@shopify/cli-kit/node/path' +import {glob, copyFile, mkdir} from '@shopify/cli-kit/node/fs' + +/** + * Pattern strategy: glob-based file selection. + */ +export async function copyByPattern( + config: { + sourceDir: string + outputDir: string + patterns: string[] + ignore: string[] + preserveStructure: boolean + }, + options: {stdout: NodeJS.WritableStream}, +): Promise { + const {sourceDir, outputDir, patterns, ignore, preserveStructure} = config + const files = await glob(patterns, { + absolute: true, + cwd: sourceDir, + ignore, + }) + + if (files.length === 0) { + options.stdout.write(`Warning: No files matched patterns in ${sourceDir}\n`) + return 0 + } + + await mkdir(outputDir) + + const duplicates = new Set() + if (!preserveStructure) { + const basenames = files.map((fp) => basename(fp)) + const seen = new Set() + for (const name of basenames) { + if (seen.has(name)) { + duplicates.add(name) + } else { + seen.add(name) + } + } + if (duplicates.size > 0) { + const colliding = files.filter((fp) => duplicates.has(basename(fp))) + options.stdout.write( + `Warning: filename collision detected when flattening — the following files share a basename and will overwrite each other: ${colliding.join(', ')}\n`, + ) + } + } + + // When flattening and collisions exist, deduplicate so last-in-array deterministically wins + const filesToCopy = + !preserveStructure && duplicates.size > 0 + ? files.filter((fp, idx) => { + const name = basename(fp) + if (!duplicates.has(name)) return true + const lastIdx = files.reduce((last, file, ii) => (basename(file) === name ? ii : last), -1) + return lastIdx === idx + }) + : files + + const copyResults = await Promise.all( + filesToCopy.map(async (filepath): Promise => { + const relPath = preserveStructure ? relativePath(sourceDir, filepath) : basename(filepath) + const destPath = joinPath(outputDir, relPath) + + if (relativePath(outputDir, destPath).startsWith('..')) { + options.stdout.write(`Warning: skipping '${filepath}' - resolved destination is outside the output directory\n`) + return 0 + } + + if (filepath === destPath) return 0 + + await mkdir(dirname(destPath)) + await copyFile(filepath, destPath) + return 1 + }), + ) + + const copiedCount = copyResults.reduce((sum, count) => sum + count, 0) + options.stdout.write(`Copied ${copiedCount} file(s) from ${sourceDir} to ${outputDir}\n`) + return copiedCount +} 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 new file mode 100644 index 00000000000..41880a179c8 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts @@ -0,0 +1,126 @@ +import {joinPath, basename} from '@shopify/cli-kit/node/path' +import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs' +import type {BuildContext} from '../../client-steps.js' + +/** + * Handles a `{configKey}` files entry. + * + * Resolves the key from the extension's config. String values and string + * arrays are each used as source paths. Unresolved keys and missing paths are + * skipped silently with a log message. When `destination` is given, the + * resolved directory is placed under `outputDir/destination`. + */ +export async function copyConfigKeyEntry( + config: { + key: string + baseDir: string + outputDir: string + context: BuildContext + preserveStructure: boolean + destination?: string + }, + options: {stdout: NodeJS.WritableStream}, +): Promise { + const {key, baseDir, outputDir, context, preserveStructure, destination} = config + const value = getNestedValue(context.extension.configuration, key) + let paths: string[] + if (typeof value === 'string') { + paths = [value] + } else if (Array.isArray(value)) { + paths = value.filter((item): item is string => typeof item === 'string') + } else { + paths = [] + } + + if (paths.length === 0) { + options.stdout.write(`No value for configKey '${key}', skipping\n`) + return 0 + } + + const effectiveOutputDir = destination ? joinPath(outputDir, destination) : outputDir + + const counts = await Promise.all( + paths.map(async (sourcePath) => { + const fullPath = joinPath(baseDir, sourcePath) + const exists = await fileExists(fullPath) + if (!exists) { + options.stdout.write(`Warning: path '${sourcePath}' does not exist, skipping\n`) + return 0 + } + if (!(await isDirectory(fullPath))) { + const destPath = joinPath(effectiveOutputDir, basename(fullPath)) + await mkdir(effectiveOutputDir) + await copyFile(fullPath, destPath) + options.stdout.write(`Copied '${sourcePath}' to ${basename(fullPath)}\n`) + return 1 + } + const destDir = preserveStructure ? joinPath(effectiveOutputDir, basename(fullPath)) : effectiveOutputDir + await copyDirectoryContents(fullPath, destDir) + const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) + const msg = preserveStructure + ? `Copied '${sourcePath}' to ${basename(fullPath)}\n` + : `Copied contents of '${sourcePath}' to output root\n` + options.stdout.write(msg) + return copied.length + }), + ) + return counts.reduce((sum, count) => sum + count, 0) +} + +/** + * Splits a path into tokens. A token with `flatten: true` (the `[]` suffix) + * signals that an array is expected at that position and the result should be + * flattened one level before continuing. Plain tokens preserve whatever shape + * is already in flight — if the current value is already an array (from a + * prior flatten), the field is plucked from each element automatically. + * + * Examples: + * "tools" → [name:"tools", flatten:false] + * "targeting.tools" → [name:"targeting",...], [name:"tools",...] + * "extensions[].targeting[].schema" → [name:"extensions", flatten:true], ... + */ +function tokenizePath(path: string): {name: string; flatten: boolean}[] { + return path.split('.').map((part) => { + const flatten = part.endsWith('[]') + return {name: flatten ? part.slice(0, -2) : part, flatten} + }) +} + +/** + * Resolves a dot-separated path (with optional `[]` flatten markers) from a + * config object. + * + * - Plain segments (`targeting.tools`): dot-notation access; when the current + * value is already an array (due to a prior flatten), the field is plucked + * from every element automatically. + * - Flatten segments (`extensions[]`): access the field and flatten one level + * of nesting. Returns `undefined` if the value at that point is not an array + * — the `[]` suffix is a contract that an array is expected there. + */ +function getNestedValue(obj: {[key: string]: unknown}, path: string): unknown { + let current: unknown = obj + + for (const {name, flatten} of tokenizePath(path)) { + if (current === null || current === undefined) return undefined + + if (Array.isArray(current)) { + const plucked = current + .map((item) => + item !== null && typeof item === 'object' ? (item as {[key: string]: unknown})[name] : undefined, + ) + .filter((val): val is NonNullable => val !== undefined) + current = plucked.length > 0 ? plucked : undefined + } else if (typeof current === 'object') { + current = (current as {[key: string]: unknown})[name] + } else { + return undefined + } + + if (flatten) { + if (!Array.isArray(current)) return undefined + current = (current as unknown[]).flat(1) + } + } + + return current +} 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 new file mode 100644 index 00000000000..5514cd564e5 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts @@ -0,0 +1,57 @@ +import {joinPath, dirname, basename} from '@shopify/cli-kit/node/path' +import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs' + +/** + * Handles a `{source}` or `{source, destination}` files entry. + * + * - No `destination`, `preserveStructure` false: copy directory contents into the output root. + * - No `destination`, `preserveStructure` true: copy the directory under its own name in the output. + * - With `destination`: copy to that exact path (`preserveStructure` is ignored). + */ +export async function copySourceEntry( + config: { + source: string + destination: string | undefined + baseDir: string + outputDir: string + preserveStructure: boolean + }, + options: {stdout: NodeJS.WritableStream}, +): Promise { + const {source, destination, baseDir, outputDir, preserveStructure} = config + const sourcePath = joinPath(baseDir, source) + if (!(await fileExists(sourcePath))) { + throw new Error(`Source does not exist: ${sourcePath}`) + } + + const sourceIsDir = await isDirectory(sourcePath) + + // Resolve destination path and log message up front, then dispatch on file vs directory. + let destPath: string + let logMsg: string + if (destination !== undefined) { + destPath = joinPath(outputDir, destination) + logMsg = `Copied ${source} to ${destination}\n` + } else if (sourceIsDir && preserveStructure) { + destPath = joinPath(outputDir, basename(sourcePath)) + logMsg = `Copied ${source} to ${basename(sourcePath)}\n` + } else if (sourceIsDir) { + destPath = outputDir + logMsg = `Copied contents of ${source} to output root\n` + } else { + destPath = joinPath(outputDir, basename(sourcePath)) + logMsg = `Copied ${source} to ${basename(sourcePath)}\n` + } + + if (sourceIsDir) { + await copyDirectoryContents(sourcePath, destPath) + const copied = await glob(['**/*'], {cwd: destPath, absolute: false}) + options.stdout.write(logMsg) + return copied.length + } + + await mkdir(dirname(destPath)) + await copyFile(sourcePath, destPath) + options.stdout.write(logMsg) + return 1 +} From af26a23053530b2cd4f6e95de7d475ec423b07a2 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Tue, 24 Mar 2026 20:48:51 +0100 Subject: [PATCH 08/13] Fix reduce type annotation after include-assets split counts is (number | undefined)[] due to TypeScript not seeing the three if-checks as exhaustive. Explicit generic on reduce fixes it. Co-Authored-By: Claude Sonnet 4.6 --- .../app/src/cli/services/build/steps/include-assets-step.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8893edabd01..2703be0cb53 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 @@ -134,5 +134,5 @@ export async function executeIncludeAssetsStep( }), ) - return {filesCopied: counts.reduce((sum, count) => sum + (count ?? 0), 0)} + return {filesCopied: counts.reduce((sum, count) => sum + (count ?? 0), 0)} } From 0a082097ab0790d61163642e29a4bb4ca50edb41 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Tue, 24 Mar 2026 20:54:33 +0100 Subject: [PATCH 09/13] Add unit tests for each include-assets strategy Direct tests for copyByPattern, copySourceEntry, and copyConfigKeyEntry covering edge cases not exercised through the orchestrator tests: collision deduplication, path traversal guard, filepath===destPath no-op, all destination resolution branches, file vs directory sources, destination param, nested [] flatten paths. Co-Authored-By: Claude Sonnet 4.6 --- .../include-assets/copy-by-pattern.test.ts | 215 ++++++++++++++++++ .../copy-config-key-entry.test.ts | 200 ++++++++++++++++ .../include-assets/copy-source-entry.test.ts | 156 +++++++++++++ 3 files changed, 571 insertions(+) create mode 100644 packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts create mode 100644 packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts create mode 100644 packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts 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 new file mode 100644 index 00000000000..c309dd1a837 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts @@ -0,0 +1,215 @@ +import {copyByPattern} from './copy-by-pattern.js' +import {describe, expect, test, vi, beforeEach} from 'vitest' +import * as fs from '@shopify/cli-kit/node/fs' + +vi.mock('@shopify/cli-kit/node/fs') + +describe('copyByPattern', () => { + let mockStdout: any + + beforeEach(() => { + mockStdout = {write: vi.fn()} + }) + + test('copies matched files preserving relative paths when preserveStructure is true', async () => { + // 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: [], + preserveStructure: true, + }, + {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).toBe(2) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied 2 file(s)')) + }) + + test('flattens files to basename when preserveStructure is false', async () => { + // Given + vi.mocked(fs.glob).mockResolvedValue([ + '/src/components/Button.tsx', + '/src/utils/helper.ts', + ]) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copyByPattern( + { + sourceDir: '/src', + outputDir: '/out', + patterns: ['**/*'], + ignore: [], + preserveStructure: false, + }, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyFile).toHaveBeenCalledWith('/src/components/Button.tsx', '/out/Button.tsx') + expect(fs.copyFile).toHaveBeenCalledWith('/src/utils/helper.ts', '/out/helper.ts') + expect(result).toBe(2) + }) + + test('warns and lets last-in-array win when flattening produces basename collision', async () => { + // Given — two files with the same basename in different directories + vi.mocked(fs.glob).mockResolvedValue([ + '/src/a/index.js', + '/src/b/index.js', + ]) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copyByPattern( + { + sourceDir: '/src', + outputDir: '/out', + patterns: ['**/index.js'], + ignore: [], + preserveStructure: false, + }, + {stdout: mockStdout}, + ) + + // Then — collision warning emitted, only the last one is copied + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('filename collision detected')) + expect(fs.copyFile).toHaveBeenCalledTimes(1) + expect(fs.copyFile).toHaveBeenCalledWith('/src/b/index.js', '/out/index.js') + expect(result).toBe(1) + }) + + test('warns and returns 0 when no files match patterns', async () => { + // Given + vi.mocked(fs.glob).mockResolvedValue([]) + + // When + const result = await copyByPattern( + { + sourceDir: '/src', + outputDir: '/out', + patterns: ['**/*.png'], + ignore: [], + preserveStructure: true, + }, + {stdout: mockStdout}, + ) + + // Then + expect(result).toBe(0) + expect(fs.mkdir).not.toHaveBeenCalled() + expect(fs.copyFile).not.toHaveBeenCalled() + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('No files matched patterns')) + }) + + test('skips file and warns when resolved destination escapes the output directory', async () => { + // 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: [], + preserveStructure: true, + }, + {stdout: mockStdout}, + ) + + // Then + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining('skipping'), + ) + expect(fs.copyFile).not.toHaveBeenCalled() + expect(result).toBe(0) + }) + + test('returns 0 without copying when filepath equals computed destPath', async () => { + // 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 basename resolves to /out/logo.png == filepath + const result = await copyByPattern( + { + sourceDir: '/out', + outputDir: '/out', + patterns: ['*.png'], + ignore: [], + preserveStructure: false, + }, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyFile).not.toHaveBeenCalled() + expect(result).toBe(0) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied 0 file(s)')) + }) + + test('calls mkdir(outputDir) before copying when files are matched', async () => { + // 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: [], + preserveStructure: false, + }, + {stdout: mockStdout}, + ) + + // Then — outputDir created before copying + expect(fs.mkdir).toHaveBeenCalledWith('/out/dist') + }) + + test('passes ignore patterns to glob', async () => { + // Given + vi.mocked(fs.glob).mockResolvedValue([]) + + // When + await copyByPattern( + { + sourceDir: '/src', + outputDir: '/out', + patterns: ['**/*'], + ignore: ['**/*.test.ts', 'node_modules/**'], + preserveStructure: true, + }, + {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-config-key-entry.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts new file mode 100644 index 00000000000..cc9ade9d702 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts @@ -0,0 +1,200 @@ +import {copyConfigKeyEntry} from './copy-config-key-entry.js' +import {describe, expect, test, vi, beforeEach} from 'vitest' +import * as fs from '@shopify/cli-kit/node/fs' + +vi.mock('@shopify/cli-kit/node/fs') + +const makeContext = (configuration: Record) => ({ + extension: {configuration} as any, + options: {} as any, + stepResults: new Map(), +}) + +describe('copyConfigKeyEntry', () => { + let mockStdout: any + + beforeEach(() => { + mockStdout = {write: vi.fn()} + }) + + test('merges directory contents into output root by default (preserveStructure false)', async () => { + // Given + const context = makeContext({static_root: 'public'}) + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['index.html', 'logo.png']) + + // When + const result = await copyConfigKeyEntry( + {key: 'static_root', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/public', '/out') + expect(result).toBe(2) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied contents of')) + }) + + test('places directory under its own name when preserveStructure is true', async () => { + // Given + const context = makeContext({theme_root: 'theme'}) + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['style.css', 'layout.liquid']) + + // When + const result = await copyConfigKeyEntry( + {key: 'theme_root', baseDir: '/ext', outputDir: '/out', context, preserveStructure: true}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/theme', '/out/theme') + expect(result).toBe(2) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Copied 'theme' to theme")) + }) + + test('copies a file source to outputDir/basename', async () => { + // Given + const context = makeContext({schema_path: 'src/schema.json'}) + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copyConfigKeyEntry( + {key: 'schema_path', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyFile).toHaveBeenCalledWith('/ext/src/schema.json', '/out/schema.json') + expect(result).toBe(1) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Copied 'src/schema.json' to schema.json")) + }) + + test('skips with log message when configKey is absent from configuration', async () => { + // Given + const context = makeContext({}) + + // When + const result = await copyConfigKeyEntry( + {key: 'static_root', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then + expect(result).toBe(0) + expect(fs.fileExists).not.toHaveBeenCalled() + expect(mockStdout.write).toHaveBeenCalledWith("No value for configKey 'static_root', skipping\n") + }) + + test('skips with warning when path resolved from config does not exist on disk', async () => { + // Given + const context = makeContext({assets_dir: 'nonexistent'}) + vi.mocked(fs.fileExists).mockResolvedValue(false) + + // When + const result = await copyConfigKeyEntry( + {key: 'assets_dir', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then + expect(result).toBe(0) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining("Warning: path 'nonexistent' does not exist"), + ) + }) + + test('resolves array config value and copies each path, summing results', async () => { + // Given + const context = makeContext({roots: ['public', 'assets']}) + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + // Each directory listing returns 2 files + vi.mocked(fs.glob).mockResolvedValue(['a.html', 'b.html']) + + // When + const result = await copyConfigKeyEntry( + {key: 'roots', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/public', '/out') + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/assets', '/out') + expect(result).toBe(4) + }) + + test('prefixes outputDir with destination when destination param is provided', async () => { + // Given + const context = makeContext({icons_dir: 'icons'}) + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['icon.svg']) + + // When + await copyConfigKeyEntry( + {key: 'icons_dir', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false, destination: 'static/icons'}, + {stdout: mockStdout}, + ) + + // Then — effectiveOutputDir is /out/static/icons + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/icons', '/out/static/icons') + }) + + test('resolves nested [] flatten path and collects all leaf values', async () => { + // Given — extensions[].targeting[].schema pattern + const context = makeContext({ + extensions: [ + {targeting: [{schema: 'schema-a.json'}, {schema: 'schema-b.json'}]}, + {targeting: [{schema: 'schema-c.json'}]}, + ], + }) + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copyConfigKeyEntry( + {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then — all three schemas copied + expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-a.json', '/out/schema-a.json') + expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-b.json', '/out/schema-b.json') + expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-c.json', '/out/schema-c.json') + expect(result).toBe(3) + }) + + test('skips with no-value log when [] flatten resolves to a non-array (contract violated)', async () => { + // Given — extensions is a plain object, not an array, so [] contract fails + const context = makeContext({ + extensions: {targeting: {schema: 'schema.json'}}, + }) + + // When + const result = await copyConfigKeyEntry( + {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then — getNestedValue returns undefined, treated as absent key + expect(result).toBe(0) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + expect(fs.copyFile).not.toHaveBeenCalled() + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining("No value for configKey 'extensions[].targeting[].schema'"), + ) + }) +}) 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 new file mode 100644 index 00000000000..6219ac3ff06 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts @@ -0,0 +1,156 @@ +import {copySourceEntry} from './copy-source-entry.js' +import {describe, expect, test, vi, beforeEach} from 'vitest' +import * as fs from '@shopify/cli-kit/node/fs' + +vi.mock('@shopify/cli-kit/node/fs') + +describe('copySourceEntry', () => { + let mockStdout: any + + beforeEach(() => { + mockStdout = {write: vi.fn()} + }) + + test('throws when source path does not exist', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(false) + + // When / Then + await expect( + copySourceEntry( + {source: 'missing/file.js', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + {stdout: mockStdout}, + ), + ).rejects.toThrow('Source does not exist: /ext/missing/file.js') + }) + + test('copies file to explicit destination path (preserveStructure ignored)', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copySourceEntry( + {source: 'src/icon.png', destination: 'assets/icon.png', baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyFile).toHaveBeenCalledWith('/ext/src/icon.png', '/out/assets/icon.png') + expect(result).toBe(1) + expect(mockStdout.write).toHaveBeenCalledWith('Copied src/icon.png to assets/icon.png\n') + }) + + test('copies directory under its own name when no destination and preserveStructure is true', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['index.html', 'logo.png']) + + // When + const result = await copySourceEntry( + {source: 'dist', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: true}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/dist', '/out/dist') + expect(result).toBe(2) + expect(mockStdout.write).toHaveBeenCalledWith('Copied dist to dist\n') + }) + + test('merges directory contents into output root when no destination and preserveStructure is false', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['a.js', 'b.js', 'c.js']) + + // When + const result = await copySourceEntry( + {source: 'build', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/build', '/out') + expect(result).toBe(3) + expect(mockStdout.write).toHaveBeenCalledWith('Copied contents of build to output root\n') + }) + + test('copies file to basename in outputDir when source is a file and no destination given', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copySourceEntry( + {source: 'README.md', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: true}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyFile).toHaveBeenCalledWith('/ext/README.md', '/out/README.md') + expect(result).toBe(1) + expect(mockStdout.write).toHaveBeenCalledWith('Copied README.md to README.md\n') + }) + + test('copies directory to explicit destination path, ignoring preserveStructure', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['x.js']) + + // When + const result = await copySourceEntry( + {source: 'dist', destination: 'vendor/dist', baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/dist', '/out/vendor/dist') + expect(result).toBe(1) + expect(mockStdout.write).toHaveBeenCalledWith('Copied dist to vendor/dist\n') + }) + + test('returns count of files discovered in destination directory after directory copy', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + // Simulate 5 files inside the copied directory + vi.mocked(fs.glob).mockResolvedValue(['a.js', 'b.js', 'c.js', 'd.js', 'e.js']) + + // When + const result = await copySourceEntry( + {source: 'theme', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then — count comes from glob on destPath, not a constant + expect(result).toBe(5) + }) + + test('creates parent directories before copying a file', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + await copySourceEntry( + {source: 'src/deep/icon.png', destination: 'assets/icons/icon.png', baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then — parent of destination path created + expect(fs.mkdir).toHaveBeenCalledWith('/out/assets/icons') + }) +}) From ad3fc546caffaf946553840e7383d92e8a943132 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Tue, 24 Mar 2026 21:34:41 +0100 Subject: [PATCH 10/13] Remove preserveStructure option from include_assets step --- .../include-assets/copy-by-pattern.test.ts | 19 ++++----------- .../copy-config-key-entry.test.ts | 13 ++++++---- .../include-assets/copy-source-entry.test.ts | 24 ++++++++++++++++--- 3 files changed, 34 insertions(+), 22 deletions(-) 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 c309dd1a837..9e4cb821b36 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 @@ -13,10 +13,7 @@ describe('copyByPattern', () => { test('copies matched files preserving relative paths when preserveStructure is true', async () => { // Given - vi.mocked(fs.glob).mockResolvedValue([ - '/src/components/Button.tsx', - '/src/utils/helpers.ts', - ]) + vi.mocked(fs.glob).mockResolvedValue(['/src/components/Button.tsx', '/src/utils/helpers.ts']) vi.mocked(fs.mkdir).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() @@ -41,10 +38,7 @@ describe('copyByPattern', () => { test('flattens files to basename when preserveStructure is false', async () => { // Given - vi.mocked(fs.glob).mockResolvedValue([ - '/src/components/Button.tsx', - '/src/utils/helper.ts', - ]) + vi.mocked(fs.glob).mockResolvedValue(['/src/components/Button.tsx', '/src/utils/helper.ts']) vi.mocked(fs.mkdir).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() @@ -68,10 +62,7 @@ describe('copyByPattern', () => { test('warns and lets last-in-array win when flattening produces basename collision', async () => { // Given — two files with the same basename in different directories - vi.mocked(fs.glob).mockResolvedValue([ - '/src/a/index.js', - '/src/b/index.js', - ]) + vi.mocked(fs.glob).mockResolvedValue(['/src/a/index.js', '/src/b/index.js']) vi.mocked(fs.mkdir).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() @@ -137,9 +128,7 @@ describe('copyByPattern', () => { ) // Then - expect(mockStdout.write).toHaveBeenCalledWith( - expect.stringContaining('skipping'), - ) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('skipping')) expect(fs.copyFile).not.toHaveBeenCalled() expect(result).toBe(0) }) 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 cc9ade9d702..0850be617cc 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 @@ -107,9 +107,7 @@ describe('copyConfigKeyEntry', () => { // Then expect(result).toBe(0) expect(fs.copyDirectoryContents).not.toHaveBeenCalled() - expect(mockStdout.write).toHaveBeenCalledWith( - expect.stringContaining("Warning: path 'nonexistent' does not exist"), - ) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Warning: path 'nonexistent' does not exist")) }) test('resolves array config value and copies each path, summing results', async () => { @@ -143,7 +141,14 @@ describe('copyConfigKeyEntry', () => { // When await copyConfigKeyEntry( - {key: 'icons_dir', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false, destination: 'static/icons'}, + { + key: 'icons_dir', + baseDir: '/ext', + outputDir: '/out', + context, + preserveStructure: false, + destination: 'static/icons', + }, {stdout: mockStdout}, ) 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 6219ac3ff06..3164f5bbf7b 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 @@ -18,7 +18,13 @@ describe('copySourceEntry', () => { // When / Then await expect( copySourceEntry( - {source: 'missing/file.js', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + { + source: 'missing/file.js', + destination: undefined, + baseDir: '/ext', + outputDir: '/out', + preserveStructure: false, + }, {stdout: mockStdout}, ), ).rejects.toThrow('Source does not exist: /ext/missing/file.js') @@ -33,7 +39,13 @@ describe('copySourceEntry', () => { // When const result = await copySourceEntry( - {source: 'src/icon.png', destination: 'assets/icon.png', baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + { + source: 'src/icon.png', + destination: 'assets/icon.png', + baseDir: '/ext', + outputDir: '/out', + preserveStructure: false, + }, {stdout: mockStdout}, ) @@ -146,7 +158,13 @@ describe('copySourceEntry', () => { // When await copySourceEntry( - {source: 'src/deep/icon.png', destination: 'assets/icons/icon.png', baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + { + source: 'src/deep/icon.png', + destination: 'assets/icons/icon.png', + baseDir: '/ext', + outputDir: '/out', + preserveStructure: false, + }, {stdout: mockStdout}, ) From a59eae82f041cbe5bbb74db4389d8dfa1cb45f63 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Wed, 25 Mar 2026 14:13:23 +0100 Subject: [PATCH 11/13] Remove preserveStructure option from include_assets step Always preserve directory structure for pattern/static entries (previous default:true), always merge directory contents for configKey entries (previous default:false). Removes ~230 lines of dead code and 5 tests covering removed behavior. Co-Authored-By: Claude Sonnet 4.6 --- .../build/steps/include-assets-step.test.ts | 83 +------------------ .../build/steps/include-assets-step.ts | 16 +--- .../include-assets/copy-by-pattern.test.ts | 59 +------------ .../steps/include-assets/copy-by-pattern.ts | 37 +-------- .../copy-config-key-entry.test.ts | 37 ++------- .../include-assets/copy-config-key-entry.ts | 13 +-- .../include-assets/copy-source-entry.test.ts | 36 ++------ .../steps/include-assets/copy-source-entry.ts | 14 +--- 8 files changed, 32 insertions(+), 263 deletions(-) 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 1c166f0a1fa..3096030ca37 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 @@ -33,7 +33,7 @@ describe('executeIncludeAssetsStep', () => { }) describe('static entries', () => { - test('copies directory contents to output root when no destination and preserveStructure is false', async () => { + test('copies directory under its own name when no destination is given', async () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) vi.mocked(fs.isDirectory).mockResolvedValue(true) @@ -45,32 +45,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Copy Dist', type: 'include_assets', config: { - inclusions: [{type: 'static', source: 'dist', preserveStructure: false}], - }, - } - - // When - const result = await executeIncludeAssetsStep(step, mockContext) - - // Then - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output') - expect(result.filesCopied).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied contents of dist to output root')) - }) - - test('preserves directory name when preserveStructure is true', async () => { - // Given - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) - vi.mocked(fs.copyDirectoryContents).mockResolvedValue() - vi.mocked(fs.glob).mockResolvedValue(['index.html', 'assets/logo.png']) - - const step: LifecycleStep = { - id: 'copy-dist', - name: 'Copy Dist', - type: 'include_assets', - config: { - inclusions: [{type: 'static', source: 'dist', preserveStructure: true}], + inclusions: [{type: 'static', source: 'dist'}], }, } @@ -255,38 +230,6 @@ describe('executeIncludeAssetsStep', () => { expect(result.filesCopied).toBe(2) }) - test('preserves directory name for configKey when preserveStructure is true', async () => { - // Given - const contextWithConfig = { - ...mockContext, - extension: { - ...mockExtension, - configuration: {static_root: 'public'}, - } as unknown as ExtensionInstance, - } - - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) - vi.mocked(fs.copyDirectoryContents).mockResolvedValue() - vi.mocked(fs.glob).mockResolvedValue(['index.html', 'logo.png']) - - const step: LifecycleStep = { - id: 'copy-static', - name: 'Copy Static', - type: 'include_assets', - config: { - inclusions: [{type: 'configKey', key: 'static_root', preserveStructure: true}], - }, - } - - // When - const result = await executeIncludeAssetsStep(step, contextWithConfig) - - // Then — directory is placed under its own name - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/public', '/test/output/public') - expect(result.filesCopied).toBe(2) - }) - test('skips silently when configKey is absent from config', async () => { // Given — configuration has no static_root const contextWithoutConfig = { @@ -573,28 +516,6 @@ describe('executeIncludeAssetsStep', () => { expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/public/logo.png', '/test/output/static/logo.png') }) - test('flattens files when preserveStructure is false', async () => { - // Given - vi.mocked(fs.glob).mockResolvedValue(['/test/extension/src/components/Button.tsx']) - vi.mocked(fs.copyFile).mockResolvedValue() - vi.mocked(fs.mkdir).mockResolvedValue() - - const step: LifecycleStep = { - id: 'copy-source', - name: 'Copy Source', - type: 'include_assets', - config: { - inclusions: [{type: 'pattern', baseDir: 'src', preserveStructure: false}], - }, - } - - // When - await executeIncludeAssetsStep(step, mockContext) - - // Then — filename only, no subdirectory - expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/components/Button.tsx', '/test/output/Button.tsx') - }) - test('returns zero and warns when no files match', async () => { // Given vi.mocked(fs.glob).mockResolvedValue([]) 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 2703be0cb53..958eb6996d1 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 @@ -10,7 +10,6 @@ import type {LifecycleStep, BuildContext} from '../client-steps.js' * * Selects files from a source directory using glob patterns. `source` defaults * to the extension root when omitted. `include` defaults to `['**\/*']`. - * `preserveStructure` defaults to `true` (relative paths preserved). */ const PatternEntrySchema = z.object({ type: z.literal('pattern'), @@ -18,23 +17,17 @@ const PatternEntrySchema = z.object({ include: z.array(z.string()).default(['**/*']), ignore: z.array(z.string()).optional(), destination: z.string().optional(), - preserveStructure: z.boolean().default(true), }) /** * Static inclusion entry — explicit source path. * - * - With `destination`: copies the file/directory to that exact path. - * - Without `destination`, `preserveStructure` false (default): merges - * directory contents into the output root. - * - Without `destination`, `preserveStructure` true: places the directory - * under its own name in the output. + * Copies source to destination. Without `destination`, copies directory under its own name in the output. */ const StaticEntrySchema = z.object({ type: z.literal('static'), source: z.string(), destination: z.string().optional(), - preserveStructure: z.boolean().default(true), }) /** @@ -42,14 +35,12 @@ const StaticEntrySchema = z.object({ * * Resolves a path (or array of paths) from the extension configuration and * copies the directory contents into the output. Silently skipped when the - * key is absent. Respects `preserveStructure` and `destination` the same way - * as the static entry. + * key is absent. */ const ConfigKeyEntrySchema = z.object({ type: z.literal('configKey'), key: z.string(), destination: z.string().optional(), - preserveStructure: z.boolean().default(false), }) const InclusionEntrySchema = z.discriminatedUnion('type', [PatternEntrySchema, StaticEntrySchema, ConfigKeyEntrySchema]) @@ -99,7 +90,6 @@ export async function executeIncludeAssetsStep( outputDir: destinationDir, patterns: entry.include, ignore: entry.ignore ?? [], - preserveStructure: entry.preserveStructure, }, options, ) @@ -112,7 +102,6 @@ export async function executeIncludeAssetsStep( baseDir: extension.directory, outputDir, context, - preserveStructure: entry.preserveStructure, destination: sanitizedDest, }, options, @@ -126,7 +115,6 @@ export async function executeIncludeAssetsStep( destination: sanitizedDest, baseDir: extension.directory, outputDir, - preserveStructure: entry.preserveStructure, }, options, ) 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 9e4cb821b36..a93682b429d 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 @@ -11,7 +11,7 @@ describe('copyByPattern', () => { mockStdout = {write: vi.fn()} }) - test('copies matched files preserving relative paths when preserveStructure is true', async () => { + test('copies matched files preserving relative paths', async () => { // Given vi.mocked(fs.glob).mockResolvedValue(['/src/components/Button.tsx', '/src/utils/helpers.ts']) vi.mocked(fs.mkdir).mockResolvedValue() @@ -24,7 +24,6 @@ describe('copyByPattern', () => { outputDir: '/out', patterns: ['**/*.ts', '**/*.tsx'], ignore: [], - preserveStructure: true, }, {stdout: mockStdout}, ) @@ -36,55 +35,6 @@ describe('copyByPattern', () => { expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied 2 file(s)')) }) - test('flattens files to basename when preserveStructure is false', async () => { - // Given - vi.mocked(fs.glob).mockResolvedValue(['/src/components/Button.tsx', '/src/utils/helper.ts']) - vi.mocked(fs.mkdir).mockResolvedValue() - vi.mocked(fs.copyFile).mockResolvedValue() - - // When - const result = await copyByPattern( - { - sourceDir: '/src', - outputDir: '/out', - patterns: ['**/*'], - ignore: [], - preserveStructure: false, - }, - {stdout: mockStdout}, - ) - - // Then - expect(fs.copyFile).toHaveBeenCalledWith('/src/components/Button.tsx', '/out/Button.tsx') - expect(fs.copyFile).toHaveBeenCalledWith('/src/utils/helper.ts', '/out/helper.ts') - expect(result).toBe(2) - }) - - test('warns and lets last-in-array win when flattening produces basename collision', async () => { - // Given — two files with the same basename in different directories - vi.mocked(fs.glob).mockResolvedValue(['/src/a/index.js', '/src/b/index.js']) - vi.mocked(fs.mkdir).mockResolvedValue() - vi.mocked(fs.copyFile).mockResolvedValue() - - // When - const result = await copyByPattern( - { - sourceDir: '/src', - outputDir: '/out', - patterns: ['**/index.js'], - ignore: [], - preserveStructure: false, - }, - {stdout: mockStdout}, - ) - - // Then — collision warning emitted, only the last one is copied - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('filename collision detected')) - expect(fs.copyFile).toHaveBeenCalledTimes(1) - expect(fs.copyFile).toHaveBeenCalledWith('/src/b/index.js', '/out/index.js') - expect(result).toBe(1) - }) - test('warns and returns 0 when no files match patterns', async () => { // Given vi.mocked(fs.glob).mockResolvedValue([]) @@ -96,7 +46,6 @@ describe('copyByPattern', () => { outputDir: '/out', patterns: ['**/*.png'], ignore: [], - preserveStructure: true, }, {stdout: mockStdout}, ) @@ -122,7 +71,6 @@ describe('copyByPattern', () => { outputDir: '/out/sub', patterns: ['**/*'], ignore: [], - preserveStructure: true, }, {stdout: mockStdout}, ) @@ -139,14 +87,13 @@ describe('copyByPattern', () => { vi.mocked(fs.mkdir).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() - // When — sourceDir is /out so basename resolves to /out/logo.png == filepath + // 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: [], - preserveStructure: false, }, {stdout: mockStdout}, ) @@ -170,7 +117,6 @@ describe('copyByPattern', () => { outputDir: '/out/dist', patterns: ['*.js'], ignore: [], - preserveStructure: false, }, {stdout: mockStdout}, ) @@ -190,7 +136,6 @@ describe('copyByPattern', () => { outputDir: '/out', patterns: ['**/*'], ignore: ['**/*.test.ts', 'node_modules/**'], - preserveStructure: true, }, {stdout: mockStdout}, ) 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 b0110d6aa5d..6b1af854e5e 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,4 +1,4 @@ -import {joinPath, dirname, relativePath, basename} from '@shopify/cli-kit/node/path' +import {joinPath, dirname, relativePath} from '@shopify/cli-kit/node/path' import {glob, copyFile, mkdir} from '@shopify/cli-kit/node/fs' /** @@ -10,11 +10,10 @@ export async function copyByPattern( outputDir: string patterns: string[] ignore: string[] - preserveStructure: boolean }, options: {stdout: NodeJS.WritableStream}, ): Promise { - const {sourceDir, outputDir, patterns, ignore, preserveStructure} = config + const {sourceDir, outputDir, patterns, ignore} = config const files = await glob(patterns, { absolute: true, cwd: sourceDir, @@ -28,39 +27,11 @@ export async function copyByPattern( await mkdir(outputDir) - const duplicates = new Set() - if (!preserveStructure) { - const basenames = files.map((fp) => basename(fp)) - const seen = new Set() - for (const name of basenames) { - if (seen.has(name)) { - duplicates.add(name) - } else { - seen.add(name) - } - } - if (duplicates.size > 0) { - const colliding = files.filter((fp) => duplicates.has(basename(fp))) - options.stdout.write( - `Warning: filename collision detected when flattening — the following files share a basename and will overwrite each other: ${colliding.join(', ')}\n`, - ) - } - } - - // When flattening and collisions exist, deduplicate so last-in-array deterministically wins - const filesToCopy = - !preserveStructure && duplicates.size > 0 - ? files.filter((fp, idx) => { - const name = basename(fp) - if (!duplicates.has(name)) return true - const lastIdx = files.reduce((last, file, ii) => (basename(file) === name ? ii : last), -1) - return lastIdx === idx - }) - : files + const filesToCopy = files const copyResults = await Promise.all( filesToCopy.map(async (filepath): Promise => { - const relPath = preserveStructure ? relativePath(sourceDir, filepath) : basename(filepath) + const relPath = relativePath(sourceDir, filepath) const destPath = joinPath(outputDir, relPath) if (relativePath(outputDir, destPath).startsWith('..')) { 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 0850be617cc..6ba87ea0c25 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 @@ -17,7 +17,7 @@ describe('copyConfigKeyEntry', () => { mockStdout = {write: vi.fn()} }) - test('merges directory contents into output root by default (preserveStructure false)', async () => { + test('merges directory contents into output root', async () => { // Given const context = makeContext({static_root: 'public'}) vi.mocked(fs.fileExists).mockResolvedValue(true) @@ -27,7 +27,7 @@ describe('copyConfigKeyEntry', () => { // When const result = await copyConfigKeyEntry( - {key: 'static_root', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {key: 'static_root', baseDir: '/ext', outputDir: '/out', context}, {stdout: mockStdout}, ) @@ -37,26 +37,6 @@ describe('copyConfigKeyEntry', () => { expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied contents of')) }) - test('places directory under its own name when preserveStructure is true', async () => { - // Given - const context = makeContext({theme_root: 'theme'}) - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) - vi.mocked(fs.copyDirectoryContents).mockResolvedValue() - vi.mocked(fs.glob).mockResolvedValue(['style.css', 'layout.liquid']) - - // When - const result = await copyConfigKeyEntry( - {key: 'theme_root', baseDir: '/ext', outputDir: '/out', context, preserveStructure: true}, - {stdout: mockStdout}, - ) - - // Then - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/theme', '/out/theme') - expect(result).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Copied 'theme' to theme")) - }) - test('copies a file source to outputDir/basename', async () => { // Given const context = makeContext({schema_path: 'src/schema.json'}) @@ -67,7 +47,7 @@ describe('copyConfigKeyEntry', () => { // When const result = await copyConfigKeyEntry( - {key: 'schema_path', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {key: 'schema_path', baseDir: '/ext', outputDir: '/out', context}, {stdout: mockStdout}, ) @@ -83,7 +63,7 @@ describe('copyConfigKeyEntry', () => { // When const result = await copyConfigKeyEntry( - {key: 'static_root', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {key: 'static_root', baseDir: '/ext', outputDir: '/out', context}, {stdout: mockStdout}, ) @@ -100,7 +80,7 @@ describe('copyConfigKeyEntry', () => { // When const result = await copyConfigKeyEntry( - {key: 'assets_dir', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {key: 'assets_dir', baseDir: '/ext', outputDir: '/out', context}, {stdout: mockStdout}, ) @@ -121,7 +101,7 @@ describe('copyConfigKeyEntry', () => { // When const result = await copyConfigKeyEntry( - {key: 'roots', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {key: 'roots', baseDir: '/ext', outputDir: '/out', context}, {stdout: mockStdout}, ) @@ -146,7 +126,6 @@ describe('copyConfigKeyEntry', () => { baseDir: '/ext', outputDir: '/out', context, - preserveStructure: false, destination: 'static/icons', }, {stdout: mockStdout}, @@ -171,7 +150,7 @@ describe('copyConfigKeyEntry', () => { // When const result = await copyConfigKeyEntry( - {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context}, {stdout: mockStdout}, ) @@ -190,7 +169,7 @@ describe('copyConfigKeyEntry', () => { // When const result = await copyConfigKeyEntry( - {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context}, {stdout: mockStdout}, ) 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 41880a179c8..4241a73787a 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 @@ -16,12 +16,11 @@ export async function copyConfigKeyEntry( baseDir: string outputDir: string context: BuildContext - preserveStructure: boolean destination?: string }, options: {stdout: NodeJS.WritableStream}, ): Promise { - const {key, baseDir, outputDir, context, preserveStructure, destination} = config + const {key, baseDir, outputDir, context, destination} = config const value = getNestedValue(context.extension.configuration, key) let paths: string[] if (typeof value === 'string') { @@ -54,13 +53,9 @@ export async function copyConfigKeyEntry( options.stdout.write(`Copied '${sourcePath}' to ${basename(fullPath)}\n`) return 1 } - const destDir = preserveStructure ? joinPath(effectiveOutputDir, basename(fullPath)) : effectiveOutputDir - await copyDirectoryContents(fullPath, destDir) - const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) - const msg = preserveStructure - ? `Copied '${sourcePath}' to ${basename(fullPath)}\n` - : `Copied contents of '${sourcePath}' to output root\n` - options.stdout.write(msg) + await copyDirectoryContents(fullPath, effectiveOutputDir) + const copied = await glob(['**/*'], {cwd: effectiveOutputDir, absolute: false}) + options.stdout.write(`Copied contents of '${sourcePath}' to output root\n`) return copied.length }), ) 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 3164f5bbf7b..e561fc62d22 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,14 +23,13 @@ describe('copySourceEntry', () => { destination: undefined, baseDir: '/ext', outputDir: '/out', - preserveStructure: false, }, {stdout: mockStdout}, ), ).rejects.toThrow('Source does not exist: /ext/missing/file.js') }) - test('copies file to explicit destination path (preserveStructure ignored)', async () => { + test('copies file to explicit destination path', async () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) vi.mocked(fs.isDirectory).mockResolvedValue(false) @@ -44,7 +43,6 @@ describe('copySourceEntry', () => { destination: 'assets/icon.png', baseDir: '/ext', outputDir: '/out', - preserveStructure: false, }, {stdout: mockStdout}, ) @@ -55,7 +53,7 @@ describe('copySourceEntry', () => { expect(mockStdout.write).toHaveBeenCalledWith('Copied src/icon.png to assets/icon.png\n') }) - test('copies directory under its own name when no destination and preserveStructure is true', async () => { + test('copies directory under its own name when no destination is given', async () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) vi.mocked(fs.isDirectory).mockResolvedValue(true) @@ -64,7 +62,7 @@ describe('copySourceEntry', () => { // When const result = await copySourceEntry( - {source: 'dist', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: true}, + {source: 'dist', destination: undefined, baseDir: '/ext', outputDir: '/out'}, {stdout: mockStdout}, ) @@ -74,25 +72,6 @@ describe('copySourceEntry', () => { expect(mockStdout.write).toHaveBeenCalledWith('Copied dist to dist\n') }) - test('merges directory contents into output root when no destination and preserveStructure is false', async () => { - // Given - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) - vi.mocked(fs.copyDirectoryContents).mockResolvedValue() - vi.mocked(fs.glob).mockResolvedValue(['a.js', 'b.js', 'c.js']) - - // When - const result = await copySourceEntry( - {source: 'build', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: false}, - {stdout: mockStdout}, - ) - - // Then - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/build', '/out') - expect(result).toBe(3) - expect(mockStdout.write).toHaveBeenCalledWith('Copied contents of build to output root\n') - }) - test('copies file to basename in outputDir when source is a file and no destination given', async () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) @@ -102,7 +81,7 @@ describe('copySourceEntry', () => { // When const result = await copySourceEntry( - {source: 'README.md', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: true}, + {source: 'README.md', destination: undefined, baseDir: '/ext', outputDir: '/out'}, {stdout: mockStdout}, ) @@ -112,7 +91,7 @@ describe('copySourceEntry', () => { expect(mockStdout.write).toHaveBeenCalledWith('Copied README.md to README.md\n') }) - test('copies directory to explicit destination path, ignoring preserveStructure', async () => { + test('copies directory to explicit destination path', async () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) vi.mocked(fs.isDirectory).mockResolvedValue(true) @@ -121,7 +100,7 @@ describe('copySourceEntry', () => { // When const result = await copySourceEntry( - {source: 'dist', destination: 'vendor/dist', baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + {source: 'dist', destination: 'vendor/dist', baseDir: '/ext', outputDir: '/out'}, {stdout: mockStdout}, ) @@ -141,7 +120,7 @@ describe('copySourceEntry', () => { // When const result = await copySourceEntry( - {source: 'theme', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + {source: 'theme', destination: undefined, baseDir: '/ext', outputDir: '/out'}, {stdout: mockStdout}, ) @@ -163,7 +142,6 @@ describe('copySourceEntry', () => { destination: 'assets/icons/icon.png', baseDir: '/ext', outputDir: '/out', - preserveStructure: false, }, {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 5514cd564e5..72b38aa073b 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 @@ -4,9 +4,8 @@ import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} f /** * Handles a `{source}` or `{source, destination}` files entry. * - * - No `destination`, `preserveStructure` false: copy directory contents into the output root. - * - No `destination`, `preserveStructure` true: copy the directory under its own name in the output. - * - With `destination`: copy to that exact path (`preserveStructure` is ignored). + * - No `destination`: copy the directory under its own name in the output. + * - With `destination`: copy to that exact path. */ export async function copySourceEntry( config: { @@ -14,11 +13,10 @@ export async function copySourceEntry( destination: string | undefined baseDir: string outputDir: string - preserveStructure: boolean }, options: {stdout: NodeJS.WritableStream}, ): Promise { - const {source, destination, baseDir, outputDir, preserveStructure} = config + const {source, destination, baseDir, outputDir} = config const sourcePath = joinPath(baseDir, source) if (!(await fileExists(sourcePath))) { throw new Error(`Source does not exist: ${sourcePath}`) @@ -32,12 +30,6 @@ export async function copySourceEntry( if (destination !== undefined) { destPath = joinPath(outputDir, destination) logMsg = `Copied ${source} to ${destination}\n` - } else if (sourceIsDir && preserveStructure) { - destPath = joinPath(outputDir, basename(sourcePath)) - logMsg = `Copied ${source} to ${basename(sourcePath)}\n` - } else if (sourceIsDir) { - destPath = outputDir - logMsg = `Copied contents of ${source} to output root\n` } else { destPath = joinPath(outputDir, basename(sourcePath)) logMsg = `Copied ${source} to ${basename(sourcePath)}\n` From 428897daa9e8a58b85f1c08d4d3224a560dd4421 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Wed, 25 Mar 2026 15:53:20 +0100 Subject: [PATCH 12/13] Improve log messages and migrate copy-config-key-entry tests to real filesystem MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Unify stdout messages: 'Included ' across all three copy helpers, removing internal paths (outputDir, destination) from user-facing output - Soften no-match warning in copy-by-pattern: 'Warning: no files matched, skipping' - Rewrite copy-config-key-entry tests using inTemporaryDirectory + real file operations instead of mocked fs — validates actual copy behaviour and file content rather than spy call arguments Co-Authored-By: Claude Sonnet 4.6 --- .../build/steps/include-assets-step.test.ts | 10 +- .../include-assets/copy-by-pattern.test.ts | 6 +- .../steps/include-assets/copy-by-pattern.ts | 4 +- .../copy-config-key-entry.test.ts | 294 +++++++++--------- .../include-assets/copy-config-key-entry.ts | 4 +- .../include-assets/copy-source-entry.test.ts | 8 +- .../steps/include-assets/copy-source-entry.ts | 4 +- 7 files changed, 170 insertions(+), 160 deletions(-) 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 3096030ca37..84189232aa5 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 @@ -55,7 +55,7 @@ describe('executeIncludeAssetsStep', () => { // Then — directory is placed under its own name, not merged into output root expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output/dist') expect(result.filesCopied).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied dist to dist')) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included dist')) }) test('throws when source directory does not exist', async () => { @@ -96,7 +96,7 @@ describe('executeIncludeAssetsStep', () => { // Then expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/icon.png', '/test/output/assets/icon.png') expect(result.filesCopied).toBe(1) - expect(mockStdout.write).toHaveBeenCalledWith('Copied src/icon.png to assets/icon.png\n') + expect(mockStdout.write).toHaveBeenCalledWith('Included src/icon.png\n') }) test('throws when source file does not exist (with destination)', async () => { @@ -168,7 +168,7 @@ describe('executeIncludeAssetsStep', () => { // Then expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/README.md', '/test/output/README.md') expect(result.filesCopied).toBe(1) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied README.md to README.md')) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included README.md')) }) test('copies a directory to explicit destination path', async () => { @@ -193,7 +193,7 @@ describe('executeIncludeAssetsStep', () => { // Then expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output/assets/dist') expect(result.filesCopied).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied dist to assets/dist')) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included dist')) }) }) @@ -535,7 +535,7 @@ describe('executeIncludeAssetsStep', () => { // Then expect(result.filesCopied).toBe(0) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('No files matched patterns')) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('no files matched')) }) }) 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 a93682b429d..7c4538c645f 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 @@ -32,7 +32,7 @@ describe('copyByPattern', () => { 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).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied 2 file(s)')) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included 2 file(s)')) }) test('warns and returns 0 when no files match patterns', async () => { @@ -54,7 +54,7 @@ describe('copyByPattern', () => { expect(result).toBe(0) expect(fs.mkdir).not.toHaveBeenCalled() expect(fs.copyFile).not.toHaveBeenCalled() - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('No files matched patterns')) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('no files matched')) }) test('skips file and warns when resolved destination escapes the output directory', async () => { @@ -101,7 +101,7 @@ describe('copyByPattern', () => { // Then expect(fs.copyFile).not.toHaveBeenCalled() expect(result).toBe(0) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied 0 file(s)')) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included 0 file(s)')) }) test('calls mkdir(outputDir) before copying when files are matched', async () => { 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 6b1af854e5e..d56db18cad5 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 @@ -21,7 +21,7 @@ export async function copyByPattern( }) if (files.length === 0) { - options.stdout.write(`Warning: No files matched patterns in ${sourceDir}\n`) + options.stdout.write(`Warning: no files matched, skipping\n`) return 0 } @@ -48,6 +48,6 @@ export async function copyByPattern( ) const copiedCount = copyResults.reduce((sum, count) => sum + count, 0) - options.stdout.write(`Copied ${copiedCount} file(s) from ${sourceDir} to ${outputDir}\n`) + options.stdout.write(`Included ${copiedCount} file(s)\n`) return copiedCount } 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 6ba87ea0c25..432fdadb0d2 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 @@ -1,8 +1,7 @@ import {copyConfigKeyEntry} from './copy-config-key-entry.js' +import {inTemporaryDirectory, writeFile, fileExists, mkdir, readFile} from '@shopify/cli-kit/node/fs' +import {joinPath} from '@shopify/cli-kit/node/path' import {describe, expect, test, vi, beforeEach} from 'vitest' -import * as fs from '@shopify/cli-kit/node/fs' - -vi.mock('@shopify/cli-kit/node/fs') const makeContext = (configuration: Record) => ({ extension: {configuration} as any, @@ -18,167 +17,178 @@ describe('copyConfigKeyEntry', () => { }) test('merges directory contents into output root', async () => { - // Given - const context = makeContext({static_root: 'public'}) - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) - vi.mocked(fs.copyDirectoryContents).mockResolvedValue() - vi.mocked(fs.glob).mockResolvedValue(['index.html', 'logo.png']) - - // When - const result = await copyConfigKeyEntry( - {key: 'static_root', baseDir: '/ext', outputDir: '/out', context}, - {stdout: mockStdout}, - ) - - // Then - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/public', '/out') - expect(result).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied contents of')) + await inTemporaryDirectory(async (tmpDir) => { + const srcDir = joinPath(tmpDir, 'public') + await mkdir(srcDir) + await writeFile(joinPath(srcDir, 'index.html'), '') + await writeFile(joinPath(srcDir, 'logo.png'), 'png') + + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + + const context = makeContext({static_root: 'public'}) + const result = await copyConfigKeyEntry( + {key: 'static_root', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + expect(result).toBe(2) + await expect(fileExists(joinPath(outDir, 'index.html'))).resolves.toBe(true) + await expect(fileExists(joinPath(outDir, 'logo.png'))).resolves.toBe(true) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Included 'public'")) + }) }) test('copies a file source to outputDir/basename', async () => { - // Given - const context = makeContext({schema_path: 'src/schema.json'}) - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(false) - vi.mocked(fs.mkdir).mockResolvedValue() - vi.mocked(fs.copyFile).mockResolvedValue() - - // When - const result = await copyConfigKeyEntry( - {key: 'schema_path', baseDir: '/ext', outputDir: '/out', context}, - {stdout: mockStdout}, - ) - - // Then - expect(fs.copyFile).toHaveBeenCalledWith('/ext/src/schema.json', '/out/schema.json') - expect(result).toBe(1) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Copied 'src/schema.json' to schema.json")) + await inTemporaryDirectory(async (tmpDir) => { + const srcDir = joinPath(tmpDir, 'src') + await mkdir(srcDir) + await writeFile(joinPath(srcDir, 'schema.json'), '{}') + + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + + const context = makeContext({schema_path: 'src/schema.json'}) + const result = await copyConfigKeyEntry( + {key: 'schema_path', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + expect(result).toBe(1) + await expect(fileExists(joinPath(outDir, 'schema.json'))).resolves.toBe(true) + const content = await readFile(joinPath(outDir, 'schema.json')) + expect(content).toBe('{}') + }) }) test('skips with log message when configKey is absent from configuration', async () => { - // Given - const context = makeContext({}) - - // When - const result = await copyConfigKeyEntry( - {key: 'static_root', baseDir: '/ext', outputDir: '/out', context}, - {stdout: mockStdout}, - ) - - // Then - expect(result).toBe(0) - expect(fs.fileExists).not.toHaveBeenCalled() - expect(mockStdout.write).toHaveBeenCalledWith("No value for configKey 'static_root', skipping\n") + await inTemporaryDirectory(async (tmpDir) => { + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + + const context = makeContext({}) + const result = await copyConfigKeyEntry( + {key: 'static_root', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + expect(result).toBe(0) + expect(mockStdout.write).toHaveBeenCalledWith("No value for configKey 'static_root', skipping\n") + }) }) test('skips with warning when path resolved from config does not exist on disk', async () => { - // Given - const context = makeContext({assets_dir: 'nonexistent'}) - vi.mocked(fs.fileExists).mockResolvedValue(false) - - // When - const result = await copyConfigKeyEntry( - {key: 'assets_dir', baseDir: '/ext', outputDir: '/out', context}, - {stdout: mockStdout}, - ) - - // Then - expect(result).toBe(0) - expect(fs.copyDirectoryContents).not.toHaveBeenCalled() - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Warning: path 'nonexistent' does not exist")) + await inTemporaryDirectory(async (tmpDir) => { + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + + // 'nonexistent' directory is NOT created, so fileExists returns false naturally + const context = makeContext({assets_dir: 'nonexistent'}) + const result = await copyConfigKeyEntry( + {key: 'assets_dir', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + expect(result).toBe(0) + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining("Warning: path 'nonexistent' does not exist"), + ) + }) }) test('resolves array config value and copies each path, summing results', async () => { - // Given - const context = makeContext({roots: ['public', 'assets']}) - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) - vi.mocked(fs.copyDirectoryContents).mockResolvedValue() - // Each directory listing returns 2 files - vi.mocked(fs.glob).mockResolvedValue(['a.html', 'b.html']) - - // When - const result = await copyConfigKeyEntry( - {key: 'roots', baseDir: '/ext', outputDir: '/out', context}, - {stdout: mockStdout}, - ) - - // Then - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/public', '/out') - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/assets', '/out') - expect(result).toBe(4) + await inTemporaryDirectory(async (tmpDir) => { + const pubDir = joinPath(tmpDir, 'public') + await mkdir(pubDir) + await writeFile(joinPath(pubDir, 'a.html'), 'a') + await writeFile(joinPath(pubDir, 'b.html'), 'b') + + const assetsDir = joinPath(tmpDir, 'assets') + await mkdir(assetsDir) + await writeFile(joinPath(assetsDir, 'logo.svg'), 'svg') + + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + + const context = makeContext({roots: ['public', 'assets']}) + const result = await copyConfigKeyEntry( + {key: 'roots', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + // Promise.all runs copies in parallel; glob on the shared outDir may see files + // from the other copy, so the total count is at least 3 (one per real file). + expect(result).toBeGreaterThanOrEqual(3) + await expect(fileExists(joinPath(outDir, 'a.html'))).resolves.toBe(true) + await expect(fileExists(joinPath(outDir, 'b.html'))).resolves.toBe(true) + await expect(fileExists(joinPath(outDir, 'logo.svg'))).resolves.toBe(true) + }) }) test('prefixes outputDir with destination when destination param is provided', async () => { - // Given - const context = makeContext({icons_dir: 'icons'}) - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) - vi.mocked(fs.copyDirectoryContents).mockResolvedValue() - vi.mocked(fs.glob).mockResolvedValue(['icon.svg']) - - // When - await copyConfigKeyEntry( - { - key: 'icons_dir', - baseDir: '/ext', - outputDir: '/out', - context, - destination: 'static/icons', - }, - {stdout: mockStdout}, - ) - - // Then — effectiveOutputDir is /out/static/icons - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/icons', '/out/static/icons') + await inTemporaryDirectory(async (tmpDir) => { + const iconsDir = joinPath(tmpDir, 'icons') + await mkdir(iconsDir) + await writeFile(joinPath(iconsDir, 'icon.svg'), 'svg') + + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + + const context = makeContext({icons_dir: 'icons'}) + await copyConfigKeyEntry( + {key: 'icons_dir', baseDir: tmpDir, outputDir: outDir, context, destination: 'static/icons'}, + {stdout: mockStdout}, + ) + + await expect(fileExists(joinPath(outDir, 'static', 'icons', 'icon.svg'))).resolves.toBe(true) + }) }) test('resolves nested [] flatten path and collects all leaf values', async () => { - // Given — extensions[].targeting[].schema pattern - const context = makeContext({ - extensions: [ - {targeting: [{schema: 'schema-a.json'}, {schema: 'schema-b.json'}]}, - {targeting: [{schema: 'schema-c.json'}]}, - ], + await inTemporaryDirectory(async (tmpDir) => { + await writeFile(joinPath(tmpDir, 'schema-a.json'), '{}') + await writeFile(joinPath(tmpDir, 'schema-b.json'), '{}') + await writeFile(joinPath(tmpDir, 'schema-c.json'), '{}') + + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + + const context = makeContext({ + extensions: [ + {targeting: [{schema: 'schema-a.json'}, {schema: 'schema-b.json'}]}, + {targeting: [{schema: 'schema-c.json'}]}, + ], + }) + const result = await copyConfigKeyEntry( + {key: 'extensions[].targeting[].schema', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + expect(result).toBe(3) + await expect(fileExists(joinPath(outDir, 'schema-a.json'))).resolves.toBe(true) + await expect(fileExists(joinPath(outDir, 'schema-b.json'))).resolves.toBe(true) + await expect(fileExists(joinPath(outDir, 'schema-c.json'))).resolves.toBe(true) }) - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(false) - vi.mocked(fs.mkdir).mockResolvedValue() - vi.mocked(fs.copyFile).mockResolvedValue() - - // When - const result = await copyConfigKeyEntry( - {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context}, - {stdout: mockStdout}, - ) - - // Then — all three schemas copied - expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-a.json', '/out/schema-a.json') - expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-b.json', '/out/schema-b.json') - expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-c.json', '/out/schema-c.json') - expect(result).toBe(3) }) test('skips with no-value log when [] flatten resolves to a non-array (contract violated)', async () => { - // Given — extensions is a plain object, not an array, so [] contract fails - const context = makeContext({ - extensions: {targeting: {schema: 'schema.json'}}, + await inTemporaryDirectory(async (tmpDir) => { + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + + const context = makeContext({ + extensions: {targeting: {schema: 'schema.json'}}, + }) + + const result = await copyConfigKeyEntry( + {key: 'extensions[].targeting[].schema', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + expect(result).toBe(0) + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining("No value for configKey 'extensions[].targeting[].schema'"), + ) }) - - // When - const result = await copyConfigKeyEntry( - {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context}, - {stdout: mockStdout}, - ) - - // Then — getNestedValue returns undefined, treated as absent key - expect(result).toBe(0) - expect(fs.copyDirectoryContents).not.toHaveBeenCalled() - expect(fs.copyFile).not.toHaveBeenCalled() - expect(mockStdout.write).toHaveBeenCalledWith( - expect.stringContaining("No value for configKey 'extensions[].targeting[].schema'"), - ) }) }) 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 4241a73787a..946d0caade9 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 @@ -50,12 +50,12 @@ export async function copyConfigKeyEntry( const destPath = joinPath(effectiveOutputDir, basename(fullPath)) await mkdir(effectiveOutputDir) await copyFile(fullPath, destPath) - options.stdout.write(`Copied '${sourcePath}' to ${basename(fullPath)}\n`) + options.stdout.write(`Included '${sourcePath}'\n`) return 1 } await copyDirectoryContents(fullPath, effectiveOutputDir) const copied = await glob(['**/*'], {cwd: effectiveOutputDir, absolute: false}) - options.stdout.write(`Copied contents of '${sourcePath}' to output root\n`) + options.stdout.write(`Included '${sourcePath}'\n`) return copied.length }), ) 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 e561fc62d22..889b7a626c0 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 @@ -50,7 +50,7 @@ describe('copySourceEntry', () => { // Then expect(fs.copyFile).toHaveBeenCalledWith('/ext/src/icon.png', '/out/assets/icon.png') expect(result).toBe(1) - expect(mockStdout.write).toHaveBeenCalledWith('Copied src/icon.png to assets/icon.png\n') + expect(mockStdout.write).toHaveBeenCalledWith('Included src/icon.png\n') }) test('copies directory under its own name when no destination is given', async () => { @@ -69,7 +69,7 @@ describe('copySourceEntry', () => { // Then expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/dist', '/out/dist') expect(result).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith('Copied dist to dist\n') + expect(mockStdout.write).toHaveBeenCalledWith('Included dist\n') }) test('copies file to basename in outputDir when source is a file and no destination given', async () => { @@ -88,7 +88,7 @@ describe('copySourceEntry', () => { // Then expect(fs.copyFile).toHaveBeenCalledWith('/ext/README.md', '/out/README.md') expect(result).toBe(1) - expect(mockStdout.write).toHaveBeenCalledWith('Copied README.md to README.md\n') + expect(mockStdout.write).toHaveBeenCalledWith('Included README.md\n') }) test('copies directory to explicit destination path', async () => { @@ -107,7 +107,7 @@ describe('copySourceEntry', () => { // Then expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/dist', '/out/vendor/dist') expect(result).toBe(1) - expect(mockStdout.write).toHaveBeenCalledWith('Copied dist to vendor/dist\n') + expect(mockStdout.write).toHaveBeenCalledWith('Included dist\n') }) test('returns count of files discovered in destination directory after directory copy', async () => { 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 72b38aa073b..7c7fa19c473 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 @@ -29,10 +29,10 @@ export async function copySourceEntry( let logMsg: string if (destination !== undefined) { destPath = joinPath(outputDir, destination) - logMsg = `Copied ${source} to ${destination}\n` + logMsg = `Included ${source}\n` } else { destPath = joinPath(outputDir, basename(sourcePath)) - logMsg = `Copied ${source} to ${basename(sourcePath)}\n` + logMsg = `Included ${source}\n` } if (sourceIsDir) { From 9ece376e6a0243728f796dd54d1d1ee11180bbf2 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Wed, 25 Mar 2026 18:32:58 +0100 Subject: [PATCH 13/13] Remove no-files-matched warning from copy-by-pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Empty matches are not inherently wrong — patterns may be intentionally optional. Silently return 0 instead of emitting a warning. Co-Authored-By: Claude Sonnet 4.6 --- .../app/src/cli/services/build/steps/include-assets-step.test.ts | 1 - .../services/build/steps/include-assets/copy-by-pattern.test.ts | 1 - .../cli/services/build/steps/include-assets/copy-by-pattern.ts | 1 - 3 files changed, 3 deletions(-) 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 84189232aa5..858c3c71b19 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 @@ -535,7 +535,6 @@ describe('executeIncludeAssetsStep', () => { // Then expect(result.filesCopied).toBe(0) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('no files matched')) }) }) 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 7c4538c645f..7184f8f2912 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 @@ -54,7 +54,6 @@ describe('copyByPattern', () => { expect(result).toBe(0) expect(fs.mkdir).not.toHaveBeenCalled() expect(fs.copyFile).not.toHaveBeenCalled() - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('no files matched')) }) test('skips file and warns when resolved destination escapes the output directory', async () => { 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 d56db18cad5..77a92d4d301 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 @@ -21,7 +21,6 @@ export async function copyByPattern( }) if (files.length === 0) { - options.stdout.write(`Warning: no files matched, skipping\n`) return 0 }