Share scanner finding conversion across integrations

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.*

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
 }

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
 }

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
 }

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

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
 }

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,21 +34,26 @@ 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.
 
 ```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)

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.