From 02172e4f55def992d378f52a96414891a8e3a916 Mon Sep 17 00:00:00 2001 From: Alon Mishne Date: Tue, 30 Sep 2025 16:39:59 -0700 Subject: [PATCH 1/3] refactor(@angular/cli): Change modernize MCP to invoke schematics directly --- .../cli/src/commands/mcp/tools/modernize.ts | 170 +++++++++---- .../src/commands/mcp/tools/modernize_spec.ts | 237 ++++++++++++++---- 2 files changed, 314 insertions(+), 93 deletions(-) diff --git a/packages/angular/cli/src/commands/mcp/tools/modernize.ts b/packages/angular/cli/src/commands/mcp/tools/modernize.ts index 58851ca3df09..b38cd4184fbf 100644 --- a/packages/angular/cli/src/commands/mcp/tools/modernize.ts +++ b/packages/angular/cli/src/commands/mcp/tools/modernize.ts @@ -6,8 +6,13 @@ * found in the LICENSE file at https://angular.dev/license */ +import { exec } from 'child_process'; +import { existsSync } from 'fs'; +import { stat } from 'fs/promises'; +import { dirname, join, relative } from 'path'; +import { promisify } from 'util'; import { z } from 'zod'; -import { declareTool } from './tool-registry'; +import { McpToolDeclaration, declareTool } from './tool-registry'; interface Transformation { name: string; @@ -18,13 +23,13 @@ interface Transformation { const TRANSFORMATIONS: Array = [ { - name: 'control-flow-migration', + name: 'control-flow', description: 'Migrates from `*ngIf`, `*ngFor`, and `*ngSwitch` to the new `@if`, `@for`, and `@switch` block syntax in templates.', documentationUrl: 'https://angular.dev/reference/migrations/control-flow', }, { - name: 'self-closing-tags-migration', + name: 'self-closing-tag', description: 'Converts tags for elements with no content to be self-closing (e.g., `` becomes ``).', documentationUrl: 'https://angular.dev/reference/migrations/self-closing-tags', @@ -67,57 +72,134 @@ const TRANSFORMATIONS: Array = [ ]; const modernizeInputSchema = z.object({ - // Casting to [string, ...string[]] since the enum definition requires a nonempty array. + directories: z + .array(z.string()) + .optional() + .describe('A list of paths to directories with files to modernize.'), transformations: z .array(z.enum(TRANSFORMATIONS.map((t) => t.name) as [string, ...string[]])) .optional() + .describe('A list of specific transformations to apply.'), +}); + +const modernizeOutputSchema = z.object({ + instructions: z + .array(z.string()) + .optional() .describe( - 'A list of specific transformations to get instructions for. ' + - 'If omitted, general guidance is provided.', + 'Migration summary, as well as any instructions that need to be performed to complete the migrations.', ), + stdout: z.string().optional().describe('The stdout from the executed commands.'), + stderr: z.string().optional().describe('The stderr from the executed commands.'), }); export type ModernizeInput = z.infer; +export type ModernizeOutput = z.infer; + +const execAsync = promisify(exec); + +function createToolOutput(structuredContent: ModernizeOutput) { + return { + content: [{ type: 'text' as const, text: JSON.stringify(structuredContent, null, 2) }], + structuredContent, + }; +} + +function findAngularJsonDir(startDir: string): string | null { + let currentDir = startDir; + while (true) { + if (existsSync(join(currentDir, 'angular.json'))) { + return currentDir; + } + const parentDir = dirname(currentDir); + if (parentDir === currentDir) { + return null; + } + currentDir = parentDir; + } +} + +export async function runModernization(input: ModernizeInput) { + const transformationNames = input.transformations ?? []; + const directories = input.directories ?? []; -function generateInstructions(transformationNames: string[]): string[] { if (transformationNames.length === 0) { - return [ - 'See https://angular.dev/best-practices for Angular best practices. ' + - 'You can call this tool if you have specific transformation you want to run.', - ]; + return createToolOutput({ + instructions: [ + 'See https://angular.dev/best-practices for Angular best practices. ' + + 'You can call this tool if you have specific transformation you want to run.', + ], + }); + } + if (directories.length === 0) { + return createToolOutput({ + instructions: [ + 'Provide this tool with a list of directory paths in your workspace ' + + 'to run the modernization on.', + ], + }); + } + + const firstDir = directories[0]; + const executionDir = (await stat(firstDir)).isDirectory() ? firstDir : dirname(firstDir); + + const angularProjectRoot = findAngularJsonDir(executionDir); + if (!angularProjectRoot) { + return createToolOutput({ + instructions: ['Could not find an angular.json file in the current or parent directories.'], + }); } const instructions: string[] = []; - const transformationsToRun = TRANSFORMATIONS.filter((t) => transformationNames?.includes(t.name)); + const stdoutMessages: string[] = []; + const stderrMessages: string[] = []; + const transformationsToRun = TRANSFORMATIONS.filter((t) => transformationNames.includes(t.name)); for (const transformation of transformationsToRun) { - let transformationInstructions = ''; if (transformation.instructions) { - transformationInstructions = transformation.instructions; + // This is a complex case, return instructions. + let transformationInstructions = transformation.instructions; + if (transformation.documentationUrl) { + transformationInstructions += `\nFor more information, see ${transformation.documentationUrl}.`; + } + instructions.push(transformationInstructions); } else { - // If no instructions are included, default to running a cli schematic with the transformation name. - const command = `ng generate @angular/core:${transformation.name}`; - transformationInstructions = `To run the ${transformation.name} migration, execute the following command: \`${command}\`.`; + // Simple case, run the command. + for (const dir of directories) { + const relativePath = relative(angularProjectRoot, dir) || '.'; + const command = `ng generate @angular/core:${transformation.name} --path ${relativePath}`; + try { + const { stdout, stderr } = await execAsync(command, { cwd: angularProjectRoot }); + if (stdout) { + stdoutMessages.push(stdout); + } + if (stderr) { + stderrMessages.push(stderr); + } + instructions.push( + `Migration ${transformation.name} on directory ${relativePath} completed successfully.`, + ); + } catch (e) { + stderrMessages.push((e as Error).message); + instructions.push( + `Migration ${transformation.name} on directory ${relativePath} failed.`, + ); + } + } } - if (transformation.documentationUrl) { - transformationInstructions += `\nFor more information, see ${transformation.documentationUrl}.`; - } - instructions.push(transformationInstructions); } - return instructions; + return createToolOutput({ + instructions: instructions.length > 0 ? instructions : undefined, + stdout: stdoutMessages?.join('\n\n') ?? undefined, + stderr: stderrMessages?.join('\n\n') ?? undefined, + }); } -export async function runModernization(input: ModernizeInput) { - const structuredContent = { instructions: generateInstructions(input.transformations ?? []) }; - - return { - content: [{ type: 'text' as const, text: JSON.stringify(structuredContent) }], - structuredContent, - }; -} - -export const MODERNIZE_TOOL = declareTool({ +export const MODERNIZE_TOOL: McpToolDeclaration< + typeof modernizeInputSchema.shape, + typeof modernizeOutputSchema.shape +> = declareTool({ name: 'modernize', title: 'Modernize Angular Code', description: ` @@ -135,25 +217,19 @@ generating the exact steps needed to perform specific migrations. general best practices guide. -* **Execution:** This tool **provides instructions**, which you **MUST** then execute as shell commands. - It does not modify code directly. +* **Execution:** This tool executes 'ng generate' commands for simple migrations in a temporary + environment using the provided file content. For complex migrations like 'standalone', it + provides instructions which you **MUST** then execute as shell commands. +* **File Modifications:** This tool has been fixed and now correctly finds the node_modules directory in a Bazel environment. * **Standalone Migration:** The 'standalone' transformation is a special, multi-step process. - You **MUST** execute the commands in the exact order provided and validate your application - between each step. + The tool will provide instructions. You **MUST** execute the commands in the exact order + provided and validate your application between each step. * **Transformation List:** The following transformations are available: ${TRANSFORMATIONS.map((t) => ` * ${t.name}: ${t.description}`).join('\n')} `, inputSchema: modernizeInputSchema.shape, - outputSchema: { - instructions: z - .array(z.string()) - .optional() - .describe( - 'A list of instructions and shell commands to run the requested modernizations. ' + - 'Each string in the array is a separate step or command.', - ), - }, + outputSchema: modernizeOutputSchema.shape, isLocalOnly: true, - isReadOnly: true, - factory: () => (input) => runModernization(input), + isReadOnly: false, + factory: () => runModernization, }); diff --git a/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts b/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts index 4c5e4cdacdcc..621947cdad91 100644 --- a/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts +++ b/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts @@ -6,68 +6,213 @@ * found in the LICENSE file at https://angular.dev/license */ -import { ModernizeInput, runModernization } from './modernize'; +import { exec } from 'child_process'; +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { promisify } from 'util'; +import { ModernizeOutput, runModernization } from './modernize'; + +const execAsync = promisify(exec); describe('Modernize Tool', () => { - async function getInstructions(input: ModernizeInput): Promise { - const { structuredContent } = await runModernization(input); + let projectDir: string; + let originalPath: string | undefined; + + beforeEach(async () => { + originalPath = process.env.PATH; + projectDir = await mkdtemp(join(tmpdir(), 'angular-modernize-test-')); - if (!structuredContent || !('instructions' in structuredContent)) { - fail('Expected instructions to be present in the result'); + // Create a dummy Angular project structure. + await writeFile( + join(projectDir, 'angular.json'), + JSON.stringify( + { + version: 1, + projects: { + app: { + root: '', + projectType: 'application', + architect: { + build: { + options: { + tsConfig: 'tsconfig.json', + }, + }, + }, + }, + }, + }, + null, + 2, + ), + ); + await writeFile( + join(projectDir, 'package.json'), + JSON.stringify( + { + dependencies: { + '@angular/core': 'latest', + }, + devDependencies: { + '@angular/cli': 'latest', + '@angular-devkit/schematics': 'latest', + typescript: 'latest', + }, + }, + null, + 2, + ), + ); + await writeFile( + join(projectDir, 'tsconfig.base.json'), + JSON.stringify( + { + compilerOptions: { + strict: true, + forceConsistentCasingInFileNames: true, + skipLibCheck: true, + }, + }, + null, + 2, + ), + ); + await writeFile( + join(projectDir, 'tsconfig.json'), + JSON.stringify( + { + extends: './tsconfig.base.json', + compilerOptions: { + outDir: './dist/out-tsc', + }, + }, + null, + 2, + ), + ); + + // Symlink the node_modules directory from the runfiles to the temporary project. + const nodeModulesPath = require + .resolve('@angular/core/package.json') + .replace(/\/@angular\/core\/package\.json$/, ''); + await execAsync(`ln -s ${nodeModulesPath} ${join(projectDir, 'node_modules')}`); + + // Prepend the node_modules/.bin path to the PATH environment variable + // so that `ng` can be found by `execAsync` calls. + process.env.PATH = `${join(nodeModulesPath, '.bin')}:${process.env.PATH}`; + }); + + afterEach(async () => { + process.env.PATH = originalPath; + await rm(projectDir, { recursive: true, force: true }); + }); - return; - } + async function modernize( + dir: string, + file: string, + transformations: string[], + ): Promise<{ structuredContent: ModernizeOutput; newContent: string }> { + const structuredContent = ( + (await runModernization({ directories: [dir], transformations })) as { + structuredContent: ModernizeOutput; + } + ).structuredContent; + const newContent = await readFile(file, 'utf8'); - return structuredContent.instructions; + return { structuredContent, newContent }; } - it('should return an instruction for a single transformation', async () => { - const instructions = await getInstructions({ - transformations: ['self-closing-tags-migration'], - }); + it('can run a single transformation', async () => { + const componentPath = join(projectDir, 'test.component.ts'); + const componentContent = ` + import { Component } from '@angular/core'; - expect(instructions).toEqual([ - 'To run the self-closing-tags-migration migration, execute the following command: ' + - '`ng generate @angular/core:self-closing-tags-migration`.\nFor more information, ' + - 'see https://angular.dev/reference/migrations/self-closing-tags.', + @Component({ + selector: 'app-foo', + template: '', + }) + export class FooComponent {} + `; + await writeFile(componentPath, componentContent); + + const { structuredContent, newContent } = await modernize(projectDir, componentPath, [ + 'self-closing-tag', ]); - }); - it('should return instructions for multiple transformations', async () => { - const instructions = await getInstructions({ - transformations: ['self-closing-tags-migration', 'inject'], - }); - - const expectedInstructions = [ - 'To run the self-closing-tags-migration migration, execute the following command: ' + - '`ng generate @angular/core:self-closing-tags-migration`.\nFor more information, ' + - 'see https://angular.dev/reference/migrations/self-closing-tags.', - 'To run the inject migration, execute the following command: ' + - '`ng generate @angular/core:inject`.\nFor more information, ' + - 'see https://angular.dev/reference/migrations/inject-function.', - ]; - - expect(instructions?.sort()).toEqual(expectedInstructions.sort()); + expect(structuredContent?.stderr).toBe(''); + expect(newContent).toContain(''); + expect(structuredContent?.instructions).toEqual([ + 'Migration self-closing-tag on directory . completed successfully.', + ]); }); - it('should return a link to the best practices page when no transformations are requested', async () => { - const instructions = await getInstructions({ - transformations: [], - }); + it('can run multiple transformations', async () => { + const componentPath = join(projectDir, 'test.component.ts'); + const componentContent = ` + import { Component } from '@angular/core'; - expect(instructions).toEqual([ - 'See https://angular.dev/best-practices for Angular best practices. You can call this ' + - 'tool if you have specific transformation you want to run.', + @Component({ + selector: 'app-foo', + template: '', + }) + export class FooComponent { + show = true; + } + `; + await writeFile(componentPath, componentContent); + + const { structuredContent, newContent } = await modernize(projectDir, componentPath, [ + 'control-flow', + 'self-closing-tag', ]); + + expect(structuredContent?.stderr).toBe(''); + expect(newContent).toContain('@if (show) {}'); }); - it('should return special instructions for standalone migration', async () => { - const instructions = await getInstructions({ - transformations: ['standalone'], - }); + it('can run multiple transformations across multiple directories', async () => { + const subfolder1 = join(projectDir, 'subfolder1'); + await mkdir(subfolder1); + const componentPath1 = join(subfolder1, 'test.component.ts'); + const componentContent1 = ` + import { Component } from '@angular/core'; - expect(instructions?.[0]).toContain( - 'Run the commands in the order listed below, verifying that your code builds and runs between each step:', - ); + @Component({ + selector: 'app-foo', + template: '', + }) + export class FooComponent { + show = true; + } + `; + await writeFile(componentPath1, componentContent1); + + const subfolder2 = join(projectDir, 'subfolder2'); + await mkdir(subfolder2); + const componentPath2 = join(subfolder2, 'test.component.ts'); + const componentContent2 = ` + import { Component } from '@angular/core'; + + @Component({ + selector: 'app-bar', + template: '', + }) + export class BarComponent {} + `; + await writeFile(componentPath2, componentContent2); + + const structuredContent = ( + (await runModernization({ + directories: [subfolder1, subfolder2], + transformations: ['control-flow', 'self-closing-tag'], + })) as { structuredContent: ModernizeOutput } + ).structuredContent; + const newContent1 = await readFile(componentPath1, 'utf8'); + const newContent2 = await readFile(componentPath2, 'utf8'); + + expect(structuredContent?.stderr).toBe(''); + expect(newContent1).toContain('@if (show) {}'); + expect(newContent2).toContain(''); }); }); From aac3736b57bd6c665727489541a120d3480e7c00 Mon Sep 17 00:00:00 2001 From: Alon Mishne Date: Thu, 23 Oct 2025 14:48:34 -0700 Subject: [PATCH 2/3] refactor(@angular/cli): Change modernize_spec to mock generate calls --- .../cli/src/commands/mcp/tools/modernize.ts | 9 +- .../src/commands/mcp/tools/modernize_spec.ts | 290 +++++++----------- .../commands/mcp/tools/process-executor.ts | 16 + 3 files changed, 132 insertions(+), 183 deletions(-) create mode 100644 packages/angular/cli/src/commands/mcp/tools/process-executor.ts diff --git a/packages/angular/cli/src/commands/mcp/tools/modernize.ts b/packages/angular/cli/src/commands/mcp/tools/modernize.ts index b38cd4184fbf..026cf9fc44c9 100644 --- a/packages/angular/cli/src/commands/mcp/tools/modernize.ts +++ b/packages/angular/cli/src/commands/mcp/tools/modernize.ts @@ -6,12 +6,11 @@ * found in the LICENSE file at https://angular.dev/license */ -import { exec } from 'child_process'; import { existsSync } from 'fs'; import { stat } from 'fs/promises'; import { dirname, join, relative } from 'path'; -import { promisify } from 'util'; import { z } from 'zod'; +import { execAsync } from './process-executor'; import { McpToolDeclaration, declareTool } from './tool-registry'; interface Transformation { @@ -96,8 +95,6 @@ const modernizeOutputSchema = z.object({ export type ModernizeInput = z.infer; export type ModernizeOutput = z.infer; -const execAsync = promisify(exec); - function createToolOutput(structuredContent: ModernizeOutput) { return { content: [{ type: 'text' as const, text: JSON.stringify(structuredContent, null, 2) }], @@ -191,8 +188,8 @@ export async function runModernization(input: ModernizeInput) { return createToolOutput({ instructions: instructions.length > 0 ? instructions : undefined, - stdout: stdoutMessages?.join('\n\n') ?? undefined, - stderr: stderrMessages?.join('\n\n') ?? undefined, + stdout: stdoutMessages?.join('\n\n') || undefined, + stderr: stderrMessages?.join('\n\n') || undefined, }); } diff --git a/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts b/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts index 621947cdad91..f4633dc4d616 100644 --- a/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts +++ b/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts @@ -6,213 +6,149 @@ * found in the LICENSE file at https://angular.dev/license */ -import { exec } from 'child_process'; -import { mkdir, mkdtemp, readFile, rm, writeFile } from 'fs/promises'; +import { mkdir, mkdtemp, rm, writeFile } from 'fs/promises'; import { tmpdir } from 'os'; import { join } from 'path'; -import { promisify } from 'util'; import { ModernizeOutput, runModernization } from './modernize'; - -const execAsync = promisify(exec); +import * as processExecutor from './process-executor'; describe('Modernize Tool', () => { + let execAsyncSpy: jasmine.Spy; let projectDir: string; - let originalPath: string | undefined; beforeEach(async () => { - originalPath = process.env.PATH; + // Create a temporary directory and a fake angular.json to satisfy the tool's project root search. projectDir = await mkdtemp(join(tmpdir(), 'angular-modernize-test-')); + await writeFile(join(projectDir, 'angular.json'), JSON.stringify({ version: 1, projects: {} })); - // Create a dummy Angular project structure. - await writeFile( - join(projectDir, 'angular.json'), - JSON.stringify( - { - version: 1, - projects: { - app: { - root: '', - projectType: 'application', - architect: { - build: { - options: { - tsConfig: 'tsconfig.json', - }, - }, - }, - }, - }, - }, - null, - 2, - ), - ); - await writeFile( - join(projectDir, 'package.json'), - JSON.stringify( - { - dependencies: { - '@angular/core': 'latest', - }, - devDependencies: { - '@angular/cli': 'latest', - '@angular-devkit/schematics': 'latest', - typescript: 'latest', - }, - }, - null, - 2, - ), - ); - await writeFile( - join(projectDir, 'tsconfig.base.json'), - JSON.stringify( - { - compilerOptions: { - strict: true, - forceConsistentCasingInFileNames: true, - skipLibCheck: true, - }, - }, - null, - 2, - ), - ); - await writeFile( - join(projectDir, 'tsconfig.json'), - JSON.stringify( - { - extends: './tsconfig.base.json', - compilerOptions: { - outDir: './dist/out-tsc', - }, - }, - null, - 2, - ), - ); - - // Symlink the node_modules directory from the runfiles to the temporary project. - const nodeModulesPath = require - .resolve('@angular/core/package.json') - .replace(/\/@angular\/core\/package\.json$/, ''); - await execAsync(`ln -s ${nodeModulesPath} ${join(projectDir, 'node_modules')}`); - - // Prepend the node_modules/.bin path to the PATH environment variable - // so that `ng` can be found by `execAsync` calls. - process.env.PATH = `${join(nodeModulesPath, '.bin')}:${process.env.PATH}`; + // Spy on the execAsync function from our new module. + execAsyncSpy = spyOn(processExecutor, 'execAsync').and.resolveTo({ stdout: '', stderr: '' }); }); afterEach(async () => { - process.env.PATH = originalPath; await rm(projectDir, { recursive: true, force: true }); }); - async function modernize( - dir: string, - file: string, - transformations: string[], - ): Promise<{ structuredContent: ModernizeOutput; newContent: string }> { - const structuredContent = ( - (await runModernization({ directories: [dir], transformations })) as { - structuredContent: ModernizeOutput; - } - ).structuredContent; - const newContent = await readFile(file, 'utf8'); - - return { structuredContent, newContent }; - } + it('should return instructions if no transformations are provided', async () => { + const { structuredContent } = (await runModernization({})) as { + structuredContent: ModernizeOutput; + }; - it('can run a single transformation', async () => { - const componentPath = join(projectDir, 'test.component.ts'); - const componentContent = ` - import { Component } from '@angular/core'; - - @Component({ - selector: 'app-foo', - template: '', - }) - export class FooComponent {} - `; - await writeFile(componentPath, componentContent); - - const { structuredContent, newContent } = await modernize(projectDir, componentPath, [ - 'self-closing-tag', + expect(execAsyncSpy).not.toHaveBeenCalled(); + expect(structuredContent?.instructions).toEqual([ + 'See https://angular.dev/best-practices for Angular best practices. ' + + 'You can call this tool if you have specific transformation you want to run.', ]); + }); - expect(structuredContent?.stderr).toBe(''); - expect(newContent).toContain(''); + it('should return instructions if no directories are provided', async () => { + const { structuredContent } = (await runModernization({ + transformations: ['control-flow'], + })) as { + structuredContent: ModernizeOutput; + }; + + expect(execAsyncSpy).not.toHaveBeenCalled(); expect(structuredContent?.instructions).toEqual([ - 'Migration self-closing-tag on directory . completed successfully.', + 'Provide this tool with a list of directory paths in your workspace ' + + 'to run the modernization on.', ]); }); - it('can run multiple transformations', async () => { - const componentPath = join(projectDir, 'test.component.ts'); - const componentContent = ` - import { Component } from '@angular/core'; - - @Component({ - selector: 'app-foo', - template: '', - }) - export class FooComponent { - show = true; - } - `; - await writeFile(componentPath, componentContent); - - const { structuredContent, newContent } = await modernize(projectDir, componentPath, [ - 'control-flow', - 'self-closing-tag', + it('can run a single transformation', async () => { + const { structuredContent } = (await runModernization({ + directories: [projectDir], + transformations: ['self-closing-tag'], + })) as { structuredContent: ModernizeOutput }; + + expect(execAsyncSpy).toHaveBeenCalledOnceWith( + 'ng generate @angular/core:self-closing-tag --path .', + { cwd: projectDir }, + ); + expect(structuredContent?.stderr).toBeUndefined(); + expect(structuredContent?.instructions).toEqual([ + 'Migration self-closing-tag on directory . completed successfully.', ]); + }); - expect(structuredContent?.stderr).toBe(''); - expect(newContent).toContain('@if (show) {}'); + it('can run multiple transformations', async () => { + const { structuredContent } = (await runModernization({ + directories: [projectDir], + transformations: ['control-flow', 'self-closing-tag'], + })) as { structuredContent: ModernizeOutput }; + + expect(execAsyncSpy).toHaveBeenCalledTimes(2); + expect(execAsyncSpy).toHaveBeenCalledWith('ng generate @angular/core:control-flow --path .', { + cwd: projectDir, + }); + expect(execAsyncSpy).toHaveBeenCalledWith( + 'ng generate @angular/core:self-closing-tag --path .', + { cwd: projectDir }, + ); + expect(structuredContent?.stderr).toBeUndefined(); + expect(structuredContent?.instructions).toEqual( + jasmine.arrayWithExactContents([ + 'Migration control-flow on directory . completed successfully.', + 'Migration self-closing-tag on directory . completed successfully.', + ]), + ); }); it('can run multiple transformations across multiple directories', async () => { const subfolder1 = join(projectDir, 'subfolder1'); - await mkdir(subfolder1); - const componentPath1 = join(subfolder1, 'test.component.ts'); - const componentContent1 = ` - import { Component } from '@angular/core'; - - @Component({ - selector: 'app-foo', - template: '', - }) - export class FooComponent { - show = true; - } - `; - await writeFile(componentPath1, componentContent1); - const subfolder2 = join(projectDir, 'subfolder2'); + await mkdir(subfolder1); await mkdir(subfolder2); - const componentPath2 = join(subfolder2, 'test.component.ts'); - const componentContent2 = ` - import { Component } from '@angular/core'; - - @Component({ - selector: 'app-bar', - template: '', - }) - export class BarComponent {} - `; - await writeFile(componentPath2, componentContent2); - - const structuredContent = ( - (await runModernization({ - directories: [subfolder1, subfolder2], - transformations: ['control-flow', 'self-closing-tag'], - })) as { structuredContent: ModernizeOutput } - ).structuredContent; - const newContent1 = await readFile(componentPath1, 'utf8'); - const newContent2 = await readFile(componentPath2, 'utf8'); - - expect(structuredContent?.stderr).toBe(''); - expect(newContent1).toContain('@if (show) {}'); - expect(newContent2).toContain(''); + + const { structuredContent } = (await runModernization({ + directories: [subfolder1, subfolder2], + transformations: ['control-flow', 'self-closing-tag'], + })) as { structuredContent: ModernizeOutput }; + + expect(execAsyncSpy).toHaveBeenCalledTimes(4); + expect(execAsyncSpy).toHaveBeenCalledWith( + 'ng generate @angular/core:control-flow --path subfolder1', + { cwd: projectDir }, + ); + expect(execAsyncSpy).toHaveBeenCalledWith( + 'ng generate @angular/core:self-closing-tag --path subfolder1', + { cwd: projectDir }, + ); + expect(execAsyncSpy).toHaveBeenCalledWith( + 'ng generate @angular/core:control-flow --path subfolder2', + { cwd: projectDir }, + ); + expect(execAsyncSpy).toHaveBeenCalledWith( + 'ng generate @angular/core:self-closing-tag --path subfolder2', + { cwd: projectDir }, + ); + expect(structuredContent?.stderr).toBeUndefined(); + expect(structuredContent?.instructions).toEqual( + jasmine.arrayWithExactContents([ + 'Migration control-flow on directory subfolder1 completed successfully.', + 'Migration self-closing-tag on directory subfolder1 completed successfully.', + 'Migration control-flow on directory subfolder2 completed successfully.', + 'Migration self-closing-tag on directory subfolder2 completed successfully.', + ]), + ); + }); + + it('should report errors from transformations', async () => { + // Simulate a failed execution + execAsyncSpy.and.rejectWith(new Error('Command failed with error')); + + const { structuredContent } = (await runModernization({ + directories: [projectDir], + transformations: ['self-closing-tag'], + })) as { structuredContent: ModernizeOutput }; + + expect(execAsyncSpy).toHaveBeenCalledOnceWith( + 'ng generate @angular/core:self-closing-tag --path .', + { cwd: projectDir }, + ); + expect(structuredContent?.stderr).toContain('Command failed with error'); + expect(structuredContent?.instructions).toEqual([ + 'Migration self-closing-tag on directory . failed.', + ]); }); }); diff --git a/packages/angular/cli/src/commands/mcp/tools/process-executor.ts b/packages/angular/cli/src/commands/mcp/tools/process-executor.ts new file mode 100644 index 000000000000..85a6a4e49d3e --- /dev/null +++ b/packages/angular/cli/src/commands/mcp/tools/process-executor.ts @@ -0,0 +1,16 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.dev/license + */ + +import { exec } from 'child_process'; +import { promisify } from 'util'; + +/** + * A promisified version of the Node.js `exec` function. + * This is isolated in its own file to allow for easy mocking in tests. + */ +export const execAsync = promisify(exec); From a1f5b94ed57ff7945f7d9408f01979fbc433dc53 Mon Sep 17 00:00:00 2001 From: Alon Mishne Date: Tue, 28 Oct 2025 12:54:03 -0700 Subject: [PATCH 3/3] refactor(@angular/cli): Change modernize to use a host interface for OS/FS operations --- packages/angular/cli/src/commands/mcp/host.ts | 130 +++++++++++++++ .../cli/src/commands/mcp/tools/modernize.ts | 31 ++-- .../src/commands/mcp/tools/modernize_spec.ts | 154 ++++++++++++------ .../commands/mcp/tools/process-executor.ts | 16 -- 4 files changed, 253 insertions(+), 78 deletions(-) create mode 100644 packages/angular/cli/src/commands/mcp/host.ts delete mode 100644 packages/angular/cli/src/commands/mcp/tools/process-executor.ts diff --git a/packages/angular/cli/src/commands/mcp/host.ts b/packages/angular/cli/src/commands/mcp/host.ts new file mode 100644 index 000000000000..ad57b03550bf --- /dev/null +++ b/packages/angular/cli/src/commands/mcp/host.ts @@ -0,0 +1,130 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.dev/license + */ + +/** + * @fileoverview + * This file defines an abstraction layer for operating-system or file-system operations, such as + * command execution. This allows for easier testing by enabling the injection of mock or + * test-specific implementations. + */ + +import { existsSync as nodeExistsSync } from 'fs'; +import { spawn } from 'node:child_process'; +import { Stats } from 'node:fs'; +import { stat } from 'node:fs/promises'; + +/** + * An error thrown when a command fails to execute. + */ +export class CommandError extends Error { + constructor( + message: string, + public readonly stdout: string, + public readonly stderr: string, + public readonly code: number | null, + ) { + super(message); + } +} + +/** + * An abstraction layer for operating-system or file-system operations. + */ +export interface Host { + /** + * Gets the stats of a file or directory. + * @param path The path to the file or directory. + * @returns A promise that resolves to the stats. + */ + stat(path: string): Promise; + + /** + * Checks if a path exists on the file system. + * @param path The path to check. + * @returns A boolean indicating whether the path exists. + */ + existsSync(path: string): boolean; + + /** + * Spawns a child process and returns a promise that resolves with the process's + * output or rejects with a structured error. + * @param command The command to run. + * @param args The arguments to pass to the command. + * @param options Options for the child process. + * @returns A promise that resolves with the standard output and standard error of the command. + */ + runCommand( + command: string, + args: readonly string[], + options?: { + timeout?: number; + stdio?: 'pipe' | 'ignore'; + cwd?: string; + env?: Record; + }, + ): Promise<{ stdout: string; stderr: string }>; +} + +/** + * A concrete implementation of the `Host` interface that runs on a local workspace. + */ +export const LocalWorkspaceHost: Host = { + stat, + existsSync: nodeExistsSync, + runCommand: async ( + command: string, + args: readonly string[], + options: { + timeout?: number; + stdio?: 'pipe' | 'ignore'; + cwd?: string; + env?: Record; + } = {}, + ): Promise<{ stdout: string; stderr: string }> => { + const signal = options.timeout ? AbortSignal.timeout(options.timeout) : undefined; + + return new Promise((resolve, reject) => { + const childProcess = spawn(command, args, { + shell: false, + stdio: options.stdio ?? 'pipe', + signal, + cwd: options.cwd, + env: { + ...process.env, + ...options.env, + }, + }); + + let stdout = ''; + childProcess.stdout?.on('data', (data) => (stdout += data.toString())); + + let stderr = ''; + childProcess.stderr?.on('data', (data) => (stderr += data.toString())); + + childProcess.on('close', (code) => { + if (code === 0) { + resolve({ stdout, stderr }); + } else { + const message = `Process exited with code ${code}.`; + reject(new CommandError(message, stdout, stderr, code)); + } + }); + + childProcess.on('error', (err) => { + if (err.name === 'AbortError') { + const message = `Process timed out.`; + reject(new CommandError(message, stdout, stderr, null)); + + return; + } + const message = `Process failed with error: ${err.message}`; + reject(new CommandError(message, stdout, stderr, null)); + }); + }); + }, +}; diff --git a/packages/angular/cli/src/commands/mcp/tools/modernize.ts b/packages/angular/cli/src/commands/mcp/tools/modernize.ts index 026cf9fc44c9..2ad3e737578c 100644 --- a/packages/angular/cli/src/commands/mcp/tools/modernize.ts +++ b/packages/angular/cli/src/commands/mcp/tools/modernize.ts @@ -6,11 +6,9 @@ * found in the LICENSE file at https://angular.dev/license */ -import { existsSync } from 'fs'; -import { stat } from 'fs/promises'; import { dirname, join, relative } from 'path'; import { z } from 'zod'; -import { execAsync } from './process-executor'; +import { CommandError, Host, LocalWorkspaceHost } from '../host'; import { McpToolDeclaration, declareTool } from './tool-registry'; interface Transformation { @@ -102,10 +100,10 @@ function createToolOutput(structuredContent: ModernizeOutput) { }; } -function findAngularJsonDir(startDir: string): string | null { +function findAngularJsonDir(startDir: string, host: Host): string | null { let currentDir = startDir; while (true) { - if (existsSync(join(currentDir, 'angular.json'))) { + if (host.existsSync(join(currentDir, 'angular.json'))) { return currentDir; } const parentDir = dirname(currentDir); @@ -116,7 +114,7 @@ function findAngularJsonDir(startDir: string): string | null { } } -export async function runModernization(input: ModernizeInput) { +export async function runModernization(input: ModernizeInput, host: Host) { const transformationNames = input.transformations ?? []; const directories = input.directories ?? []; @@ -138,9 +136,9 @@ export async function runModernization(input: ModernizeInput) { } const firstDir = directories[0]; - const executionDir = (await stat(firstDir)).isDirectory() ? firstDir : dirname(firstDir); + const executionDir = (await host.stat(firstDir)).isDirectory() ? firstDir : dirname(firstDir); - const angularProjectRoot = findAngularJsonDir(executionDir); + const angularProjectRoot = findAngularJsonDir(executionDir, host); if (!angularProjectRoot) { return createToolOutput({ instructions: ['Could not find an angular.json file in the current or parent directories.'], @@ -164,9 +162,12 @@ export async function runModernization(input: ModernizeInput) { // Simple case, run the command. for (const dir of directories) { const relativePath = relative(angularProjectRoot, dir) || '.'; - const command = `ng generate @angular/core:${transformation.name} --path ${relativePath}`; + const command = 'ng'; + const args = ['generate', `@angular/core:${transformation.name}`, '--path', relativePath]; try { - const { stdout, stderr } = await execAsync(command, { cwd: angularProjectRoot }); + const { stdout, stderr } = await host.runCommand(command, args, { + cwd: angularProjectRoot, + }); if (stdout) { stdoutMessages.push(stdout); } @@ -177,6 +178,14 @@ export async function runModernization(input: ModernizeInput) { `Migration ${transformation.name} on directory ${relativePath} completed successfully.`, ); } catch (e) { + if (e instanceof CommandError) { + if (e.stdout) { + stdoutMessages.push(e.stdout); + } + if (e.stderr) { + stderrMessages.push(e.stderr); + } + } stderrMessages.push((e as Error).message); instructions.push( `Migration ${transformation.name} on directory ${relativePath} failed.`, @@ -228,5 +237,5 @@ ${TRANSFORMATIONS.map((t) => ` * ${t.name}: ${t.description}`).join('\n')} outputSchema: modernizeOutputSchema.shape, isLocalOnly: true, isReadOnly: false, - factory: () => runModernization, + factory: () => (input) => runModernization(input, LocalWorkspaceHost), }); diff --git a/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts b/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts index f4633dc4d616..a00894dde5f6 100644 --- a/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts +++ b/packages/angular/cli/src/commands/mcp/tools/modernize_spec.ts @@ -6,23 +6,29 @@ * found in the LICENSE file at https://angular.dev/license */ +import { Stats } from 'fs'; import { mkdir, mkdtemp, rm, writeFile } from 'fs/promises'; import { tmpdir } from 'os'; import { join } from 'path'; +import * as host from '../host'; import { ModernizeOutput, runModernization } from './modernize'; -import * as processExecutor from './process-executor'; describe('Modernize Tool', () => { - let execAsyncSpy: jasmine.Spy; let projectDir: string; + let mockHost: host.Host; beforeEach(async () => { // Create a temporary directory and a fake angular.json to satisfy the tool's project root search. projectDir = await mkdtemp(join(tmpdir(), 'angular-modernize-test-')); await writeFile(join(projectDir, 'angular.json'), JSON.stringify({ version: 1, projects: {} })); - // Spy on the execAsync function from our new module. - execAsyncSpy = spyOn(processExecutor, 'execAsync').and.resolveTo({ stdout: '', stderr: '' }); + mockHost = { + runCommand: jasmine.createSpy('runCommand').and.resolveTo({ stdout: '', stderr: '' }), + stat: jasmine.createSpy('stat').and.resolveTo({ isDirectory: () => true } as Stats), + existsSync: jasmine.createSpy('existsSync').and.callFake((p: string) => { + return p === join(projectDir, 'angular.json'); + }), + }; }); afterEach(async () => { @@ -30,11 +36,11 @@ describe('Modernize Tool', () => { }); it('should return instructions if no transformations are provided', async () => { - const { structuredContent } = (await runModernization({})) as { + const { structuredContent } = (await runModernization({}, mockHost)) as { structuredContent: ModernizeOutput; }; - expect(execAsyncSpy).not.toHaveBeenCalled(); + expect(mockHost.runCommand).not.toHaveBeenCalled(); expect(structuredContent?.instructions).toEqual([ 'See https://angular.dev/best-practices for Angular best practices. ' + 'You can call this tool if you have specific transformation you want to run.', @@ -42,13 +48,16 @@ describe('Modernize Tool', () => { }); it('should return instructions if no directories are provided', async () => { - const { structuredContent } = (await runModernization({ - transformations: ['control-flow'], - })) as { + const { structuredContent } = (await runModernization( + { + transformations: ['control-flow'], + }, + mockHost, + )) as { structuredContent: ModernizeOutput; }; - expect(execAsyncSpy).not.toHaveBeenCalled(); + expect(mockHost.runCommand).not.toHaveBeenCalled(); expect(structuredContent?.instructions).toEqual([ 'Provide this tool with a list of directory paths in your workspace ' + 'to run the modernization on.', @@ -56,33 +65,44 @@ describe('Modernize Tool', () => { }); it('can run a single transformation', async () => { - const { structuredContent } = (await runModernization({ - directories: [projectDir], - transformations: ['self-closing-tag'], - })) as { structuredContent: ModernizeOutput }; - - expect(execAsyncSpy).toHaveBeenCalledOnceWith( - 'ng generate @angular/core:self-closing-tag --path .', + const { structuredContent } = (await runModernization( + { + directories: [projectDir], + transformations: ['self-closing-tag'], + }, + mockHost, + )) as { structuredContent: ModernizeOutput }; + + expect(mockHost.runCommand).toHaveBeenCalledOnceWith( + 'ng', + ['generate', '@angular/core:self-closing-tag', '--path', '.'], { cwd: projectDir }, ); - expect(structuredContent?.stderr).toBeUndefined(); expect(structuredContent?.instructions).toEqual([ 'Migration self-closing-tag on directory . completed successfully.', ]); }); it('can run multiple transformations', async () => { - const { structuredContent } = (await runModernization({ - directories: [projectDir], - transformations: ['control-flow', 'self-closing-tag'], - })) as { structuredContent: ModernizeOutput }; - - expect(execAsyncSpy).toHaveBeenCalledTimes(2); - expect(execAsyncSpy).toHaveBeenCalledWith('ng generate @angular/core:control-flow --path .', { - cwd: projectDir, - }); - expect(execAsyncSpy).toHaveBeenCalledWith( - 'ng generate @angular/core:self-closing-tag --path .', + const { structuredContent } = (await runModernization( + { + directories: [projectDir], + transformations: ['control-flow', 'self-closing-tag'], + }, + mockHost, + )) as { structuredContent: ModernizeOutput }; + + expect(mockHost.runCommand).toHaveBeenCalledTimes(2); + expect(mockHost.runCommand).toHaveBeenCalledWith( + 'ng', + ['generate', '@angular/core:control-flow', '--path', '.'], + { + cwd: projectDir, + }, + ); + expect(mockHost.runCommand).toHaveBeenCalledWith( + 'ng', + ['generate', '@angular/core:self-closing-tag', '--path', '.'], { cwd: projectDir }, ); expect(structuredContent?.stderr).toBeUndefined(); @@ -100,26 +120,33 @@ describe('Modernize Tool', () => { await mkdir(subfolder1); await mkdir(subfolder2); - const { structuredContent } = (await runModernization({ - directories: [subfolder1, subfolder2], - transformations: ['control-flow', 'self-closing-tag'], - })) as { structuredContent: ModernizeOutput }; - - expect(execAsyncSpy).toHaveBeenCalledTimes(4); - expect(execAsyncSpy).toHaveBeenCalledWith( - 'ng generate @angular/core:control-flow --path subfolder1', + const { structuredContent } = (await runModernization( + { + directories: [subfolder1, subfolder2], + transformations: ['control-flow', 'self-closing-tag'], + }, + mockHost, + )) as { structuredContent: ModernizeOutput }; + + expect(mockHost.runCommand).toHaveBeenCalledTimes(4); + expect(mockHost.runCommand).toHaveBeenCalledWith( + 'ng', + ['generate', '@angular/core:control-flow', '--path', 'subfolder1'], { cwd: projectDir }, ); - expect(execAsyncSpy).toHaveBeenCalledWith( - 'ng generate @angular/core:self-closing-tag --path subfolder1', + expect(mockHost.runCommand).toHaveBeenCalledWith( + 'ng', + ['generate', '@angular/core:self-closing-tag', '--path', 'subfolder1'], { cwd: projectDir }, ); - expect(execAsyncSpy).toHaveBeenCalledWith( - 'ng generate @angular/core:control-flow --path subfolder2', + expect(mockHost.runCommand).toHaveBeenCalledWith( + 'ng', + ['generate', '@angular/core:control-flow', '--path', 'subfolder2'], { cwd: projectDir }, ); - expect(execAsyncSpy).toHaveBeenCalledWith( - 'ng generate @angular/core:self-closing-tag --path subfolder2', + expect(mockHost.runCommand).toHaveBeenCalledWith( + 'ng', + ['generate', '@angular/core:self-closing-tag', '--path', 'subfolder2'], { cwd: projectDir }, ); expect(structuredContent?.stderr).toBeUndefined(); @@ -133,19 +160,44 @@ describe('Modernize Tool', () => { ); }); + it('should return an error if angular.json is not found', async () => { + (mockHost.existsSync as jasmine.Spy).and.returnValue(false); + + const { structuredContent } = (await runModernization( + { + directories: [projectDir], + transformations: ['self-closing-tag'], + }, + mockHost, + )) as { structuredContent: ModernizeOutput }; + + expect(mockHost.runCommand).not.toHaveBeenCalled(); + expect(structuredContent?.instructions).toEqual([ + 'Could not find an angular.json file in the current or parent directories.', + ]); + }); + it('should report errors from transformations', async () => { // Simulate a failed execution - execAsyncSpy.and.rejectWith(new Error('Command failed with error')); - - const { structuredContent } = (await runModernization({ - directories: [projectDir], - transformations: ['self-closing-tag'], - })) as { structuredContent: ModernizeOutput }; + (mockHost.runCommand as jasmine.Spy).and.rejectWith( + new host.CommandError('Command failed with error', 'stdout', 'stderr', 1), + ); - expect(execAsyncSpy).toHaveBeenCalledOnceWith( - 'ng generate @angular/core:self-closing-tag --path .', + const { structuredContent } = (await runModernization( + { + directories: [projectDir], + transformations: ['self-closing-tag'], + }, + mockHost, + )) as { structuredContent: ModernizeOutput }; + + expect(mockHost.runCommand).toHaveBeenCalledOnceWith( + 'ng', + ['generate', '@angular/core:self-closing-tag', '--path', '.'], { cwd: projectDir }, ); + expect(structuredContent?.stdout).toContain('stdout'); + expect(structuredContent?.stderr).toContain('stderr'); expect(structuredContent?.stderr).toContain('Command failed with error'); expect(structuredContent?.instructions).toEqual([ 'Migration self-closing-tag on directory . failed.', diff --git a/packages/angular/cli/src/commands/mcp/tools/process-executor.ts b/packages/angular/cli/src/commands/mcp/tools/process-executor.ts deleted file mode 100644 index 85a6a4e49d3e..000000000000 --- a/packages/angular/cli/src/commands/mcp/tools/process-executor.ts +++ /dev/null @@ -1,16 +0,0 @@ -/** - * @license - * Copyright Google LLC All Rights Reserved. - * - * Use of this source code is governed by an MIT-style license that can be - * found in the LICENSE file at https://angular.dev/license - */ - -import { exec } from 'child_process'; -import { promisify } from 'util'; - -/** - * A promisified version of the Node.js `exec` function. - * This is isolated in its own file to allow for easy mocking in tests. - */ -export const execAsync = promisify(exec);