From d1bdf6d15d627efcc079835ea4304725275792a0 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Sat, 10 Oct 2026 22:39:22 -0700 Subject: [PATCH 1/3] Share scanner finding conversion across integrations Normalize adapted finding fields through scanfindings.NewFinding while preserving each scanner's native schema, message, locations, and defaults. Add native JSON regression coverage and document the helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pkg/cli/grype.go | 11 +- pkg/cli/poutine.go | 13 +-- pkg/cli/runner_guard.go | 13 +-- pkg/cli/scanner_findings_test.go | 146 ++++++++++++++++++++++++++ pkg/cli/zizmor.go | 13 +-- pkg/scanfindings/README.md | 19 ++-- pkg/scanfindings/scanfindings.go | 15 +++ pkg/scanfindings/scanfindings_test.go | 53 ++++++++++ 8 files changed, 240 insertions(+), 43 deletions(-) create mode 100644 pkg/cli/scanner_findings_test.go diff --git a/pkg/cli/grype.go b/pkg/cli/grype.go index 9c83f1c9be0..c15388ba873 100644 --- a/pkg/cli/grype.go +++ b/pkg/cli/grype.go @@ -452,14 +452,9 @@ func grypeFindingsToShared(imageTag string, matches []grypeFinding) []scanfindin message = fmt.Sprintf("%s (%s)", message, vuln.DataSource) } - findings = append(findings, scanfindings.Finding{ - RuleID: vuln.ID, - Severity: scanfindings.ParseSeverity(severityLabel), - Message: message, - File: imageTag, - Line: 1, - Column: 1, - }) + findings = append(findings, scanfindings.NewFinding( + vuln.ID, severityLabel, message, imageTag, 1, 1, nil, + )) } return findings } diff --git a/pkg/cli/poutine.go b/pkg/cli/poutine.go index 6e4755c9c33..3bc04ec8695 100644 --- a/pkg/cli/poutine.go +++ b/pkg/cli/poutine.go @@ -463,15 +463,10 @@ func poutineFindingsToShared(findings []poutineFinding, rules poutineRules, file message = fmt.Sprintf("%s - %s", message, finding.Meta.Details) } - shared = append(shared, scanfindings.Finding{ - RuleID: finding.RuleID, - Severity: scanfindings.ParseSeverity(severityLabel), - Message: message, - File: firstNonEmpty(finding.Meta.Path, filePath), - Line: lineNum, - Column: 1, // poutine doesn't provide column info - Context: scanfindings.ContextLines(fileLines, lineNum), - }) + shared = append(shared, scanfindings.NewFinding( + finding.RuleID, severityLabel, message, + firstNonEmpty(finding.Meta.Path, filePath), lineNum, 1, fileLines, + )) } return shared } diff --git a/pkg/cli/runner_guard.go b/pkg/cli/runner_guard.go index 811324e8b6e..cc1b0258760 100644 --- a/pkg/cli/runner_guard.go +++ b/pkg/cli/runner_guard.go @@ -317,15 +317,10 @@ func runnerGuardFindingsToShared(findings []runnerGuardFinding, fileLines []stri message = fmt.Sprintf("%s - %s", message, finding.Description) } - shared = append(shared, scanfindings.Finding{ - RuleID: finding.RuleID, - Severity: scanfindings.ParseSeverity(finding.Severity), - Message: message, - File: finding.File, - Line: lineNum, - Column: 1, - Context: scanfindings.ContextLines(fileLines, lineNum), - }) + shared = append(shared, scanfindings.NewFinding( + finding.RuleID, finding.Severity, message, + finding.File, lineNum, 1, fileLines, + )) } return shared } diff --git a/pkg/cli/scanner_findings_test.go b/pkg/cli/scanner_findings_test.go new file mode 100644 index 00000000000..258ea243515 --- /dev/null +++ b/pkg/cli/scanner_findings_test.go @@ -0,0 +1,146 @@ +package cli + +import ( + "encoding/json" + "testing" + + "github.com/github/gh-aw/pkg/scanfindings" + "github.com/stretchr/testify/require" +) + +func TestZizmorFindingsToSharedPreservesNativeSemantics(t *testing.T) { + var findings []zizmorFinding + require.NoError(t, json.Unmarshal([]byte(`[ + { + "ident":"template-injection","desc":"untrusted input","url":"https://example.com/rule", + "determinations":{"severity":"High"}, + "locations":[ + {"symbolic":{"key":{"Local":{"given_path":"first.yml"}}}, + "concrete":{"location":{"start_point":{"row":2,"column":4}}}}, + {"symbolic":{"key":{"Local":{"given_path":"second.yml"}}}, + "concrete":{"location":{"start_point":{"row":8,"column":9}}}} + ] + }, + {"ident":"no-location","determinations":{"severity":"Critical"}}, + {"ident":"informational","desc":"notice","determinations":{"severity":"note"}, + "locations":[{"concrete":{"location":{"start_point":{"row":0,"column":0}}}}]} + ]`), &findings)) + + lines := []string{"one", "two", "three", "four", "five"} + require.Equal(t, []scanfindings.Finding{ + { + RuleID: "template-injection", Severity: scanfindings.SeverityHigh, + Message: "[High] template-injection: untrusted input (https://example.com/rule)", + File: "display.yml", Line: 3, Column: 5, Context: lines, + }, + { + RuleID: "informational", Severity: scanfindings.SeverityInfo, + Message: "[note] informational: notice", + File: "display.yml", Line: 1, Column: 1, Context: []string{"one"}, + }, + }, zizmorFindingsToShared("display.yml", findings, lines)) +} + +func TestPoutineFindingsToSharedPreservesNativeSemantics(t *testing.T) { + var output poutineOutput + require.NoError(t, json.Unmarshal([]byte(`{ + "findings":[ + {"rule_id":"known","purl":"pkg:github/example", + "meta":{"path":"native.yml","line":3,"details":"extra detail"}}, + {"rule_id":"missing","meta":{}}, + {"rule_id":"note","meta":{"path":"note.yml","line":-1}} + ], + "rules":{ + "known":{"id":"known","title":"Rule title","description":"unused","level":"error"}, + "note":{"id":"note","level":"note"} + } + }`), &output)) + + lines := []string{"one", "two", "three", "four", "five"} + require.Equal(t, []scanfindings.Finding{ + { + RuleID: "known", Severity: scanfindings.SeverityHigh, + Message: "[error] known: Rule title - extra detail", + File: "native.yml", Line: 3, Column: 1, Context: lines, + }, + { + RuleID: "missing", Severity: scanfindings.SeverityMedium, + Message: "[warning] missing: missing", + File: "fallback.yml", Line: 1, Column: 1, Context: []string{"one"}, + }, + { + RuleID: "note", Severity: scanfindings.SeverityInfo, + Message: "[note] note: note", + File: "note.yml", Line: -1, Column: 1, + }, + }, poutineFindingsToShared(output.Findings, output.Rules, "fallback.yml", lines)) +} + +func TestGrypeFindingsToSharedPreservesNativeSemantics(t *testing.T) { + var output grypeOutput + require.NoError(t, json.Unmarshal([]byte(`{"matches":[ + { + "vulnerability":{"id":"CVE-1","dataSource":"https://example.com/CVE-1", + "severity":"Critical","fix":{"versions":["1.2","1.3"],"state":"fixed"}}, + "artifact":{"name":"lib","version":"1.0","type":"deb"} + }, + {"vulnerability":{"id":"CVE-2"},"artifact":{"name":"other","version":"2.0"}}, + {"vulnerability":{"id":"CVE-3","severity":"Negligible"}, + "artifact":{"name":"minor","version":"3.0"}} + ]}`), &output)) + + require.Equal(t, []scanfindings.Finding{ + { + RuleID: "CVE-1", Severity: scanfindings.SeverityCritical, + Message: "[Critical] CVE-1: lib@1.0 (fix: 1.2, 1.3) (https://example.com/CVE-1)", + File: "image:tag", Line: 1, Column: 1, + }, + { + RuleID: "CVE-2", Severity: scanfindings.SeverityUnknown, + Message: "[Unknown] CVE-2: other@2.0", + File: "image:tag", Line: 1, Column: 1, + }, + { + RuleID: "CVE-3", Severity: scanfindings.SeverityLow, + Message: "[Negligible] CVE-3: minor@3.0", + File: "image:tag", Line: 1, Column: 1, + }, + }, grypeFindingsToShared("image:tag", output.Matches)) +} + +func TestRunnerGuardFindingsToSharedPreservesNativeSemantics(t *testing.T) { + var output runnerGuardOutput + require.NoError(t, json.Unmarshal([]byte(`{"score":50,"grade":"D","findings":[ + {"rule_id":"RGS-001","name":"Rule name","severity":"HIGH", + "description":"extra detail","remediation":"unused","file":"native.yml", + "job_id":"agent","line":3}, + {"rule_id":"RGS-002","name":"Unnamed severity","file":"default.yml","line":0}, + {"rule_id":"RGS-003","severity":"low","file":"negative.yml","line":-1} + ]}`), &output)) + + lines := []string{"one", "two", "three", "four", "five"} + require.Equal(t, []scanfindings.Finding{ + { + RuleID: "RGS-001", Severity: scanfindings.SeverityHigh, + Message: "[HIGH] RGS-001: Rule name - extra detail", + File: "native.yml", Line: 3, Column: 1, Context: lines, + }, + { + RuleID: "RGS-002", Severity: scanfindings.SeverityUnknown, + Message: "RGS-002: Unnamed severity", + File: "default.yml", Line: 1, Column: 1, Context: []string{"one"}, + }, + { + RuleID: "RGS-003", Severity: scanfindings.SeverityLow, + Message: "[low] RGS-003", + File: "negative.yml", Line: -1, Column: 1, + }, + }, runnerGuardFindingsToShared(output.Findings, lines)) +} + +func TestScannerFindingsToSharedEmptyInput(t *testing.T) { + require.Empty(t, zizmorFindingsToShared("file.yml", nil, nil)) + require.Empty(t, poutineFindingsToShared(nil, nil, "file.yml", nil)) + require.Empty(t, grypeFindingsToShared("image:tag", nil)) + require.Empty(t, runnerGuardFindingsToShared(nil, nil)) +} diff --git a/pkg/cli/zizmor.go b/pkg/cli/zizmor.go index f233948a720..165f999b35b 100644 --- a/pkg/cli/zizmor.go +++ b/pkg/cli/zizmor.go @@ -393,15 +393,10 @@ func zizmorFindingsToShared(filePath string, findings []zizmorFinding, fileLines message = fmt.Sprintf("%s (%s)", message, finding.URL) } - shared = append(shared, scanfindings.Finding{ - RuleID: finding.Ident, - Severity: scanfindings.ParseSeverity(finding.Determinations.Severity), - Message: message, - File: filePath, - Line: lineNum, - Column: colNum, - Context: scanfindings.ContextLines(fileLines, lineNum), - }) + shared = append(shared, scanfindings.NewFinding( + finding.Ident, finding.Determinations.Severity, message, + filePath, lineNum, colNum, fileLines, + )) } return shared } diff --git a/pkg/scanfindings/README.md b/pkg/scanfindings/README.md index d3368c800f4..fcb54402e36 100644 --- a/pkg/scanfindings/README.md +++ b/pkg/scanfindings/README.md @@ -19,6 +19,7 @@ Each scanner speaks its own native JSON dialect, with severities spelled in a di | Function | Signature | Description | |----------|-----------|-------------| +| `NewFinding` | `func NewFinding(ruleID, severityLabel, message, file string, line, column int, fileLines []string) Finding` | Maps scanner-adapted fields, normalizes severity, and attaches source context | | `ParseSeverity` | `func ParseSeverity(raw string) SeverityLevel` | Normalizes a native severity label (case-insensitive) | | `SeverityLevel.String` | `func (s SeverityLevel) String() string` | Canonical lowercase severity name | | `SeverityLevel.Rank` | `func (s SeverityLevel) Rank() int` | Relative ordering, higher is more severe | @@ -33,6 +34,11 @@ Each scanner speaks its own native JSON dialect, with severities spelled in a di ## Usage Examples +Keep native wire structs and scanner-specific messages, location selection, and +defaults in the integration. Use `NewFinding` for the common conversion once +those fields are resolved. It preserves the supplied message and coordinates; +missing positions are defaulted only when rendering through `CompilerError`. + `Render` uses stderr terminal/color detection for scanner diagnostics, even when `w` is a buffer for testing. It does not redirect writes: pass `os.Stderr` for the production diagnostic stream. @@ -40,14 +46,11 @@ the production diagnostic stream. ```go import "github.com/github/gh-aw/pkg/scanfindings" -findings := []scanfindings.Finding{{ - RuleID: "template-injection", - Severity: scanfindings.ParseSeverity("High"), - Message: scanfindings.FormatMessage("High", "template-injection", "template injection with untrusted input"), - File: ".github/workflows/demo.lock.yml", - Line: 12, - Column: 24, -}} +findings := []scanfindings.Finding{scanfindings.NewFinding( + "template-injection", "High", + scanfindings.FormatMessage("High", "template-injection", "template injection with untrusted input"), + ".github/workflows/demo.lock.yml", 12, 24, nil, +)} scanfindings.Sort(findings) scanfindings.Render(os.Stderr, findings) diff --git a/pkg/scanfindings/scanfindings.go b/pkg/scanfindings/scanfindings.go index 52317c7903b..1f334319218 100644 --- a/pkg/scanfindings/scanfindings.go +++ b/pkg/scanfindings/scanfindings.go @@ -116,6 +116,21 @@ type Finding struct { Context []string `json:"-"` } +// NewFinding converts scanner-adapted fields into a shared finding, normalizing +// severity and attaching source context. The integration supplies its formatted +// message and display location, including any scanner-specific defaults. +func NewFinding(ruleID, severityLabel, message, file string, line, column int, fileLines []string) Finding { + return Finding{ + RuleID: ruleID, + Severity: ParseSeverity(severityLabel), + Message: message, + File: file, + Line: line, + Column: column, + Context: ContextLines(fileLines, line), + } +} + // CompilerError converts the finding into the console error format shared by all // scanner output. Missing line and column values default to 1 so that the // rendered position stays well formed. diff --git a/pkg/scanfindings/scanfindings_test.go b/pkg/scanfindings/scanfindings_test.go index b19e53bbb20..9f3a0a6a94d 100644 --- a/pkg/scanfindings/scanfindings_test.go +++ b/pkg/scanfindings/scanfindings_test.go @@ -2,10 +2,63 @@ package scanfindings import ( "bytes" + "reflect" "strings" "testing" ) +func TestNewFinding(t *testing.T) { + fileLines := []string{"one", "two", "three", "four", "five"} + tests := []struct { + name string + severity string + line int + column int + lines []string + want Finding + }{ + { + name: "normalizes severity and attaches context", severity: " High ", + line: 3, column: 7, lines: fileLines, + want: Finding{ + RuleID: "rule", Severity: SeverityHigh, Message: "native message", + File: "workflow.yml", Line: 3, Column: 7, Context: fileLines, + }, + }, + { + name: "preserves absent coordinates", severity: "", + want: Finding{ + RuleID: "rule", Severity: SeverityUnknown, Message: "native message", + File: "workflow.yml", + }, + }, + { + name: "preserves negative coordinates", severity: "custom", + line: -1, column: -2, lines: fileLines, + want: Finding{ + RuleID: "rule", Severity: SeverityUnknown, Message: "native message", + File: "workflow.yml", Line: -1, Column: -2, + }, + }, + { + name: "out of range has no context", severity: "note", + line: 9, column: 1, lines: fileLines, + want: Finding{ + RuleID: "rule", Severity: SeverityInfo, Message: "native message", + File: "workflow.yml", Line: 9, Column: 1, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := NewFinding("rule", tt.severity, "native message", "workflow.yml", tt.line, tt.column, tt.lines) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("NewFinding() = %#v, want %#v", got, tt.want) + } + }) + } +} + func TestParseSeverity(t *testing.T) { tests := []struct { raw string From 67438b9abb5996242c8715043d23203a323d8123 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 11 Oct 2026 05:47:22 +0000 Subject: [PATCH 2/3] Add draft ADR-67643 for shared scanner finding conversion --- ...nding-conversion-via-constructor-helper.md | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 docs/adr/67643-share-scanner-finding-conversion-via-constructor-helper.md diff --git a/docs/adr/67643-share-scanner-finding-conversion-via-constructor-helper.md b/docs/adr/67643-share-scanner-finding-conversion-via-constructor-helper.md new file mode 100644 index 00000000000..26fc4462cdb --- /dev/null +++ b/docs/adr/67643-share-scanner-finding-conversion-via-constructor-helper.md @@ -0,0 +1,72 @@ +# ADR-67643: Share Scanner Finding Conversion via a Constructor Helper + +**Date**: 2026-10-10 +**Status**: Draft +**Deciders**: pelikhan (PR author); gh-aw maintainers (review pending) + +--- + +### Context + +Four scanner integrations in `pkg/cli` — zizmor (`zizmor.go`), poutine (`poutine.go`), grype (`grype.go`), +and runner-guard (`runner_guard.go`) — each parse a third-party tool's own JSON schema into a +tool-specific wire struct, then hand-write a near-identical conversion into the shared +`scanfindings.Finding` type (`pkg/scanfindings/scanfindings.go`). The copy-pasted blocks repeated the same +`RuleID` / `ParseSeverity(...)` / `File` / `Line` / `Column` / `ContextLines(...)` mapping, so every new +scanner integration paid a copy-paste tax and risked diverging on severity normalization or context +attachment (issue #67202). The native wire structs themselves must stay separate because they mirror each +tool's external JSON contract, and the user-visible behaviour of each scanner — message formatting, +location selection, per-scanner defaults, filtering, and rendering — must not change. + +### Decision + +We will add a single shared constructor, `scanfindings.NewFinding(ruleID, severityLabel, message, file, +line, column, fileLines)`, that owns the common mapping: severity normalization via `ParseSeverity` and +source-context attachment via `ContextLines`. Each of the four adapters calls it with the values it has +already computed, so scanner-specific message formatting, location selection, and defaults remain in the +adapter where they belong. The helper is deliberately the smallest possible shared surface: it centralizes +only the mechanical, identical part of the conversion and leaves the semantics of each scanner untouched. +`AuditFinding` and `SecurityFinding` are explicitly out of scope for this change. + +### Alternatives Considered + +#### Alternative 1: A `rawScanFinding` interface implemented by each wire struct + +The original issue suggested defining an interface (e.g. `RuleID() string`, `Severity() string`, +`Location() (string, int, int)`) that each tool-specific struct implements, with one generic conversion +function. This was rejected because the per-scanner logic that actually differs — message composition from +rule tables, multi-location selection, fallback file paths, row/column offsets — is not uniform enough to +hide behind accessor methods; each struct would need bespoke methods that simply relocate the same code +behind more indirection, while making the adapters harder to read. + +#### Alternative 2: Leave the duplication in place + +Four short, obvious literal constructions are cheap to read, and duplication in adapters is a common, +defensible pattern. This was rejected because the duplication is exact rather than incidental: severity +normalization and context attachment are invariants of the shared `Finding` type, and letting each adapter +restate them invites silent drift as scanners are added — the concrete motivation recorded in issue #67202. + +### Consequences + +#### Positive +- New scanner integrations get severity normalization and context attachment for free by calling one helper. +- Severity/context behaviour can be changed in one place instead of four, removing a class of drift bugs. +- Native-JSON regression tests added for all four adapters pin the existing output semantics, so future + refactors of the helper are guarded. + +#### Negative +- `NewFinding` takes seven positional parameters, so call sites are less self-documenting than the previous + keyed struct literals and are easy to mis-order (e.g. swapping `line` and `column`). +- The helper always calls `ContextLines`; an adapter that wants a finding without context must pass `nil`, + which couples all adapters to one context policy. + +#### Neutral +- Adapters keep their own wire structs and message-formatting code, so the duplication reduction is partial + by design. +- `AuditFinding` (`audit_report.go`) and `SecurityFinding` (`markdown_security_scanner.go`) remain separate + cousin types; folding them in is deferred to a future decision. +- No public CLI behaviour or JSON schema changes; the change is internal to `pkg/cli` and `pkg/scanfindings`. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From de643e79fff042b9d0a9535ce41931874ded1901 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 11 Oct 2026 07:21:58 +0000 Subject: [PATCH 3/3] Use named params for shared scanner findings Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- ...nding-conversion-via-constructor-helper.md | 27 ++++++++++++------- pkg/cli/grype.go | 7 ++--- pkg/cli/poutine.go | 9 ++++--- pkg/cli/runner_guard.go | 8 +++--- pkg/cli/zizmor.go | 9 ++++--- pkg/scanfindings/README.md | 15 ++++++----- pkg/scanfindings/scanfindings.go | 27 +++++++++++++------ pkg/scanfindings/scanfindings_test.go | 5 +++- 8 files changed, 67 insertions(+), 40 deletions(-) diff --git a/docs/adr/67643-share-scanner-finding-conversion-via-constructor-helper.md b/docs/adr/67643-share-scanner-finding-conversion-via-constructor-helper.md index 26fc4462cdb..76a0452eefb 100644 --- a/docs/adr/67643-share-scanner-finding-conversion-via-constructor-helper.md +++ b/docs/adr/67643-share-scanner-finding-conversion-via-constructor-helper.md @@ -1,7 +1,7 @@ # ADR-67643: Share Scanner Finding Conversion via a Constructor Helper **Date**: 2026-10-10 -**Status**: Draft +**Status**: Proposed **Deciders**: pelikhan (PR author); gh-aw maintainers (review pending) --- @@ -20,13 +20,15 @@ location selection, per-scanner defaults, filtering, and rendering — must not ### Decision -We will add a single shared constructor, `scanfindings.NewFinding(ruleID, severityLabel, message, file, -line, column, fileLines)`, that owns the common mapping: severity normalization via `ParseSeverity` and -source-context attachment via `ContextLines`. Each of the four adapters calls it with the values it has +We will add a single shared constructor, `scanfindings.NewFinding(params)`, taking a +`scanfindings.NewFindingParams` value with named fields for the rule, severity label, message, display +location, and source lines. It owns the common mapping: severity normalization via `ParseSeverity` and +source-context attachment via `ContextLines`. Named fields make call sites self-documenting and prevent +same-typed values from being silently transposed. Each of the four adapters supplies values it has already computed, so scanner-specific message formatting, location selection, and defaults remain in the -adapter where they belong. The helper is deliberately the smallest possible shared surface: it centralizes -only the mechanical, identical part of the conversion and leaves the semantics of each scanner untouched. -`AuditFinding` and `SecurityFinding` are explicitly out of scope for this change. +adapter where they belong. The helper centralizes only the mechanical, identical part of the conversion +and leaves the semantics of each scanner untouched. `AuditFinding` and `SecurityFinding` are explicitly +out of scope for this change. ### Alternatives Considered @@ -46,17 +48,22 @@ defensible pattern. This was rejected because the duplication is exact rather th normalization and context attachment are invariants of the shared `Finding` type, and letting each adapter restate them invites silent drift as scanners are added — the concrete motivation recorded in issue #67202. +#### Alternative 3: Use positional constructor parameters + +A constructor with one argument per field would avoid duplicating conversion logic, but adjacent +same-typed values could be accidentally transposed without a compiler error. A parameter struct was +chosen instead so each call site names the field it supplies. + ### Consequences #### Positive - New scanner integrations get severity normalization and context attachment for free by calling one helper. - Severity/context behaviour can be changed in one place instead of four, removing a class of drift bugs. +- Named parameter fields make scanner call sites clearer and avoid silent argument-order mistakes. - Native-JSON regression tests added for all four adapters pin the existing output semantics, so future refactors of the helper are guarded. #### Negative -- `NewFinding` takes seven positional parameters, so call sites are less self-documenting than the previous - keyed struct literals and are easy to mis-order (e.g. swapping `line` and `column`). - The helper always calls `ContextLines`; an adapter that wants a finding without context must pass `nil`, which couples all adapters to one context policy. @@ -69,4 +76,4 @@ restate them invites silent drift as scanners are added — the concrete motivat --- -*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* +*ADR proposed by the PR author; acceptance is pending maintainer review.* diff --git a/pkg/cli/grype.go b/pkg/cli/grype.go index c15388ba873..a7f4f0ce90c 100644 --- a/pkg/cli/grype.go +++ b/pkg/cli/grype.go @@ -452,9 +452,10 @@ func grypeFindingsToShared(imageTag string, matches []grypeFinding) []scanfindin message = fmt.Sprintf("%s (%s)", message, vuln.DataSource) } - findings = append(findings, scanfindings.NewFinding( - vuln.ID, severityLabel, message, imageTag, 1, 1, nil, - )) + findings = append(findings, scanfindings.NewFinding(scanfindings.NewFindingParams{ + RuleID: vuln.ID, SeverityLabel: severityLabel, Message: message, + File: imageTag, Line: 1, Column: 1, + })) } return findings } diff --git a/pkg/cli/poutine.go b/pkg/cli/poutine.go index 3bc04ec8695..b732bd87ec0 100644 --- a/pkg/cli/poutine.go +++ b/pkg/cli/poutine.go @@ -463,10 +463,11 @@ func poutineFindingsToShared(findings []poutineFinding, rules poutineRules, file message = fmt.Sprintf("%s - %s", message, finding.Meta.Details) } - shared = append(shared, scanfindings.NewFinding( - finding.RuleID, severityLabel, message, - firstNonEmpty(finding.Meta.Path, filePath), lineNum, 1, fileLines, - )) + shared = append(shared, scanfindings.NewFinding(scanfindings.NewFindingParams{ + RuleID: finding.RuleID, SeverityLabel: severityLabel, Message: message, + File: firstNonEmpty(finding.Meta.Path, filePath), Line: lineNum, Column: 1, + FileLines: fileLines, + })) } return shared } diff --git a/pkg/cli/runner_guard.go b/pkg/cli/runner_guard.go index cc1b0258760..3c83938a5e9 100644 --- a/pkg/cli/runner_guard.go +++ b/pkg/cli/runner_guard.go @@ -317,10 +317,10 @@ func runnerGuardFindingsToShared(findings []runnerGuardFinding, fileLines []stri message = fmt.Sprintf("%s - %s", message, finding.Description) } - shared = append(shared, scanfindings.NewFinding( - finding.RuleID, finding.Severity, message, - finding.File, lineNum, 1, fileLines, - )) + shared = append(shared, scanfindings.NewFinding(scanfindings.NewFindingParams{ + RuleID: finding.RuleID, SeverityLabel: finding.Severity, Message: message, + File: finding.File, Line: lineNum, Column: 1, FileLines: fileLines, + })) } return shared } diff --git a/pkg/cli/zizmor.go b/pkg/cli/zizmor.go index 165f999b35b..5dfe396e7fe 100644 --- a/pkg/cli/zizmor.go +++ b/pkg/cli/zizmor.go @@ -393,10 +393,11 @@ func zizmorFindingsToShared(filePath string, findings []zizmorFinding, fileLines message = fmt.Sprintf("%s (%s)", message, finding.URL) } - shared = append(shared, scanfindings.NewFinding( - finding.Ident, finding.Determinations.Severity, message, - filePath, lineNum, colNum, fileLines, - )) + shared = append(shared, scanfindings.NewFinding(scanfindings.NewFindingParams{ + RuleID: finding.Ident, SeverityLabel: finding.Determinations.Severity, + Message: message, File: filePath, Line: lineNum, Column: colNum, + FileLines: fileLines, + })) } return shared } diff --git a/pkg/scanfindings/README.md b/pkg/scanfindings/README.md index fcb54402e36..9892a671f16 100644 --- a/pkg/scanfindings/README.md +++ b/pkg/scanfindings/README.md @@ -19,7 +19,7 @@ Each scanner speaks its own native JSON dialect, with severities spelled in a di | Function | Signature | Description | |----------|-----------|-------------| -| `NewFinding` | `func NewFinding(ruleID, severityLabel, message, file string, line, column int, fileLines []string) Finding` | Maps scanner-adapted fields, normalizes severity, and attaches source context | +| `NewFinding` | `func NewFinding(params NewFindingParams) Finding` | Maps scanner-adapted fields, normalizes severity, and attaches source context | | `ParseSeverity` | `func ParseSeverity(raw string) SeverityLevel` | Normalizes a native severity label (case-insensitive) | | `SeverityLevel.String` | `func (s SeverityLevel) String() string` | Canonical lowercase severity name | | `SeverityLevel.Rank` | `func (s SeverityLevel) Rank() int` | Relative ordering, higher is more severe | @@ -46,11 +46,14 @@ the production diagnostic stream. ```go import "github.com/github/gh-aw/pkg/scanfindings" -findings := []scanfindings.Finding{scanfindings.NewFinding( - "template-injection", "High", - scanfindings.FormatMessage("High", "template-injection", "template injection with untrusted input"), - ".github/workflows/demo.lock.yml", 12, 24, nil, -)} +findings := []scanfindings.Finding{scanfindings.NewFinding(scanfindings.NewFindingParams{ + RuleID: "template-injection", + SeverityLabel: "High", + Message: scanfindings.FormatMessage("High", "template-injection", "template injection with untrusted input"), + File: ".github/workflows/demo.lock.yml", + Line: 12, + Column: 24, +})} scanfindings.Sort(findings) scanfindings.Render(os.Stderr, findings) diff --git a/pkg/scanfindings/scanfindings.go b/pkg/scanfindings/scanfindings.go index 1f334319218..5a3d65433b1 100644 --- a/pkg/scanfindings/scanfindings.go +++ b/pkg/scanfindings/scanfindings.go @@ -116,18 +116,29 @@ type Finding struct { Context []string `json:"-"` } +// NewFindingParams contains scanner-adapted fields for NewFinding. +type NewFindingParams struct { + RuleID string + SeverityLabel string + Message string + File string + Line int + Column int + FileLines []string +} + // NewFinding converts scanner-adapted fields into a shared finding, normalizing // severity and attaching source context. The integration supplies its formatted // message and display location, including any scanner-specific defaults. -func NewFinding(ruleID, severityLabel, message, file string, line, column int, fileLines []string) Finding { +func NewFinding(params NewFindingParams) Finding { return Finding{ - RuleID: ruleID, - Severity: ParseSeverity(severityLabel), - Message: message, - File: file, - Line: line, - Column: column, - Context: ContextLines(fileLines, line), + RuleID: params.RuleID, + Severity: ParseSeverity(params.SeverityLabel), + Message: params.Message, + File: params.File, + Line: params.Line, + Column: params.Column, + Context: ContextLines(params.FileLines, params.Line), } } diff --git a/pkg/scanfindings/scanfindings_test.go b/pkg/scanfindings/scanfindings_test.go index 9f3a0a6a94d..04d9d461d49 100644 --- a/pkg/scanfindings/scanfindings_test.go +++ b/pkg/scanfindings/scanfindings_test.go @@ -51,7 +51,10 @@ func TestNewFinding(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := NewFinding("rule", tt.severity, "native message", "workflow.yml", tt.line, tt.column, tt.lines) + got := NewFinding(NewFindingParams{ + RuleID: "rule", SeverityLabel: tt.severity, Message: "native message", + File: "workflow.yml", Line: tt.line, Column: tt.column, FileLines: tt.lines, + }) if !reflect.DeepEqual(got, tt.want) { t.Errorf("NewFinding() = %#v, want %#v", got, tt.want) }