From bc744250e3fa03020c13e5ed80525c641146f997 Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Mon, 24 Nov 2025 13:00:02 -0500 Subject: [PATCH] refactor(@angular/cli): introduce Host abstraction for `find_examples` MCP tool This commit introduces a `Host` abstraction layer for the `find_examples` MCP tool, enhancing its testability and modularity by decoupling it from direct Node.js APIs. --- packages/angular/cli/src/commands/mcp/host.ts | 43 +++++- .../cli/src/commands/mcp/mcp-server.ts | 2 + .../cli/src/commands/mcp/testing/mock-host.ts | 5 +- .../mcp/tools/examples/database-discovery.ts | 11 +- .../tools/examples/database-discovery_spec.ts | 137 ++++++++++++++++++ .../src/commands/mcp/tools/examples/index.ts | 5 +- .../mcp/tools/examples/runtime-database.ts | 11 +- .../src/commands/mcp/tools/tool-registry.ts | 2 + 8 files changed, 202 insertions(+), 14 deletions(-) create mode 100644 packages/angular/cli/src/commands/mcp/tools/examples/database-discovery_spec.ts diff --git a/packages/angular/cli/src/commands/mcp/host.ts b/packages/angular/cli/src/commands/mcp/host.ts index 0be6ff67a7fc..1ff0bb9724b3 100644 --- a/packages/angular/cli/src/commands/mcp/host.ts +++ b/packages/angular/cli/src/commands/mcp/host.ts @@ -16,7 +16,8 @@ import { existsSync as nodeExistsSync } from 'fs'; import { ChildProcess, spawn } from 'node:child_process'; import { Stats } from 'node:fs'; -import { stat } from 'node:fs/promises'; +import { glob as nodeGlob, readFile as nodeReadFile, stat } from 'node:fs/promises'; +import { createRequire } from 'node:module'; import { createServer } from 'node:net'; /** @@ -50,6 +51,33 @@ export interface Host { */ existsSync(path: string): boolean; + /** + * Reads a file and returns its content. + * @param path The path to the file. + * @param encoding The encoding to use. + * @returns A promise that resolves to the file content. + */ + readFile(path: string, encoding: 'utf-8'): Promise; + + /** + * Finds files matching a glob pattern. + * @param pattern The glob pattern. + * @param options Options for the glob search. + * @returns An async iterable of file entries. + */ + glob( + pattern: string, + options: { cwd: string }, + ): AsyncIterable<{ name: string; parentPath: string; isFile(): boolean }>; + + /** + * Resolves a module request from a given path. + * @param request The module request to resolve. + * @param from The path from which to resolve the request. + * @returns The resolved module path. + */ + resolveModule(request: string, from: string): string; + /** * Spawns a child process and returns a promise that resolves with the process's * output or rejects with a structured error. @@ -100,6 +128,19 @@ export const LocalWorkspaceHost: Host = { existsSync: nodeExistsSync, + readFile: nodeReadFile, + + glob: function ( + pattern: string, + options: { cwd: string }, + ): AsyncIterable<{ name: string; parentPath: string; isFile(): boolean }> { + return nodeGlob(pattern, { ...options, withFileTypes: true }); + }, + + resolveModule(request: string, from: string): string { + return createRequire(from).resolve(request); + }, + runCommand: async ( command: string, args: readonly string[], diff --git a/packages/angular/cli/src/commands/mcp/mcp-server.ts b/packages/angular/cli/src/commands/mcp/mcp-server.ts index 27ef2880336c..cdea2a232b3c 100644 --- a/packages/angular/cli/src/commands/mcp/mcp-server.ts +++ b/packages/angular/cli/src/commands/mcp/mcp-server.ts @@ -11,6 +11,7 @@ import { join } from 'node:path'; import type { AngularWorkspace } from '../../utilities/config'; import { VERSION } from '../../utilities/version'; import type { DevServer } from './dev-server'; +import { LocalWorkspaceHost } from './host'; import { registerInstructionsResource } from './resources/instructions'; import { AI_TUTOR_TOOL } from './tools/ai-tutor'; import { BEST_PRACTICES_TOOL } from './tools/best-practices'; @@ -115,6 +116,7 @@ equivalent actions. logger, exampleDatabasePath: join(__dirname, '../../../lib/code-examples.db'), devServers: new Map(), + host: LocalWorkspaceHost, }, toolDeclarations, ); diff --git a/packages/angular/cli/src/commands/mcp/testing/mock-host.ts b/packages/angular/cli/src/commands/mcp/testing/mock-host.ts index 1720b1377792..29f41c24e101 100644 --- a/packages/angular/cli/src/commands/mcp/testing/mock-host.ts +++ b/packages/angular/cli/src/commands/mcp/testing/mock-host.ts @@ -13,9 +13,12 @@ import type { Host } from '../host'; * 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: '' }); + runCommand = jasmine.createSpy('runCommand').and.resolveTo({ logs: [] }); stat = jasmine.createSpy('stat'); existsSync = jasmine.createSpy('existsSync'); + readFile = jasmine.createSpy('readFile').and.resolveTo(''); + glob = jasmine.createSpy('glob').and.returnValue((async function* () {})()); + resolveModule = jasmine.createSpy('resolveRequest').and.returnValue('/dev/null'); spawn = jasmine.createSpy('spawn'); getAvailablePort = jasmine.createSpy('getAvailablePort'); } diff --git a/packages/angular/cli/src/commands/mcp/tools/examples/database-discovery.ts b/packages/angular/cli/src/commands/mcp/tools/examples/database-discovery.ts index 9c9d0cd98d3b..aeab96e15871 100644 --- a/packages/angular/cli/src/commands/mcp/tools/examples/database-discovery.ts +++ b/packages/angular/cli/src/commands/mcp/tools/examples/database-discovery.ts @@ -6,8 +6,6 @@ * found in the LICENSE file at https://angular.dev/license */ -import { readFile, stat } from 'node:fs/promises'; -import { createRequire } from 'node:module'; import { dirname, isAbsolute, relative, resolve } from 'node:path'; import type { McpToolContext } from '../tool-registry'; @@ -36,20 +34,21 @@ const KNOWN_EXAMPLE_PACKAGES = ['@angular/core', '@angular/aria', '@angular/form * * @param workspacePath The absolute path to the user's `angular.json` file. * @param logger The MCP tool context logger for reporting warnings. + * @param host The host interface for file system and module resolution operations. * @returns A promise that resolves to an array of objects, each containing a database path and source. */ export async function getVersionSpecificExampleDatabases( workspacePath: string, logger: McpToolContext['logger'], + host: McpToolContext['host'], ): Promise<{ dbPath: string; source: string }[]> { - const workspaceRequire = createRequire(workspacePath); const databases: { dbPath: string; source: string }[] = []; for (const packageName of KNOWN_EXAMPLE_PACKAGES) { // 1. Resolve the path to package.json let pkgJsonPath: string; try { - pkgJsonPath = workspaceRequire.resolve(`${packageName}/package.json`); + pkgJsonPath = host.resolveModule(`${packageName}/package.json`, workspacePath); } catch (e) { // This is not a warning because the user may not have all known packages installed. continue; @@ -57,7 +56,7 @@ export async function getVersionSpecificExampleDatabases( // 2. Read and parse package.json, then find the database. try { - const pkgJsonContent = await readFile(pkgJsonPath, 'utf-8'); + const pkgJsonContent = await host.readFile(pkgJsonPath, 'utf-8'); const pkgJson = JSON.parse(pkgJsonContent); const examplesInfo = pkgJson['angular']?.examples; @@ -81,7 +80,7 @@ export async function getVersionSpecificExampleDatabases( } // Check the file size to prevent reading a very large file. - const stats = await stat(dbPath); + const stats = await host.stat(dbPath); if (stats.size > 10 * 1024 * 1024) { // 10MB logger.warn( diff --git a/packages/angular/cli/src/commands/mcp/tools/examples/database-discovery_spec.ts b/packages/angular/cli/src/commands/mcp/tools/examples/database-discovery_spec.ts new file mode 100644 index 000000000000..0d5680d01c06 --- /dev/null +++ b/packages/angular/cli/src/commands/mcp/tools/examples/database-discovery_spec.ts @@ -0,0 +1,137 @@ +/** + * @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 type { Stats } from 'node:fs'; +import { Host } from '../../host'; +import { getVersionSpecificExampleDatabases } from './database-discovery'; + +describe('getVersionSpecificExampleDatabases', () => { + let mockHost: jasmine.SpyObj; + let mockLogger: { warn: jasmine.Spy }; + + beforeEach(() => { + mockHost = jasmine.createSpyObj('Host', ['resolveModule', 'readFile', 'stat']); + mockLogger = { + warn: jasmine.createSpy('warn'), + }; + }); + + it('should find a valid example database from a package', async () => { + mockHost.resolveModule.and.callFake((specifier) => { + if (specifier === '@angular/core/package.json') { + return '/path/to/node_modules/@angular/core/package.json'; + } + throw new Error(`Unexpected module specifier: ${specifier}`); + }); + mockHost.readFile.and.resolveTo( + JSON.stringify({ + name: '@angular/core', + version: '18.1.0', + angular: { + examples: { + format: 'sqlite', + path: './resources/code-examples.db', + }, + }, + }), + ); + mockHost.stat.and.resolveTo({ size: 1024 } as Stats); + + const databases = await getVersionSpecificExampleDatabases( + '/path/to/workspace', + mockLogger, + mockHost, + ); + + expect(databases.length).toBe(1); + expect(databases[0].dbPath).toBe( + '/path/to/node_modules/@angular/core/resources/code-examples.db', + ); + expect(databases[0].source).toBe('package @angular/core@18.1.0'); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + + it('should skip packages without angular.examples metadata', async () => { + mockHost.resolveModule.and.returnValue('/path/to/node_modules/@angular/core/package.json'); + mockHost.readFile.and.resolveTo(JSON.stringify({ name: '@angular/core', version: '18.1.0' })); + + const databases = await getVersionSpecificExampleDatabases( + '/path/to/workspace', + mockLogger, + mockHost, + ); + + expect(databases.length).toBe(0); + }); + + it('should handle packages that are not found', async () => { + mockHost.resolveModule.and.throwError(new Error('Cannot find module')); + + const databases = await getVersionSpecificExampleDatabases( + '/path/to/workspace', + mockLogger, + mockHost, + ); + + expect(databases.length).toBe(0); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + + it('should reject database paths that attempt path traversal', async () => { + mockHost.resolveModule.and.returnValue('/path/to/node_modules/@angular/core/package.json'); + mockHost.readFile.and.resolveTo( + JSON.stringify({ + name: '@angular/core', + version: '18.1.0', + angular: { + examples: { + format: 'sqlite', + path: '../outside-package/danger.db', + }, + }, + }), + ); + + const databases = await getVersionSpecificExampleDatabases( + '/path/to/workspace', + mockLogger, + mockHost, + ); + + expect(databases.length).toBe(0); + expect(mockLogger.warn).toHaveBeenCalledWith( + jasmine.stringMatching(/Detected a potential path traversal attempt/), + ); + }); + + it('should skip database files larger than 10MB', async () => { + mockHost.resolveModule.and.returnValue('/path/to/node_modules/@angular/core/package.json'); + mockHost.readFile.and.resolveTo( + JSON.stringify({ + name: '@angular/core', + version: '18.1.0', + angular: { + examples: { + format: 'sqlite', + path: './resources/code-examples.db', + }, + }, + }), + ); + mockHost.stat.and.resolveTo({ size: 11 * 1024 * 1024 } as Stats); // 11MB + + const databases = await getVersionSpecificExampleDatabases( + '/path/to/workspace', + mockLogger, + mockHost, + ); + + expect(databases.length).toBe(0); + expect(mockLogger.warn).toHaveBeenCalledWith(jasmine.stringMatching(/is larger than 10MB/)); + }); +}); diff --git a/packages/angular/cli/src/commands/mcp/tools/examples/index.ts b/packages/angular/cli/src/commands/mcp/tools/examples/index.ts index 9ee35b8f1c2c..8ceecf2d840d 100644 --- a/packages/angular/cli/src/commands/mcp/tools/examples/index.ts +++ b/packages/angular/cli/src/commands/mcp/tools/examples/index.ts @@ -71,9 +71,9 @@ new or evolving features. factory: createFindExampleHandler, }); -async function createFindExampleHandler({ logger, exampleDatabasePath }: McpToolContext) { +async function createFindExampleHandler({ logger, exampleDatabasePath, host }: McpToolContext) { const runtimeDb = process.env['NG_MCP_EXAMPLES_DIR'] - ? await setupRuntimeExamples(process.env['NG_MCP_EXAMPLES_DIR']) + ? await setupRuntimeExamples(process.env['NG_MCP_EXAMPLES_DIR'], host) : undefined; suppressSqliteWarning(); @@ -91,6 +91,7 @@ async function createFindExampleHandler({ logger, exampleDatabasePath }: McpTool const versionSpecificDbs = await getVersionSpecificExampleDatabases( input.workspacePath, logger, + host, ); for (const db of versionSpecificDbs) { resolvedDbs.push({ path: db.dbPath, source: db.source }); diff --git a/packages/angular/cli/src/commands/mcp/tools/examples/runtime-database.ts b/packages/angular/cli/src/commands/mcp/tools/examples/runtime-database.ts index 2f243eb402b3..5ca74dc60d63 100644 --- a/packages/angular/cli/src/commands/mcp/tools/examples/runtime-database.ts +++ b/packages/angular/cli/src/commands/mcp/tools/examples/runtime-database.ts @@ -6,10 +6,10 @@ * found in the LICENSE file at https://angular.dev/license */ -import { glob, readFile } from 'node:fs/promises'; import { join } from 'node:path'; import type { DatabaseSync } from 'node:sqlite'; import { z } from 'zod'; +import type { McpToolContext } from '../tool-registry'; /** * A simple YAML front matter parser. @@ -80,7 +80,10 @@ function parseFrontmatter(content: string): Record { return data; } -export async function setupRuntimeExamples(examplesPath: string): Promise { +export async function setupRuntimeExamples( + examplesPath: string, + host: McpToolContext['host'], +): Promise { const { DatabaseSync } = await import('node:sqlite'); const db = new DatabaseSync(':memory:'); @@ -156,12 +159,12 @@ export async function setupRuntimeExamples(examplesPath: string): Promise; + host: Host; } export type McpToolFactory = (