Add diff logic and parallel logger support for audit by eyalk007 · Pull Request #642 · jfrog/jfrog-cli-security

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

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

doen tell me if you like it now

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

sca/scan/scascan.go

jas/runner/jasrunner.go

utils/results/diff.go

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

utils/results/diff.go

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)

i acutally think they are good info logs
its for hte userr to undestand the state post diff, im waiting on or to tell me what she thinks on logs @orto17

talked to or she wants them as info

utils/results/diff.go

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

dont know if i agrree with it, i think it should be info, we can maybe do it in frogbot and log it info there
@orto17

talked to or she wants them on info

utils/results/diff.go

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

- 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

When merging SCA and JAS diff results, the ResultsStatus field was not being copied, causing all status codes (SastStatusCode, IacStatusCode, etc.) to remain nil. This resulted in Frogbot displaying 'Not Scanned' for JAS scans even when they ran successfully but produced 0 new findings after diff. Now properly merging SCA status codes (including ContextualAnalysis) and JAS-only status codes (SAST, Secrets, IaC) into the unified results.

When computing JAS diff, the ResultsStatus was not being copied to the diff results, causing all JAS scan status codes (SAST, Secrets, IaC) to be nil. This led to 'Not Scanned' being displayed in Frogbot PR comments even when scans ran successfully. Now copying ResultsStatus from source to diff results so status codes are preserved and merged correctly.

- Rename CompareJasResults → FilterNewJasFindings (more accurate name) - Rename test functions: TestFilterSarifRuns_* → TestFilterNewSarifFindings_* - Update test calls to use variadic signatures for extractLocationsOnly/extractFingerprints - Use sarifutils.GetSastDiffFingerprint instead of local getSastFingerprint Addresses code review feedback about misleading function names.

attiasas

Nice! take a look at my comments

attiasas · 2026-01-28T07:48:17Z

commands/audit/audit.go

+	if collector := auditParams.GetLogCollector(); collector != nil {
+		log.SetLoggerForGoroutine(collector)
+		defer log.ClearLoggerForGoroutine()
+	}


the collector is running on main routine so it will first record all the logs from main and it will collect the other after?

attiasas · 2026-01-28T07:50:28Z

commands/audit/audit.go

+	// Wrap task to propagate parent's logger for isolated parallel logging
+	wrappedJasTask := utils.WrapTaskWithLoggerPropagation(jasTask)
+	if _, jasErr := auditParallelRunner.Runner.AddTaskWithError(wrappedJasTask, func(taskErr error) {


we only want to wrap and change logs if collector is set.
why are we always wrapping?

attiasas · 2026-01-28T08:08:33Z

utils/formats/sarifutils/sarifutils.go

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


lets move the const to jasutils to be near the other used fingerprint value (easy to maintain and see what we use)

if you can also add to GetResultFingerprint check for result nil case as well. just saw its missing

attiasas · 2026-01-28T08:10:28Z

sca/bom/bomgenerator.go

+	filteredSbom := cdxutils.Exclude(*sbom, *params.TargetResultToCompare.ScaResults.Sbom.Components...)
+	return filteredSbom


Suggested change

filteredSbom := cdxutils.Exclude(*sbom, *params.TargetResultToCompare.ScaResults.Sbom.Components...)

return filteredSbom

return cdxutils.Exclude(*sbom, *params.TargetResultToCompare.ScaResults.Sbom.Components...)

why change is needed?

attiasas · 2026-01-28T10:51:37Z

utils/results/diff.go

+
+	sourceSecrets, sourceIac, sourceSast := countJasFindings(sourceJasResults)
+
+	log.Debug("[DIFF] Source findings before diff - Secrets:", sourceSecrets, "| IaC:", sourceIac, "| SAST:", sourceSast)


maybe you missed that, info as well?

attiasas · 2026-01-28T10:54:14Z

utils/results/diff.go

+	count := 0
+	for _, run := range runs {
+		if run != nil {
+			count += len(run.Results)


I don't want to count on JAS, the correct impl should be the SARIF spec.
They have to follow it, so we should do the same.

attiasas · 2026-01-28T10:57:25Z

utils/results/diff.go

+	count := 0
+	for _, run := range runs {
+		if run != nil {
+			count += len(run.Results)


at the end, the CLI result parser to format goes over each location as diff issue
take a look at ForEachJasIssue

attiasas · 2026-01-28T11:00:10Z

utils/results/diff_test.go

+func strPtr(s string) *string {
+	return &s
+}


remove and use utils.NewStringPtr() instead

attiasas · 2026-01-28T11:01:43Z

utils/results/diff_test.go

+// Note: Tests for extractRelativePath, getLocationSnippetText, getLocationFileName, and
+// getInvocationWorkingDirectory have been removed as these now use sarifutils functions.
+
+func TestGetSastFingerprint(t *testing.T) {


move to sarifutils_test.go

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

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

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

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

eyalk007 force-pushed the parallel-scanning-v3-compat branch from 5e7efdc to bccd932 Compare January 22, 2026 13:39

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

eyalk007 added 2 commits January 22, 2026 16:10

Debug: Add SBOM diff debugging - dump SBOMs to /tmp for comparison

7a90d14

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

Remove debug SBOM dump code

4485151

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

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

attiasas self-requested a review January 28, 2026 07:26

attiasas requested changes Jan 28, 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")

		filteredSbom := cdxutils.Exclude(sbom, params.TargetResultToCompare.ScaResults.Sbom.Components...)
		return filteredSbom

	filteredSbom := cdxutils.Exclude(sbom, params.TargetResultToCompare.ScaResults.Sbom.Components...)
	return filteredSbom
	return cdxutils.Exclude(sbom, params.TargetResultToCompare.ScaResults.Sbom.Components...)


		sourceSecrets, sourceIac, sourceSast := countJasFindings(sourceJasResults)

		log.Debug("[DIFF] Source findings before diff - Secrets:", sourceSecrets, "\| IaC:", sourceIac, "\| SAST:", sourceSast)

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

attiasas left a comment

Choose a reason for hiding this comment

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!

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

eyalk007 commented Jan 13, 2026 •

edited by attiasas

Loading