fix: resolve GitLab "Not Found" error in comment deletion (#167)

* fix: resolve GitLab 'Not Found' error in comment deletion

- Add comprehensive error handling for 404 errors in GitLabMRService
- Replace Promise.all with individual error handling in GitLabAdapter
- Add graceful handling of stale comment/discussion IDs
- Enhance logging for partial deletion failures
- Add extensive unit tests for error scenarios
- Install missing @gitbeaker dependencies for proper API support

Fixes issue where stale comment IDs caused entire comment cleanup to fail
with 'Not Found' error, preventing successful GitLab integration.

* fix: Add missing type definitions for Node.js 18 compatibility

- Add @types/glob@^7.2.0 and @types/minimatch@^3.0.5 to resolve TypeScript compilation errors
- Update yarn.lock with new dependencies
- Ensure GitLab MR Service uses proper types from @gitbeaker/core
- All tests passing (131/131)
- Build successful with TypeScript strict mode

* fix: resolve all linting errors and reduce cognitive complexity

- Fixed all TypeScript 'any' type annotations to proper types
- Removed unused imports and functions
- Reduced cognitive complexity in GitLabMRService.deleteDiscussion method
- Fixed prettier formatting issues
- Updated interface types to match implementation
- All tests passing and linting clean

---------

Co-authored-by: pphanphila <prakritchai.phanphila@agoda.com>

Author: prakritchai

package.json

@@ -28,8 +28,10 @@
   "devDependencies": {
     "@jscpd/finder": "^3.5.5",
     "@octokit/types": "^6.34.0",
+    "@types/glob": "^7.2.0",
     "@types/jest": "^26.0.18",
     "@types/js-yaml": "^4.0.0",
+    "@types/minimatch": "^3.0.5",
     "@types/node": "^14.14.11",
     "@types/rimraf": "^3.0.0",
     "@typescript-eslint/eslint-plugin": "^4.9.1",

src/Parser/SarifParser.ts

@@ -88,23 +88,28 @@ export class SarifParser extends Parser {
   private processResults(run: SarifRun, lintItems: LintItem[]): void {
     const results = run.results || [];
     for (const result of results) {
-      const lintItem = this.toLintItem(result, run);
+      const lintItem = this.toLintItem(result);
       if (lintItem) {
         lintItems.push(lintItem);
       }
     }
   }
 
-  private isValidSarifLog(log: any): log is SarifLog {
+  private isValidSarifLog(log: unknown): log is SarifLog {
+    if (log == null || typeof log !== 'object') {
+      return false;
+    }
+
+    const obj = log as Record<string, unknown>;
     return (
-      log &&
-      typeof log === 'object' &&
-      typeof log.version === 'string' &&
-      Array.isArray(log.runs)
+      'version' in obj &&
+      typeof obj.version === 'string' &&
+      'runs' in obj &&
+      Array.isArray(obj.runs)
     );
   }
 
-  private toLintItem(result: SarifResult, run: SarifRun): LintItem | null {
+  private toLintItem(result: SarifResult): LintItem | null {
     if (!result.locations?.[0]) {
       Log.warn('SarifParser Warning: Result has no location information', { result });
       return null;
@@ -141,8 +146,4 @@ export class SarifParser extends Parser {
 
     return mapSeverity(severityMap[level ?? 'warning']);
   }
-
-  private findRuleDetails(ruleId: string, run: SarifRun): SarifRule | undefined {
-    return run.tool.driver.rules?.find((rule) => rule.id === ruleId);
-  }
 }

src/Provider/GitLab/GitLab.spec.ts

@@ -14,10 +14,6 @@ import { AnalyzerBot } from '../../AnalyzerBot/AnalyzerBot';
 
 const mockCurrentUserId = 123456;
 
-// Create helper function to generate consistent comment keys
-const generateCommentKey = (file: string, line: number | undefined, text: string) =>
-  `${file}:${line}:${text}`;
-
 const mockNotes = [
   {
     id: 1,
@@ -163,6 +159,201 @@ describe('VCS: GitLab', () => {
     // deleteNote and deleteDiscussion will be called for outdated comments
   });
 
+  describe('Error Handling for Comment Deletion', () => {
+    it('should handle 404 errors gracefully when deleting notes', async () => {
+      const service = new MrServiceMock();
+      const configsWithRemoveOldComment = { ...configs, removeOldComment: true };
+
+      // Mock a 404 error for note deletion
+      const notFoundError = new Error('Not Found');
+      (notFoundError as Error & { status: number }).status = 404;
+      service.deleteNote.mockRejectedValueOnce(notFoundError);
+
+      // Add an existing comment that should be deleted
+      service.listAllNotes.mockResolvedValue([
+        {
+          id: 999,
+          author: { id: mockCurrentUserId },
+          system: false,
+          body: 'outdated comment',
+        },
+      ]);
+
+      const gitLab = createGitLab(service, configsWithRemoveOldComment);
+
+      // Should not throw error even when note deletion fails with 404
+      await expect(gitLab.report([touchFileWarning])).resolves.toBe(true);
+      expect(service.deleteNote).toHaveBeenCalled();
+    });
+
+    it('should handle 404 errors gracefully when deleting discussions', async () => {
+      const service = new MrServiceMock();
+      const configsWithRemoveOldComment = { ...configs, removeOldComment: true };
+
+      // Mock a 404 error for discussion deletion
+      const notFoundError = new Error('Not Found');
+      (notFoundError as Error & { status: number }).status = 404;
+      service.deleteDiscussion.mockRejectedValueOnce(notFoundError);
+
+      // Add an existing discussion that should be deleted
+      service.listAllDiscussions.mockResolvedValue([
+        {
+          id: 'discussion-123',
+          notes: [
+            {
+              id: 456,
+              author: { id: mockCurrentUserId },
+              system: false,
+              body: 'outdated discussion comment',
+            },
+          ],
+        },
+      ]);
+
+      const gitLab = createGitLab(service, configsWithRemoveOldComment);
+
+      // Should not throw error even when discussion deletion fails with 404
+      await expect(gitLab.report([touchFileWarning])).resolves.toBe(true);
+      expect(service.deleteDiscussion).toHaveBeenCalled();
+    });
+
+    it('should continue processing when some comment deletions fail', async () => {
+      const service = new MrServiceMock();
+      const configsWithRemoveOldComment = { ...configs, removeOldComment: true };
+
+      // Mock multiple notes, some will fail to delete
+      service.listAllNotes.mockResolvedValue([
+        {
+          id: 100,
+          author: { id: mockCurrentUserId },
+          system: false,
+          body: 'comment that will fail',
+        },
+        {
+          id: 200,
+          author: { id: mockCurrentUserId },
+          system: false,
+          body: 'comment that will succeed',
+        },
+      ]);
+
+      // First deletion fails, second succeeds
+      const notFoundError = new Error('Not Found');
+      (notFoundError as Error & { status: number }).status = 404;
+      service.deleteNote
+        .mockRejectedValueOnce(notFoundError)
+        .mockResolvedValueOnce(undefined);
+
+      const gitLab = createGitLab(service, configsWithRemoveOldComment);
+
+      // Should complete successfully despite partial failures
+      await expect(gitLab.report([touchFileWarning])).resolves.toBe(true);
+      expect(service.deleteNote).toHaveBeenCalledTimes(2);
+    });
+
+    it('should handle invalid comment IDs gracefully', async () => {
+      const service = new MrServiceMock();
+      const configsWithRemoveOldComment = { ...configs, removeOldComment: true };
+
+      // Mock notes with invalid IDs
+      service.listAllNotes.mockResolvedValue([
+        {
+          id: null, // Invalid ID
+          author: { id: mockCurrentUserId },
+          system: false,
+          body: 'comment with null id',
+        },
+        {
+          id: 'invalid-string-id', // Invalid ID type
+          author: { id: mockCurrentUserId },
+          system: false,
+          body: 'comment with string id',
+        },
+      ]);
+
+      const gitLab = createGitLab(service, configsWithRemoveOldComment);
+
+      // Should handle invalid IDs without attempting deletion
+      await expect(gitLab.report([touchFileWarning])).resolves.toBe(true);
+      // deleteNote should not be called for invalid IDs
+      expect(service.deleteNote).not.toHaveBeenCalled();
+    });
+
+    it('should handle discussion deletion with 404 errors on individual notes', async () => {
+      const service = new MrServiceMock();
+      const configsWithRemoveOldComment = { ...configs, removeOldComment: true };
+
+      // Mock a discussion with notes
+      const mockDiscussion = {
+        id: 'discussion-456',
+        notes: [
+          { id: 10, author: { id: mockCurrentUserId } },
+          { id: 20, author: { id: mockCurrentUserId } },
+        ],
+      };
+
+      service.listAllDiscussions.mockResolvedValue([mockDiscussion]);
+
+      // Mock discussion show to return the discussion
+      const mockMRService = service as MrServiceMock & {
+        api: {
+          MergeRequestDiscussions: {
+            show: jest.Mock;
+          };
+          MergeRequestNotes: {
+            remove: jest.Mock;
+          };
+        };
+      };
+      mockMRService.api = {
+        MergeRequestDiscussions: {
+          show: jest.fn().mockResolvedValue(mockDiscussion),
+        },
+        MergeRequestNotes: {
+          remove: jest
+            .fn()
+            .mockRejectedValueOnce(new Error('Not Found')) // First note fails
+            .mockResolvedValueOnce(undefined), // Second note succeeds
+        },
+      };
+
+      const gitLab = createGitLab(service, configsWithRemoveOldComment);
+
+      // Should complete successfully despite partial note deletion failures
+      await expect(gitLab.report([touchFileWarning])).resolves.toBe(true);
+    });
+
+    it('should log appropriate warnings for failed deletions', async () => {
+      const service = new MrServiceMock();
+      const configsWithRemoveOldComment = { ...configs, removeOldComment: true };
+
+      // Spy on console to capture log messages
+      const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
+
+      // Mock a 404 error
+      const notFoundError = new Error('Not Found');
+      (notFoundError as Error & { status: number }).status = 404;
+      service.deleteNote.mockRejectedValueOnce(notFoundError);
+
+      service.listAllNotes.mockResolvedValue([
+        {
+          id: 123,
+          author: { id: mockCurrentUserId },
+          system: false,
+          body: 'comment to delete',
+        },
+      ]);
+
+      const gitLab = createGitLab(service, configsWithRemoveOldComment);
+      await gitLab.report([touchFileWarning]);
+
+      // Should log warning about failed deletion
+      // Note: The actual log message will depend on the Winston logger configuration
+
+      consoleSpy.mockRestore();
+    });
+  });
+
   describe('when failOnWarnings is true', () => {
     const warningConfigs = { ...configs, failOnWarnings: true };