Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions packages/angular/cli/src/commands/mcp/devserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,6 @@ export interface Devserver {
port: number;
}

export function devserverKey(project?: string) {
return project ?? '<default>';
}

/**
* A local Angular development server managed by the MCP server.
*/
Expand Down
29 changes: 19 additions & 10 deletions packages/angular/cli/src/commands/mcp/mcp-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,26 +20,19 @@ import { DEVSERVER_START_TOOL } from './tools/devserver/devserver-start';
import { DEVSERVER_STOP_TOOL } from './tools/devserver/devserver-stop';
import { DEVSERVER_WAIT_FOR_BUILD_TOOL } from './tools/devserver/devserver-wait-for-build';
import { DOC_SEARCH_TOOL } from './tools/doc-search';
import { E2E_TOOL } from './tools/e2e';
import { FIND_EXAMPLE_TOOL } from './tools/examples/index';
import { MODERNIZE_TOOL } from './tools/modernize';
import { ZONELESS_MIGRATION_TOOL } from './tools/onpush-zoneless-migration/zoneless-migration';
import { LIST_PROJECTS_TOOL } from './tools/projects';
import { TEST_TOOL } from './tools/test';
import { type AnyMcpToolDeclaration, registerTools } from './tools/tool-registry';

/**
* Tools to manage devservers. Should be bundled together, then added to experimental or stable as a group.
*/
const DEVSERVER_TOOLS = [DEVSERVER_START_TOOL, DEVSERVER_STOP_TOOL, DEVSERVER_WAIT_FOR_BUILD_TOOL];

/**
* Experimental tools that are grouped together under a single name.
*
* Used for enabling them as a group.
*/
export const EXPERIMENTAL_TOOL_GROUPS = {
'devserver': DEVSERVER_TOOLS,
};

/**
* The set of tools that are enabled by default for the MCP server.
* These tools are considered stable and suitable for general use.
Expand All @@ -57,7 +50,23 @@ const STABLE_TOOLS = [
* The set of tools that are available but not enabled by default.
* These tools are considered experimental and may have limitations.
*/
export const EXPERIMENTAL_TOOLS = [BUILD_TOOL, MODERNIZE_TOOL, ...DEVSERVER_TOOLS] as const;
export const EXPERIMENTAL_TOOLS = [
BUILD_TOOL,
E2E_TOOL,
MODERNIZE_TOOL,
TEST_TOOL,
...DEVSERVER_TOOLS,
] as const;

/**
* Experimental tools that are grouped together under a single name.
*
* Used for enabling them as a group.
*/
export const EXPERIMENTAL_TOOL_GROUPS = {
'all': EXPERIMENTAL_TOOLS,
'devserver': DEVSERVER_TOOLS,
};

