refactor(@angular/cli): Polish MCP build/serve tests and corner cases

Author: amishne

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

@@ -108,14 +108,18 @@ export class LocalDevServer implements DevServer {
     this.devServerProcess.stderr?.on('data', (data) => {
       this.addLog(data.toString());
     });
+    this.devServerProcess.stderr?.on('close', () => {
+      this.stop();
+    });
+    this.buildInProgress = true;
   }
 
   private addLog(log: string) {
     this.serverLogs.push(log);
 
     if (BUILD_START_MESSAGES.some((message) => log.startsWith(message))) {
       this.buildInProgress = true;
-      this.latestBuildLogStartIndex = this.serverLogs.length;
+      this.latestBuildLogStartIndex = this.serverLogs.length - 1;
     } else if (BUILD_END_MESSAGES.some((message) => log.startsWith(message))) {
       this.buildInProgress = false;
       // We consider everything except a specific failure message to be a success.

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

@@ -26,7 +26,7 @@ import { AnyMcpToolDeclaration, registerTools } from './tools/tool-registry';
 import { WAIT_FOR_DEVSERVER_BUILD_TOOL } from './tools/wait-for-devserver-build';
 
 /**
- * Tools to manage devservers. Should be bundled together.
+ * Tools to manage devservers. Should be bundled together, then added to experimental or stable as a group.
  */
 const SERVE_TOOLS = [START_DEVSERVER_TOOL, STOP_DEVSERVER_TOOL, WAIT_FOR_DEVSERVER_BUILD_TOOL];
 

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

@@ -8,7 +8,7 @@
 
 import { z } from 'zod';
 import { CommandError, Host, LocalWorkspaceHost } from '../host';
-import { createStructureContentOutput } from '../utils';
+import { createStructuredContentOutput } from '../utils';
 import { McpToolDeclaration, declareTool } from './tool-registry';
 
 const CONFIGURATIONS = {
@@ -20,6 +20,8 @@ const CONFIGURATIONS = {
   },
 };
 
+const DEFAULT_CONFIGURATION_NAME = 'development';
+
 const BUILD_STATUSES = ['success', 'failure'] as const;
 type BuildStatus = (typeof BUILD_STATUSES)[number];
 
@@ -42,15 +44,16 @@ 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`.'),
-  path: z.string().optional().describe('The output path the build project was written into.'),
+  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 ?? 'development';
+  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);
@@ -89,7 +92,7 @@ export async function runBuild(input: BuildToolInput, host: Host) {
     path: outputPath,
   };
 
-  return createStructureContentOutput(structuredContent);
+  return createStructuredContentOutput(structuredContent);
 }
 
 export const BUILD_TOOL: McpToolDeclaration<
@@ -100,11 +103,11 @@ export const BUILD_TOOL: McpToolDeclaration<
   title: 'Build Tool',
   description: `
 <Purpose>
-Perform a one-off, non-watched build with "ng build". Use this tool whenever the user wants to build an Angular project; this is similar to
+Perform a one-off, non-watched build using "ng build". Use this tool whenever the user wants to build an Angular project; this is similar to
 "ng build", but the tool is smarter about using the right configuration and collecting the output logs.
 </Purpose>
 <Use Cases>
-* Building the Angular project and getting build logs back.
+* Building an Angular project and getting build logs back.
 </Use Cases>
 <Operational Notes>
 * This tool runs "ng build" so it expects to run within an Angular workspace.

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

@@ -7,7 +7,7 @@
  */
 
 import { CommandError, Host } from '../host';
-import { BuildToolInput, runBuild } from './build';
+import { runBuild } from './build';
 
 describe('Build Tool', () => {
   let mockHost: Host;
@@ -20,25 +20,39 @@ describe('Build Tool', () => {
     } as Partial<Host> as Host;
   });
 
-  it('should handle a successful build and extract the output path', async () => {
-    const buildStdout =
-      'Build successful!\nSome other log lines...\nOutput location: dist/my-cool-app';
+  it('should construct the command correctly with default configuration', async () => {
+    await runBuild({}, mockHost);
+    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['build', '-c development']);
+  });
+
+  it('should construct the command correctly with a specified project', async () => {
+    await runBuild({ project: 'another-app' }, mockHost);
+    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', [
+      'build',
+      'another-app',
+      '-c development',
+    ]);
+  });
+
+  it('should construct the command correctly for production configuration', async () => {
+    await runBuild({ configuration: 'production' }, mockHost);
+    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['build']);
+  });
+
+  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 { structuredContent } = await runBuild({ project: 'my-cool-app' }, mockHost);
+    const { structuredContent } = await runBuild({ project: 'my-app' }, mockHost);
 
-    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', [
-      'build',
-      'my-cool-app',
-      '-c development',
-    ]);
+    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.path).toBe('dist/my-cool-app');
+    expect(structuredContent.path).toBe('dist/my-app');
   });
 
   it('should handle a failed build and capture logs', async () => {
@@ -62,25 +76,6 @@ describe('Build Tool', () => {
     expect(structuredContent.path).toBeUndefined();
   });
 
-  it('should construct the command correctly with default configuration', async () => {
-    await runBuild({}, mockHost);
-    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['build', '-c development']);
-  });
-
-  it('should construct the command correctly with a specified project', async () => {
-    await runBuild({ project: 'another-app' }, mockHost);
-    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', [
-      'build',
-      'another-app',
-      '-c development',
-    ]);
-  });
-
-  it('should construct the command correctly for production configuration', async () => {
-    await runBuild({ configuration: 'production' }, mockHost);
-    expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['build']);
-  });
-
   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: '' });

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

