Fix: Model loader callback consistency

src/loaders/JsonFileModelLoader.js

@@ -18,7 +18,6 @@
 
 const fs = require('fs');
 const path = require('path');
-const crypto = require('crypto');
 const ModelLoader = require('./ModelLoader');
 
 /**
@@ -108,7 +107,6 @@ class JsonFileModelLoader extends ModelLoader {
     this.filePath = path.resolve(filePath);
     this.pollingInterval = null;
     this.watchCallback = null;
-    this.lastHash = null;
 
     // Watch interval from env or default (in seconds, convert to milliseconds)
     const watchIntervalEnv = envValues.MODELS_POLL_INTERVAL || '1';
@@ -177,33 +175,24 @@ class JsonFileModelLoader extends ModelLoader {
     return this.loadSync();
   }
 
-  /**
-   * Calculate hash of file content
-   *
-   * @returns {string|null} Hash of file content, or null if file doesn't exist
-   */
-  getFileHash() {
-    try {
-      const content = fs.readFileSync(this.filePath, 'utf8');
-      return crypto.createHash('md5').update(content).digest('hex');
-    } catch {
-      return null;
-    }
-  }
-
   /**
    * Watch the JSON file for changes and reload automatically
    *
    * This enables hot-reload: changes to models.json are automatically detected
    * and loaded without restarting the application.
    *
    * Watch Flow:
-   * 1. Calculate initial file hash
-   * 2. Poll file at configured interval (default: 1s)
-   * 3. Calculate new hash and compare with previous
-   * 4. If different: load() → validateModels() → callback fires
+   * 1. Poll file at configured interval (default: 1s)
+   * 2. Load and parse models
+   * 3. Calculate hash of models and compare with previous
+   * 4. If different: notify callback with new models
    * 5. Update hash and continue polling
    *
+   * Change Detection:
+   * - Uses hash-based comparison (consistent with N8nApiModelLoader)
+   * - Only fires callback when models actually change
+   * - Formatting changes (whitespace) do not trigger reload
+   *
    * Why Simple Hash-Based Polling?
    * - fs.watch() is unreliable in Docker/CI environments
    * - fs.watchFile() has internal Node.js complexity
@@ -212,8 +201,8 @@ class JsonFileModelLoader extends ModelLoader {
    *
    * Error Handling:
    * - Invalid models (bad URL, etc): Logged as warnings, not thrown
-   * - File deleted during watch: Hash returns null, no reload
-   * - File permission changed: Hash returns null, no reload
+   * - File read errors: Logged, no callback fired
+   * - Parse errors: Logged, no callback fired
    *
    * Platform Notes:
    * - Works reliably in all environments (Docker, CI, network filesystems)
@@ -232,23 +221,22 @@ class JsonFileModelLoader extends ModelLoader {
 
     this.watchCallback = callback;
 
-    // Get initial hash
-    this.lastHash = this.getFileHash();
     console.log(
       `Watching ${this.filePath} for changes (polling every ${this.watchInterval / 1000}s)...`,
     );
 
     // Start polling
     this.pollingInterval = setInterval(async () => {
-      const currentHash = this.getFileHash();
+      try {
+        // Load and validate new models
+        const models = await this.load();
 
-      // Check if file content changed
-      if (currentHash && currentHash !== this.lastHash) {
-        console.log(`${this.filePath} changed, reloading...`);
+        // Calculate hash of current models
+        const currentHash = this.getModelsHash(models);
 
-        try {
-          // Load and validate new models
-          const models = await this.load();
+        // Check if models changed
+        if (currentHash !== this.lastHash) {
+          console.log('Models changed, reloading...');
           console.log(`Models reloaded successfully (${Object.keys(models).length} models)`);
 
           // Update hash
@@ -258,11 +246,11 @@ class JsonFileModelLoader extends ModelLoader {
           if (this.watchCallback) {
             this.watchCallback(models);
           }
-        } catch (error) {
-          // Log error but don't throw - watcher continues running
-          // This allows fixing the file and it will reload on next save
-          console.error(`Error reloading models: ${error.message}`);
         }
+      } catch (error) {
+        // Log error but don't throw - watcher continues running
+        // This allows fixing the file and it will reload on next save
+        console.error(`Error reloading models: ${error.message}`);
       }
     }, this.watchInterval);
   }

src/loaders/ModelLoader.js

@@ -16,6 +16,8 @@
  * along with this program. If not, see <https://www.gnu.org/licenses/>.
  */
 