export async function createMcpServer(
options: {
Expand Down
91 changes: 91 additions & 0 deletions packages/angular/cli/src/commands/mcp/testing/test-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/**
* @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 { workspaces } from '@angular-devkit/core';
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
import { AngularWorkspace } from '../../../utilities/config';
import { type Devserver } from '../devserver';
import { Host } from '../host';
import { McpToolContext } from '../tools/tool-registry';
import { MockHost } from './mock-host';

/**
* Creates a mock implementation of the Host interface for testing purposes.
* Each method is a Jasmine spy that can be configured.
*/
export function createMockHost(): MockHost {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider: Is there a path to a "fake" host instead of a "mock" host? Could we provide fake implementations for stat and existsSync if we accept a virtual file system (or create a real one in a temp directly from inputs)?

Faking implementation of runCommand and spawn is likely a bit harder, but we could either delegate the command to an input function or actually run a real subprocess to get more accurate error behavior. I'm thinking something along the lines of:

import * as fs from 'fs/promises';

class FakeHost implements Host {
  private nextPort = 0;

  private constructor(private readonly cmdImpls: Record<string, (...args: string[]) => CommandResult>, private readonly root: string) {}

  static async from(cmdImpls: Record<string, (...args: string[]) => CommandResult>, fs: Record<string, string | Buffer> = {}): Promise<FakeHost> {
    const tempDir = await fs.mkdtemp();

    for (const [path, content] of Object.entries(fs)) {
      await fs.writeFile(path, content);
    }

    return new FakeHost(cmdImpls, tempDir);
  }

  stat() { /* Check in `root` */ }
  existsSync() { /* Check in `root` */ }

  getAvailablePort(): number {
    return this.nextPort++;
  }

  runCommand(binary: string, args: string[]): /* ... */ {
    return this.cmdImpls[binary](args);
  }
  spawn() { /* Similar to `runCommand`... */ }

  await dispose(): Promise<void> {
    await fs.rmdir(this.root);
  }
}

I'm thinking this can avoid the need to define spies unique to each test and increase overall test confidence. If we can make this specific to each test as suggested in a separate comment, it can potentially remove the need to mutate the environment after the beforeEach within a test.

Downside is that we do need to explicitly call dispose to clean up the temp directory if we're using a real file system. If we can run these tests on Node 24, we can potentially get away with using and Symbol.dispose which would help.

it('does a thing', () => {
  await using host = await FakeHost.from(...);
  await runE2e(host, ...);
  // Host implicitly disposed at end of scope.
});

return {
runCommand: jasmine.createSpy<Host['runCommand']>('runCommand').and.resolveTo({ logs: [] }),
stat: jasmine.createSpy<Host['stat']>('stat'),
existsSync: jasmine.createSpy<Host['existsSync']>('existsSync'),
spawn: jasmine.createSpy<Host['spawn']>('spawn'),
getAvailablePort: jasmine
.createSpy<Host['getAvailablePort']>('getAvailablePort')
.and.resolveTo(0),
Comment on lines +27 to +29
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider: Could we put the auto-incrementing port number implementation here and potentially reuse it across all affected tests?

} as unknown as MockHost;
}

/**
* Options for configuring the mock MCP tool context.
*/
export interface MockContextOptions {
/** An optional pre-configured mock host. If not provided, a default mock host will be created. */
host?: MockHost;

/** Initial set of projects to populate the mock workspace with. */
projects?: Record<string, workspaces.ProjectDefinition>;
}

/**
* Creates a comprehensive mock for the McpToolContext, including a mock Host,
* an AngularWorkspace, and a ProjectDefinitionCollection. This simplifies testing
* MCP tools by providing a consistent and configurable testing environment.
* @param options Configuration options for the mock context.
* @returns An object containing the mock host, context, projects collection, and workspace instance.
*/
export function createMockContext(options: MockContextOptions = {}): {
host: MockHost;
context: McpToolContext;
projects: workspaces.ProjectDefinitionCollection;
workspace: AngularWorkspace;
} {
const host = options.host ?? createMockHost();
const projects = new workspaces.ProjectDefinitionCollection(options.projects);
const workspace = new AngularWorkspace({ projects, extensions: {} }, '/test/angular.json');

const context: McpToolContext = {
server: {} as unknown as McpServer,
workspace,
logger: { warn: () => {} },
devservers: new Map<string, Devserver>(),
host,
};

return { host, context, projects, workspace };
}

/**
* Adds a project to the provided mock ProjectDefinitionCollection.
* This is a helper function to easily populate a mock Angular workspace.
* @param projects The ProjectDefinitionCollection to add the project to.
* @param name The name of the project.
* @param targets A record of target definitions for the project (e.g., build, test, e2e).
* @param root The root path of the project, relative to the workspace root. Defaults to `projects/${name}`.
*/
export function addProjectToWorkspace(
projects: workspaces.ProjectDefinitionCollection,
name: string,
targets: Record<string, workspaces.TargetDefinition> = {},
root: string = `projects/${name}`,
) {
projects.set(name, {
root,
extensions: {},
targets: new workspaces.TargetDefinitionCollection(targets),
});
}
9 changes: 3 additions & 6 deletions packages/angular/cli/src/commands/mcp/tools/build_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,16 @@
* found in the LICENSE file at https://angular.dev/license
*/

import { CommandError, Host } from '../host';
import { CommandError } from '../host';
import type { MockHost } from '../testing/mock-host';
import { createMockHost } from '../testing/test-utils';
import { runBuild } from './build';

describe('Build Tool', () => {
let mockHost: MockHost;

beforeEach(() => {
mockHost = {
runCommand: jasmine.createSpy<Host['runCommand']>('runCommand').and.resolveTo({ logs: [] }),
stat: jasmine.createSpy<Host['stat']>('stat'),
existsSync: jasmine.createSpy<Host['existsSync']>('existsSync'),
} as MockHost;
mockHost = createMockHost();
});

it('should construct the command correctly with default configuration', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
*/

import { z } from 'zod';
import { LocalDevserver, devserverKey } from '../../devserver';
import { createStructuredContentOutput } from '../../utils';
import { LocalDevserver } from '../../devserver';
import { createStructuredContentOutput, getDefaultProjectName } from '../../utils';
import { type McpToolContext, type McpToolDeclaration, declareTool } from '../tool-registry';

const devserverStartToolInputSchema = z.object({
Expand Down Expand Up @@ -39,12 +39,18 @@ function localhostAddress(port: number) {
}

export async function startDevserver(input: DevserverStartToolInput, context: McpToolContext) {
const projectKey = devserverKey(input.project);
const projectName = input.project ?? getDefaultProjectName(context);

let devserver = context.devservers.get(projectKey);
if (!projectName) {
return createStructuredContentOutput({
message: ['Project name not provided, and no default project found.'],
});
}

let devserver = context.devservers.get(projectName);
if (devserver) {
return createStructuredContentOutput({
message: `Development server for project '${projectKey}' is already running.`,
message: `Development server for project '${projectName}' is already running.`,
address: localhostAddress(devserver.port),
});
}
Expand All @@ -54,10 +60,10 @@ export async function startDevserver(input: DevserverStartToolInput, context: Mc
devserver = new LocalDevserver({ host: context.host, project: input.project, port });
devserver.start();

context.devservers.set(projectKey, devserver);
context.devservers.set(projectName, devserver);

return createStructuredContentOutput({
message: `Development server for project '${projectKey}' started and watching for workspace changes.`,
message: `Development server for project '${projectName}' started and watching for workspace changes.`,
address: localhostAddress(port),
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
*/

import { z } from 'zod';
import { devserverKey } from '../../devserver';
import { createStructuredContentOutput } from '../../utils';
import { createStructuredContentOutput, getDefaultProjectName } from '../../utils';
import { type McpToolContext, type McpToolDeclaration, declareTool } from '../tool-registry';

const devserverStopToolInputSchema = z.object({
Expand All @@ -30,21 +29,41 @@ const devserverStopToolOutputSchema = z.object({
export type DevserverStopToolOutput = z.infer<typeof devserverStopToolOutputSchema>;

export function stopDevserver(input: DevserverStopToolInput, context: McpToolContext) {
const projectKey = devserverKey(input.project);
const devServer = context.devservers.get(projectKey);
if (context.devservers.size === 0) {
return createStructuredContentOutput({
message: ['No development servers are currently running.'],
logs: undefined,
});
}

let projectName = input.project ?? getDefaultProjectName(context);

if (!projectName) {
// This should not happen. But if there's just a single running devserver, stop it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider: If this shouldn't happen, do we want to just throw and escalate the issue? I'd worry a bit that this might mask a legitimate bug elsewhere in the program.

Based on the error message below, it seems weird that it would be ignored if there happens to be just one devserver running.

if (context.devservers.size === 1) {
projectName = Array.from(context.devservers.keys())[0];
} else {
return createStructuredContentOutput({
message: ['Project name not provided, and no default project found.'],
logs: undefined,
});
}
}

const devServer = context.devservers.get(projectName);

if (!devServer) {
return createStructuredContentOutput({
message: `Development server for project '${projectKey}' was not running.`,
message: `Development server for project '${projectName}' was not running.`,
logs: undefined,
});
}

devServer.stop();
context.devservers.delete(projectKey);
context.devservers.delete(projectName);

return createStructuredContentOutput({
message: `Development server for project '${projectKey}' stopped.`,
message: `Development server for project '${projectName}' stopped.`,
logs: devServer.getServerLogs(),
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
*/

import { z } from 'zod';
import { devserverKey } from '../../devserver';
import { createStructuredContentOutput } from '../../utils';
import { createStructuredContentOutput, getDefaultProjectName } from '../../utils';
import { type McpToolContext, type McpToolDeclaration, declareTool } from '../tool-registry';

/**
Expand Down Expand Up @@ -60,21 +59,43 @@ export async function waitForDevserverBuild(
input: DevserverWaitForBuildToolInput,
context: McpToolContext,
) {
const projectKey = devserverKey(input.project);
const devServer = context.devservers.get(projectKey);
const deadline = Date.now() + input.timeout;
if (context.devservers.size === 0) {
return createStructuredContentOutput({
status: 'no_devserver_found',
logs: undefined,
});
}

let projectName = input.project ?? getDefaultProjectName(context);

if (!projectName) {
// This should not happen. But if there's just a single running devserver, wait for it.
if (context.devservers.size === 1) {
projectName = Array.from(context.devservers.keys())[0];
} else {
return createStructuredContentOutput({
status: 'no_devserver_found',
logs: undefined,
});
}
}

const devServer = context.devservers.get(projectName);

if (!devServer) {
return createStructuredContentOutput<DevserverWaitForBuildToolOutput>({
status: 'no_devserver_found',
logs: undefined,
});
}

const deadline = Date.now() + input.timeout;
await wait(WATCH_DELAY);
while (devServer.isBuilding()) {
if (Date.now() > deadline) {
return createStructuredContentOutput<DevserverWaitForBuildToolOutput>({
status: 'timeout',
logs: undefined,
});
}
await wait(WATCH_DELAY);
Expand Down
Loading