@@ -9,6 +9,7 @@
 import { dirname, join, relative } from 'path';
 import { z } from 'zod';
 import { CommandError, Host, LocalWorkspaceHost } from '../host';
+import { createStructuredContentOutput } from '../utils';
 import { McpToolDeclaration, declareTool } from './tool-registry';
 
 interface Transformation {
@@ -93,13 +94,6 @@ const modernizeOutputSchema = z.object({
 export type ModernizeInput = z.infer<typeof modernizeInputSchema>;
 export type ModernizeOutput = z.infer<typeof modernizeOutputSchema>;
 
-function createToolOutput(structuredContent: ModernizeOutput) {
-  return {
-    content: [{ type: 'text' as const, text: JSON.stringify(structuredContent, null, 2) }],
-    structuredContent,
-  };
-}
-
 function findAngularJsonDir(startDir: string, host: Host): string | null {
   let currentDir = startDir;
   while (true) {
@@ -119,15 +113,15 @@ export async function runModernization(input: ModernizeInput, host: Host) {
   const directories = input.directories ?? [];
 
   if (transformationNames.length === 0) {
-    return createToolOutput({
+    return createStructuredContentOutput({
       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({
+    return createStructuredContentOutput({
       instructions: [
         'Provide this tool with a list of directory paths in your workspace ' +
           'to run the modernization on.',
@@ -140,7 +134,7 @@ export async function runModernization(input: ModernizeInput, host: Host) {
 
   const angularProjectRoot = findAngularJsonDir(executionDir, host);
   if (!angularProjectRoot) {
-    return createToolOutput({
+    return createStructuredContentOutput({
       instructions: ['Could not find an angular.json file in the current or parent directories.'],
     });
   }
@@ -195,7 +189,7 @@ export async function runModernization(input: ModernizeInput, host: Host) {
     }
   }
 
-  return createToolOutput({
+  return createStructuredContentOutput({
     instructions: instructions.length > 0 ? instructions : undefined,
     stdout: stdoutMessages?.join('\n\n') || undefined,
     stderr: stderrMessages?.join('\n\n') || undefined,

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

@@ -12,7 +12,7 @@ import { Host } from '../host';
 import { startDevServer } from './start-devserver';
 import { stopDevserver } from './stop-devserver';
 import { McpToolContext } from './tool-registry';
-import { waitForDevserverBuild } from './wait-for-devserver-build';
+import { WATCH_DELAY, waitForDevserverBuild } from './wait-for-devserver-build';
 
 class MockChildProcess extends EventEmitter {
   stdout = new EventEmitter();
@@ -55,17 +55,12 @@ describe('Serve Tools', () => {
     expect(mockProcess.kill).toHaveBeenCalled();
   });
 
-  it('should find a free port if none is provided', async () => {
-    await startDevServer({}, mockContext, mockHost);
-    expect(mockHost.getAvailablePort).toHaveBeenCalled();
-  });
-
   it('should wait for a build to complete', async () => {
     await startDevServer({}, mockContext, mockHost);
 
     const waitPromise = waitForDevserverBuild({ timeout: 10 }, mockContext);
 
-    // Simulate build logs
+    // Simulate build logs.
     mockProcess.stdout.emit('data', '... building ...');
     mockProcess.stdout.emit('data', '✔ Changes detected. Rebuilding...');
     mockProcess.stdout.emit('data', '... more logs ...');
@@ -74,28 +69,28 @@ describe('Serve Tools', () => {
     const waitResult = await waitPromise;
     expect(waitResult.structuredContent.status).toBe('success');
     expect(waitResult.structuredContent.logs).toEqual([
+      '... building ...',
       '✔ Changes detected. Rebuilding...',
       '... more logs ...',
       'Application bundle generation complete.',
     ]);
   });
 
   it('should handle multiple dev servers', async () => {
-    // Start server for project 1
+    // Start server for project 1. This uses the basic mockProcess created for the tests.
     const startResult1 = await startDevServer({ project: 'app-one' }, mockContext, mockHost);
     expect(startResult1.structuredContent.message).toBe(
       `Development server for project 'app-one' started and watching for workspace changes.`,
     );
     const process1 = mockProcess;
 
-    // Start server for project 2
-    mockProcess = new MockChildProcess();
-    (mockHost.spawn as jasmine.Spy).and.returnValue(mockProcess as unknown as ChildProcess);
+    // Start server for project 2, returning a new mock process.
+    const process2 = new MockChildProcess();
+    (mockHost.spawn as jasmine.Spy).and.returnValue(process2 as unknown as ChildProcess);
     const startResult2 = await startDevServer({ project: 'app-two' }, mockContext, mockHost);
     expect(startResult2.structuredContent.message).toBe(
       `Development server for project 'app-two' started and watching for workspace changes.`,
     );
-    const process2 = mockProcess;
 
     expect(mockHost.spawn).toHaveBeenCalledWith('ng', ['serve', 'app-one', '--port=12345'], {
       stdio: 'pipe',
@@ -122,10 +117,14 @@ describe('Serve Tools', () => {
 
   it('should handle server crash', async () => {
     await startDevServer({ project: 'crash-app' }, mockContext, mockHost);
-    mockProcess.emit('close', 1); // Simulate a crash with exit code 1
+
+    // Simulate a crash with exit code 1
+    mockProcess.stdout.emit('data', 'Fatal error.');
+    mockProcess.emit('close', 1);
 
     const stopResult = stopDevserver({ project: 'crash-app' }, mockContext);
-    expect(stopResult.structuredContent.message).toContain('is not running');
+    expect(stopResult.structuredContent.message).toContain('stopped');
+    expect(stopResult.structuredContent.logs).toEqual(['Fatal error.']);
   });
 
   it('wait should timeout if build takes too long', async () => {
@@ -136,4 +135,38 @@ describe('Serve Tools', () => {
     );
     expect(waitResult.structuredContent.status).toBe('timeout');
   });
+
+  it('should wait through multiple cycles for a build to complete', async () => {
+    jasmine.clock().install();
+    try {
+      await startDevServer({}, mockContext, mockHost);
+
+      // Immediately simulate a build starting so isBuilding() is true.
+      mockProcess.stdout.emit('data', '❯ Changes detected. Rebuilding...');
+
+      const waitPromise = waitForDevserverBuild({ timeout: 5 * WATCH_DELAY }, mockContext);
+
+      // Tick past the first debounce. The while loop will be entered.
+      jasmine.clock().tick(WATCH_DELAY + 1);
+
+      // Tick past the second debounce (inside the loop).
+      jasmine.clock().tick(WATCH_DELAY + 1);
+
+      // Now finish the build.
+      mockProcess.stdout.emit('data', 'Application bundle generation complete.');
+
+      // Tick past another debounce to exit the loop.
+      jasmine.clock().tick(WATCH_DELAY + 1);
+
+      const waitResult = await waitPromise;
+
+      expect(waitResult.structuredContent.status).toBe('success');
+      expect(waitResult.structuredContent.logs).toEqual([
+        '❯ Changes detected. Rebuilding...',
+        'Application bundle generation complete.',
+      ]);
+    } finally {
+      jasmine.clock().uninstall();
+    }
+  });
 });

packages/angular/cli/src/commands/mcp/tools/start-devserver.ts

@@ -9,7 +9,7 @@
 import { z } from 'zod';
 import { LocalDevServer, devServerKey } from '../dev-server';
 import { Host, LocalWorkspaceHost } from '../host';
-import { createStructureContentOutput } from '../utils';
+import { createStructuredContentOutput } from '../utils';
 import { McpToolContext, McpToolDeclaration, declareTool } from './tool-registry';
 
 const startDevServerToolInputSchema = z.object({
@@ -48,7 +48,7 @@ export async function startDevServer(
 
   let devServer = context.devServers.get(projectKey);
   if (devServer) {
-    return createStructureContentOutput({
+    return createStructuredContentOutput({
       message: `Development server for project '${projectKey}' is already running.`,
       address: localhostAddress(devServer.port),
     });
@@ -61,7 +61,7 @@ export async function startDevServer(
 
   context.devServers.set(projectKey, devServer);
 
-  return createStructureContentOutput({
+  return createStructuredContentOutput({
     message: `Development server for project '${projectKey}' started and watching for workspace changes.`,
     address: localhostAddress(port),
   });