Add diff logic and parallel logger support for audit #642

eyalk007 · 2026-01-13T10:42:28Z

The pull request is targeting the dev branch.
The code has been validated to compile successfully by running go vet ./....
The code has been formatted properly using go fmt ./....
All static analysis checks passed.
All tests have passed. If this feature is not already covered by the tests, new tests have been added.
Updated the Contributing page / ReadMe page / CI Workflow files if needed.
All changes are detailed at the description. if not already covered at JFrog Documentation, new documentation have been added.

Depends on:

Add goroutine-local logger and BufferedLogger for parallel operations jfrog-client-go#1297

…Results) - based on v1.24.2

- Add branchdiff.go with RunBranchDiffAudit function - Scans target branch first, then source branch (sequential) - SCA diff handled internally by CLI - JAS diff handled by CompareJasResults - Add MergeStatusCodes for partial results filtering - Status codes merged (worst of target + source) for frogbot filtering

- Add logger field to AuditBasicParams with getter/setter - RunAudit swaps global logger if custom logger provided - Enables frogbot to capture logs per-scan for ordered output

- Add LogCollector that captures logs in isolated buffer per audit - Enables parallel audits without log interleaving - Uses goroutine-local logger from jfrog-client-go - Propagate logger to child goroutines in JAS runner and SCA scan - Remove unused diff completion log message

eyalk007 · 2026-01-13T10:59:48Z

utils/results/diff.go

+)
+
+// UnifyScaAndJasResults merges SCA and JAS diff results into a single SecurityCommandResults.
+func UnifyScaAndJasResults(scaResults, jasDiffResults *SecurityCommandResults) *SecurityCommandResults {


myabe needs to be moved to frogbot considering the specific use
and selected parallel audit implementation, your call @attiasas

Mergeing results could stay here if the logic is adjusted to be more general like MergeCommandResults

utils/results/diff.go

…eIncludedBuilds)

utils/results/diff.go

attiasas

Nice work! checkout my comments, also:

Update the PR description with the changes
Adjust tests base on changes

attiasas · 2026-01-18T07:40:01Z

commands/audit/logcollector.go

@@ -0,0 +1,41 @@
+package audit


maybe this should be at jfrog/jfrog-client-go#1297?

these are the options

Move LogCollector to client-go - but it's really just a wrapper
Delete LogCollector entirely - Frogbot can use BufferedLogger directly from client-go

or we Keep it as is

Removing code from repo is always good :)
If you already passing it in params, just use it no need for wrapper here

attiasas · 2026-01-18T07:40:36Z

go.mod


 // replace github.com/jfrog/froggit-go => github.com/jfrog/froggit-go master
+
+replace github.com/jfrog/jfrog-client-go => github.com/eyalk007/jfrog-client-go v0.0.0-20260114112951-67b77f49255f


Reminder to remove replace after merging dependend PR

tests/testdata/other/diff-scan/target.sarif

attiasas · 2026-01-18T07:47:12Z

sca/scan/scascan.go

+	currentLogger := log.GetLogger()
+	scaTask := createScaScanTaskWithRunner(params.Runner, strategy, params)
+	wrappedScaTask := func(threadId int) error {
+		log.SetLoggerForGoroutine(currentLogger)
+		defer log.ClearLoggerForGoroutine()
+		return scaTask(threadId)
+	}


why not change in createScaScanTaskWithRunner directly?

I don't get the chnage (looking only here) you call log.GetLogger and setting it for the routine but at the end you clear it? code is not clear why you do it. (looks like you are just setting default log to be default and clear it).

I suggest improving this part or at least adding comment about this

attiasas · 2026-01-18T07:48:23Z

