Add client steps implementation to support existing functionalities

[alfonso-noriega]

Add build step infrastructure for extension asset management

WHY are these changes introduced?

This introduces a flexible build step system to handle various asset copying and processing needs for different extension types, replacing hardcoded build logic with configurable steps.

WHAT is this pull request doing?

• Adds 'admin' to the CONFIG_EXTENSION_IDS array for admin extensions
• Creates new build step executors:
  • executeIncludeAssetsStep - handles pattern-based, static, and config-key-driven asset copying with comprehensive test coverage
  • executeCopyStaticAssetsStep - copies static assets defined in extension build manifests
  • executeCreateTaxStubStep - generates minimal JavaScript stub files for tax calculation extensions
• Implements a centralized step router (executeStepByType) that dispatches to appropriate handlers based on step type
• Supports multiple inclusion strategies:
  • Pattern-based file selection using glob patterns
  • Static file/directory copying with optional destination paths
  • Configuration key resolution for dynamic path discovery
  • Configurable structure preservation and flattening options

How to test your changes?

1. Create an extension with various asset types (static files, pattern-matched files, config-driven paths)
2. Configure build steps using the new include_assets step type with different inclusion strategies
3. Run the build process and verify assets are copied correctly to output directories
4. Test with admin extensions to ensure they're recognized as config extensions
5. Run the comprehensive test suite for include-assets-step.test.ts

Measuring impact

How do we know this change was effective? Please choose one:

• n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've considered possible documentation changes

[alfonso-noriega]

• Add client steps implementation to support existing functionalities #6966 : 2 dependent PRs (#6867 , #6941 ) 👈 (View in Graphite)
• Create abstract client steps infrastracture to externalize the ap modules client configurations #6965
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟢	Statements	82.07%	15105/18404
🟡	Branches	74.61%	7480/10025
🟢	Functions	81.17%	3798/4679
🟢	Lines	82.48%	14286/17320

Test suite run success

3951 tests passing in 1518 suites.

Report generated by 🧪jest coverage report action from b32f3a9

[binks-code-reviewer]

Path traversal: include_assets can write outside outputDir

joinPath(outputDir, entry.destination) is used directly with user-controlled config (destination). If destination is absolute (/etc) or contains ../.., it can escape outputDir and overwrite arbitrary files on the machine running the build (CI runner, developer machine). Same class of issue exists for entry.baseDir (controls sourceDir), copySourceEntry(...destination) (static destination), copyConfigKeyEntry(...destination) (configKey destination), and copyConfigKeyEntry where the config value itself can be ../../something.

Because build manifests/config are extension-provided, a malicious extension (or compromised repo) could cause the CLI to overwrite sensitive files or poison other build artifacts.

[binks-code-reviewer]

🤖 Code Review · #projects-dev-ai for questions
React with 👍/👎 or reply — all feedback helps improve the agent.

✅ Complete - 2 findings

📋 History

❌ Failed → ✅ 1 findings → ❌ Failed → ❌ Failed → ✅ 1 findings → ✅ 2 findings

[isaacroldan]

---

Review assisted by pair-review

[isaacroldan]

Human: I guess the AI is right, but not sure if the fix is necessary? how important is this count?

🐛 Bug: When a static entry has an explicit destination (lines 177-183), copyFile is called and the count is hardcoded to 1. Since copyFile wraps fs-extra's copy which handles directories recursively, this works functionally — but if the source is a directory containing many files, the reported count is still 1. This is inconsistent with the no-destination path (lines 193-200) which correctly counts files via glob.

Suggestion: Add an isDirectory branch in the destination-provided path: if the source is a directory, count the actual files after copy (via glob on the destination), otherwise return 1. This mirrors the logic in the no-destination path below.

[isaacroldan]

🐛 Bug: copyConfigKeyEntry unconditionally calls copyDirectoryContents(fullPath, destDir) at line 246 without checking whether the resolved path is actually a directory. If a config key resolves to a file (e.g., tools: 'tools-a.js'), copyDirectoryContents internally globs srcDir/**/* which matches nothing for a file — the copy silently becomes a no-op. The copySourceEntry function in this same PR correctly handles this with an isDirectory check (line 185), but the same fix wasn't applied here. Tests pass only because copyDirectoryContents is mocked.

Suggestion: Add the same isDirectory check that exists in copySourceEntry. When the resolved path is a file, use copyFile instead:

Suggested change

	await copyDirectoryContents(fullPath, destDir)
	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

[isaacroldan]

🐛 Bug: When preserveStructure is false and duplicate basenames are detected (lines 282-299), a warning is emitted but Promise.all at line 301 proceeds to copy all files concurrently. Since colliding files target the same destination path, the final file content depends on which copyFile promise resolves last — this is a race condition. The warning correctly identifies the problem but the behavior should be deterministic.

Suggestion: Consider either: (1) processing files sequentially when collisions are detected so the last entry in the array deterministically wins, or (2) deduplicating the file list before copying so only one file per basename is copied (e.g., last-in-array wins), adjusting the count accordingly.

[isaacroldan]

🐛 Bug: copyByPattern returns files.length as the copy count, but files can be skipped in two places: the path-traversal guard (lines 306-308, early return) and the same-path check (line 311, early return). The returned count overstates actual copies, and this value propagates to stepResults via BuildContext, affecting downstream logging or steps that consume it.

Suggestion: Track actual copies by having each map callback return 1 or 0, then sum:

Suggested change

	return {filesCopied: files.length}
	const copyResults = await Promise.all(
	files.map(async (filepath) => {
	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, n) => sum + n, 0)
	options.stdout.write(`Copied ${copiedCount} file(s) from ${sourceDir} to ${outputDir}\n`)
	return {filesCopied: copiedCount}