mirror of
https://github.com/czlonkowski/n8n-mcp.git
synced 2026-02-06 05:23:08 +00:00
refactor: implement high-value code quality improvements
Implemented three high-value fixes identified in code review:
1. NPM Registry Response Validation (npm-version-checker.ts)
- Added NpmRegistryResponse TypeScript interface
- Added JSON parsing validation with try-catch error handling
- Added response structure validation (checking required fields)
- Added semver format validation with regex pattern
- Prevents crashes from malformed npm registry responses
2. TypeScript Type Safety (handlers-n8n-manager.ts)
- Added 5 comprehensive TypeScript interfaces:
* HealthCheckResponseData
* CloudPlatformGuide
* WorkflowValidationResponse
* DiagnosticResponseData
- Replaced 'any' types with proper interfaces in 6 locations
- Imported ExpressionFormatIssue from expression-format-validator
- Improved compile-time type checking and IDE support
3. Cache Hit Rate Calculation (handlers-n8n-manager.ts)
- Improved division-by-zero protection
- Changed condition from 'size > 0' to explicit operation count check
- More robust against edge cases in cache metrics
All changes verified with:
- TypeScript compilation (0 errors)
- Integration tests (195/195 passed)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -30,7 +30,7 @@ import { NodeRepository } from '../database/node-repository';
|
|||||||
import { InstanceContext, validateInstanceContext } from '../types/instance-context';
|
import { InstanceContext, validateInstanceContext } from '../types/instance-context';
|
||||||
import { NodeTypeNormalizer } from '../utils/node-type-normalizer';
|
import { NodeTypeNormalizer } from '../utils/node-type-normalizer';
|
||||||
import { WorkflowAutoFixer, AutoFixConfig } from '../services/workflow-auto-fixer';
|
import { WorkflowAutoFixer, AutoFixConfig } from '../services/workflow-auto-fixer';
|
||||||
import { ExpressionFormatValidator } from '../services/expression-format-validator';
|
import { ExpressionFormatValidator, ExpressionFormatIssue } from '../services/expression-format-validator';
|
||||||
import { handleUpdatePartialWorkflow } from './handlers-workflow-diff';
|
import { handleUpdatePartialWorkflow } from './handlers-workflow-diff';
|
||||||
import { telemetry } from '../telemetry';
|
import { telemetry } from '../telemetry';
|
||||||
import {
|
import {
|
||||||
@@ -44,6 +44,143 @@ import {
|
|||||||
import { processExecution } from '../services/execution-processor';
|
import { processExecution } from '../services/execution-processor';
|
||||||
import { checkNpmVersion, formatVersionMessage } from '../utils/npm-version-checker';
|
import { checkNpmVersion, formatVersionMessage } from '../utils/npm-version-checker';
|
||||||
|
|
||||||
|
// ========================================================================
|
||||||
|
// TypeScript Interfaces for Type Safety
|
||||||
|
// ========================================================================
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Health Check Response Data Structure
|
||||||
|
*/
|
||||||
|
interface HealthCheckResponseData {
|
||||||
|
status: string;
|
||||||
|
instanceId?: string;
|
||||||
|
n8nVersion?: string;
|
||||||
|
features?: Record<string, unknown>;
|
||||||
|
apiUrl?: string;
|
||||||
|
mcpVersion: string;
|
||||||
|
supportedN8nVersion?: string;
|
||||||
|
versionCheck: {
|
||||||
|
current: string;
|
||||||
|
latest: string | null;
|
||||||
|
upToDate: boolean;
|
||||||
|
message: string;
|
||||||
|
updateCommand?: string;
|
||||||
|
};
|
||||||
|
performance: {
|
||||||
|
responseTimeMs: number;
|
||||||
|
cacheHitRate: string;
|
||||||
|
cachedInstances: number;
|
||||||
|
};
|
||||||
|
nextSteps?: string[];
|
||||||
|
updateWarning?: string;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Cloud Platform Guide Structure
|
||||||
|
*/
|
||||||
|
interface CloudPlatformGuide {
|
||||||
|
name: string;
|
||||||
|
troubleshooting: string[];
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Workflow Validation Response Data
|
||||||
|
*/
|
||||||
|
interface WorkflowValidationResponse {
|
||||||
|
valid: boolean;
|
||||||
|
workflowId?: string;
|
||||||
|
workflowName?: string;
|
||||||
|
summary: {
|
||||||
|
totalNodes: number;
|
||||||
|
enabledNodes: number;
|
||||||
|
triggerNodes: number;
|
||||||
|
validConnections: number;
|
||||||
|
invalidConnections: number;
|
||||||
|
expressionsValidated: number;
|
||||||
|
errorCount: number;
|
||||||
|
warningCount: number;
|
||||||
|
};
|
||||||
|
errors?: Array<{
|
||||||
|
node: string;
|
||||||
|
nodeName?: string;
|
||||||
|
message: string;
|
||||||
|
details?: Record<string, unknown>;
|
||||||
|
}>;
|
||||||
|
warnings?: Array<{
|
||||||
|
node: string;
|
||||||
|
nodeName?: string;
|
||||||
|
message: string;
|
||||||
|
details?: Record<string, unknown>;
|
||||||
|
}>;
|
||||||
|
suggestions?: unknown[];
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Diagnostic Response Data Structure
|
||||||
|
*/
|
||||||
|
interface DiagnosticResponseData {
|
||||||
|
timestamp: string;
|
||||||
|
environment: {
|
||||||
|
N8N_API_URL: string | null;
|
||||||
|
N8N_API_KEY: string | null;
|
||||||
|
NODE_ENV: string;
|
||||||
|
MCP_MODE: string;
|
||||||
|
isDocker: boolean;
|
||||||
|
cloudPlatform: string | null;
|
||||||
|
nodeVersion: string;
|
||||||
|
platform: string;
|
||||||
|
};
|
||||||
|
apiConfiguration: {
|
||||||
|
configured: boolean;
|
||||||
|
status: {
|
||||||
|
configured: boolean;
|
||||||
|
connected: boolean;
|
||||||
|
error: string | null;
|
||||||
|
version: string | null;
|
||||||
|
};
|
||||||
|
config: {
|
||||||
|
baseUrl: string;
|
||||||
|
timeout: number;
|
||||||
|
maxRetries: number;
|
||||||
|
} | null;
|
||||||
|
};
|
||||||
|
versionInfo: {
|
||||||
|
current: string;
|
||||||
|
latest: string | null;
|
||||||
|
upToDate: boolean;
|
||||||
|
message: string;
|
||||||
|
updateCommand?: string;
|
||||||
|
};
|
||||||
|
toolsAvailability: {
|
||||||
|
documentationTools: {
|
||||||
|
count: number;
|
||||||
|
enabled: boolean;
|
||||||
|
description: string;
|
||||||
|
};
|
||||||
|
managementTools: {
|
||||||
|
count: number;
|
||||||
|
enabled: boolean;
|
||||||
|
description: string;
|
||||||
|
};
|
||||||
|
totalAvailable: number;
|
||||||
|
};
|
||||||
|
performance: {
|
||||||
|
diagnosticResponseTimeMs: number;
|
||||||
|
cacheHitRate: string;
|
||||||
|
cachedInstances: number;
|
||||||
|
};
|
||||||
|
modeSpecificDebug: Record<string, unknown>;
|
||||||
|
dockerDebug?: Record<string, unknown>;
|
||||||
|
cloudPlatformDebug?: CloudPlatformGuide;
|
||||||
|
nextSteps?: Record<string, unknown>;
|
||||||
|
troubleshooting?: Record<string, unknown>;
|
||||||
|
setupGuide?: Record<string, unknown>;
|
||||||
|
updateWarning?: Record<string, unknown>;
|
||||||
|
debug?: Record<string, unknown>;
|
||||||
|
[key: string]: unknown; // Allow dynamic property access for optional fields
|
||||||
|
}
|
||||||
|
|
||||||
|
// ========================================================================
|
||||||
// Singleton n8n API client instance (backward compatibility)
|
// Singleton n8n API client instance (backward compatibility)
|
||||||
let defaultApiClient: N8nApiClient | null = null;
|
let defaultApiClient: N8nApiClient | null = null;
|
||||||
let lastDefaultConfigUrl: string | null = null;
|
let lastDefaultConfigUrl: string | null = null;
|
||||||
@@ -732,7 +869,7 @@ export async function handleValidateWorkflow(
|
|||||||
const validationResult = await validator.validateWorkflow(workflow, input.options);
|
const validationResult = await validator.validateWorkflow(workflow, input.options);
|
||||||
|
|
||||||
// Format the response (same format as the regular validate_workflow tool)
|
// Format the response (same format as the regular validate_workflow tool)
|
||||||
const response: any = {
|
const response: WorkflowValidationResponse = {
|
||||||
valid: validationResult.valid,
|
valid: validationResult.valid,
|
||||||
workflowId: workflow.id,
|
workflowId: workflow.id,
|
||||||
workflowName: workflow.name,
|
workflowName: workflow.name,
|
||||||
@@ -833,7 +970,7 @@ export async function handleAutofixWorkflow(
|
|||||||
});
|
});
|
||||||
|
|
||||||
// Check for expression format issues
|
// Check for expression format issues
|
||||||
const allFormatIssues: any[] = [];
|
const allFormatIssues: ExpressionFormatIssue[] = [];
|
||||||
for (const node of workflow.nodes) {
|
for (const node of workflow.nodes) {
|
||||||
const formatContext = {
|
const formatContext = {
|
||||||
nodeType: node.type,
|
nodeType: node.type,
|
||||||
@@ -1248,7 +1385,7 @@ export async function handleHealthCheck(context?: InstanceContext): Promise<McpT
|
|||||||
const responseTime = Date.now() - startTime;
|
const responseTime = Date.now() - startTime;
|
||||||
|
|
||||||
// Build response data
|
// Build response data
|
||||||
const responseData: any = {
|
const responseData: HealthCheckResponseData = {
|
||||||
status: health.status,
|
status: health.status,
|
||||||
instanceId: health.instanceId,
|
instanceId: health.instanceId,
|
||||||
n8nVersion: health.n8nVersion,
|
n8nVersion: health.n8nVersion,
|
||||||
@@ -1265,7 +1402,7 @@ export async function handleHealthCheck(context?: InstanceContext): Promise<McpT
|
|||||||
},
|
},
|
||||||
performance: {
|
performance: {
|
||||||
responseTimeMs: responseTime,
|
responseTimeMs: responseTime,
|
||||||
cacheHitRate: cacheMetricsData.size > 0
|
cacheHitRate: (cacheMetricsData.hits + cacheMetricsData.misses) > 0
|
||||||
? ((cacheMetricsData.hits / (cacheMetricsData.hits + cacheMetricsData.misses)) * 100).toFixed(2) + '%'
|
? ((cacheMetricsData.hits / (cacheMetricsData.hits + cacheMetricsData.misses)) * 100).toFixed(2) + '%'
|
||||||
: 'N/A',
|
: 'N/A',
|
||||||
cachedInstances: cacheMetricsData.size
|
cachedInstances: cacheMetricsData.size
|
||||||
@@ -1497,7 +1634,7 @@ function getDockerDebug(isDocker: boolean) {
|
|||||||
function getCloudPlatformDebug(cloudPlatform: string | null) {
|
function getCloudPlatformDebug(cloudPlatform: string | null) {
|
||||||
if (!cloudPlatform) return null;
|
if (!cloudPlatform) return null;
|
||||||
|
|
||||||
const platformGuides: Record<string, any> = {
|
const platformGuides: Record<string, CloudPlatformGuide> = {
|
||||||
railway: {
|
railway: {
|
||||||
name: 'Railway',
|
name: 'Railway',
|
||||||
troubleshooting: [
|
troubleshooting: [
|
||||||
@@ -1623,7 +1760,7 @@ export async function handleDiagnostic(request: any, context?: InstanceContext):
|
|||||||
const responseTime = Date.now() - startTime;
|
const responseTime = Date.now() - startTime;
|
||||||
|
|
||||||
// Build diagnostic report
|
// Build diagnostic report
|
||||||
const diagnostic: any = {
|
const diagnostic: DiagnosticResponseData = {
|
||||||
timestamp: new Date().toISOString(),
|
timestamp: new Date().toISOString(),
|
||||||
environment: envVars,
|
environment: envVars,
|
||||||
apiConfiguration: {
|
apiConfiguration: {
|
||||||
@@ -1659,11 +1796,12 @@ export async function handleDiagnostic(request: any, context?: InstanceContext):
|
|||||||
},
|
},
|
||||||
performance: {
|
performance: {
|
||||||
diagnosticResponseTimeMs: responseTime,
|
diagnosticResponseTimeMs: responseTime,
|
||||||
cacheHitRate: cacheMetricsData.size > 0
|
cacheHitRate: (cacheMetricsData.hits + cacheMetricsData.misses) > 0
|
||||||
? ((cacheMetricsData.hits / (cacheMetricsData.hits + cacheMetricsData.misses)) * 100).toFixed(2) + '%'
|
? ((cacheMetricsData.hits / (cacheMetricsData.hits + cacheMetricsData.misses)) * 100).toFixed(2) + '%'
|
||||||
: 'N/A',
|
: 'N/A',
|
||||||
cachedInstances: cacheMetricsData.size
|
cachedInstances: cacheMetricsData.size
|
||||||
}
|
},
|
||||||
|
modeSpecificDebug: getModeSpecificDebug(mcpMode)
|
||||||
};
|
};
|
||||||
|
|
||||||
// Enhanced guidance based on telemetry insights
|
// Enhanced guidance based on telemetry insights
|
||||||
@@ -1783,17 +1921,16 @@ export async function handleDiagnostic(request: any, context?: InstanceContext):
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
// Add environment-aware debugging guidance
|
|
||||||
diagnostic.modeSpecificDebug = getModeSpecificDebug(mcpMode);
|
|
||||||
|
|
||||||
// Add Docker-specific debugging if in container
|
// Add Docker-specific debugging if in container
|
||||||
if (isDocker) {
|
const dockerDebug = getDockerDebug(isDocker);
|
||||||
diagnostic.dockerDebug = getDockerDebug(true);
|
if (dockerDebug) {
|
||||||
|
diagnostic.dockerDebug = dockerDebug;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Add cloud platform-specific debugging if detected
|
// Add cloud platform-specific debugging if detected
|
||||||
if (cloudPlatform) {
|
const cloudDebug = getCloudPlatformDebug(cloudPlatform);
|
||||||
diagnostic.cloudPlatformDebug = getCloudPlatformDebug(cloudPlatform);
|
if (cloudDebug) {
|
||||||
|
diagnostic.cloudPlatformDebug = cloudDebug;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Add verbose debug info if requested
|
// Add verbose debug info if requested
|
||||||
|
|||||||
@@ -7,6 +7,15 @@
|
|||||||
|
|
||||||
import { logger } from './logger';
|
import { logger } from './logger';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* NPM Registry Response structure
|
||||||
|
* Based on npm registry JSON format for package metadata
|
||||||
|
*/
|
||||||
|
interface NpmRegistryResponse {
|
||||||
|
version: string;
|
||||||
|
[key: string]: unknown;
|
||||||
|
}
|
||||||
|
|
||||||
export interface VersionCheckResult {
|
export interface VersionCheckResult {
|
||||||
currentVersion: string;
|
currentVersion: string;
|
||||||
latestVersion: string | null;
|
latestVersion: string | null;
|
||||||
@@ -71,8 +80,26 @@ export async function checkNpmVersion(forceRefresh: boolean = false): Promise<Ve
|
|||||||
return result;
|
return result;
|
||||||
}
|
}
|
||||||
|
|
||||||
const data: any = await response.json();
|
// Parse and validate JSON response
|
||||||
const latestVersion = data.version as string;
|
let data: unknown;
|
||||||
|
try {
|
||||||
|
data = await response.json();
|
||||||
|
} catch (error) {
|
||||||
|
throw new Error('Failed to parse npm registry response as JSON');
|
||||||
|
}
|
||||||
|
|
||||||
|
// Validate response structure
|
||||||
|
if (!data || typeof data !== 'object' || !('version' in data)) {
|
||||||
|
throw new Error('Invalid response format from npm registry');
|
||||||
|
}
|
||||||
|
|
||||||
|
const registryData = data as NpmRegistryResponse;
|
||||||
|
const latestVersion = registryData.version;
|
||||||
|
|
||||||
|
// Validate version format (semver: x.y.z or x.y.z-prerelease)
|
||||||
|
if (!latestVersion || !/^\d+\.\d+\.\d+/.test(latestVersion)) {
|
||||||
|
throw new Error(`Invalid version format from npm registry: ${latestVersion}`);
|
||||||
|
}
|
||||||
|
|
||||||
// Compare versions
|
// Compare versions
|
||||||
const isOutdated = compareVersions(currentVersion, latestVersion) < 0;
|
const isOutdated = compareVersions(currentVersion, latestVersion) < 0;
|
||||||
|
|||||||
Reference in New Issue
Block a user