diff --git a/CHANGELOG.md b/CHANGELOG.md index 19db5ef..d1916e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,82 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [2.18.4] - 2025-10-09 + +### 🐛 Bug Fixes + +**Issue #296: sql.js Adapter Bypass Causing MCP Tool Failures** + +This release fixes a critical constructor bug in `NodeRepository` that caused the sql.js database adapter to be bypassed, resulting in empty object returns and MCP tool failures. + +#### Problem + +When using the sql.js fallback adapter (pure JavaScript implementation without native dependencies), three critical MCP tools were failing with "Cannot read properties of undefined" errors: +- `get_node_essentials` +- `get_node_info` +- `validate_node_operation` + +**Root Cause:** +The `NodeRepository` constructor used duck typing (`'db' in object`) to determine whether to unwrap the database adapter. This check incorrectly matched BOTH `SQLiteStorageService` AND `DatabaseAdapter` instances because both have a `.db` property. + +When sql.js was used: +1. `createDatabaseAdapter()` returned a `SQLJSAdapter` instance (wrapped) +2. `NodeRepository` constructor saw `'db' in adapter` was true +3. Constructor unwrapped it: `this.db = adapter.db` +4. This exposed the raw sql.js `Database` object, bypassing all wrapper logic +5. Raw sql.js API has completely different behavior (returns typed arrays instead of objects) +6. Result: Empty objects `{}` with no properties, causing undefined property access errors + +#### Fixed + +**NodeRepository Constructor Type Discrimination** +- Changed from duck typing (`'db' in object`) to precise instanceof check +- Only unwrap `SQLiteStorageService` instances (intended behavior) +- Keep `DatabaseAdapter` instances intact (preserves wrapper logic) +- File: `src/database/node-repository.ts` + +#### Technical Details + +**Before (Broken):** +```typescript +constructor(dbOrService: DatabaseAdapter | SQLiteStorageService) { + if ('db' in dbOrService) { // ❌ Matches EVERYTHING with .db property + this.db = dbOrService.db; // Unwraps both SQLiteStorageService AND DatabaseAdapter + } else { + this.db = dbOrService; + } +} +``` + +**After (Fixed):** +```typescript +constructor(dbOrService: DatabaseAdapter | SQLiteStorageService) { + if (dbOrService instanceof SQLiteStorageService) { // ✅ Only matches SQLiteStorageService + this.db = dbOrService.db; + return; + } + + this.db = dbOrService; // ✅ Keep DatabaseAdapter intact +} +``` + +**Why instanceof is Critical:** +- `'db' in object` is property checking (duck typing) - too permissive +- `instanceof` is class hierarchy checking - precise type discrimination +- With instanceof, sql.js queries flow through `SQLJSAdapter` → `SQLJSStatement` wrapper chain +- Wrapper normalizes sql.js behavior to match better-sqlite3 API (object returns) + +**Impact:** +- Fixes MCP tool failures on systems where better-sqlite3 cannot compile (Node.js version mismatches, ARM architectures) +- Ensures sql.js fallback works correctly with proper data normalization +- No performance impact (same code path, just preserved wrapper) + +#### Related + +- Closes issue #296 +- Affects environments where better-sqlite3 falls back to sql.js +- Common in Docker containers, CI environments, and ARM-based systems + ## [2.18.3] - 2025-10-09 ### 🔒 Critical Safety Fixes diff --git a/DEEP_CODE_REVIEW_SIMILAR_BUGS.md b/DEEP_CODE_REVIEW_SIMILAR_BUGS.md deleted file mode 100644 index 89e5b9b..0000000 --- a/DEEP_CODE_REVIEW_SIMILAR_BUGS.md +++ /dev/null @@ -1,478 +0,0 @@ -# DEEP CODE REVIEW: Similar Bugs Analysis -## Context: Version Extraction and Validation Issues (v2.17.4) - -**Date**: 2025-10-07 -**Scope**: Identify similar bugs to the two issues fixed in v2.17.4: -1. Version Extraction Bug: Checked non-existent `instance.baseDescription.defaultVersion` -2. Validation Bypass Bug: Langchain nodes skipped ALL validation before typeVersion check - ---- - -## CRITICAL FINDINGS - -### BUG #1: CRITICAL - Version 0 Incorrectly Rejected in typeVersion Validation -**Severity**: CRITICAL -**Affects**: AI Agent ecosystem specifically - -**Location**: `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/services/workflow-validator.ts:462` - -**Issue**: -```typescript -// Line 462 - INCORRECT: Rejects typeVersion = 0 -else if (typeof node.typeVersion !== 'number' || node.typeVersion < 1) { - result.errors.push({ - type: 'error', - nodeId: node.id, - nodeName: node.name, - message: `Invalid typeVersion: ${node.typeVersion}. Must be a positive number` - }); -} -``` - -**Why This is Critical**: -- n8n allows `typeVersion: 0` as a valid version (rare but legal) -- The check `node.typeVersion < 1` rejects version 0 -- This is inconsistent with how we handle version extraction -- Could break workflows using nodes with version 0 - -**Similar to Fixed Bug**: -- Makes incorrect assumptions about version values -- Breaks for edge cases (0 is valid, just like checking wrong property paths) -- Uses wrong comparison operator (< 1 instead of <= 0 or !== undefined) - -**Test Case**: -```typescript -const node = { - id: 'test', - name: 'Test Node', - type: 'nodes-base.someNode', - typeVersion: 0, // Valid but rejected! - parameters: {} -}; -// Current code: ERROR "Invalid typeVersion: 0. Must be a positive number" -// Expected: Should be valid -``` - -**Recommended Fix**: -```typescript -// Line 462 - CORRECT: Allow version 0 -else if (typeof node.typeVersion !== 'number' || node.typeVersion < 0) { - result.errors.push({ - type: 'error', - nodeId: node.id, - nodeName: node.name, - message: `Invalid typeVersion: ${node.typeVersion}. Must be a non-negative number (>= 0)` - }); -} -``` - -**Verification**: Check if n8n core uses version 0 anywhere: -```bash -# Need to search n8n source for nodes with version 0 -grep -r "typeVersion.*:.*0" node_modules/n8n-nodes-base/ -``` - ---- - -### BUG #2: HIGH - Inconsistent baseDescription Checks in simple-parser.ts -**Severity**: HIGH -**Affects**: Node loading and parsing - -**Locations**: -1. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/simple-parser.ts:195-196` -2. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/simple-parser.ts:208-209` - -**Issue #1 - Instance Check**: -```typescript -// Lines 195-196 - POTENTIALLY WRONG for VersionedNodeType -if (instance?.baseDescription?.defaultVersion) { - return instance.baseDescription.defaultVersion.toString(); -} -``` - -**Issue #2 - Class Check**: -```typescript -// Lines 208-209 - POTENTIALLY WRONG for VersionedNodeType -if (nodeClass.baseDescription?.defaultVersion) { - return nodeClass.baseDescription.defaultVersion.toString(); -} -``` - -**Why This is Similar**: -- **EXACTLY THE SAME BUG** we just fixed in `node-parser.ts`! -- VersionedNodeType stores base info in `description`, not `baseDescription` -- These checks will FAIL for VersionedNodeType instances -- `simple-parser.ts` was not updated when `node-parser.ts` was fixed - -**Evidence from Fixed Code** (node-parser.ts): -```typescript -// Line 149 comment: -// "Critical Fix (v2.17.4): Removed check for non-existent instance.baseDescription.defaultVersion" - -// Line 167 comment: -// "VersionedNodeType stores baseDescription as 'description', not 'baseDescription'" -``` - -**Impact**: -- `simple-parser.ts` is used as a fallback parser -- Will return incorrect versions for VersionedNodeType nodes -- Could cause version mismatches between parsers - -**Recommended Fix**: -```typescript -// REMOVE Lines 195-196 entirely (non-existent property) -// REMOVE Lines 208-209 entirely (non-existent property) - -// Instead, use the correct property path: -if (instance?.description?.defaultVersion) { - return instance.description.defaultVersion.toString(); -} - -if (nodeClass.description?.defaultVersion) { - return nodeClass.description.defaultVersion.toString(); -} -``` - -**Test Case**: -```typescript -// Test with AI Agent (VersionedNodeType) -const AIAgent = require('@n8n/n8n-nodes-langchain').Agent; -const instance = new AIAgent(); - -// BUG: simple-parser checks instance.baseDescription.defaultVersion (doesn't exist) -// CORRECT: Should check instance.description.defaultVersion (exists) -console.log('baseDescription exists?', !!instance.baseDescription); // false -console.log('description exists?', !!instance.description); // true -console.log('description.defaultVersion?', instance.description?.defaultVersion); -``` - ---- - -### BUG #3: MEDIUM - Inconsistent Math.max Usage Without Validation -**Severity**: MEDIUM -**Affects**: All versioned nodes - -**Locations**: -1. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/property-extractor.ts:19` -2. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/property-extractor.ts:75` -3. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/property-extractor.ts:181` -4. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/node-parser.ts:175` -5. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/node-parser.ts:202` - -**Issue**: -```typescript -// property-extractor.ts:19 - NO VALIDATION -if (instance?.nodeVersions) { - const versions = Object.keys(instance.nodeVersions); - const latestVersion = Math.max(...versions.map(Number)); // DANGER! - const versionedNode = instance.nodeVersions[latestVersion]; - // ... -} -``` - -**Why This is Problematic**: -1. **No empty array check**: `Math.max()` returns `-Infinity` for empty arrays -2. **No NaN check**: Non-numeric keys cause `Math.max(NaN, NaN) = NaN` -3. **Ignores defaultVersion**: Should check `defaultVersion` BEFORE falling back to max -4. **Inconsistent with fixed code**: node-parser.ts was fixed to prioritize `currentVersion` and `defaultVersion` - -**Edge Cases That Break**: -```typescript -// Case 1: Empty nodeVersions -const nodeVersions = {}; -const versions = Object.keys(nodeVersions); // [] -const latestVersion = Math.max(...versions.map(Number)); // -Infinity -const versionedNode = nodeVersions[-Infinity]; // undefined - -// Case 2: Non-numeric keys -const nodeVersions = { 'v1': {}, 'v2': {} }; -const versions = Object.keys(nodeVersions); // ['v1', 'v2'] -const latestVersion = Math.max(...versions.map(Number)); // Math.max(NaN, NaN) = NaN -const versionedNode = nodeVersions[NaN]; // undefined -``` - -**Similar to Fixed Bug**: -- Assumes data structure without validation -- Could return undefined and cause downstream errors -- Doesn't follow the correct priority: `currentVersion` > `defaultVersion` > `max(nodeVersions)` - -**Recommended Fix**: -```typescript -// property-extractor.ts - Consistent with node-parser.ts fix -if (instance?.nodeVersions) { - // PRIORITY 1: Check currentVersion (already computed by VersionedNodeType) - if (instance.currentVersion !== undefined) { - const versionedNode = instance.nodeVersions[instance.currentVersion]; - if (versionedNode?.description?.properties) { - return this.normalizeProperties(versionedNode.description.properties); - } - } - - // PRIORITY 2: Check defaultVersion - if (instance.description?.defaultVersion !== undefined) { - const versionedNode = instance.nodeVersions[instance.description.defaultVersion]; - if (versionedNode?.description?.properties) { - return this.normalizeProperties(versionedNode.description.properties); - } - } - - // PRIORITY 3: Fallback to max with validation - const versions = Object.keys(instance.nodeVersions); - if (versions.length > 0) { - const numericVersions = versions.map(Number).filter(v => !isNaN(v)); - if (numericVersions.length > 0) { - const latestVersion = Math.max(...numericVersions); - const versionedNode = instance.nodeVersions[latestVersion]; - if (versionedNode?.description?.properties) { - return this.normalizeProperties(versionedNode.description.properties); - } - } - } -} -``` - -**Applies to 5 locations** - all need same fix pattern. - ---- - -### BUG #4: MEDIUM - Expression Validation Skip for Langchain Nodes (Line 972) -**Severity**: MEDIUM -**Affects**: AI Agent ecosystem - -**Location**: `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/services/workflow-validator.ts:972` - -**Issue**: -```typescript -// Line 969-974 - Another early skip for langchain -// Skip expression validation for langchain nodes -// They have AI-specific validators and different expression rules -const normalizedType = NodeTypeNormalizer.normalizeToFullForm(node.type); -if (normalizedType.startsWith('nodes-langchain.')) { - continue; // Skip ALL expression validation -} -``` - -**Why This Could Be Problematic**: -- Similar to the bug we fixed where langchain nodes skipped typeVersion validation -- Langchain nodes CAN use expressions (especially in AI Agent system prompts, tool configurations) -- Skipping ALL expression validation means we won't catch: - - Syntax errors in expressions - - Invalid node references - - Missing input data references - -**Similar to Fixed Bug**: -- Early return/continue before running validation -- Assumes langchain nodes don't need a certain type of validation -- We already fixed this pattern once for typeVersion - might need fixing here too - -**Investigation Required**: -Need to determine if langchain nodes: -1. Use n8n expressions in their parameters? (YES - AI Agent uses expressions) -2. Need different expression validation rules? (MAYBE) -3. Should have AI-specific expression validation? (PROBABLY YES) - -**Recommended Action**: -1. **Short-term**: Add comment explaining WHY we skip (currently missing) -2. **Medium-term**: Implement langchain-specific expression validation -3. **Long-term**: Never skip validation entirely - always have appropriate validation - -**Example of Langchain Expressions**: -```typescript -// AI Agent system prompt can contain expressions -{ - type: '@n8n/n8n-nodes-langchain.agent', - parameters: { - text: 'You are an assistant. User input: {{ $json.userMessage }}' // Expression! - } -} -``` - ---- - -### BUG #5: LOW - Inconsistent Version Property Access Patterns -**Severity**: LOW -**Affects**: Code maintainability - -**Locations**: Multiple files use different patterns - -**Issue**: Three different patterns for accessing version: -```typescript -// Pattern 1: Direct access with fallback (SAFE) -const version = nodeInfo.version || 1; - -// Pattern 2: Direct access without fallback (UNSAFE) -if (nodeInfo.version && node.typeVersion < nodeInfo.version) { ... } - -// Pattern 3: Falsy check (BREAKS for version 0) -if (nodeInfo.version) { ... } // Fails if version = 0 -``` - -**Why This Matters**: -- Pattern 3 breaks for `version = 0` (falsy but valid) -- Inconsistency makes code harder to maintain -- Similar issue to version < 1 check - -**Examples**: -```typescript -// workflow-validator.ts:471 - UNSAFE for version 0 -else if (nodeInfo.version && node.typeVersion < nodeInfo.version) { - // If nodeInfo.version = 0, this never executes (falsy check) -} - -// workflow-validator.ts:480 - UNSAFE for version 0 -else if (nodeInfo.version && node.typeVersion > nodeInfo.version) { - // If nodeInfo.version = 0, this never executes (falsy check) -} -``` - -**Recommended Fix**: -```typescript -// Use !== undefined for version checks -else if (nodeInfo.version !== undefined && node.typeVersion < nodeInfo.version) { - // Now works correctly for version 0 -} - -else if (nodeInfo.version !== undefined && node.typeVersion > nodeInfo.version) { - // Now works correctly for version 0 -} -``` - ---- - -### BUG #6: LOW - Missing Type Safety for VersionedNodeType Properties -**Severity**: LOW -**Affects**: TypeScript type safety - -**Issue**: No TypeScript interface for VersionedNodeType properties - -**Current Code**: -```typescript -// We access these properties everywhere but no type definition: -instance.currentVersion // any -instance.description // any -instance.nodeVersions // any -instance.baseDescription // any (doesn't exist but not caught!) -``` - -**Why This Matters**: -- TypeScript COULD HAVE caught the `baseDescription` bug -- Using `any` everywhere defeats type safety -- Makes refactoring dangerous - -**Recommended Fix**: -```typescript -// Create types/versioned-node.ts -export interface VersionedNodeTypeInstance { - currentVersion: number; - description: { - name: string; - displayName: string; - defaultVersion?: number; - version?: number | number[]; - properties?: any[]; - // ... other properties - }; - nodeVersions: { - [version: number]: { - description: { - properties?: any[]; - // ... other properties - }; - }; - }; -} - -// Then use in code: -const instance = new nodeClass() as VersionedNodeTypeInstance; -instance.baseDescription // TypeScript error: Property 'baseDescription' does not exist -``` - ---- - -## SUMMARY OF FINDINGS - -### By Severity: - -**CRITICAL (1 bug)**: -1. Version 0 incorrectly rejected (workflow-validator.ts:462) - -**HIGH (1 bug)**: -2. Inconsistent baseDescription checks in simple-parser.ts (EXACT DUPLICATE of fixed bug) - -**MEDIUM (2 bugs)**: -3. Unsafe Math.max usage in property-extractor.ts (5 locations) -4. Expression validation skip for langchain nodes (workflow-validator.ts:972) - -**LOW (2 issues)**: -5. Inconsistent version property access patterns -6. Missing TypeScript types for VersionedNodeType - -### By Category: - -**Property Name Assumptions** (Similar to Bug #1): -- BUG #2: baseDescription checks in simple-parser.ts - -**Validation Order Issues** (Similar to Bug #2): -- BUG #4: Expression validation skip for langchain nodes - -**Version Logic Issues**: -- BUG #1: Version 0 rejected incorrectly -- BUG #3: Math.max without validation -- BUG #5: Inconsistent version checks - -**Type Safety Issues**: -- BUG #6: Missing VersionedNodeType types - -### Affects AI Agent Ecosystem: -- BUG #1: Critical - blocks valid typeVersion values -- BUG #2: High - affects AI Agent version extraction -- BUG #4: Medium - skips expression validation -- All others: Indirectly affect stability - ---- - -## RECOMMENDED ACTIONS - -### Immediate (Critical): -1. Fix version 0 rejection in workflow-validator.ts:462 -2. Fix baseDescription checks in simple-parser.ts - -### Short-term (High Priority): -3. Add validation to all Math.max usages in property-extractor.ts -4. Investigate and document expression validation skip for langchain - -### Medium-term: -5. Standardize version property access patterns -6. Add TypeScript types for VersionedNodeType - -### Testing: -7. Add test cases for version 0 -8. Add test cases for empty nodeVersions -9. Add test cases for langchain expression validation - ---- - -## VERIFICATION CHECKLIST - -For each bug found: -- [x] File and line number identified -- [x] Code snippet showing issue -- [x] Why it's similar to fixed bugs -- [x] Severity assessment -- [x] Test case provided -- [x] Fix recommended with code -- [x] Impact on AI Agent ecosystem assessed - ---- - -## NOTES - -1. **Pattern Recognition**: The baseDescription bug in simple-parser.ts is EXACTLY the same bug we just fixed in node-parser.ts, suggesting these files should be refactored to share version extraction logic. - -2. **Validation Philosophy**: We're seeing a pattern of skipping validation for langchain nodes. This was correct for PARAMETER validation but WRONG for typeVersion. Need to review each skip carefully. - -3. **Version 0 Edge Case**: If n8n doesn't use version 0 in practice, the critical bug might be theoretical. However, rejecting valid values is still a bug. - -4. **Math.max Safety**: The Math.max pattern is used 5+ times. Should extract to a utility function with proper validation. - -5. **Type Safety**: Adding proper TypeScript types would have prevented the baseDescription bug entirely. Strong recommendation for future work. diff --git a/data/nodes.db b/data/nodes.db index 5dd3887..bf3edbf 100644 Binary files a/data/nodes.db and b/data/nodes.db differ diff --git a/package.json b/package.json index d9a032a..cb43e19 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "n8n-mcp", - "version": "2.18.3", + "version": "2.18.4", "description": "Integration between n8n workflow automation and Model Context Protocol (MCP)", "main": "dist/index.js", "bin": { diff --git a/src/database/node-repository.ts b/src/database/node-repository.ts index 85b7205..7ba8797 100644 --- a/src/database/node-repository.ts +++ b/src/database/node-repository.ts @@ -7,11 +7,12 @@ export class NodeRepository { private db: DatabaseAdapter; constructor(dbOrService: DatabaseAdapter | SQLiteStorageService) { - if ('db' in dbOrService) { + if (dbOrService instanceof SQLiteStorageService) { this.db = dbOrService.db; - } else { - this.db = dbOrService; + return; } + + this.db = dbOrService; } /**