refactor(@angular/cli): Address review comments.

Author: amishne

packages/angular/cli/BUILD.bazel

@@ -38,6 +38,7 @@ ts_project(
         ],
         exclude = [
             "**/*_spec.ts",
+            "**/testing/**",
         ],
     ) + [
         # These files are generated from the JSON schema
@@ -116,7 +117,10 @@ ts_project(
     name = "angular-cli_test_lib",
     testonly = True,
     srcs = glob(
-        include = ["**/*_spec.ts"],
+        include = [
+            "**/*_spec.ts",
+            "**/testing/**",
+        ],
         exclude = [
             # NB: we need to exclude the nested node_modules that is laid out by yarn workspaces
             "node_modules/**",

packages/angular/cli/src/commands/mcp/dev-server.ts

@@ -97,9 +97,8 @@ export class LocalDevServer implements DevServer {
     if (this.project) {
       args.push(this.project);
     }
-    if (this.port) {
-      args.push(`--port=${this.port}`);
-    }
+
+    args.push(`--port=${this.port}`);
 
     this.devServerProcess = this.host.spawn('ng', args, { stdio: 'pipe' });
     this.devServerProcess.stdout?.on('data', (data) => {

packages/angular/cli/src/commands/mcp/host.ts

@@ -25,8 +25,7 @@ import { createServer } from 'node:net';
 export class CommandError extends Error {
   constructor(
     message: string,
-    public readonly stdout: string,
-    public readonly stderr: string,
+    public readonly logs: string[],
     public readonly code: number | null,
   ) {
     super(message);
@@ -68,7 +67,7 @@ export interface Host {
       cwd?: string;
       env?: Record<string, string>;
     },
-  ): Promise<{ stdout: string; stderr: string }>;
+  ): Promise<{ logs: string[] }>;
 
   /**
    * Spawns a long-running child process and returns the `ChildProcess` object.
@@ -110,7 +109,7 @@ export const LocalWorkspaceHost: Host = {
       cwd?: string;
       env?: Record<string, string>;
     } = {},
-  ): Promise<{ stdout: string; stderr: string }> => {
+  ): Promise<{ logs: string[] }> => {
     const signal = options.timeout ? AbortSignal.timeout(options.timeout) : undefined;
 
     return new Promise((resolve, reject) => {
@@ -125,30 +124,28 @@ export const LocalWorkspaceHost: Host = {
         },
       });
 
-      let stdout = '';
-      childProcess.stdout?.on('data', (data) => (stdout += data.toString()));
-
-      let stderr = '';
-      childProcess.stderr?.on('data', (data) => (stderr += data.toString()));
+      const logs: string[] = [];
+      childProcess.stdout?.on('data', (data) => logs.push(data.toString()));
+      childProcess.stderr?.on('data', (data) => logs.push(data.toString()));
 
       childProcess.on('close', (code) => {
         if (code === 0) {
-          resolve({ stdout, stderr });
+          resolve({ logs });
         } else {
           const message = `Process exited with code ${code}.`;
-          reject(new CommandError(message, stdout, stderr, code));
+          reject(new CommandError(message, logs, code));
         }
       });
 
       childProcess.on('error', (err) => {
         if (err.name === 'AbortError') {
           const message = `Process timed out.`;
-          reject(new CommandError(message, stdout, stderr, null));
+          reject(new CommandError(message, logs, null));
 
           return;
         }
         const message = `Process failed with error: ${err.message}`;
-        reject(new CommandError(message, stdout, stderr, null));
+        reject(new CommandError(message, logs, null));
       });
     });
   },

packages/angular/cli/src/commands/mcp/testing/mock-host.ts

@@ -0,0 +1,21 @@
+/**
+ * @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 { Host } from '../host';
+
+/**
+ * A mock implementation of the `Host` interface for testing purposes.
+ * This class allows spying on host methods and controlling their return values.
+ */
+export class MockHost implements Host {
+  runCommand = jasmine.createSpy('runCommand').and.resolveTo({ stdout: '', stderr: '' });
+  stat = jasmine.createSpy('stat');
+  existsSync = jasmine.createSpy('existsSync');
+  spawn = jasmine.createSpy('spawn');
+  getAvailablePort = jasmine.createSpy('getAvailablePort');
+}

packages/angular/cli/src/commands/mcp/tools/build.ts

@@ -11,84 +11,68 @@ import { CommandError, Host, LocalWorkspaceHost } from '../host';
 import { createStructuredContentOutput } from '../utils';
 import { McpToolDeclaration, declareTool } from './tool-registry';
 
-const CONFIGURATIONS = {
-  development: {
-    args: '-c development',
-  },
-  production: {
-    args: '',
-  },
-};
+const DEFAULT_CONFIGURATION = 'development';
 
-const DEFAULT_CONFIGURATION_NAME = 'development';
-
-const BUILD_STATUSES = ['success', 'failure'] as const;
-type BuildStatus = (typeof BUILD_STATUSES)[number];
+const buildStatusSchema = z.enum(['success', 'failure']);
+type BuildStatus = z.infer<typeof buildStatusSchema>;
 
 const buildToolInputSchema = z.object({
   project: z
     .string()
     .optional()
     .describe(
-      'Which project to build in a monorepo context. If not provided, builds the top-level project.',
+      'Which project to build in a monorepo context. If not provided, builds the default project.',
     ),
   configuration: z
-    .enum(Object.keys(CONFIGURATIONS) as [string, ...string[]])
+    .string()
     .optional()
     .describe('Which build configuration to use. Defaults to "development".'),
 });
 
 export type BuildToolInput = z.infer<typeof buildToolInputSchema>;
 
 const buildToolOutputSchema = z.object({
-  status: z.enum(BUILD_STATUSES).describe('Build status.'),
-  stdout: z.string().optional().describe('The standard output from `ng build`.'),
-  stderr: z.string().optional().describe('The standard error from `ng build`.'),
+  status: buildStatusSchema.describe('Build status.'),
+  logs: z.array(z.string()).optional().describe('Output logs from `ng build`.'),
   path: z.string().optional().describe('The output location for the build, if successful.'),
 });
 
 export type BuildToolOutput = z.infer<typeof buildToolOutputSchema>;
 
 export async function runBuild(input: BuildToolInput, host: Host) {
-  const configurationName = input.configuration ?? DEFAULT_CONFIGURATION_NAME;
-  const configuration = CONFIGURATIONS[configurationName as keyof typeof CONFIGURATIONS];
-
   // Build "ng"'s command line.
   const args = ['build'];
   if (input.project) {
     args.push(input.project);
   }
-  if (configuration.args) {
-    args.push(configuration.args);
-  }
+  args.push(`-c ${input.configuration ?? DEFAULT_CONFIGURATION}`);
 
   let status: BuildStatus = 'success';
-  let stdout = '';
-  let stderr = '';
+  let logs: string[] = [];
   let outputPath: string | undefined;
 
   try {
-    const result = await host.runCommand('ng', args);
-    stdout = result.stdout;
-    stderr = result.stderr;
-    const match = stdout.match(/Output location: (.*)/);
-    if (match) {
-      outputPath = match[1].trim();
-    }
+    logs = (await host.runCommand('ng', args)).logs;
   } catch (e) {
     status = 'failure';
     if (e instanceof CommandError) {
-      stdout = e.stdout;
-      stderr = e.stderr;
+      logs = e.logs;
     } else if (e instanceof Error) {
-      stderr = e.message;
+      logs = [e.message];
+    }
+  }
+
+  for (const line of logs) {
+    const match = line.match(/Output location: (.*)/);
+    if (match) {
+      outputPath = match[1].trim();
+      break;
     }
   }
 
   const structuredContent: BuildToolOutput = {
     status,
-    stdout,
-    stderr,
+    logs,
     path: outputPath,
   };
 

packages/angular/cli/src/commands/mcp/tools/build_spec.ts

@@ -7,17 +7,18 @@
  */
 
 import { CommandError, Host } from '../host';
+import { MockHost } from '../testing/mock-host';
 import { runBuild } from './build';
 
 describe('Build Tool', () => {
-  let mockHost: Host;
+  let mockHost: MockHost;
 
   beforeEach(() => {
     mockHost = {
-      runCommand: jasmine.createSpy('runCommand').and.resolveTo({ stdout: '', stderr: '' }),
-      stat: jasmine.createSpy('stat'),
-      existsSync: jasmine.createSpy('existsSync'),
-    } as Partial<Host> as Host;
+      runCommand: jasmine.createSpy<Host['runCommand']>('runCommand').and.resolveTo({ logs: [] }),
+      stat: jasmine.createSpy<Host['stat']>('stat'),
+      existsSync: jasmine.createSpy<Host['existsSync']>('existsSync'),
+    } as Partial<MockHost> as MockHost;
   });
 
   it('should construct the command correctly with default configuration', async () => {
@@ -34,56 +35,58 @@ describe('Build Tool', () => {
     ]);
   });
 
-  it('should construct the command correctly for production configuration', async () => {
-    await runBuild({ configuration: 'production' }, mockHost);
-    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['build']);
+  it('should construct the command correctly for a custom configuration', async () => {
+    await runBuild({ configuration: 'myconfig' }, mockHost);
+    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['build', '-c myconfig']);
   });
 
   it('should handle a successful build and extract the output path and logs', async () => {
-    const buildStdout = 'Build successful!\nSome other log lines...\nOutput location: dist/my-app';
-    (mockHost.runCommand as jasmine.Spy).and.resolveTo({
-      stdout: buildStdout,
-      stderr: 'some warning',
+    const buildLogs = [
+      'Build successful!',
+      'Some other log lines...',
+      'some warning',
+      'Output location: dist/my-app',
+    ];
+    mockHost.runCommand.and.resolveTo({
+      logs: buildLogs,
     });
 
     const { structuredContent } = await runBuild({ project: 'my-app' }, mockHost);
 
     expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['build', 'my-app', '-c development']);
     expect(structuredContent.status).toBe('success');
-    expect(structuredContent.stdout).toBe(buildStdout);
-    expect(structuredContent.stderr).toBe('some warning');
+    expect(structuredContent.logs).toEqual(buildLogs);
     expect(structuredContent.path).toBe('dist/my-app');
   });
 
   it('should handle a failed build and capture logs', async () => {
-    const error = new CommandError(
-      'Build failed',
-      'Some output before the crash.',
-      'Error: Something went wrong!',
-      1,
-    );
-    (mockHost.runCommand as jasmine.Spy).and.rejectWith(error);
+    const buildLogs = ['Some output before the crash.', 'Error: Something went wrong!'];
+    const error = new CommandError('Build failed', buildLogs, 1);
+    mockHost.runCommand.and.rejectWith(error);
 
     const { structuredContent } = await runBuild(
       { project: 'my-failed-app', configuration: 'production' },
       mockHost,
     );
 
-    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['build', 'my-failed-app']);
+    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', [
+      'build',
+      'my-failed-app',
+      '-c production',
+    ]);
     expect(structuredContent.status).toBe('failure');
-    expect(structuredContent.stdout).toBe('Some output before the crash.');
-    expect(structuredContent.stderr).toBe('Error: Something went wrong!');
+    expect(structuredContent.logs).toEqual(buildLogs);
     expect(structuredContent.path).toBeUndefined();
   });
 
   it('should handle builds where the output path is not found in logs', async () => {
-    const buildStdout = 'Build finished, but we could not find the output path string.';
-    (mockHost.runCommand as jasmine.Spy).and.resolveTo({ stdout: buildStdout, stderr: '' });
+    const buildLogs = ["Some logs that don't match any output path."];
+    mockHost.runCommand.and.resolveTo({ logs: buildLogs });
 
     const { structuredContent } = await runBuild({}, mockHost);
 
     expect(structuredContent.status).toBe('success');
-    expect(structuredContent.stdout).toBe(buildStdout);
+    expect(structuredContent.logs).toEqual(buildLogs);
     expect(structuredContent.path).toBeUndefined();
   });
 });

packages/angular/cli/src/commands/mcp/tools/modernize.ts

@@ -87,8 +87,7 @@ const modernizeOutputSchema = z.object({
     .describe(
       '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.'),
+  logs: z.array(z.string()).optional().describe('All logs from all executed commands.'),
 });
 
 export type ModernizeInput = z.infer<typeof modernizeInputSchema>;
@@ -140,8 +139,7 @@ export async function runModernization(input: ModernizeInput, host: Host) {
   }
 
   const instructions: string[] = [];
-  const stdoutMessages: string[] = [];
-  const stderrMessages: string[] = [];
+  let logs: string[] = [];
   const transformationsToRun = TRANSFORMATIONS.filter((t) => transformationNames.includes(t.name));
 
   for (const transformation of transformationsToRun) {
@@ -159,28 +157,19 @@ export async function runModernization(input: ModernizeInput, host: Host) {
         const command = 'ng';
         const args = ['generate', `@angular/core:${transformation.name}`, '--path', relativePath];
         try {
-          const { stdout, stderr } = await host.runCommand(command, args, {
-            cwd: angularProjectRoot,
-          });
-          if (stdout) {
-            stdoutMessages.push(stdout);
-          }
-          if (stderr) {
-            stderrMessages.push(stderr);
-          }
+          logs = (
+            await host.runCommand(command, args, {
+              cwd: angularProjectRoot,
+            })
+          ).logs;
           instructions.push(
             `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);
-            }
+            logs = e.logs;
           }
-          stderrMessages.push((e as Error).message);
+          logs.push((e as Error).message);
           instructions.push(
             `Migration ${transformation.name} on directory ${relativePath} failed.`,
           );
@@ -191,8 +180,7 @@ export async function runModernization(input: ModernizeInput, host: Host) {
 
   return createStructuredContentOutput({
     instructions: instructions.length > 0 ? instructions : undefined,
-    stdout: stdoutMessages?.join('\n\n') || undefined,
-    stderr: stderrMessages?.join('\n\n') || undefined,
+    logs,
   });
 }