Introduce a shared Finding/SeverityLevel type across scanner integrations#54690
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail review: mostly a clean consolidation, one unused-API finding.
net: -30 lines possible.
Generated by ✂️ Ponytail Reviewer for #54690 · auto · 25.8 AIC · ⌖ 3.46 AIC · ⊞ 7.3K
Comment /ponytail to run again
| // Sort orders findings by file, then line, then column, then by decreasing | ||
| // severity, then by rule identifier. The ordering is stable and deterministic so | ||
| // that scanner output can be compared across runs. | ||
| func Sort(findings []Finding) { | ||
| slices.SortStableFunc(findings, func(a, b Finding) int { | ||
| if c := strings.Compare(a.File, b.File); c != 0 { | ||
| return c | ||
| } | ||
| if c := cmp.Compare(a.Line, b.Line); c != 0 { | ||
| return c | ||
| } | ||
| if c := cmp.Compare(a.Column, b.Column); c != 0 { | ||
| return c | ||
| } | ||
| if c := cmp.Compare(b.Severity.Rank(), a.Severity.Rank()); c != 0 { | ||
| return c | ||
| } | ||
| return strings.Compare(a.RuleID, b.RuleID) | ||
| }) | ||
| } |
There was a problem hiding this comment.
L172-191: delete: Sort exported but never called outside its own test. Nothing replaces it — drop until a caller needs it.
L194-202: delete: CountAtLeast has the same fate, unused outside tests.
There was a problem hiding this comment.
Pull request overview
Introduces a shared scanner finding model to centralize severity normalization, rendering, sorting, and source context handling.
Changes:
- Adds
scanfindings.FindingandSeverityLevel. - Migrates scanner output through shared adapters.
- Integrates audit, validation, and Markdown security findings.
Show a summary per file
| File | Description |
|---|---|
pkg/scanfindings/scanfindings.go |
Adds shared finding APIs. |
pkg/scanfindings/scanfindings_test.go |
Tests shared behavior. |
pkg/scanfindings/README.md |
Documents the package. |
pkg/cli/zizmor.go |
Adapts zizmor findings. |
pkg/cli/poutine.go |
Consolidates poutine rendering. |
pkg/cli/grype.go |
Adapts vulnerability findings. |
pkg/cli/grant.go |
Adapts license findings. |
pkg/cli/runner_guard.go |
Adapts runner-guard findings. |
pkg/cli/yamllint.go |
Parses directly into shared findings. |
pkg/cli/yamllint_test.go |
Updates parser expectations. |
pkg/cli/validation_issue.go |
Adds finding conversion. |
pkg/cli/audit_report.go |
Uses shared severity values. |
pkg/cli/audit_report_test.go |
Updates severity assertions. |
pkg/cli/audit_report_render.go |
Uses shared severity operations. |
pkg/cli/audit_agentic_analysis.go |
Normalizes assessment severity. |
pkg/cli/audit_agent_output_test.go |
Updates severity assertions. |
pkg/workflow/markdown_security_scanner.go |
Converts security findings for rendering. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Balanced
| start := max(1, line-2) | ||
| end := min(len(fileLines), line+2) | ||
|
|
||
| context := make([]string, 0, end-start+1) | ||
| for i := start; i <= end; i++ { | ||
| context = append(context, fileLines[i-1]) |
| // Severity maps the issue type ("error", "warning", ...) onto the shared | ||
| // severity vocabulary used by the scanner integrations. | ||
| func (v ValidationIssue) Severity() scanfindings.SeverityLevel { | ||
| return scanfindings.ParseSeverity(v.Type) | ||
| } | ||
|
|
||
| // ToFinding converts the validation issue to the shared finding representation. | ||
| func (v ValidationIssue) ToFinding() scanfindings.Finding { | ||
| return scanfindings.Finding{ | ||
| Severity: v.Severity(), | ||
| Message: v.Message, | ||
| File: v.File, | ||
| Line: v.Line, | ||
| } | ||
| } |
There was a problem hiding this comment.
Review: Shared scanfindings Package
This is a clean, well-motivated refactoring. The new pkg/scanfindings package successfully eliminates the per-tool severity string scattered across grype.go, zizmor.go, poutine.go, and grant.go, and replaces it with a single typed vocabulary.
Strengths:
SeverityLevelas a string type preserves JSON marshalling compatibility and keeps the zero value ("") distinct from the mapped values.ParseSeveritycovers all known scanner dialects with a thorough test table.Sort,CountAtLeast, andContextLinesare useful shared utilities that would otherwise be duplicated.- The
Renderhelper centralises theconsole.FormatErrorloop.
One non-blocking note (inline): SecurityFinding.ToFinding hardcodes SeverityHigh for all markdown security findings. Whether this is intentional or an oversight is unclear without a comment — worth clarifying.
Overall the change is correct and improves maintainability.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 40.1 AIC · ⌖ 9.1 AIC · ⊞ 6.2K
| // used by the scanner integrations. Markdown security findings are hard | ||
| // rejections, so they are always reported at high severity. | ||
| func (f SecurityFinding) ToFinding(filePath string) scanfindings.Finding { | ||
| return scanfindings.Finding{ |
There was a problem hiding this comment.
Hardcoded SeverityHigh may underclassify critical findings
ToFinding always emits SeverityHigh regardless of the SecurityFindingCategory. Categories like CategoryTemplateInjection or CategorySecretExposure are arguably critical-severity violations — a consumer applying CountAtLeast(findings, SeverityCritical) would silently miss them.
Consider either:
- Using
SeverityCriticalfor the highest-risk categories, or - Adding a comment explicitly documenting that
ToFindingis intentionally capped atSeverityHighand why — otherwise it looks like an oversight rather than a deliberate design choice.
@copilot please address this.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Verdict
No blocking issues jumped out in the changed lines.
The refactor centralizes scanner finding rendering and severity normalization, and the changed paths are covered by targeted tests. I did not find a correctness or regression risk in the diff that is specific enough to justify a review comment.
I spot-checked the shared severity mapping, per-tool adapters, and the audit/report call sites. The remaining red test output from go test ./pkg/cli/... ./pkg/workflow/... is in an unrelated interactive test (TestConfirmRunAddedWorkflow_ContextCancelled), not in the touched code.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 9.61 AIC · ⌖ 6.85 AIC · ⊞ 7K
Comment /review to run again
Add draft Architecture Decision Record for the introduction of the shared Finding/SeverityLevel type across scanner integrations (PR #54690). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (765 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, and /diagnosing-bugs — requesting changes on a path regression, a silent severity downgrade, and missing adapter tests.
📋 Key Themes & Highlights
Key Themes
- Path regression in poutine:
poutineFindingsToShareduses caller-suppliedfilePathunconditionally instead offinding.Meta.Path, which was used in the old code. Directory-scan findings for nested files will show the wrong path. - Silent severity downgrade:
ParseSeverity(assessment.Severity)inaudit_agentic_analysis.goreturnsSeverityUnknownfor unrecognised values with no log — previously the raw string was preserved. - Missing adapter unit tests: none of the five
...FindingsToSharedfunctions are directly tested; field-mapping bugs (wrong severity, swapped file/line) are invisible to the test suite. ValidationIssue.ToFinding()leavesRuleIDempty: inconsistent with every other adapter.SeverityLevelis an unguarded string alias: callers can pass bare literals without compile-time errors.
Positive Highlights
- ✅ Clean, deep module design: one
Findingtype, oneRender, one severity table — exactly the right level of abstraction. - ✅ Thorough
scanfindingspackage tests coveringParseSeverity,Sort,AtLeast,ContextLines,Render, andCompilerError. - ✅ Poutine's duplicate rendering loops correctly collapsed into a single shared adapter.
- ✅
filterActionableFindingsreplaced with the idiomaticAtLeast(SeverityLow)— much cleaner. - ✅ README documents the public API clearly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 80 AIC · ⌖ 11.6 AIC · ⊞ 7.6K
Comment /matt to run again
|
|
||
| fmt.Fprint(os.Stderr, console.FormatError(compilerErr)) | ||
| message := scanfindings.FormatMessage(severityLabel, finding.RuleID, title) | ||
| if finding.Meta.Details != "" { |
There was a problem hiding this comment.
[/codebase-design] filePath replaces finding.Meta.Path — if poutine reports a finding under a path that differs from the caller-supplied filePath (e.g. a nested file in a directory scan), the displayed location will be wrong.
💡 Suggestion
Prefer the path embedded in the finding and fall back to filePath only when empty:
file := finding.Meta.Path
if file == "" {
file = filePath
}The same applies in parseAndDisplayPoutineOutput, which also passes targetFile as the blanket filePath.
@copilot please address this.
| findings = append(findings, Finding{ | ||
| Category: category, | ||
| Severity: assessment.Severity, | ||
| Severity: scanfindings.ParseSeverity(assessment.Severity), |
There was a problem hiding this comment.
[/diagnosing-bugs] ParseSeverity(assessment.Severity) silently returns SeverityUnknown for any value not in the lookup table. If the upstream AI emits e.g. "MEDIUM" (upper-case), the severity disappears without a trace — the finding still surfaces but reads as "unknown".
💡 Suggestion
Log unrecognised values so they don't silently degrade:
sev := scanfindings.ParseSeverity(assessment.Severity)
if sev == scanfindings.SeverityUnknown && assessment.Severity != "" {
log.Debug("unrecognised assessment severity", "raw", assessment.Severity)
}A test covering the full range of AI-emitted severity strings would also guard against future changes.
@copilot please address this.
| func (v ValidationIssue) Severity() scanfindings.SeverityLevel { | ||
| return scanfindings.ParseSeverity(v.Type) | ||
| } | ||
|
|
There was a problem hiding this comment.
[/codebase-design] ToFinding() leaves RuleID empty. Every other adapter populates RuleID, which enables deduplication, filtering, and future cross-run correlation.
💡 Suggestion
Use the issue type as a fallback rule-id proxy:
func (v ValidationIssue) ToFinding() scanfindings.Finding {
return scanfindings.Finding{
RuleID: v.Type, // type used as rule-id proxy until a proper rule field is added
Severity: v.Severity(),
Message: v.Message,
File: v.File,
Line: v.Line,
}
}@copilot please address this.
| findings = append(findings, scanfindings.Finding{ | ||
| RuleID: "license-policy", | ||
| Severity: scanfindings.SeverityHigh, | ||
| Message: fmt.Sprintf("license policy violation: %s (%s)", grantPackageRef(pkg), licenses), |
There was a problem hiding this comment.
[/codebase-design] SeverityHigh is hardcoded for all license violations. This matches the old "error" type, so there's no regression, but the intent isn't visible to future contributors.
💡 Suggestion
Add a brief comment explaining the deliberate choice:
// All denied licenses are treated as high-severity policy violations.
// Grant doesn't provide per-package severity, so a fixed level is used here.
Severity: scanfindings.SeverityHigh,@copilot please address this.
|
|
||
| // SeverityLevel is the shared severity vocabulary used by every scanner | ||
| // integration. Native severity labels are normalized with ParseSeverity. | ||
| type SeverityLevel string |
There was a problem hiding this comment.
[/codebase-design] SeverityLevel is a plain string alias. Callers can pass bare string literals ("high", "High") where a SeverityLevel is expected without a compile-time error — the very fragmentation this package was introduced to eliminate.
💡 Suggestion
A godoc warning makes the risk explicit at the definition site:
// SeverityLevel is the canonical severity type. Always construct values
// via the declared constants or ParseSeverity — do NOT pass untyped string
// literals; they are not validated at compile time.
type SeverityLevel stringIf compile-time safety matters, a struct wrapper (type SeverityLevel struct{ v string }) prevents unguarded literals entirely — though that's a more invasive change.
@copilot please address this.
| @@ -515,69 +462,51 @@ func parseAndDisplayPoutineOutputForDirectory(stdout string, verbose bool, gitRo | |||
| fileLines = strings.Split(string(fileContent), "\n") | |||
There was a problem hiding this comment.
[/tdd] None of the five ...FindingsToShared adapters (poutine, grype, zizmor, runner_guard, grant) have direct unit tests. These functions are the integration seam between each tool's native output and the shared type — a silent field-mapping error (wrong severity, swapped file/line) won't be caught by existing tests.
💡 Suggestion
Add a small table-driven test per adapter in each tool's _test.go. Example for grypeFindingsToShared:
func TestGrypeFindingsToShared(t *testing.T) {
matches := []grypeFinding{{
Vulnerability: grypeVulnerability{ID: "CVE-2024-1234", Severity: "High"},
Artifact: grypeArtifact{Name: "foo", Version: "1.0"},
}}
got := grypeFindingsToShared("myimage:latest", matches)
require.Len(t, got, 1)
assert.Equal(t, scanfindings.SeverityHigh, got[0].Severity)
assert.Equal(t, "CVE-2024-1234", got[0].RuleID)
assert.Equal(t, "myimage:latest", got[0].File)
}Covering at least severity mapping and RuleID/File population would give immediate regression safety.
@copilot please address this.
|
@copilot this PR still needs forward progress before maintainer review.
Run: https://github.com/github/gh-aw/actions/runs/32552825826
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Follow-up pushed in |
PR Triage
|
PR TriageCategory: Unifies Finding/SeverityLevel across 9 scanner integrations, 852/344 across 20 files. CI 2 failures need resolution first. Automated triage — run 32572524009
|
|
🛠️ Agentic Maintenance updated this pull request branch. |
Nine scanner/finding integrations each declared their own finding struct with a different severity vocabulary (
High,error,Negligible,note, …) and its own location shape, so severity classification, message building and rendering were reimplemented per tool.Changes
pkg/scanfindingsSeverityLevelenum (unknown<info<low<medium<high<critical) withParseSeveritynormalization,Rank,AtLeast, andErrorType(consoleerror/warning/infomapping).Finding{RuleID, Severity, Message, File, Line, Column, Context}plusCompilerError,FormatMessage,Render,Sort,CountAtLeast, andContextLines(the ±2 source-line window each tool used to duplicate).…FindingsToSharedadapter and rendered once viascanfindings.Render. Poutine's two duplicated rendering loops collapse into a single adapter; zizmor's high-severity counting becomesAtLeast(SeverityHigh).yamllintIssueremoved —parseYamllintLinereturns ascanfindings.Findingdirectly.Finding.Severityis nowSeverityLevel(JSON representation unchanged);ValidationIssuegainsSeverity()/ToFinding();workflow.SecurityFindinggainsToFinding()andFormatSecurityFindingsrenders through it.Adding a scanner is now mostly field mapping:
Behavior
Severity → console error type is now defined once: critical/high →
error, medium/unknown →warning, low/info →info. This matches the previous per-tool mapping for every severity exercised by existing tests; the only divergence is thatlow-severity zizmor and runner-guard findings now render asinfoinstead ofwarning, aligning them with grype.Run: https://github.com/github/gh-aw/actions/runs/32552086004> Generated by 👨🍳 PR Sous Chef · gpt54 · 28.1 AIC · ⌖ 8.19 AIC · ⊞ 9.5K · ◷
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/32552825826> Generated by 👨🍳 PR Sous Chef · gpt54 · 24.2 AIC · ⌖ 8.28 AIC · ⊞ 9.5K · ◷