jas/runner/jasrunner.go

 func addModuleJasScanTask(scanType jasutils.JasScanType, securityParallelRunner *utils.SecurityParallelRunner, task parallel.TaskFunc, scanResults *results.TargetResults, allowSkippingErrors bool) (generalError error) {
 	securityParallelRunner.JasScannersWg.Add(1)
-	if _, addTaskErr := securityParallelRunner.Runner.AddTaskWithError(task, func(err error) {
+	currentLogger := log.GetLogger()


same comment as in scascan.
If this logic is used multiple times (same code), try to create static method to be used in multiple places

utils/results/diff.go

attiasas · 2026-01-18T08:52:33Z

utils/results/diff.go

+)
+
+// UnifyScaAndJasResults merges SCA and JAS diff results into a single SecurityCommandResults.
+func UnifyScaAndJasResults(scaResults, jasDiffResults *SecurityCommandResults) *SecurityCommandResults {


Mergeing results could stay here if the logic is adjusted to be more general like MergeCommandResults

attiasas · 2026-01-18T08:53:51Z

utils/results/diff.go

+
+// CompareJasResults computes the diff between target and source JAS results.
+// Returns only NEW findings in source that don't exist in target.
+func CompareJasResults(targetResults, sourceResults *SecurityCommandResults) *SecurityCommandResults {


IDK about the func name, it does not compare, it filters Source results that exists at target

utils/results/diff.go

attiasas · 2026-01-18T09:00:30Z

utils/results/diff.go

+func extractLocationsOnly(run *sarif.Run, targetKeys map[string]bool) {
+	for _, result := range run.Results {
+		for _, location := range result.Locations {
+			key := getRelativeLocationFileName(location, run.Invocations) + getLocationSnippetText(location)


what about file name changes? it will not catch this...
why are we first filtering only locations and than by fingerprints, can we do it in one go? (reducing go over results multile times)

For SAST: Uses fingerprints (precise_sink_and_sink_function) which are content-based, so renames don't matter

for secrets and iac -
This is a known limitation - AM doesn't handle file renames either. If you rename a file, existing findings in that file will show as "new" in the diff. This is acceptable

We can write a todo and create a ticket for it, but i dont believe we should handle it now

yeah, we should

attiasas · 2026-01-19T10:56:04Z

commands/audit/logcollector.go

@@ -0,0 +1,41 @@
+package audit


Removing code from repo is always good :)
If you already passing it in params, just use it no need for wrapper here

attiasas · 2026-01-19T10:57:59Z

commands/audit/audit.go

+	// 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)
+	}
+	if _, jasErr := auditParallelRunner.Runner.AddTaskWithError(wrappedJasTask, func(taskErr error) {


I think you can use WrapTaskWithLoggerPropagation here as well

attiasas · 2026-01-19T11:02:17Z

utils/results/diff.go

+// 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
+		}
+	}
+	return ""
+}


as mentioned before, should be moved to utils/formats/sarifutils/sarifutils.go
also adjust comments, make sure what to use and when precise_sink_and_sink_function or significant_full_path fingerprint

attiasas · 2026-01-19T11:07:43Z

utils/results/diff.go

+	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)


Suggested change

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)

log.Debug("[DIFF] New findings after diff - Secrets:", diffSecrets, "| IaC:", diffIac, "| SAST:", diffSast)

log.Debug("[DIFF] Filtered out - Secrets:", sourceSecrets-diffSecrets, "| IaC:", sourceIac-diffIac, "| SAST:", sourceSast-diffSast)

attiasas · 2026-01-19T11:10:19Z

utils/results/diff.go

+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
+		}
+	}
+}


Suggested change

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

}

}

}

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

}

}

}

}

you can use it to reduce the amount of for loops in code where it is called

attiasas · 2026-01-19T11:12:59Z

utils/results/diff.go

+// CompareJasResults computes the diff between target and source JAS results.
+// Returns only NEW findings in source that don't exist in target.
+func CompareJasResults(targetResults, sourceResults *SecurityCommandResults) *SecurityCommandResults {
+	log.Info("[DIFF] Starting JAS diff calculation")


Suggested change

log.Info("[DIFF] Starting JAS diff calculation")

log.Debug("[DIFF] Starting JAS diff calculation")

attiasas · 2026-01-19T11:17:13Z

utils/results/diff.go

+}
+
+// filterExistingFindings removes findings from source that already exist in target.
+func filterExistingFindings(allTargetJasResults []*JasScansResults, sourceJasResults *JasScansResults) *JasScansResults {


Suggested change

func filterExistingFindings(allTargetJasResults []*JasScansResults, sourceJasResults *JasScansResults) *JasScansResults {

func excludeExistingFindingsInTargets(sourceJasResults *JasScansResults, targetJasResultsToExclude ...*JasScansResults) *JasScansResults {

outside user will have hard time understanding what the func does, consider adjusting the name.
I would also consider changing how params are passed

attiasas · 2026-01-19T11:17:59Z

utils/results/diff.go

+	targetKeys := make(map[string]bool)
+
+	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)
+		}
+	}


this could be extracted to a helper func

attiasas · 2026-01-19T11:19:06Z

utils/results/diff.go

+	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)
+


this could be extracted to a helper func, used twice

attiasas

If we are moving diff logic here, do we still need the logic in:
jas/sast/jas/secrets/jas/iac?

talking about:

log.Debug("Diff mode - SAST results to compare provided")
	manager.resultsToCompareFileName = filepath.Join(scannerTempDir, "target.sarif")
	// Save the sast results to compare as a report
	err = jas.SaveScanResultsToCompareAsReport(manager.resultsToCompareFileName, resultsToCompare...)

make sure if not needed to delete related logic

eyalk007 added 6 commits January 5, 2026 15:07

Add parallel PR scanning functions (CompareJasResults, UnifyScaAndJas…

18c22a1

…Results) - based on v1.24.2

Add Logger field to AuditBasicParams for parallel scan log separation

caf59eb

- Add logger field to AuditBasicParams with getter/setter - RunAudit swaps global logger if custom logger provided - Enables frogbot to capture logs per-scan for ordered output

Update go.mod/go.sum

96ae51a

Update LogCollector with ReplayTo for proper log formatting

b22d7c4

eyalk007 self-assigned this Jan 13, 2026

eyalk007 requested a deployment to frogbot January 13, 2026 10:42 — with GitHub Actions Waiting

eyalk007 added the improvement Automatically generated release notes label Jan 13, 2026

Simplify comments to match repo style

16f7f21

eyalk007 requested a deployment to frogbot January 13, 2026 10:44 — with GitHub Actions Waiting

Remove accidentally added test file

c38b6c3

eyalk007 requested a deployment to frogbot January 13, 2026 10:45 — with GitHub Actions Waiting

eyalk007 added 2 commits January 13, 2026 12:52

Remove unused branchdiff.go

5e4a231

Clean up diff functions: remove AM references, simplify comments

86e70fd

eyalk007 requested a deployment to frogbot January 13, 2026 10:54 — with GitHub Actions Waiting

Move diff functions to separate file: utils/results/diff.go

37f3743

eyalk007 requested a deployment to frogbot January 13, 2026 10:58 — with GitHub Actions Waiting

eyalk007 commented Jan 13, 2026

View reviewed changes

utils/results/diff.go Outdated Show resolved Hide resolved

Remove extra comments from logCollector field

b0eb167

eyalk007 requested a deployment to frogbot January 13, 2026 11:03 — with GitHub Actions Waiting

Add diff function tests adapted from analyzer-manager

957b363

eyalk007 requested a deployment to frogbot January 13, 2026 11:14 — with GitHub Actions Waiting

Merge upstream/dev - resolve conflicts (keep both logCollector and us…

809a52f

…eIncludedBuilds)

eyalk007 requested a deployment to frogbot January 13, 2026 11:17 — with GitHub Actions Waiting

eyalk007 added the safe to test Approve running integration tests on a pull request label Jan 13, 2026

github-actions bot removed the safe to test Approve running integration tests on a pull request label Jan 13, 2026

Remove unused MergeStatusCodes function and tests

a5897a9

eyalk007 requested a deployment to frogbot January 14, 2026 08:17 — with GitHub Actions Waiting

Fix UnifyScaAndJasResults to preserve Applicability from SCA scan

22f7713

eyalk007 requested a deployment to frogbot January 14, 2026 08:39 — with GitHub Actions Waiting

Fix: Copy GitContext in UnifyScaAndJasResults for proper upload paths

56609db

eyalk007 requested a deployment to frogbot January 14, 2026 10:30 — with GitHub Actions Waiting

CR fixes - update go.mod replace directive to use git commit

816605d

eyalk007 commented Jan 15, 2026

View reviewed changes

utils/results/diff.go Outdated Show resolved Hide resolved

eyalk007 requested a deployment to frogbot January 15, 2026 15:26 — with GitHub Actions Waiting

attiasas requested changes Jan 18, 2026

View reviewed changes

CR fixes: extract logger helper, use sarifutils, rename functions

1ca4a0a

eyalk007 requested a deployment to frogbot January 18, 2026 10:19 — with GitHub Actions Waiting

attiasas reviewed Jan 19, 2026

View reviewed changes


		// replace github.com/jfrog/froggit-go => github.com/jfrog/froggit-go master

		replace github.com/jfrog/jfrog-client-go => github.com/eyalk007/jfrog-client-go v0.0.0-20260114112951-67b77f49255f

		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)

	log.Info("[DIFF] Starting JAS diff calculation")
	log.Debug("[DIFF] Starting JAS diff calculation")

	func filterExistingFindings(allTargetJasResults []JasScansResults, sourceJasResults JasScansResults) *JasScansResults {
	func excludeExistingFindingsInTargets(sourceJasResults JasScansResults, targetJasResultsToExclude ...JasScansResults) *JasScansResults {

Add diff logic and parallel logger support for audit #642

Are you sure you want to change the base?

Add diff logic and parallel logger support for audit #642

Uh oh!

Conversation

eyalk007 commented Jan 13, 2026 • edited by attiasas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eyalk007 Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

attiasas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

attiasas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eyalk007 commented Jan 13, 2026 •

edited by attiasas

Loading

eyalk007 Jan 13, 2026 •

edited

Loading