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..76a0452eefb --- /dev/null +++ b/docs/adr/67643-share-scanner-finding-conversion-via-constructor-helper.md @@ -0,0 +1,79 @@ +# ADR-67643: Share Scanner Finding Conversion via a Constructor Helper + +**Date**: 2026-10-10 +**Status**: Proposed +**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(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 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. + +#### 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 +- 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 proposed by the PR author; acceptance is pending maintainer review.* diff --git a/pkg/cli/grype.go b/pkg/cli/grype.go index 9c83f1c9be0..a7f4f0ce90c 100644 --- a/pkg/cli/grype.go +++ b/pkg/cli/grype.go @@ -452,14 +452,10 @@ 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(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 6e4755c9c33..b732bd87ec0 100644 --- a/pkg/cli/poutine.go +++ b/pkg/cli/poutine.go @@ -463,15 +463,11 @@ 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(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 811324e8b6e..3c83938a5e9 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(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/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..5dfe396e7fe 100644 --- a/pkg/cli/zizmor.go +++ b/pkg/cli/zizmor.go @@ -393,15 +393,11 @@ 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(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 d3368c800f4..9892a671f16 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(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 | @@ -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,14 @@ 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(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 52317c7903b..5a3d65433b1 100644 --- a/pkg/scanfindings/scanfindings.go +++ b/pkg/scanfindings/scanfindings.go @@ -116,6 +116,32 @@ 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(params NewFindingParams) Finding { + return Finding{ + 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), + } +} + // 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..04d9d461d49 100644 --- a/pkg/scanfindings/scanfindings_test.go +++ b/pkg/scanfindings/scanfindings_test.go @@ -2,10 +2,66 @@ 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(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) + } + }) + } +} + func TestParseSeverity(t *testing.T) { tests := []struct { raw string