Skip to content

Add client steps implementation to support existing functionalities#6966

Open
alfonso-noriega wants to merge 1 commit into03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurationsfrom
03-10-add_client_steps_implementation_to_support_existing_functionalities
Open

Add client steps implementation to support existing functionalities#6966
alfonso-noriega wants to merge 1 commit into03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurationsfrom
03-10-add_client_steps_implementation_to_support_existing_functionalities

Conversation

@alfonso-noriega
Copy link
Contributor

@alfonso-noriega alfonso-noriega commented Mar 10, 2026

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

Copy link
Contributor Author

alfonso-noriega commented Mar 10, 2026

@alfonso-noriega alfonso-noriega marked this pull request as ready for review March 10, 2026 08:35
@alfonso-noriega alfonso-noriega requested a review from a team as a code owner March 10, 2026 08:35
@github-actions
Copy link
Contributor

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
Copy link
Contributor

github-actions bot commented Mar 10, 2026

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 78.76% 14572/18502
🟡 Branches 73% 7245/9925
🟡 Functions 78.97% 3714/4703
🟡 Lines 79.1% 13768/17405

Test suite run success

3814 tests passing in 1458 suites.

Report generated by 🧪jest coverage report action from 6a3974c

@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch 2 times, most recently from 9c2e1fd to 7e48497 Compare March 10, 2026 09:04
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch from 16768f3 to 59f0139 Compare March 10, 2026 09:59
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from 7e48497 to 159bb73 Compare March 10, 2026 09:59
@alfonso-noriega alfonso-noriega changed the base branch from 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations to graphite-base/6966 March 10, 2026 10:29
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from 159bb73 to e6f3577 Compare March 10, 2026 10:29
@alfonso-noriega alfonso-noriega changed the base branch from graphite-base/6966 to 03-10-clean_up_module_outputfile_calculation_and_move_it_to_the_specifications March 10, 2026 10:29
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

Choose a reason for hiding this comment

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

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
Copy link

binks-code-reviewer bot commented Mar 10, 2026

🤖 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

@alfonso-noriega alfonso-noriega force-pushed the 03-10-clean_up_module_outputfile_calculation_and_move_it_to_the_specifications branch from bbc0817 to 7b68eb7 Compare March 10, 2026 11:05
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from e6f3577 to 3f28156 Compare March 10, 2026 11:05
@alfonso-noriega alfonso-noriega changed the base branch from 03-10-clean_up_module_outputfile_calculation_and_move_it_to_the_specifications to graphite-base/6966 March 10, 2026 11:11
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from 3f28156 to 44034c5 Compare March 10, 2026 11:11
@alfonso-noriega alfonso-noriega changed the base branch from graphite-base/6966 to 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations March 10, 2026 11:12
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch from f9f9e63 to 59dcaa7 Compare March 10, 2026 11:24
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from 44034c5 to 094a68c Compare March 10, 2026 11:24
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch from 59dcaa7 to fb4c01d Compare March 10, 2026 11:35
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch 2 times, most recently from 0478573 to a8b39c0 Compare March 10, 2026 12:15
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch 2 times, most recently from 68f4751 to e79e9ec Compare March 10, 2026 12:41
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch 2 times, most recently from 74a2028 to e39fbcd Compare March 10, 2026 13:09
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch 2 times, most recently from 61573e1 to 9257821 Compare March 10, 2026 13:21
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch 2 times, most recently from 5e27f1f to e03b479 Compare March 10, 2026 14:45
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch 2 times, most recently from 596be94 to e088f88 Compare March 10, 2026 14:52
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from e03b479 to ca267e2 Compare March 10, 2026 14:52
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch from e088f88 to b42a87b Compare March 10, 2026 15:18
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch 2 times, most recently from 991bfa9 to c673b4a Compare March 10, 2026 15:23

const destDir = preserveStructure ? joinPath(outputDir, basename(sourcePath)) : outputDir
await copyDirectoryContents(sourcePath, destDir)
const copied = await glob(['**/*'], {cwd: destDir, absolute: false})

Choose a reason for hiding this comment

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

Static include_assets copy treats sourcePath as directory when destination is omitted

In copySourceEntry, when destination is omitted it unconditionally calls copyDirectoryContents(sourcePath, ...). If sourcePath is a file (e.g., {type:'static', source:'README.md'} without destination), copyDirectoryContents will likely throw (ENOTDIR). Docs/schema imply static entries can be file/directory, but implementation only supports directory when destination is omitted.

@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from c673b4a to 6a3974c Compare March 10, 2026 15:51
* 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<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

are we ever using _step? why is it included?

export async function executeCreateTaxStubStep(_step: LifecycleStep, context: BuildContext): Promise<void> {
const {extension} = context
await touchFile(extension.outputPath)
await writeFile(extension.outputPath, '(()=>{})();')

Choose a reason for hiding this comment

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

Tax stub step can overwrite a directory with a JS file

The step writes the stub to extension.outputPath directly. If outputPath is configured as a directory (which the include_assets step supports when “no extension”), this will attempt to write a file over a directory path and fail, or clobber expected directory structure.

Evidence:

await touchFile(extension.outputPath)
await writeFile(extension.outputPath, '(()=>{})();')


await mkdir(dirname(destPath))
await copyFile(filepath, destPath)
}),

Choose a reason for hiding this comment

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

include_assets pattern copy can silently overwrite on filename collisions when flattening

When preserveStructure is false, it flattens to basename(filepath). If multiple matched files share a basename (e.g. index.js, styles.css in different folders), later copies overwrite earlier ones with no warning, producing nondeterministic artifacts depending on glob ordering.

Evidence:

const relPath = preserveStructure ? relativePath(sourceDir, filepath) : basename(filepath)
const destPath = joinPath(outputDir, relPath)
await copyFile(filepath, destPath)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants