Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for custom tools through a new extraTools option that allows developers to extend the project scaffolding with custom actions and shell commands. The implementation integrates custom tools into the existing tool selection workflow and execution pipeline.
Key Changes:
- Introduces
ExtraTooltype definition andextraToolsparameter to thecreatefunction - Extends tool filtering and multiselect options to include custom tools alongside built-in tools (Biome, ESLint, Prettier)
- Implements command execution via
cross-spawnfor custom tool commands
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/index.ts |
Core implementation: adds ExtraTool type, runCommand helper, and integrates custom tools into tool selection and execution logic |
test/custom-tools.test.ts |
Test suite for custom tools covering both action callbacks and shell commands |
test/fixtures/basic/package.json |
Test fixture for basic project scaffolding tests |
test/fixtures/basic/template-vanilla/package.json |
Test fixture for vanilla template tests |
test/fixtures/basic/template-common/package.json |
Test fixture for common template tests |
package.json |
Adds cross-spawn and fs-extra dependencies with corresponding type definitions |
pnpm-lock.yaml |
Lock file updates for new dependencies and libc field additions to platform-specific packages |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (extraTools) { | ||
| options.push( | ||
| ...extraTools.map((tool) => ({ | ||
| value: tool.value, | ||
| label: tool.label, | ||
| hint: tool.command, | ||
| })), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing validation to ensure extra tool values don't conflict with built-in tool names ('eslint', 'prettier', 'biome'). If an extra tool has a value that matches a built-in tool name, it could cause unexpected behavior in the tool processing loop. Consider adding validation:
if (extraTools) {
const conflicts = extraTools.filter(tool =>
BUILTIN_TOOLS.includes(tool.value)
);
if (conflicts.length > 0) {
throw new Error(`Extra tool values cannot conflict with built-in tools: ${conflicts.map(t => t.value).join(', ')}`);
}
options.push(...);
}| // Handle extra tools first | ||
| if (extraTools) { | ||
| const matchedTool = extraTools.find( | ||
| (extraTool) => extraTool.value === tool, | ||
| ); | ||
| if (matchedTool?.action) { | ||
| await matchedTool.action(); | ||
| } | ||
| if (matchedTool?.command) { | ||
| await runCommand(matchedTool.command, distFolder); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Critical logic error: The continue statement on line 347 will be executed for ALL tools when extraTools is defined, not just for matched extra tools. This means that if extraTools is defined but the current tool is a built-in tool (like 'eslint', 'prettier', or 'biome'), the code will skip processing the built-in tool entirely.
The condition should check if a matching extra tool was found before continuing. Consider changing the logic to:
const matchedTool = extraTools?.find(
(extraTool) => extraTool.value === tool,
);
if (matchedTool) {
if (matchedTool.action) {
await matchedTool.action();
}
if (matchedTool.command) {
await runCommand(matchedTool.command, distFolder);
}
continue;
}| spawn.sync(bin, args, { | ||
| stdio: 'inherit', | ||
| cwd, | ||
| }); |
There was a problem hiding this comment.
The runCommand function lacks error handling. If the command fails or the binary is not found, spawn.sync will not throw by default, but you should check the return value's status or error property and handle failures appropriately. Consider adding:
const result = spawn.sync(bin, args, {
stdio: 'inherit',
cwd,
});
if (result.error) {
throw new Error(`Failed to run command "${command}": ${result.error.message}`);
}
if (result.status !== 0) {
throw new Error(`Command "${command}" exited with code ${result.status}`);
}| spawn.sync(bin, args, { | |
| stdio: 'inherit', | |
| cwd, | |
| }); | |
| const result = spawn.sync(bin, args, { | |
| stdio: 'inherit', | |
| cwd, | |
| }); | |
| if (result.error) { | |
| throw new Error(`Failed to run command "${command}": ${result.error.message}`); | |
| } | |
| if (result.status !== 0) { | |
| throw new Error(`Command "${command}" exited with code ${result.status}`); | |
| } |
| }; | ||
|
|
||
| async function runCommand(command: string, cwd: string) { | ||
| const [bin, ...args] = command.split(' '); |
There was a problem hiding this comment.
The naive command.split(' ') approach will fail for commands with quoted arguments (e.g., echo "hello world" would split into ['echo', '"hello', 'world"'] instead of ['echo', 'hello world']). Consider using a proper shell command parser library or documenting this limitation clearly.
| await fse.remove(touchedFile); | ||
|
|
There was a problem hiding this comment.
The await fse.remove(touchedFile) call is unnecessary here since the file doesn't exist yet (the project directory is freshly created in beforeEach). This line can be safely removed.
| await fse.remove(touchedFile); |
| test('should run extra tool action', async () => { | ||
| const projectDir = path.join(testDir, 'extra-tool-action'); | ||
| let actionCalled = false; | ||
|
|
||
| process.argv = [ | ||
| 'node', | ||
| 'test', | ||
| '--dir', | ||
| projectDir, | ||
| '--template', | ||
| 'vanilla', | ||
| '--tools', | ||
| 'custom-action', | ||
| ]; | ||
|
|
||
| await create({ | ||
| name: 'test', | ||
| root: fixturesDir, | ||
| templates: ['vanilla'], | ||
| getTemplateName: async () => 'vanilla', | ||
| mapESLintTemplate: () => null, | ||
| extraTools: [ | ||
| { | ||
| value: 'custom-action', | ||
| label: 'Custom Action', | ||
| action: () => { | ||
| actionCalled = true; | ||
| }, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| assert.strictEqual(actionCalled, true); | ||
| }); | ||
|
|
||
| test('should run extra tool command', async () => { | ||
| const projectDir = path.join(testDir, 'extra-tool-command'); | ||
| const touchedFile = path.join(projectDir, 'touched-by-command.txt'); | ||
|
|
||
| await fse.remove(touchedFile); | ||
|
|
||
| process.argv = [ | ||
| 'node', | ||
| 'test', | ||
| '--dir', | ||
| projectDir, | ||
| '--template', | ||
| 'vanilla', | ||
| '--tools', | ||
| 'custom-command', | ||
| ]; | ||
|
|
||
| await create({ | ||
| name: 'test', | ||
| root: fixturesDir, | ||
| templates: ['vanilla'], | ||
| getTemplateName: async () => 'vanilla', | ||
| mapESLintTemplate: () => null, | ||
| extraTools: [ | ||
| { | ||
| value: 'custom-command', | ||
| label: 'Custom Command', | ||
| command: 'touch touched-by-command.txt', | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| assert.strictEqual(fs.existsSync(touchedFile), true); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for the scenario where extra tools are displayed in the interactive multiselect prompt (when --tools is not specified). The code in src/index.ts lines 131-145 adds extra tools to the prompt options, but there's no test verifying this behavior works correctly.
| type ExtraTool = { | ||
| /** | ||
| * The value of the multiselect option. | ||
| */ | ||
| value: string; | ||
| /** | ||
| * The label of the multiselect option. | ||
| */ | ||
| label: string; | ||
| /** | ||
| * The action to perform when the tool is selected. | ||
| */ | ||
| action?: () => unknown; | ||
| /** | ||
| * The custom command to run when the tool is selected. | ||
| */ | ||
| command?: string; | ||
| }; |
There was a problem hiding this comment.
The ExtraTool type documentation should clarify that action and command are mutually exclusive or can be used together. Currently it's unclear from the documentation whether:
- Only one of
actionorcommandshould be provided - Both can be provided (in which case, what's the execution order?)
- If neither is provided, what happens?
Consider adding a note like: "Note: Either action or command must be provided. If both are provided, action will execute first, followed by command."
| async function runCommand(command: string, cwd: string) { | ||
| const [bin, ...args] = command.split(' '); | ||
| spawn.sync(bin, args, { | ||
| stdio: 'inherit', | ||
| cwd, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The runCommand function is defined as async but doesn't return anything or await any async operations. The spawn.sync call is synchronous. Either remove the async keyword or use the asynchronous version spawn instead of spawn.sync if you want to support async operations.
| test('should run extra tool action', async () => { | ||
| const projectDir = path.join(testDir, 'extra-tool-action'); | ||
| let actionCalled = false; | ||
|
|
||
| process.argv = [ | ||
| 'node', | ||
| 'test', | ||
| '--dir', | ||
| projectDir, | ||
| '--template', | ||
| 'vanilla', | ||
| '--tools', | ||
| 'custom-action', | ||
| ]; | ||
|
|
||
| await create({ | ||
| name: 'test', | ||
| root: fixturesDir, | ||
| templates: ['vanilla'], | ||
| getTemplateName: async () => 'vanilla', | ||
| mapESLintTemplate: () => null, | ||
| extraTools: [ | ||
| { | ||
| value: 'custom-action', | ||
| label: 'Custom Action', | ||
| action: () => { | ||
| actionCalled = true; | ||
| }, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| assert.strictEqual(actionCalled, true); | ||
| }); | ||
|
|
||
| test('should run extra tool command', async () => { | ||
| const projectDir = path.join(testDir, 'extra-tool-command'); | ||
| const touchedFile = path.join(projectDir, 'touched-by-command.txt'); | ||
|
|
||
| await fse.remove(touchedFile); | ||
|
|
||
| process.argv = [ | ||
| 'node', | ||
| 'test', | ||
| '--dir', | ||
| projectDir, | ||
| '--template', | ||
| 'vanilla', | ||
| '--tools', | ||
| 'custom-command', | ||
| ]; | ||
|
|
||
| await create({ | ||
| name: 'test', | ||
| root: fixturesDir, | ||
| templates: ['vanilla'], | ||
| getTemplateName: async () => 'vanilla', | ||
| mapESLintTemplate: () => null, | ||
| extraTools: [ | ||
| { | ||
| value: 'custom-command', | ||
| label: 'Custom Command', | ||
| command: 'touch touched-by-command.txt', | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| assert.strictEqual(fs.existsSync(touchedFile), true); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for the scenario where both extra tools and built-in tools (like 'eslint', 'prettier', or 'biome') are selected together. This is important to test since there's complex logic in src/index.ts (lines 335-348) that handles both types of tools. Consider adding a test case like:
test('should handle both extra tools and built-in tools', async () => {
// Test with --tools custom-action,eslint
// Verify both custom action runs AND eslint is set up
});| /** | ||
| * The action to perform when the tool is selected. | ||
| */ | ||
| action?: () => unknown; |
There was a problem hiding this comment.
The action callback has no parameters, which limits its usefulness. Consider providing context like distFolder, templateName, or other relevant information to allow the action to perform more useful operations. For example:
action?: (context: { distFolder: string; templateName: string }) => unknown;| action?: () => unknown; | |
| action?: (context: { distFolder: string; templateName: string }) => unknown; |
Summary
extraToolsoption to allow custom actions and commands