CR fixes: refactor diff.go, remove LogCollector wrapper, use helpers

- Rename filterExistingFindings to excludeExistingFindingsInTargets
- Extract countJasFindings and extractAllJasResultKeys helpers
- Move getSastFingerprint to sarifutils.GetSastDiffFingerprint
- Use WrapTaskWithLoggerPropagation helper in audit.go
- Remove LogCollector wrapper - use BufferedLogger directly from client-go

Author: eyalk007

commands/audit/audit.go

@@ -292,9 +292,9 @@ func (auditCmd *AuditCommand) CommandName() string {
 // Returns an audit Results object containing all the scan results.
 // If the current server is entitled for JAS, the advanced security results will be included in the scan results.
 func RunAudit(auditParams *AuditParams) (cmdResults *results.SecurityCommandResults) {
-	// Set up isolated logging if a log collector is provided
+	// Set up isolated logging if a BufferedLogger is provided for parallel log capture
 	if collector := auditParams.GetLogCollector(); collector != nil {
-		log.SetLoggerForGoroutine(collector.Logger())
+		log.SetLoggerForGoroutine(collector)
 		defer log.ClearLoggerForGoroutine()
 	}
 	// Prepare the command for the scan.
@@ -628,16 +628,9 @@ func addJasScansToRunner(auditParallelRunner *utils.SecurityParallelRunner, audi
 		return
 	}
 	auditParallelRunner.JasWg.Add(1)
-	// Capture current logger (may be a BufferedLogger for isolated parallel logging).
-	// Worker goroutines need this propagated so their logs are captured in the same buffer.
-	currentLogger := log.GetLogger()
 	jasTask := createJasScansTask(auditParallelRunner, scanResults, serverDetails, auditParams, jasScanner)
-	wrappedJasTask := func(threadId int) error {
-		// Propagate parent's logger to this worker goroutine for isolated log capture
-		log.SetLoggerForGoroutine(currentLogger)
-		defer log.ClearLoggerForGoroutine()
-		return jasTask(threadId)
-	}
+	// Wrap task to propagate parent's logger for isolated parallel logging
+	wrappedJasTask := utils.WrapTaskWithLoggerPropagation(jasTask)
 	if _, jasErr := auditParallelRunner.Runner.AddTaskWithError(wrappedJasTask, func(taskErr error) {
 		scanResults.AddGeneralError(fmt.Errorf("failed while adding JAS scan tasks: %s", taskErr.Error()), auditParams.AllowPartialResults())
 	}); jasErr != nil {

commands/audit/auditbasicparams.go

@@ -5,6 +5,7 @@ import (
 	"github.com/jfrog/jfrog-cli-core/v2/utils/config"
 	"github.com/jfrog/jfrog-cli-security/utils"
 	ioUtils "github.com/jfrog/jfrog-client-go/utils/io"
+	"github.com/jfrog/jfrog-client-go/utils/log"
 	xscservices "github.com/jfrog/jfrog-client-go/xsc/services"
 )
 
@@ -82,7 +83,7 @@ type AuditBasicParams struct {
 	xscVersion                       string
 	configProfile                    *xscservices.ConfigProfile
 	solutionFilePath  string
-	logCollector      *LogCollector
+	logCollector      *log.BufferedLogger
 	useIncludedBuilds bool
 }
 
@@ -345,12 +346,12 @@ func (abp *AuditBasicParams) SetSolutionFilePath(solutionFilePath string) *Audit
 	return abp
 }
 
-func (abp *AuditBasicParams) SetLogCollector(collector *LogCollector) *AuditBasicParams {
+func (abp *AuditBasicParams) SetLogCollector(collector *log.BufferedLogger) *AuditBasicParams {
 	abp.logCollector = collector
 	return abp
 }
 
-func (abp *AuditBasicParams) GetLogCollector() *LogCollector {
+func (abp *AuditBasicParams) GetLogCollector() *log.BufferedLogger {
 	return abp.logCollector
 }
 

commands/audit/logcollector.go

@@ -904,6 +904,21 @@ func GetResultFingerprint(result *sarif.Result) string {
 	return ""
 }
 
+// SastDiffFingerprintKey is the fingerprint key used by Analyzer Manager for SAST diff matching.
+// This differs from jasutils.SastFingerprintKey ("significant_full_path") which is used for general SAST operations.
+const SastDiffFingerprintKey = "precise_sink_and_sink_function"
+
+// GetSastDiffFingerprint extracts the SAST fingerprint used specifically for diff matching.
+// Uses "precise_sink_and_sink_function" key (generated by Analyzer Manager for diff purposes).
+func GetSastDiffFingerprint(result *sarif.Result) string {
+	if result != nil && result.Fingerprints != nil {
+		if value, ok := result.Fingerprints[SastDiffFingerprintKey]; ok {
+			return value
+		}
+	}
+	return ""
+}
+
 func GetResultsByRuleId(ruleId string, runs ...*sarif.Run) (results []*sarif.Result) {
 	for _, run := range runs {
 		for _, result := range run.Results {

utils/formats/sarifutils/sarifutils.go

@@ -75,7 +75,7 @@ func CompareJasResults(targetResults, sourceResults *SecurityCommandResults) *Se
 			}
 		}
 
-		diffJasResults := filterExistingFindings(allTargetJasResults, sourceTarget.JasResults)
+		diffJasResults := excludeExistingFindingsInTargets(sourceTarget.JasResults, allTargetJasResults...)
 
 		diffTarget := &TargetResults{
 			ScanTarget: sourceTarget.ScanTarget,
@@ -88,49 +88,20 @@ func CompareJasResults(targetResults, sourceResults *SecurityCommandResults) *Se
 	return diffResults
 }
 
-// filterExistingFindings removes findings from source that already exist in target.
-func filterExistingFindings(allTargetJasResults []*JasScansResults, sourceJasResults *JasScansResults) *JasScansResults {
+// excludeExistingFindingsInTargets removes findings from source that already exist in any of the target results.
+// Returns a new JasScansResults containing only findings that are NEW in source (not present in targets).
+func excludeExistingFindingsInTargets(sourceJasResults *JasScansResults, targetJasResultsToExclude ...*JasScansResults) *JasScansResults {
 	if sourceJasResults == nil {
 		return nil
 	}
 
-	if len(allTargetJasResults) == 0 {
+	if len(targetJasResultsToExclude) == 0 {
 		return sourceJasResults
 	}
 
-	targetKeys := make(map[string]bool)
+	targetKeys := extractAllJasResultKeys(targetJasResultsToExclude...)
 
-	for _, targetJasResults := range allTargetJasResults {
-		if targetJasResults == nil {
-			continue
-		}
-
-		for _, targetRun := range targetJasResults.GetVulnerabilitiesResults(jasutils.Secrets) {
-			extractLocationsOnly(targetRun, targetKeys)
-		}
-		for _, targetRun := range targetJasResults.GetViolationsResults(jasutils.Secrets) {
-			extractLocationsOnly(targetRun, targetKeys)
-		}
-		for _, targetRun := range targetJasResults.GetVulnerabilitiesResults(jasutils.IaC) {
-			extractLocationsOnly(targetRun, targetKeys)
-		}
-		for _, targetRun := range targetJasResults.GetViolationsResults(jasutils.IaC) {
-			extractLocationsOnly(targetRun, targetKeys)
-		}
-		for _, targetRun := range targetJasResults.GetVulnerabilitiesResults(jasutils.Sast) {
-			extractFingerprints(targetRun, targetKeys)
-		}
-		for _, targetRun := range targetJasResults.GetViolationsResults(jasutils.Sast) {
-			extractFingerprints(targetRun, targetKeys)
-		}
-	}
-
-	sourceSecrets := countSarifResults(sourceJasResults.JasVulnerabilities.SecretsScanResults) +
-		countSarifResults(sourceJasResults.JasViolations.SecretsScanResults)
-	sourceIac := countSarifResults(sourceJasResults.JasVulnerabilities.IacScanResults) +
-		countSarifResults(sourceJasResults.JasViolations.IacScanResults)
-	sourceSast := countSarifResults(sourceJasResults.JasVulnerabilities.SastScanResults) +
-		countSarifResults(sourceJasResults.JasViolations.SastScanResults)
+	sourceSecrets, sourceIac, sourceSast := countJasFindings(sourceJasResults)
 
 	log.Debug("[DIFF] Source findings before diff - Secrets:", sourceSecrets, "| IaC:", sourceIac, "| SAST:", sourceSast)
 
@@ -150,19 +121,28 @@ func filterExistingFindings(allTargetJasResults []*JasScansResults, sourceJasRes
 	filteredJasResults.JasViolations.SastScanResults = filterNewSarifFindings(
 		sourceJasResults.JasViolations.SastScanResults, targetKeys)
 
-	diffSecrets := countSarifResults(filteredJasResults.JasVulnerabilities.SecretsScanResults) +
-		countSarifResults(filteredJasResults.JasViolations.SecretsScanResults)
-	diffIac := countSarifResults(filteredJasResults.JasVulnerabilities.IacScanResults) +
-		countSarifResults(filteredJasResults.JasViolations.IacScanResults)
-	diffSast := countSarifResults(filteredJasResults.JasVulnerabilities.SastScanResults) +
-		countSarifResults(filteredJasResults.JasViolations.SastScanResults)
+	diffSecrets, diffIac, diffSast := countJasFindings(filteredJasResults)
 
 	log.Info("[DIFF] New findings after diff - Secrets:", diffSecrets, "| IaC:", diffIac, "| SAST:", diffSast)
 	log.Info("[DIFF] Filtered out - Secrets:", sourceSecrets-diffSecrets, "| IaC:", sourceIac-diffIac, "| SAST:", sourceSast-diffSast)
 
 	return filteredJasResults
 }
 
+// countJasFindings returns the count of (secrets, iac, sast) findings in the JAS results.
+func countJasFindings(jasResults *JasScansResults) (secrets, iac, sast int) {
+	if jasResults == nil {
+		return
+	}
+	secrets = countSarifResults(jasResults.JasVulnerabilities.SecretsScanResults) +
+		countSarifResults(jasResults.JasViolations.SecretsScanResults)
+	iac = countSarifResults(jasResults.JasVulnerabilities.IacScanResults) +
+		countSarifResults(jasResults.JasViolations.IacScanResults)
+	sast = countSarifResults(jasResults.JasVulnerabilities.SastScanResults) +
+		countSarifResults(jasResults.JasViolations.SastScanResults)
+	return
+}
+
 func countSarifResults(runs []*sarif.Run) int {
 	count := 0
 	for _, run := range runs {
@@ -173,41 +153,62 @@ func countSarifResults(runs []*sarif.Run) int {
 	return count
 }
 
-func extractFingerprints(run *sarif.Run, targetKeys map[string]bool) {
-	for _, result := range run.Results {
-		if sarifutils.IsFingerprintsExists(result) {
-			key := getSastFingerprint(result)
-			if key != "" {
-				targetKeys[key] = true
-			}
-		} else {
-			for _, location := range result.Locations {
-				key := sarifutils.GetRelativeLocationFileName(location, run.Invocations) + sarifutils.GetLocationSnippetText(location)
-				targetKeys[key] = true
-			}
+// extractAllJasResultKeys extracts unique identifiers from all JAS results for diff comparison.
+// For Secrets/IaC: uses file path + snippet as key (location-based matching).
+// For SAST: uses fingerprint when available, falls back to location-based matching.
+func extractAllJasResultKeys(jasResults ...*JasScansResults) map[string]bool {
+	targetKeys := make(map[string]bool)
+	for _, jasResult := range jasResults {
+		if jasResult == nil {
+			continue
 		}
+		// Secrets and IaC use location-based matching
+		extractLocationsOnly(targetKeys,
+			jasResult.GetVulnerabilitiesResults(jasutils.Secrets)...)
+		extractLocationsOnly(targetKeys,
+			jasResult.GetViolationsResults(jasutils.Secrets)...)
+		extractLocationsOnly(targetKeys,
+			jasResult.GetVulnerabilitiesResults(jasutils.IaC)...)
+		extractLocationsOnly(targetKeys,
+			jasResult.GetViolationsResults(jasutils.IaC)...)
+		// SAST uses fingerprint-based matching when available
+		extractFingerprints(targetKeys,
+			jasResult.GetVulnerabilitiesResults(jasutils.Sast)...)
+		extractFingerprints(targetKeys,
+			jasResult.GetViolationsResults(jasutils.Sast)...)
 	}
+	return targetKeys
 }
 
-func extractLocationsOnly(run *sarif.Run, targetKeys map[string]bool) {
-	for _, result := range run.Results {
-		for _, location := range result.Locations {
-			key := sarifutils.GetRelativeLocationFileName(location, run.Invocations) + sarifutils.GetLocationSnippetText(location)
-			targetKeys[key] = true
+// extractFingerprints extracts SAST fingerprints (or falls back to locations) for diff matching.
+func extractFingerprints(targetKeys map[string]bool, runs ...*sarif.Run) {
+	for _, run := range runs {
+		for _, result := range run.Results {
+			if sarifutils.IsFingerprintsExists(result) {
+				key := sarifutils.GetSastDiffFingerprint(result)
+				if key != "" {
+					targetKeys[key] = true
+				}
+			} else {
+				for _, location := range result.Locations {
+					key := sarifutils.GetRelativeLocationFileName(location, run.Invocations) + sarifutils.GetLocationSnippetText(location)
+					targetKeys[key] = true
+				}
+			}
 		}
 	}
 }
 
-// getSastFingerprint extracts the SAST fingerprint used for diff matching.
-// Note: Uses "precise_sink_and_sink_function" key (from Analyzer Manager for diff purposes),
-// which differs from jasutils.SastFingerprintKey ("significant_full_path") used elsewhere.
-func getSastFingerprint(result *sarif.Result) string {
-	if result.Fingerprints != nil {
-		if value, ok := result.Fingerprints["precise_sink_and_sink_function"]; ok {
-			return value
+// extractLocationsOnly extracts location-based keys (file path + snippet) for diff matching.
+func extractLocationsOnly(targetKeys map[string]bool, runs ...*sarif.Run) {
+	for _, run := range runs {
+		for _, result := range run.Results {
+			for _, location := range result.Locations {
+				key := sarifutils.GetRelativeLocationFileName(location, run.Invocations) + sarifutils.GetLocationSnippetText(location)
+				targetKeys[key] = true
+			}
 		}
 	}
-	return ""
 }
 
 // filterNewSarifFindings removes findings from sourceRuns that already exist in targetKeys.
@@ -221,7 +222,7 @@ func filterNewSarifFindings(sourceRuns []*sarif.Run, targetKeys map[string]bool)
 
 		for _, result := range run.Results {
 			if sarifutils.IsFingerprintsExists(result) {
-				if !targetKeys[getSastFingerprint(result)] {
+				if !targetKeys[sarifutils.GetSastDiffFingerprint(result)] {
 					filteredResults = append(filteredResults, result)
 				}
 			} else {