From 347718d50b9e8ee066e84193eac3477b7b350c5b Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Wed, 24 Jun 2026 05:09:00 -0700 Subject: [PATCH 1/5] fix(osv): fall back to GHSA textual severity when no CVSS vector exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GHSA-sourced OSV entries (common for, e.g., Apache project CVEs) frequently publish only a textual database_specific.severity rating (LOW/MODERATE/HIGH/ CRITICAL) with no CVSS vector at all. extractSeverity returned SeverityUnknown for these, which drops the SARIF security-severity property entirely and leaves GitHub showing a generic "Warning" badge instead of the real Low/ Medium/High/Critical — the root cause of code-scanning alerts still showing the wrong severity after the SARIF security-severity fix. Fall back to the GHSA text rating when no CVSS-derived band is available; a real CVSS vector still takes precedence when both are present. Co-Authored-By: Claude Opus 4.8 --- internal/matchers/osv/mapper.go | 45 ++++++++++++++++++++++++--- internal/matchers/osv/matcher_test.go | 41 ++++++++++++++++++++++-- internal/matchers/osv/response.go | 5 +++ 3 files changed, 85 insertions(+), 6 deletions(-) diff --git a/internal/matchers/osv/mapper.go b/internal/matchers/osv/mapper.go index 6a8ff22c..698af534 100644 --- a/internal/matchers/osv/mapper.go +++ b/internal/matchers/osv/mapper.go @@ -31,7 +31,7 @@ func MapVulnerability(v Vulnerability) sdk.Vulnerability { // Bomly extensions Source: "osv", Title: firstNonEmpty(v.Summary, v.ID), - ParsedSeverity: extractSeverity(v.Severity), + ParsedSeverity: extractSeverity(v.Severity, v.DatabaseSpecific), Reasons: buildReasons(v), CVSS: buildCVSS(v.Severity), CWEs: mapCWEs(v), @@ -74,10 +74,20 @@ func mapAffected(affected []Affected) []sdk.Affected { } func mapDatabaseSpecific(ds *DatabaseSpecific) map[string]any { - if ds == nil || len(ds.CweIDs) == 0 { + if ds == nil { + return nil + } + out := map[string]any{} + if len(ds.CweIDs) > 0 { + out["cwe_ids"] = append([]string(nil), ds.CweIDs...) + } + if ds.Severity != "" { + out["severity"] = ds.Severity + } + if len(out) == 0 { return nil } - return map[string]any{"cwe_ids": append([]string(nil), ds.CweIDs...)} + return out } func mapCWEs(v Vulnerability) []sdk.CWE { @@ -101,7 +111,7 @@ func firstNonEmpty(a, b string) string { // extractSeverity derives a normalized severity band from OSV severity entries. // Prefers CVSS v4 > v3.1 > v3 > v2 > unknown. -func extractSeverity(severities []Severity) sdk.SeverityLevel { +func extractSeverity(severities []Severity, ds *DatabaseSpecific) sdk.SeverityLevel { scores := map[string]float64{} for _, s := range severities { if score := parseCVSSScore(s.Type, s.Score); score > 0 { @@ -113,9 +123,36 @@ func extractSeverity(severities []Severity) sdk.SeverityLevel { return cvssScoreToBand(score) } } + // GHSA-sourced advisories (e.g. many Apache project CVEs) frequently + // publish no CVSS vector at all, only a textual database_specific.severity + // rating. Without this fallback those findings carry SeverityUnknown, + // which drops the SARIF security-severity property entirely and leaves + // GitHub showing a generic "Warning" badge instead of Low/Medium/High. + if ds != nil { + if band := severityFromGHSAText(ds.Severity); band != sdk.SeverityUnknown { + return band + } + } return sdk.SeverityUnknown } +// severityFromGHSAText maps GitHub Security Advisory's textual severity +// rating to Bomly's CVSS band vocabulary. +func severityFromGHSAText(raw string) sdk.SeverityLevel { + switch strings.ToUpper(strings.TrimSpace(raw)) { + case "CRITICAL": + return sdk.SeverityCritical + case "HIGH": + return sdk.SeverityHigh + case "MODERATE", "MEDIUM": + return sdk.SeverityMedium + case "LOW": + return sdk.SeverityLow + default: + return sdk.SeverityUnknown + } +} + func parseCVSSScore(kind, raw string) float64 { f, err := strconv.ParseFloat(raw, 64) if err == nil { diff --git a/internal/matchers/osv/matcher_test.go b/internal/matchers/osv/matcher_test.go index f79719ab..83f75250 100644 --- a/internal/matchers/osv/matcher_test.go +++ b/internal/matchers/osv/matcher_test.go @@ -338,7 +338,7 @@ func TestExtractSeverity_CalculatesFromVector(t *testing.T) { got := extractSeverity([]Severity{{ Type: "CVSS_V3", Score: "CVSS:3.1/AV:L/AC:H/PR:N/UI:R/S:U/C:N/I:N/A:L", - }}) + }}, nil) if got != "low" { t.Fatalf("extractSeverity() = %q, want %q", got, "low") @@ -349,9 +349,46 @@ func TestExtractSeverity_PrefersHigherVersion(t *testing.T) { got := extractSeverity([]Severity{ {Type: "CVSS_V2", Score: "AV:N/AC:L/Au:N/C:P/I:P/A:P"}, {Type: "CVSS_V4", Score: "AV:L/AC:H/AT:N/PR:N/UI:P/VC:N/VI:N/VA:L/SC:N/SI:N/SA:N"}, - }) + }, nil) if got != "low" { t.Fatalf("extractSeverity() = %q, want %q", got, "low") } } + +func TestExtractSeverity_FallsBackToGHSATextWhenNoCVSSVector(t *testing.T) { + // Mirrors GHSA-sourced OSV entries (e.g. many Apache project CVEs) that + // publish database_specific.severity but no CVSS vector at all. + tests := []struct { + text string + want sdk.SeverityLevel + }{ + {"CRITICAL", sdk.SeverityCritical}, + {"HIGH", sdk.SeverityHigh}, + {"MODERATE", sdk.SeverityMedium}, + {"MEDIUM", sdk.SeverityMedium}, + {"LOW", sdk.SeverityLow}, + {"low", sdk.SeverityLow}, + {"", sdk.SeverityUnknown}, + {"UNKNOWN", sdk.SeverityUnknown}, + } + for _, tt := range tests { + got := extractSeverity(nil, &DatabaseSpecific{Severity: tt.text}) + if got != tt.want { + t.Errorf("extractSeverity(nil, {Severity: %q}) = %q, want %q", tt.text, got, tt.want) + } + } +} + +func TestExtractSeverity_CVSSVectorTakesPrecedenceOverGHSAText(t *testing.T) { + // A real CVSS vector is more precise than the coarse GHSA text rating, so + // it must win when both are present. + got := extractSeverity([]Severity{{ + Type: "CVSS_V3", + Score: "CVSS:3.1/AV:L/AC:H/PR:N/UI:R/S:U/C:N/I:N/A:L", // low + }}, &DatabaseSpecific{Severity: "CRITICAL"}) + + if got != sdk.SeverityLow { + t.Fatalf("extractSeverity() = %q, want %q (CVSS should win)", got, sdk.SeverityLow) + } +} diff --git a/internal/matchers/osv/response.go b/internal/matchers/osv/response.go index 505e9a72..b1a515e1 100644 --- a/internal/matchers/osv/response.go +++ b/internal/matchers/osv/response.go @@ -84,4 +84,9 @@ type Event struct { // DatabaseSpecific holds ecosystem-specific metadata (e.g., CWE IDs from GitHub). type DatabaseSpecific struct { CweIDs []string `json:"cwe_ids,omitempty"` + // Severity is GitHub Security Advisory's textual rating (LOW / MODERATE / + // HIGH / CRITICAL). GHSA-sourced OSV entries frequently omit a CVSS vector + // in the top-level `severity` array, so this is the only severity signal + // available for them. + Severity string `json:"severity,omitempty"` } From 2a4e36cbcb0ca2c3cb8f92731a55528f643412ab Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Wed, 24 Jun 2026 05:09:00 -0700 Subject: [PATCH 2/5] fix(sarif): derive level from job impact, not severity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SARIF level (error/warning/note) was derived from the finding's severity band, so a Low-severity finding that fails the build rendered as "note" and a Critical-severity warning rendered as "error" — neither reflects whether the finding actually blocks the job. Per review feedback, level should be a pure function of disposition (does this fail the run, or just warn), while severity continues to display separately via security-severity for vulnerabilities. severityToSARIFLevel -> dispositionToSARIFLevel: fail (or unset, treated as failing, matching FailingFindingCount) -> error; warn -> warning. Co-Authored-By: Claude Opus 4.8 --- internal/output/sarif.go | 29 +++++++++------- internal/output/sarif_test.go | 64 +++++++++++++++++++++++------------ 2 files changed, 59 insertions(+), 34 deletions(-) diff --git a/internal/output/sarif.go b/internal/output/sarif.go index 19129d59..acc4d73b 100644 --- a/internal/output/sarif.go +++ b/internal/output/sarif.go @@ -188,7 +188,7 @@ func WriteSARIF(w io.Writer, findings []sdk.Finding, registry *sdk.PackageRegist ID: f.ID, ShortDescription: sarifMessage{Text: f.Title}, FullDescription: sarifReasonsMessage(f.Reasons), - DefaultConfig: sarifRuleConfig{Level: severityToSARIFLevel(string(f.Severity))}, + DefaultConfig: sarifRuleConfig{Level: dispositionToSARIFLevel(f.Disposition)}, HelpURI: helpURI, } if help := sarifHelpMessage(f.Reasons, helpURI); help != nil { @@ -209,7 +209,7 @@ func WriteSARIF(w io.Writer, findings []sdk.Finding, registry *sdk.PackageRegist locations, locationURIs := sarifLocationsForFinding(f, includeReachability, options) result := sarifResult{ RuleID: f.ID, - Level: severityToSARIFLevel(string(f.Severity)), + Level: dispositionToSARIFLevel(f.Disposition), Message: sarifMessage{Text: msgText}, Locations: locations, BaselineState: sarifBaselineState(options), @@ -624,19 +624,22 @@ func sarifPropertiesFromVulnerability(v *sdk.Vulnerability, includeReachability return props } -// severityToSARIFLevel maps an SDK severity to a SARIF level. The CVSS bands -// and the GitHub-aligned levels share one ladder: critical/high/error → error, -// medium/warning → warning, low/note → note. Unknown/N-A degrade to note. -func severityToSARIFLevel(severity string) string { - switch sdk.ParseSeverityLevel(severity) { - case sdk.SeverityCritical, sdk.SeverityHigh, sdk.SeverityError: - return "error" - case sdk.SeverityMedium, sdk.SeverityWarning: +// dispositionToSARIFLevel maps a finding's disposition to a SARIF level. The +// level reflects whether the finding blocks the job — error for a failing +// finding, warning for an advisory one — never the underlying severity band. +// Severity is reported separately via security-severity for vulnerabilities +// (see sarifRulePropertiesForFinding), so a Low-severity finding that fails +// the build still surfaces as "error" here, and a Critical one that's merely +// a warning still surfaces as "warning": job impact and severity are +// orthogonal, and GitHub's level/badge should track the former. +func dispositionToSARIFLevel(disposition sdk.FindingDisposition) string { + switch disposition { + case sdk.FindingDispositionWarn: return "warning" - case sdk.SeverityLow, sdk.SeverityNote: - return "note" default: - return "note" + // FindingDispositionFail, and "" (findings with no explicit + // disposition are treated as failing — see FailingFindingCount). + return "error" } } diff --git a/internal/output/sarif_test.go b/internal/output/sarif_test.go index 50e2cda0..b05289c0 100644 --- a/internal/output/sarif_test.go +++ b/internal/output/sarif_test.go @@ -133,30 +133,51 @@ func TestWriteSARIF_EmptyFindingsEncodeArrayFields(t *testing.T) { } } -func TestSeverityToSARIFLevel(t *testing.T) { +func TestDispositionToSARIFLevel(t *testing.T) { tests := []struct { - sev string - level string + disposition sdk.FindingDisposition + level string }{ - {"critical", "error"}, - {"high", "error"}, - {"medium", "warning"}, - {"low", "note"}, - {"unknown", "note"}, - {"", "note"}, - {"error", "error"}, - {"warning", "warning"}, - {"note", "note"}, - {"n/a", "note"}, + {sdk.FindingDispositionFail, "error"}, + {sdk.FindingDispositionWarn, "warning"}, + {"", "error"}, // unset disposition is treated as failing, like FailingFindingCount } for _, tt := range tests { - got := severityToSARIFLevel(tt.sev) + got := dispositionToSARIFLevel(tt.disposition) if got != tt.level { - t.Errorf("severityToSARIFLevel(%q) = %q, want %q", tt.sev, got, tt.level) + t.Errorf("dispositionToSARIFLevel(%q) = %q, want %q", tt.disposition, got, tt.level) } } } +// TestSARIFLevelIgnoresSeverity locks in the point that job impact and +// severity are orthogonal: a Low-severity finding that fails the build is +// still "error", and a Critical one that's only a warning is still "warning". +func TestSARIFLevelIgnoresSeverity(t *testing.T) { + findings := []sdk.Finding{ + {ID: "fail-low", PackageRef: sarifTestPURL, Title: "x", Severity: sdk.SeverityLow, Disposition: sdk.FindingDispositionFail}, + {ID: "warn-critical", PackageRef: sarifTestPURL, Title: "y", Severity: sdk.SeverityCritical, Disposition: sdk.FindingDispositionWarn}, + } + var buf bytes.Buffer + if err := WriteSARIF(&buf, findings, nil, "bomly", "0.1.0"); err != nil { + t.Fatalf("WriteSARIF: %v", err) + } + var doc sarifLog + if err := json.Unmarshal(buf.Bytes(), &doc); err != nil { + t.Fatalf("decode SARIF: %v\n%s", err, buf.String()) + } + byID := map[string]sarifRule{} + for _, r := range doc.Runs[0].Tool.Driver.Rules { + byID[r.ID] = r + } + if got := byID["fail-low"].DefaultConfig.Level; got != "error" { + t.Errorf("low-severity failing finding level = %q, want error", got) + } + if got := byID["warn-critical"].DefaultConfig.Level; got != "warning" { + t.Errorf("critical-severity warning finding level = %q, want warning", got) + } +} + func TestWriteSARIF_SecuritySeverityAndFormattedHelp(t *testing.T) { findings := []sdk.Finding{ { @@ -173,12 +194,13 @@ func TestWriteSARIF_SecuritySeverityAndFormattedHelp(t *testing.T) { }, }, { - ID: "INVALID-abcd-efgh-ijkl", - Kind: sdk.FindingKindLicense, - PackageRef: sarifTestPURL, - Title: "Package has invalid SPDX license: non-standard", - Severity: sdk.SeverityWarning, - Source: "license", + ID: "INVALID-abcd-efgh-ijkl", + Kind: sdk.FindingKindLicense, + PackageRef: sarifTestPURL, + Title: "Package has invalid SPDX license: non-standard", + Severity: sdk.SeverityWarning, + Disposition: sdk.FindingDispositionWarn, + Source: "license", }, } From a6194889c40131bd0249f6c0e24346d34e79b042 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Wed, 24 Jun 2026 05:09:00 -0700 Subject: [PATCH 3/5] fix(diff): persisted findings must still fail --fail-on and stay in SARIF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Classifying a same-CVE version bump as "persisted" (instead of one introduced + one resolved) fixed the markdown summary's section agreement, but introduced a regression: diffPolicyExit/diffSARIFFindings only ever looked at audit.Introduced, so a persisted-but-still-failing finding no longer failed the job under --fail-on, and was dropped from the uploaded SARIF entirely — which made GitHub close the alert as if the issue had been resolved. Every Persisted finding is, by construction, tied to a package the diff actually changed: engine/diff.Run's focused audit graph only ever audits added, removed, or version-changed packages. So "persisted" never means unrelated pre-existing debt — it always means "this exact version bump still ships a known issue" — and must gate the run and remain an open alert exactly like an introduced finding. Add auditBlockingFindings (Introduced + Persisted) and use it for: the --fail-on exit code (both output paths), the uploaded SARIF, and the "Evaluated policy" progress summary. Co-Authored-By: Claude Opus 4.8 --- internal/cli/cmd_progress.go | 16 ++++++----- internal/cli/cmd_progress_test.go | 18 ++++++++++++- internal/cli/diff_cmd.go | 31 ++++++++++++++++----- internal/cli/diff_cmd_test.go | 45 ++++++++++++++++++++++++++++--- 4 files changed, 91 insertions(+), 19 deletions(-) diff --git a/internal/cli/cmd_progress.go b/internal/cli/cmd_progress.go index 3e604815..fff0fe2e 100644 --- a/internal/cli/cmd_progress.go +++ b/internal/cli/cmd_progress.go @@ -286,8 +286,9 @@ func analyzerProgressDetail(stats sdk.ReachabilityStats) string { return "[" + strings.Join(parts, ", ") + "]" } -// diffPolicyOutcomeProgressChild summarizes introduced diff findings using -// the same introduced-only failure semantics as `bomly diff --audit`. +// diffPolicyOutcomeProgressChild summarizes the diff findings that gate the +// run, using the same blocking semantics as `bomly diff --audit` (introduced +// plus persisted — see auditBlockingFindings). func diffPolicyOutcomeProgressChild(audit *diffengine.Audit) progress.Child { if audit == nil { return progress.Child{ @@ -296,8 +297,9 @@ func diffPolicyOutcomeProgressChild(audit *diffengine.Audit) progress.Child { Detail: "[no audit delta]", } } - failing := output.FailingFindingCount(audit.Introduced) - warnings := len(audit.Introduced) - failing + blocking := auditBlockingFindings(audit) + failing := output.FailingFindingCount(blocking) + warnings := len(blocking) - failing child := progress.Child{ Icon: progress.CheckMark, Label: "Policy Outcome", @@ -305,12 +307,12 @@ func diffPolicyOutcomeProgressChild(audit *diffengine.Audit) progress.Child { switch { case failing > 0: child.Icon = progress.CrossMark - child.Detail = fmt.Sprintf("[%d introduced failing, %d warnings]", failing, warnings) + child.Detail = fmt.Sprintf("[%d failing, %d warnings]", failing, warnings) case warnings > 0: child.Icon = progress.WarningMark - child.Detail = fmt.Sprintf("[passed, %d introduced warnings]", warnings) + child.Detail = fmt.Sprintf("[passed, %d warnings]", warnings) default: - child.Detail = "[passed, no introduced findings]" + child.Detail = "[passed, no findings]" } return child } diff --git a/internal/cli/cmd_progress_test.go b/internal/cli/cmd_progress_test.go index 23d7a14e..6ee24636 100644 --- a/internal/cli/cmd_progress_test.go +++ b/internal/cli/cmd_progress_test.go @@ -92,7 +92,23 @@ func TestDiffPolicyOutcomeProgressChild_ReportsIntroducedOutcome(t *testing.T) { if child.Icon != progress.CrossMark { t.Fatalf("expected failing outcome icon, got %#v", child) } - if child.Detail != "[1 introduced failing, 1 warnings]" { + if child.Detail != "[1 failing, 1 warnings]" { + t.Fatalf("unexpected policy outcome detail: %#v", child) + } +} + +func TestDiffPolicyOutcomeProgressChild_PersistedFindingsAlsoFail(t *testing.T) { + // A persisted finding is tied to a package the diff actually changed, so + // it must gate the run exactly like an introduced one. + child := diffPolicyOutcomeProgressChild(&diffengine.Audit{ + Persisted: []sdk.Finding{ + {Disposition: sdk.FindingDispositionFail}, + }, + }) + if child.Icon != progress.CrossMark { + t.Fatalf("expected failing outcome icon for a persisted failing finding, got %#v", child) + } + if child.Detail != "[1 failing, 0 warnings]" { t.Fatalf("unexpected policy outcome detail: %#v", child) } } diff --git a/internal/cli/diff_cmd.go b/internal/cli/diff_cmd.go index ce112e70..0815e990 100644 --- a/internal/cli/diff_cmd.go +++ b/internal/cli/diff_cmd.go @@ -211,9 +211,9 @@ func newDiffCmd() *cobra.Command { prog.SeparateReport() } err = output.Write(streams.reportWriter(), outputFormat, payload, reportRenderers) - if err == nil && current.Audit && diffResult.Audit != nil { - if failing := output.FailingFindingCount(diffResult.Audit.Introduced); failing > 0 { - return exit.PolicyViolationFindings(failing) + if err == nil { + if exitErr := diffPolicyExit(current.Audit, diffResult.Audit); exitErr != nil { + return exitErr } } return err @@ -227,16 +227,33 @@ func newDiffCmd() *cobra.Command { return cmd } -func diffSARIFFindings(audit *diffengine.Audit) []sdk.Finding { - if audit == nil || len(audit.Introduced) == 0 { +// auditBlockingFindings returns the findings that must keep gating this diff +// and remain open code-scanning alerts: newly introduced findings, plus +// persisted ones. +// +// Every Persisted finding is, by construction, tied to a package the diff +// actually changed: the focused audit graph built in engine/diff.Run only +// ever audits added, removed, or version-changed packages (see +// focusedAuditGraphs), so a finding can only be classified "persisted" when +// the same finding key survives a real change to that package. In other +// words, "persisted" never means unrelated pre-existing debt — it always +// means "this exact version bump still ships a known issue" — so it must +// fail the job under --fail-on just like an introduced finding, and must stay +// in the uploaded SARIF so GitHub doesn't close its alert. +func auditBlockingFindings(audit *diffengine.Audit) []sdk.Finding { + if audit == nil { return nil } - return append([]sdk.Finding(nil), audit.Introduced...) + return append(append([]sdk.Finding(nil), audit.Introduced...), audit.Persisted...) +} + +func diffSARIFFindings(audit *diffengine.Audit) []sdk.Finding { + return auditBlockingFindings(audit) } func diffPolicyExit(auditEnabled bool, audit *diffengine.Audit) error { if auditEnabled && audit != nil { - if failing := output.FailingFindingCount(audit.Introduced); failing > 0 { + if failing := output.FailingFindingCount(auditBlockingFindings(audit)); failing > 0 { return exit.PolicyViolationFindings(failing) } } diff --git a/internal/cli/diff_cmd_test.go b/internal/cli/diff_cmd_test.go index a45dac64..8fa11371 100644 --- a/internal/cli/diff_cmd_test.go +++ b/internal/cli/diff_cmd_test.go @@ -49,7 +49,10 @@ func TestRenderDiffTextShowsFindingsSummaryLine(t *testing.T) { } } -func TestDiffSARIFFindingsOnlyIncludesIntroduced(t *testing.T) { +func TestDiffSARIFFindingsIncludesIntroducedAndPersisted(t *testing.T) { + // Persisted findings are always tied to a package the diff changed, so + // they must stay in the SARIF output or GitHub closes their alert as if + // the issue had been resolved. introduced := sdk.Finding{ID: "new", PackageRef: "pkg:npm/new@1.0.0"} resolved := sdk.Finding{ID: "old", PackageRef: "pkg:npm/old@1.0.0"} persisted := sdk.Finding{ID: "kept", PackageRef: "pkg:npm/kept@1.0.0"} @@ -59,8 +62,12 @@ func TestDiffSARIFFindingsOnlyIncludesIntroduced(t *testing.T) { Resolved: []sdk.Finding{resolved}, Persisted: []sdk.Finding{persisted}, }) - if len(got) != 1 || got[0].ID != introduced.ID { - t.Fatalf("diffSARIFFindings() = %#v, want only introduced finding", got) + gotIDs := make(map[string]bool, len(got)) + for _, f := range got { + gotIDs[f.ID] = true + } + if len(got) != 2 || !gotIDs[introduced.ID] || !gotIDs[persisted.ID] { + t.Fatalf("diffSARIFFindings() = %#v, want introduced + persisted, excluding resolved", got) } if got := diffSARIFFindings(&diffengine.Audit{Resolved: []sdk.Finding{resolved}}); len(got) != 0 { @@ -68,6 +75,36 @@ func TestDiffSARIFFindingsOnlyIncludesIntroduced(t *testing.T) { } } +func TestDiffPolicyExit_PersistedFailingFindingsAlsoFailTheJob(t *testing.T) { + // Regression: a version bump that doesn't remediate an advisory classifies + // as persisted (see PR fixing diff section agreement), and that must still + // fail --fail-on any, since the finding is tied to a package this diff + // actually changed. + err := diffPolicyExit(true, &diffengine.Audit{ + Persisted: []sdk.Finding{{ID: "CVE-PERSISTS", Disposition: sdk.FindingDispositionFail}}, + }) + if err == nil { + t.Fatal("expected a policy violation error for a failing persisted finding") + } +} + +func TestDiffPolicyExit_PersistedWarningsDoNotFailTheJob(t *testing.T) { + err := diffPolicyExit(true, &diffengine.Audit{ + Persisted: []sdk.Finding{{ID: "license:warn", Disposition: sdk.FindingDispositionWarn}}, + }) + if err != nil { + t.Fatalf("expected no policy violation for a warning-only persisted finding, got %v", err) + } +} + +func TestDiffPolicyExit_NoAuditNoExit(t *testing.T) { + if err := diffPolicyExit(false, &diffengine.Audit{ + Persisted: []sdk.Finding{{ID: "x", Disposition: sdk.FindingDispositionFail}}, + }); err != nil { + t.Fatalf("expected no exit error when audit is disabled, got %v", err) + } +} + func TestRenderDiffTextShowsHighFindingsWhenIntroduced(t *testing.T) { payload := output.DiffResponse{ Audit: &output.DiffAudit{ @@ -134,7 +171,7 @@ func TestRenderDiffMarkdownIncludesPatchedVersionsByDefault(t *testing.T) { "| added | react@18.2.0 | 18.2.0 | - | unknown | - |", "| changed | zod | 3.22.0 → 3.23.0 | - | unknown | - |", "## Vulnerabilities", - "| ❌ | introduced | HIGH | OSV-123 | react@18.2.0 | 18.2.1 | osv | Prototype pollution in react |", + "| introduced | HIGH | OSV-123 | react@18.2.0 | 18.2.1 | osv | Prototype pollution in react |", "## Policy Findings", "| ❌ | introduced | vulnerability | HIGH | OSV-123 | react@18.2.0 | 18.2.1 | Prototype pollution in react |", } { From b82bd7e77a9442b8452552cc2a0675057d0877f7 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Wed, 24 Jun 2026 05:09:00 -0700 Subject: [PATCH 4/5] =?UTF-8?q?fix(diff):=20reserve=20=E2=9C=85/=E2=9D=8C/?= =?UTF-8?q?=E2=9A=A0=EF=B8=8F=20icons=20for=20Policy=20Findings,=20base=20?= =?UTF-8?q?them=20on=20disposition?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes per review feedback: - The Vulnerabilities table carried a leading icon column computed from severity, conflicting with the Policy Findings table's icon (which is disposition-based). Icons are now exclusive to Policy Findings, where they belong — the Vulnerabilities table relies on its existing Severity column. - findingIcon no longer branches on severity at all: a row's icon is purely ✅ resolved / ❌ failing / ⚠️ warning, a function of status and disposition. A Low-severity failing finding now correctly shows ❌; a Critical-severity warning now correctly shows ⚠️. - The Overview status badge combines Introduced + Persisted failing counts (a persisted finding is just as blocking — see the diff_cmd.go fix in this series) and drops the now-inaccurate "introduced" qualifier from its wording. Co-Authored-By: Claude Opus 4.8 --- internal/cli/render/diff_markdown.go | 35 ++++++++++++----------- internal/cli/render/diff_markdown_test.go | 29 +++++++++++++++++++ 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/internal/cli/render/diff_markdown.go b/internal/cli/render/diff_markdown.go index acf54537..fecc82a3 100644 --- a/internal/cli/render/diff_markdown.go +++ b/internal/cli/render/diff_markdown.go @@ -35,13 +35,18 @@ func diffOverviewMarkdown(payload output.DiffResponse) []string { persisted = len(payload.Audit.Persisted) resolved = len(payload.Audit.Resolved) } + // A persisted finding is always tied to a package this diff actually + // changed (see auditBlockingFindings in internal/cli/diff_cmd.go), so it + // gates the run exactly like an introduced one. status := "✅ Pass" - if payload.Audit != nil && outputAuditFailingCount(payload.Audit.Introduced) > 0 { - status = "❌ Failing findings introduced" - } else if payload.Audit != nil && introduced > 0 { - status = "⚠️ Warnings introduced" - } else if payload.Audit != nil && outputAuditFailingCount(payload.Audit.Persisted) > 0 { - status = "⚠️ Pre-existing findings unresolved" + if payload.Audit != nil { + failing := outputAuditFailingCount(payload.Audit.Introduced) + outputAuditFailingCount(payload.Audit.Persisted) + switch { + case failing > 0: + status = "❌ Failing findings" + case introduced > 0 || persisted > 0: + status = "⚠️ Warnings" + } } return markdownTable( []string{"Status", "Manifests", "Dependencies", "Findings", "Duration"}, @@ -169,7 +174,6 @@ func diffVulnerabilityTable(title, status string, changes []output.DiffVulnerabi for _, change := range sortVulnerabilityChanges(changes) { vuln := change.Vulnerability row := []string{ - findingIcon(status, string(vuln.Severity), ""), status, strings.ToUpper(valueOrDash(string(vuln.Severity))), vuln.ID, @@ -185,7 +189,7 @@ func diffVulnerabilityTable(title, status string, changes []output.DiffVulnerabi ) rows = append(rows, row) } - header := []string{"", "Change", "Severity", "ID", "Package"} + header := []string{"Change", "Severity", "ID", "Package"} if includeReachability { header = append(header, "Reachability") } @@ -307,7 +311,7 @@ func diffAuditFindingTable(title, status string, findings []output.AuditFinding, rows := make([][]string, 0, len(findings)) for _, finding := range sortDiffAuditFindings(findings) { row := []string{ - findingIcon(status, string(finding.Severity), string(finding.Disposition)), + findingIcon(status, string(finding.Disposition)), status, valueOrDash(finding.Auditor), strings.ToUpper(valueOrDash(string(finding.Severity))), @@ -338,19 +342,18 @@ func findingIconLegend() []string { return []string{"> **Legend:** ✅ resolved · ❌ failing · ⚠️ warning"} } -func findingIcon(status, severity, disposition string) string { +// findingIcon represents whether a Policy Findings row will fail the run +// (❌), only warn (⚠️), or has been resolved (✅) — purely a function of +// status/disposition, never of severity. A Low-severity failing finding still +// gets ❌; a Critical-severity warning still gets ⚠️. +func findingIcon(status, disposition string) string { if status == "resolved" { return "✅" } if strings.EqualFold(disposition, "warn") { return "⚠️" } - switch strings.ToLower(strings.TrimSpace(severity)) { - case "critical", "high": - return "❌" - default: - return "⚠️" - } + return "❌" } func outputAuditFailingCount(findings []output.AuditFinding) int { diff --git a/internal/cli/render/diff_markdown_test.go b/internal/cli/render/diff_markdown_test.go index 6c22a4fc..90fa488a 100644 --- a/internal/cli/render/diff_markdown_test.go +++ b/internal/cli/render/diff_markdown_test.go @@ -9,6 +9,35 @@ import ( "github.com/bomly-dev/bomly-cli/sdk" ) +func TestDiffOverviewMarkdownPersistedFailingCountsAsFailing(t *testing.T) { + // A persisted finding is tied to a package the diff actually changed, so + // the Overview status must reflect it as a failure, not a softer warning. + payload := output.DiffResponse{ + Audit: &output.DiffAudit{ + Persisted: []output.AuditFinding{{ID: "CVE-PERSISTS", Disposition: sdk.FindingDispositionFail}}, + }, + } + got := strings.Join(diffOverviewMarkdown(payload), "\n") + if !strings.Contains(got, "❌ Failing findings") { + t.Errorf("expected failing status for a failing persisted finding, got %q", got) + } +} + +func TestDiffOverviewMarkdownPersistedWarningsAreWarnings(t *testing.T) { + payload := output.DiffResponse{ + Audit: &output.DiffAudit{ + Persisted: []output.AuditFinding{{ID: "license:warn", Disposition: sdk.FindingDispositionWarn}}, + }, + } + got := strings.Join(diffOverviewMarkdown(payload), "\n") + if !strings.Contains(got, "⚠️ Warnings") { + t.Errorf("expected a warning status for a warn-only persisted finding, got %q", got) + } + if strings.Contains(got, "❌") { + t.Errorf("did not expect a failing status, got %q", got) + } +} + func TestHumanizeDurationMS(t *testing.T) { cases := []struct { ms int64 From 3dc0e13257b7e847ba4788220b938ce4921c4363 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Wed, 24 Jun 2026 05:24:58 -0700 Subject: [PATCH 5/5] fix(diff): apply policy gating to the --format sarif output path too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spotted in review: the `bomly diff --format sarif` branch returned right after writing the SARIF document, never calling diffPolicyExit — so that output mode could exit 0 even with failing introduced/persisted findings. The other two output paths (stdout outputSpecs, and the default text/json/markdown path) already call diffPolicyExit after writing; bring this branch in line with the same pattern. Co-Authored-By: Claude Opus 4.8 --- internal/cli/diff_cmd.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/cli/diff_cmd.go b/internal/cli/diff_cmd.go index 0815e990..331075dc 100644 --- a/internal/cli/diff_cmd.go +++ b/internal/cli/diff_cmd.go @@ -199,7 +199,10 @@ func newDiffCmd() *cobra.Command { } if outputFormat == output.FormatSARIF { prog.Success("Resolved Graph") - return sarifRenderer(streams.reportWriter()) + if err := sarifRenderer(streams.reportWriter()); err != nil { + return err + } + return diffPolicyExit(current.Audit, diffResult.Audit) } if current.Interactive { prog.Stop()