refactor: Improve logging in processors/action chain

src/proxy/processors/push-action/checkAuthorEmails.ts

@@ -39,8 +39,6 @@ const exec = async (req: any, action: Action): Promise<Action> => {
   const illegalEmails = uniqueAuthorEmails.filter((email) => !isEmailAllowed(email));
 
   if (illegalEmails.length > 0) {
-    console.log(`The following commit author e-mails are illegal: ${illegalEmails}`);
-
     step.error = true;
     step.log(`The following commit author e-mails are illegal: ${illegalEmails}`);
     step.setError(
@@ -51,7 +49,7 @@ const exec = async (req: any, action: Action): Promise<Action> => {
     return action;
   }
 
-  console.log(`The following commit author e-mails are legal: ${uniqueAuthorEmails}`);
+  step.log(`The following commit author e-mails are legal: ${uniqueAuthorEmails}`);
   action.addStep(step);
   return action;
 };

src/proxy/processors/push-action/checkCommitMessages.ts

@@ -1,20 +1,20 @@
 import { Action, Step } from '../../actions';
 import { getCommitConfig } from '../../../config';
 
-const isMessageAllowed = (commitMessage: string): boolean => {
+const isMessageAllowed = (commitMessage: any): string | null => {
   try {
     const commitConfig = getCommitConfig();
 
     // Commit message is empty, i.e. '', null or undefined
     if (!commitMessage) {
-      console.log('No commit message included...');
-      return false;
+      // console.log('No commit message included...');
+      return 'No commit message included...';
     }
 
     // Validation for configured block pattern(s) check...
     if (typeof commitMessage !== 'string') {
-      console.log('A non-string value has been captured for the commit message...');
-      return false;
+      // console.log('A non-string value has been captured for the commit message...');
+      return 'A non-string value has been captured for the commit message...';
     }
 
     // Configured blocked literals and patterns
@@ -36,15 +36,15 @@ const isMessageAllowed = (commitMessage: string): boolean => {
 
     // Commit message matches configured block pattern(s)
     if (literalMatches.length || patternMatches.length) {
-      console.log('Commit message is blocked via configured literals/patterns...');
-      return false;
+      // console.log('Commit message is blocked via configured literals/patterns...');
+      return 'Commit message is blocked via configured literals/patterns...';
     }
   } catch (error) {
-    console.log('Invalid regex pattern...');
-    return false;
+    // console.log('Invalid regex pattern...');
+    return 'Invalid regex pattern...';
   }
 
-  return true;
+  return null;
 };
 
 // Execute if the repo is approved
@@ -53,13 +53,23 @@ const exec = async (req: any, action: Action): Promise<Action> => {
 
   const uniqueCommitMessages = [...new Set(action.commitData?.map((commit) => commit.message))];
 
-  const illegalMessages = uniqueCommitMessages.filter((message) => !isMessageAllowed(message));
+  // const illegalMessages = uniqueCommitMessages.filter((message) => !isMessageAllowed(message));
+  const illegalMessages = uniqueCommitMessages.filter(
+    (message) => isMessageAllowed(message) !== null,
+  );
 
   if (illegalMessages.length > 0) {
-    console.log(`The following commit messages are illegal: ${illegalMessages}`);
+    // console.log(`The following commit messages are illegal: ${illegalMessages}`);
 
-    step.error = true;
-    step.log(`The following commit messages are illegal: ${illegalMessages}`);
+    illegalMessages.forEach((message) => {
+      const error = isMessageAllowed(message);
+      step.log(
+        `Illegal commit message detected: "${message}" - Reason: ${error ?? 'Unknown reason'}`,
+      );
+    });
+
+    // step.error = true;
+    // step.log(`The following commit messages are illegal: ${illegalMessages}`);
     step.setError(
       `\n\n\nYour push has been blocked.\nPlease ensure your commit message(s) does not contain sensitive information or URLs.\n\nThe following commit messages are illegal: ${JSON.stringify(illegalMessages)}\n\n`,
     );
@@ -68,7 +78,8 @@ const exec = async (req: any, action: Action): Promise<Action> => {
     return action;
   }
 
-  console.log(`The following commit messages are legal: ${uniqueCommitMessages}`);
+  // console.log(`The following commit messages are legal: ${uniqueCommitMessages}`);
+  step.log(`The following commit messages are legal: ${JSON.stringify(uniqueCommitMessages)}`);
   action.addStep(step);
   return action;
 };

src/proxy/processors/push-action/checkEmptyBranch.ts

@@ -2,15 +2,15 @@ import { Action, Step } from '../../actions';
 import simpleGit from 'simple-git';
 import { EMPTY_COMMIT_HASH } from '../constants';
 
-const isEmptyBranch = async (action: Action) => {
+const isEmptyBranch = async (action: Action, step: Step) => {
   if (action.commitFrom === EMPTY_COMMIT_HASH) {
     try {
       const git = simpleGit(`${action.proxyGitPath}/${action.repoName}`);
 
       const type = await git.raw(['cat-file', '-t', action.commitTo || '']);
       return type.trim() === 'commit';
     } catch (err) {
-      console.log(`Commit ${action.commitTo} not found: ${err}`);
+      step.log(`Commit ${action.commitTo} not found: ${err}`);
     }
   }
 
@@ -24,7 +24,7 @@ const exec = async (req: any, action: Action): Promise<Action> => {
     return action;
   }
 
-  if (await isEmptyBranch(action)) {
+  if (await isEmptyBranch(action, step)) {
     step.setError('Push blocked: Empty branch. Please make a commit before pushing a new branch.');
     action.addStep(step);
     step.error = true;

src/proxy/processors/push-action/checkUserPushPermission.ts

@@ -31,8 +31,8 @@ const validateUser = async (userEmail: string, action: Action, step: Step): Prom
   const list = await getUsers({ email: userEmail });
 
   if (list.length > 1) {
-    console.error(`Multiple users found with email address ${userEmail}, ending`);
     step.error = true;
+    step.log(`Multiple users found with email address ${userEmail}, ending`);
     step.log(
       `Multiple Users have email <${userEmail}> so we cannot uniquely identify the user, ending`,
     );
@@ -43,15 +43,15 @@ const validateUser = async (userEmail: string, action: Action, step: Step): Prom
     action.addStep(step);
     return action;
   } else if (list.length == 0) {
-    console.error(`No user with email address ${userEmail} found`);
+    step.log(`No user with email address ${userEmail} found`);
   } else {
     isUserAllowed = await isUserPushAllowed(action.url, list[0].username);
   }
 
-  console.log(`User ${userEmail} permission on Repo ${action.url} : ${isUserAllowed}`);
+  step.log(`User ${userEmail} permission on Repo ${action.url} : ${isUserAllowed}`);
 
   if (!isUserAllowed) {
-    console.log('User not allowed to Push');
+    step.log('User not allowed to Push');
     step.error = true;
     step.log(`User ${userEmail} is not allowed to push on repo ${action.url}, ending`);
     step.setError(

src/proxy/processors/push-action/clearBareClone.ts

@@ -9,7 +9,7 @@ const exec = async (req: any, action: Action): Promise<Action> => {
     if (err) {
       throw err;
     }
-    console.log(`.remote is deleted!`);
+    step.log(`.remote is deleted!`);
   });
 
   action.addStep(step);

src/proxy/processors/push-action/gitleaks.ts

@@ -116,22 +116,22 @@ const exec = async (req: any, action: Action): Promise<Action> => {
   try {
     config = await getPluginConfig();
   } catch (e) {
-    console.error('failed to get gitleaks config, please fix the error:', e);
+    step.log(`failed to get gitleaks config, please fix the error: ${String(e)}`);
     action.error = true;
     step.setError('failed setup gitleaks, please contact an administrator\n');
     action.addStep(step);
     return action;
   }
 
   if (!config.enabled) {
-    console.log('gitleaks is disabled, skipping');
+    step.log('gitleaks is disabled, skipping');
     action.addStep(step);
     return action;
   }
 
   const { commitFrom, commitTo } = action;
   const workingDir = `${action.proxyGitPath}/${action.repoName}`;
-  console.log(`Scanning range with gitleaks: ${commitFrom}:${commitTo}`, workingDir);
+  step.log(`Scanning range with gitleaks: ${commitFrom}:${commitTo}, ${workingDir}`);
 
   try {
     const gitRootCommit = await runCommand(workingDir, 'git', [
@@ -171,8 +171,8 @@ const exec = async (req: any, action: Action): Promise<Action> => {
         step.setError('\n' + gitleaks.stdout + gitleaks.stderr);
       }
     } else {
-      console.log('succeeded');
-      console.log(gitleaks.stderr);
+      step.log('succeeded');
+      step.log(gitleaks.stderr);
     }
   } catch (e) {
     action.error = true;

test/processors/checkAuthorEmails.test.ts

@@ -1,6 +1,6 @@
 import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
 import { exec } from '../../src/proxy/processors/push-action/checkAuthorEmails';
-import { Action } from '../../src/proxy/actions';
+import { Action, Step } from '../../src/proxy/actions';
 import * as configModule from '../../src/config';
 import * as validator from 'validator';
 import { CommitData } from '../../src/proxy/processors/types';
@@ -47,7 +47,7 @@ describe('checkAuthorEmails', () => {
     });
 
     // mock console.log to suppress output and verify calls
-    consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
+    consoleLogSpy = vi.spyOn(Step.prototype, 'log').mockImplementation(() => {});
 
     // setup mock action
     mockAction = {

test/processors/checkCommitMessages.test.ts

@@ -46,7 +46,7 @@ describe('checkCommitMessages', () => {
         const result = await exec({}, action);
 
         expect(result.steps[0].error).toBe(true);
-        expect(consoleLogSpy).toHaveBeenCalledWith('No commit message included...');
+        expect(result.steps[0].logs.join('\n')).toContain('No commit message included...');
       });
 
       it('should block null commit messages', async () => {
@@ -74,7 +74,7 @@ describe('checkCommitMessages', () => {
         const result = await exec({}, action);
 
         expect(result.steps[0].error).toBe(true);
-        expect(consoleLogSpy).toHaveBeenCalledWith(
+        expect(JSON.stringify(result.steps[0])).toContain(
           'A non-string value has been captured for the commit message...',
         );
       });
@@ -106,7 +106,7 @@ describe('checkCommitMessages', () => {
         const result = await exec({}, action);
 
         expect(result.steps[0].error).toBe(true);
-        expect(consoleLogSpy).toHaveBeenCalledWith(
+        expect(JSON.stringify(result.steps[0])).toContain(
           'Commit message is blocked via configured literals/patterns...',
         );
       });
@@ -241,8 +241,8 @@ describe('checkCommitMessages', () => {
         const result = await exec({}, action);
 
         expect(result.steps[0].error).toBe(false);
-        expect(consoleLogSpy).toHaveBeenCalledWith(
-          expect.stringContaining('The following commit messages are legal:'),
+        expect(JSON.stringify(result.steps[0])).toContain(
+          'The following commit messages are legal:',
         );
       });
 

test/processors/checkUserPushPermission.test.ts

@@ -15,14 +15,10 @@ import { exec } from '../../src/proxy/processors/push-action/checkUserPushPermis
 describe('checkUserPushPermission', () => {
   let getUsersMock: Mock;
   let isUserPushAllowedMock: Mock;
-  let consoleLogSpy: ReturnType<typeof vi.spyOn>;
-  let consoleErrorSpy: ReturnType<typeof vi.spyOn>;
 
   beforeEach(() => {
     getUsersMock = vi.mocked(getUsers);
     isUserPushAllowedMock = vi.mocked(isUserPushAllowed);
-    consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
-    consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
   });
 
   afterEach(() => {
@@ -62,7 +58,7 @@ describe('checkUserPushPermission', () => {
       expect(stepLogSpy).toHaveBeenLastCalledWith(
         'User db-user@test.com is allowed to push on repo https://github.com/finos/git-proxy.git',
       );
-      expect(consoleLogSpy).toHaveBeenLastCalledWith(
+      expect(stepLogSpy).toHaveBeenCalledWith(
         'User db-user@test.com permission on Repo https://github.com/finos/git-proxy.git : true',
       );
     });
@@ -81,7 +77,7 @@ describe('checkUserPushPermission', () => {
         `Your push has been blocked (db-user@test.com is not allowed to push on repo https://github.com/finos/git-proxy.git)`,
       );
       expect(result.steps[0].errorMessage).toContain('Your push has been blocked');
-      expect(consoleLogSpy).toHaveBeenLastCalledWith('User not allowed to Push');
+      expect(stepLogSpy).toHaveBeenCalledWith('User not allowed to Push');
     });
 
     it('should reject push when no user found for git account', async () => {
@@ -110,7 +106,7 @@ describe('checkUserPushPermission', () => {
       expect(stepLogSpy).toHaveBeenLastCalledWith(
         'Your push has been blocked (there are multiple users with email db-user@test.com)',
       );
-      expect(consoleErrorSpy).toHaveBeenLastCalledWith(
+      expect(stepLogSpy).toHaveBeenCalledWith(
         'Multiple users found with email address db-user@test.com, ending',
       );
     });

test/processors/gitLeaks.test.ts

@@ -43,6 +43,7 @@ describe('gitleaks', () => {
     let getAPIs: any;
     let fsModule: any;
     let spawn: any;
+    const stepLogSpy = vi.spyOn(Step.prototype, 'log');
 
     beforeEach(async () => {
       vi.clearAllMocks();
@@ -89,9 +90,8 @@ describe('gitleaks', () => {
       expect(stepSpy).toHaveBeenCalledWith(
         'failed setup gitleaks, please contact an administrator\n',
       );
-      expect(errorStub).toHaveBeenCalledWith(
-        'failed to get gitleaks config, please fix the error:',
-        expect.any(Error),
+      expect(stepLogSpy).toHaveBeenCalledWith(
+        expect.stringContaining('failed to get gitleaks config, please fix the error:'),
       );
     });
 
@@ -103,7 +103,7 @@ describe('gitleaks', () => {
       expect(result.error).toBe(false);
       expect(result.steps).toHaveLength(1);
       expect(result.steps[0].error).toBe(false);
-      expect(logStub).toHaveBeenCalledWith('gitleaks is disabled, skipping');
+      expect(JSON.stringify(result.steps[0])).toContain('gitleaks is disabled, skipping');
     });
 
     it('should handle successful scan with no findings', async () => {
@@ -140,12 +140,13 @@ describe('gitleaks', () => {
         } as any);
 
       const result = await exec(req, action);
+      const stepRes = JSON.stringify(result.steps[0]);
 
       expect(result.error).toBe(false);
       expect(result.steps).toHaveLength(1);
       expect(result.steps[0].error).toBe(false);
-      expect(logStub).toHaveBeenCalledWith('succeeded');
-      expect(logStub).toHaveBeenCalledWith('No leaks found');
+      expect(stepRes).toContain('succeeded');
+      expect(stepRes).toContain('No leaks found');
     });
 
     it('should handle scan with findings', async () => {