+const crypto = require('crypto');
+
 /**
  * Base class for model loaders
  *
@@ -40,8 +42,17 @@
  * - validateModels() uses graceful degradation: invalid entries are filtered
  *   out with warnings rather than failing the entire load. This ensures the
  *   bridge continues running even with partially invalid configurations.
+ *
+ * Change Detection:
+ * - getModelsHash() provides hash-based change detection for watch() implementations
+ * - lastHash stores the last computed hash for comparison
+ * - Ensures callbacks only fire when models actually change
  */
 class ModelLoader {
+  constructor() {
+    // Hash of last loaded models for change detection
+    this.lastHash = null;
+  }
   /**
    * Get required environment variables for this loader
    *
@@ -117,6 +128,26 @@ class ModelLoader {
     // Optional: implement in subclass if watching is supported
   }
 
+  /**
+   * Calculate hash of models object
+   *
+   * Used for change detection in watch() implementations.
+   * Ensures callbacks only fire when models actually change.
+   *
+   * Implementation:
+   * - Sorts object keys for consistent hash calculation
+   * - Uses MD5 for fast hashing (security not required here)
+   * - Works with any models object structure
+   *
+   * @param {Object} models Models object to hash
+   * @returns {string} MD5 hash of models object
+   * @protected
+   */
+  getModelsHash(models) {
+    const content = JSON.stringify(models, Object.keys(models).sort());
+    return crypto.createHash('md5').update(content).digest('hex');
+  }
+
   /**
    * Validate the loaded models structure
    *

src/loaders/N8nApiModelLoader.js

@@ -367,9 +367,15 @@ class N8nApiModelLoader extends ModelLoader {
    *
    * Polling Flow:
    * 1. Start timer with configured interval
-   * 2. On timer: fetch workflows → convert to models → validate → callback
-   * 3. On error: log but continue polling (don't give up on temporary failures)
-   * 4. Repeat until stopWatching() is called
+   * 2. On timer: fetch workflows → convert to models → validate → compare hash
+   * 3. If hash changed: notify callback with new models
+   * 4. On error: log but continue polling (don't give up on temporary failures)
+   * 5. Repeat until stopWatching() is called
+   *
+   * Change Detection:
+   * - Uses hash-based comparison (consistent with JsonFileModelLoader)
+   * - Only fires callback when models actually change
+   * - Prevents unnecessary reloads and webhook notifications
    *
    * Why polling instead of webhooks?
    * - Simpler setup (no need for n8n to know about bridge)
@@ -402,9 +408,20 @@ class N8nApiModelLoader extends ModelLoader {
         console.log('Polling n8n for workflow changes...');
         const models = await this.load();
 
-        // Notify callback about new models
-        if (this.watchCallback) {
-          this.watchCallback(models);
+        // Calculate hash of current models
+        const currentHash = this.getModelsHash(models);
+
+        // Check if models changed
+        if (currentHash !== this.lastHash) {
+          console.log('Models changed, reloading...');
+
+          // Update hash
+          this.lastHash = currentHash;
+
+          // Notify callback about new models
+          if (this.watchCallback) {
+            this.watchCallback(models);
+          }
         }
       } catch (error) {
         // Log error but don't stop polling
@@ -418,7 +435,7 @@ class N8nApiModelLoader extends ModelLoader {
    * Stop polling for changes
    *
    * Called during application shutdown to cleanup resources.
-   * Clears polling timer and callback reference.
+   * Clears polling timer, callback reference, and hash state.
    *
    * Safe to call multiple times (idempotent).
    */
@@ -427,6 +444,7 @@ class N8nApiModelLoader extends ModelLoader {
       clearInterval(this.pollingTimer);
       this.pollingTimer = null;
       this.watchCallback = null;
+      this.lastHash = null;
       console.log('Stopped polling n8n API');
     }
   }

tests/loaders/JsonFileModelLoader/watch.test.js

@@ -93,40 +93,35 @@ describe('JsonFileModelLoader - watch', () => {
     });
     activeLoaders.push(loader);
 
-    let callbackCalled = false;
+    let callCount = 0;
     const startTime = Date.now();
+    const receivedModels = [];
 
-    const callbackPromise = new Promise((resolve, reject) => {
+    const callbackPromise = new Promise((resolve) => {
       loader.watch((models) => {
-        if (callbackCalled) {
-          return; // Prevent double-call
-        }
-        callbackCalled = true;
-
+        callCount++;
         const elapsed = Date.now() - startTime;
-        console.log(`Callback fired after ${elapsed}ms`);
+        console.log(`Callback fired (call ${callCount}) after ${elapsed}ms`);
+
+        receivedModels.push(models);
 
-        try {
-          expect(models).toEqual({
-            'updated-model': 'https://example.com/updated',
-          });
+        // Wait for second call before resolving
+        if (callCount >= 2) {
           resolve();
-        } catch (error) {
-          reject(error);
         }
       });
 
       console.log('Polling started, initial hash:', loader.lastHash);
 
-      // Wait a bit for polling to start, then change file
+      // Wait for first poll, then change file
       setTimeout(() => {
         console.log('Writing file...');
         const newModels = {
           'updated-model': 'https://example.com/updated',
         };
         fs.writeFileSync(testFile, JSON.stringify(newModels));
-        console.log('File written, new hash should be:', loader.getFileHash());
-      }, 50);
+        console.log('File written');
+      }, 250); // Wait for first poll to complete
 
       // Debug: Log if polling interval exists
       setTimeout(() => {
@@ -135,5 +130,16 @@ describe('JsonFileModelLoader - watch', () => {
     });
 
     await callbackPromise;
+
+    // Verify first call had initial models
+    expect(receivedModels[0]).toEqual({
+      'test-model-1': 'https://example.com/webhook1',
+      'test-model-2': 'https://example.com/webhook2',
+    });
+
+    // Verify second call had updated models
+    expect(receivedModels[1]).toEqual({
+      'updated-model': 'https://example.com/updated',
+    });
   }, 5000); // 5s timeout for debugging
 });