From a0e3f339d0eaf4472ee60bdc21a621fe014bd4d7 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 00:35:09 +0530 Subject: [PATCH 1/6] fix: add strict severity and phase parsers that reject unknown values ParseSeverity and ParsePhase fail open: unknown input silently maps to SeverityInfo / PhaseUnknown, so a typo like "critcal" is indistinguishable from a legitimate value when the input is untrusted. Add ParseSeverityStrict and ParsePhaseStrict (same matching rules, but a descriptive error for unknown input) and mark the lenient parsers Deprecated so callers handling untrusted input migrate. --- CHANGELOG.md | 13 ++++++++++++ sessions/sessions.go | 21 +++++++++++++++++-- sessions/sessions_test.go | 39 +++++++++++++++++++++++++++++++++- types/severity.go | 32 ++++++++++++++++++++++------ types/severity_test.go | 44 ++++++++++++++++++++++++++++++++++++++- 5 files changed, 139 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 867f3b2..612ae27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `types.ParseSeverityStrict` and `sessions.ParsePhaseStrict` — error-reporting + variants of the lenient parsers, for callers handling untrusted input. + +### Deprecated + +- `types.ParseSeverity` — fails open to `SeverityInfo` for unknown input, so a + typo like "critcal" is indistinguishable from a legitimate "info" finding; + use `ParseSeverityStrict`. +- `sessions.ParsePhase` — fails open to `PhaseUnknown` for unknown input; use + `ParsePhaseStrict`. + ## [0.1.8] — 2026-07-22 ### Fixed diff --git a/sessions/sessions.go b/sessions/sessions.go index 31b2b22..c2147dd 100644 --- a/sessions/sessions.go +++ b/sessions/sessions.go @@ -43,12 +43,29 @@ const ( // ParsePhase parses a phase name string into a Phase constant. // Returns PhaseUnknown for unrecognised values rather than an error, so // callers that receive phase names from JSON/TOML do not need to handle errors. +// +// Deprecated: ParsePhase fails open — misspelled or otherwise unknown phase +// names silently map to PhaseUnknown, which is indistinguishable from +// "phase attribution unavailable". Callers handling untrusted input should +// use ParsePhaseStrict, which reports unknown values as errors instead. func ParsePhase(s string) Phase { + p, err := ParsePhaseStrict(s) + if err != nil { + return PhaseUnknown + } + return p +} + +// ParsePhaseStrict converts a phase name string into a Phase constant, +// reporting unknown values as errors instead of failing open to PhaseUnknown. +// Matching is exact, exactly like ParsePhase; the two accept the same set of +// valid names. +func ParsePhaseStrict(s string) (Phase, error) { switch Phase(s) { case PhaseLocalize, PhaseRepair, PhaseValidate, PhaseReview, PhasePlanning: - return Phase(s) + return Phase(s), nil default: - return PhaseUnknown + return PhaseUnknown, fmt.Errorf("unknown phase %q (want one of localize, repair, validate, review, planning)", s) } } diff --git a/sessions/sessions_test.go b/sessions/sessions_test.go index cc373fd..5f82468 100644 --- a/sessions/sessions_test.go +++ b/sessions/sessions_test.go @@ -16,17 +16,54 @@ func TestParsePhase(t *testing.T) { {"validate", sessions.PhaseValidate}, {"review", sessions.PhaseReview}, {"planning", sessions.PhasePlanning}, + // Fail-open behavior pinned by design; see ParsePhaseStrict for + // the error-reporting variant. {"", sessions.PhaseUnknown}, {"bogus", sessions.PhaseUnknown}, + {"LOCALIZE", sessions.PhaseUnknown}, } for _, tc := range cases { - got := sessions.ParsePhase(tc.input) + got := sessions.ParsePhase(tc.input) //nolint:staticcheck // pinning deprecated fail-open behavior if got != tc.want { t.Errorf("ParsePhase(%q) = %q, want %q", tc.input, got, tc.want) } } } +func TestParsePhaseStrict(t *testing.T) { + cases := []struct { + input string + want sessions.Phase + wantOk bool + }{ + {"localize", sessions.PhaseLocalize, true}, + {"repair", sessions.PhaseRepair, true}, + {"validate", sessions.PhaseValidate, true}, + {"review", sessions.PhaseReview, true}, + {"planning", sessions.PhasePlanning, true}, + {"", sessions.PhaseUnknown, false}, + {"bogus", sessions.PhaseUnknown, false}, + {"localizes", sessions.PhaseUnknown, false}, + {"LOCALIZE", sessions.PhaseUnknown, false}, + } + for _, tc := range cases { + got, err := sessions.ParsePhaseStrict(tc.input) + if tc.wantOk { + if err != nil { + t.Errorf("ParsePhaseStrict(%q) unexpected error: %v", tc.input, err) + continue + } + if got != tc.want { + t.Errorf("ParsePhaseStrict(%q) = %q, want %q", tc.input, got, tc.want) + } + continue + } + if err == nil { + t.Errorf("ParsePhaseStrict(%q) err = nil, want error", tc.input) + } + } +} + func TestPhaseString(t *testing.T) { if sessions.PhaseLocalize.String() != "localize" { t.Errorf("PhaseLocalize.String() = %q, want %q", sessions.PhaseLocalize.String(), "localize") diff --git a/types/severity.go b/types/severity.go index 62c9c46..983075e 100644 --- a/types/severity.go +++ b/types/severity.go @@ -1,6 +1,9 @@ package types -import "strings" +import ( + "fmt" + "strings" +) // Severity represents the impact level of a finding. type Severity int @@ -23,18 +26,35 @@ func (s Severity) String() string { } // ParseSeverity converts a string to a Severity. +// +// Deprecated: ParseSeverity fails open — unknown input (typos such as +// "critcal", empty strings, arbitrary text) silently maps to SeverityInfo, +// so a malformed value is indistinguishable from a legitimate "info". +// Callers handling untrusted input should use ParseSeverityStrict, which +// reports unknown values as errors instead. func ParseSeverity(s string) Severity { + sev, _ := ParseSeverityStrict(s) + return sev +} + +// ParseSeverityStrict converts a string to a Severity, reporting unknown +// values as errors instead of failing open to SeverityInfo. Matching is +// case-insensitive and ignores surrounding whitespace, exactly like +// ParseSeverity; the two accept the same set of valid names. +func ParseSeverityStrict(s string) (Severity, error) { switch strings.ToLower(strings.TrimSpace(s)) { case "critical": - return SeverityCritical + return SeverityCritical, nil case "high": - return SeverityHigh + return SeverityHigh, nil case "medium": - return SeverityMedium + return SeverityMedium, nil case "low": - return SeverityLow + return SeverityLow, nil + case "info": + return SeverityInfo, nil default: - return SeverityInfo + return SeverityInfo, fmt.Errorf("unknown severity %q (want one of info, low, medium, high, critical)", s) } } diff --git a/types/severity_test.go b/types/severity_test.go index 470d712..498e0d3 100644 --- a/types/severity_test.go +++ b/types/severity_test.go @@ -15,16 +15,58 @@ func TestParseSeverity(t *testing.T) { {in: "HIGH", want: types.SeverityHigh}, {in: " medium ", want: types.SeverityMedium}, {in: "low", want: types.SeverityLow}, + {in: "info", want: types.SeverityInfo}, + // Fail-open behavior pinned by design; see ParseSeverityStrict for + // the error-reporting variant. {in: "unknown", want: types.SeverityInfo}, + {in: "critcal", want: types.SeverityInfo}, + {in: "", want: types.SeverityInfo}, } for _, tt := range tests { - if got := types.ParseSeverity(tt.in); got != tt.want { + if got := types.ParseSeverity(tt.in); got != tt.want { //nolint:staticcheck // pinning deprecated fail-open behavior t.Fatalf("ParseSeverity(%q) = %v, want %v", tt.in, got, tt.want) } } } +func TestParseSeverityStrict(t *testing.T) { + tests := []struct { + in string + want types.Severity + wantErr bool + }{ + {in: "critical", want: types.SeverityCritical}, + {in: "CRITICAL", want: types.SeverityCritical}, + {in: "High", want: types.SeverityHigh}, + {in: " medium ", want: types.SeverityMedium}, + {in: "low", want: types.SeverityLow}, + {in: "info", want: types.SeverityInfo}, + {in: "INFO", want: types.SeverityInfo}, + {in: "critcal", wantErr: true}, + {in: "unknown", wantErr: true}, + {in: "severe", wantErr: true}, + {in: "", wantErr: true}, + {in: " ", wantErr: true}, + } + + for _, tt := range tests { + got, err := types.ParseSeverityStrict(tt.in) + if tt.wantErr { + if err == nil { + t.Fatalf("ParseSeverityStrict(%q) err = nil, want error", tt.in) + } + continue + } + if err != nil { + t.Fatalf("ParseSeverityStrict(%q) unexpected error: %v", tt.in, err) + } + if got != tt.want { + t.Fatalf("ParseSeverityStrict(%q) = %v, want %v", tt.in, got, tt.want) + } + } +} + func TestSeverityString(t *testing.T) { cases := map[types.Severity]string{ types.SeverityInfo: "info", From 051a704a483bbf44ffa46658e512c4ceae1358ea Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 00:36:18 +0530 Subject: [PATCH 2/6] fix(review): default unset FailOn threshold to critical, not info MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Result.Failed treated an unset FailOn (zero value SeverityInfo) as "fail on any finding", so a review result assembled without an explicit threshold failed on informational findings. Add a FailOnSet field plus a SetFailOn method, and make Failed use SeverityCritical as the effective threshold when the threshold was never set — matching the sight and inspect engine defaults. Explicit thresholds (including Info) keep their meaning when set via SetFailOn. BREAKING CHANGE: FailOn assigned by direct field assignment without SetFailOn now falls back to the critical effective threshold. --- CHANGELOG.md | 11 ++++++ review/review.go | 34 ++++++++++++++---- review/review_test.go | 80 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 118 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 612ae27..e4e5cc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed (breaking) + +- `review.Result.Failed` no longer treats an unset `FailOn` as "fail on any + finding": when the threshold was not explicitly configured (new + `FailOnSet` field, set via the new `Result.SetFailOn`), the effective + threshold is now `SeverityCritical`, matching the sight and inspect engine + defaults. Previously a zero-value `FailOn` (`SeverityInfo`) failed the + review on informational findings. Producers that set `FailOn` by direct + field assignment should migrate to `SetFailOn` so an explicitly configured + threshold (including Info) keeps taking effect. + ### Added - `types.ParseSeverityStrict` and `sessions.ParsePhaseStrict` — error-reporting diff --git a/review/review.go b/review/review.go index 52d1803..a49992f 100644 --- a/review/review.go +++ b/review/review.go @@ -61,22 +61,44 @@ type SASTFusionResult struct { // Result is the neutral review result contract. type Result struct { - Findings []Finding `json:"findings"` - Comments []InlineComment `json:"comments"` - Stats Stats `json:"stats"` - Report string `json:"report"` - FailOn contracts.Severity `json:"fail_on"` + Findings []Finding `json:"findings"` + Comments []InlineComment `json:"comments"` + Stats Stats `json:"stats"` + Report string `json:"report"` + FailOn contracts.Severity `json:"fail_on"` + // FailOnSet reports whether FailOn was explicitly configured via + // SetFailOn. When it is false, Failed() treats SeverityCritical as the + // effective threshold: an unset FailOn must not fail the review on + // informational findings just because SeverityInfo is the zero value. + FailOnSet bool `json:"fail_on_set,omitempty"` SASTFusion *SASTFusionResult `json:"sast_fusion,omitempty"` ConfidenceBreakdown *ConfidenceBreakdown `json:"confidence_breakdown,omitempty"` } +// SetFailOn sets the fail threshold used by Failed. Set the threshold +// through this method rather than assigning FailOn directly, so that the +// threshold is recorded as explicitly configured. +func (r *Result) SetFailOn(sev contracts.Severity) { + r.FailOn = sev + r.FailOnSet = true +} + // Failed reports whether any finding meets or exceeds the configured fail threshold. +// When the threshold was never set — a zero Result, or a Result whose FailOn +// field was assigned directly — SeverityCritical is used as the effective +// threshold, matching the sight and inspect engine defaults. Set the +// threshold via SetFailOn to make an explicit choice (including Info) take +// effect. func (r *Result) Failed() bool { if r == nil { return false } + threshold := r.FailOn + if !r.FailOnSet { + threshold = contracts.SeverityCritical + } for _, f := range r.Findings { - if f.Severity.AtLeast(r.FailOn) { + if f.Severity.AtLeast(threshold) { return true } } diff --git a/review/review_test.go b/review/review_test.go index 4718e12..6e3e962 100644 --- a/review/review_test.go +++ b/review/review_test.go @@ -10,21 +10,99 @@ func TestResultFailedAndMaxSeverity(t *testing.T) { t.Parallel() result := &Result{ - FailOn: contracts.SeverityHigh, Findings: []Finding{ {Severity: contracts.SeverityMedium}, {Severity: contracts.SeverityCritical}, }, } + result.SetFailOn(contracts.SeverityHigh) if !result.Failed() { t.Fatal("expected result to fail at high threshold") } + if !result.FailOnSet { + t.Fatal("expected SetFailOn to mark the threshold as set") + } + if result.FailOn != contracts.SeverityHigh { + t.Fatalf("FailOn = %v, want %v", result.FailOn, contracts.SeverityHigh) + } if got := result.MaxSeverity(); got != contracts.SeverityCritical { t.Fatalf("MaxSeverity = %v, want %v", got, contracts.SeverityCritical) } } +func TestResultFailedExplicitThresholds(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + failOn contracts.Severity + findings []Finding + want bool + }{ + { + name: "critical threshold ignores high finding", + failOn: contracts.SeverityCritical, + findings: []Finding{{Severity: contracts.SeverityHigh}}, + want: false, + }, + { + name: "critical threshold trips on critical finding", + failOn: contracts.SeverityCritical, + findings: []Finding{{Severity: contracts.SeverityCritical}}, + want: true, + }, + { + name: "explicit info threshold fails on any finding", + failOn: contracts.SeverityInfo, + findings: []Finding{{Severity: contracts.SeverityInfo}}, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &Result{Findings: tt.findings} + r.SetFailOn(tt.failOn) + if got := r.Failed(); got != tt.want { + t.Fatalf("Failed() with SetFailOn(%v) = %v, want %v", tt.failOn, got, tt.want) + } + }) + } +} + +func TestResultFailedUnsetThresholdDefaultsToCritical(t *testing.T) { + t.Parallel() + + // Zero-value Result: FailOnSet is false, so the effective threshold is + // SeverityCritical — an informational finding must not fail the review + // merely because SeverityInfo is the zero value of FailOn. + zero := &Result{ + Findings: []Finding{{Severity: contracts.SeverityInfo}}, + } + if zero.Failed() { + t.Fatal("zero-value result with info finding should not fail") + } + + // A critical finding still fails an unset threshold. + critical := &Result{ + Findings: []Finding{{Severity: contracts.SeverityCritical}}, + } + if !critical.Failed() { + t.Fatal("zero-value result with critical finding should fail") + } + + // Direct FailOn assignment without SetFailOn also falls back to the + // critical effective threshold; use SetFailOn to make it explicit. + direct := &Result{ + FailOn: contracts.SeverityLow, + Findings: []Finding{{Severity: contracts.SeverityHigh}}, + } + if direct.Failed() { + t.Fatal("directly assigned FailOn without SetFailOn should not take effect") + } +} + func TestNilResultMethods(t *testing.T) { t.Parallel() From 4433e1d7f8770d4e22a7c2eec32ac877438b22c0 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 00:36:50 +0530 Subject: [PATCH 3/6] feat(review): surface non-fatal LLM provider errors in Stats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add Stats.LLMErrors so providers can report non-fatal errors encountered during analysis; a non-empty list signals that findings may be partial. Additive field, no behavior change — engines populate it in follow-up changes. --- CHANGELOG.md | 2 ++ review/review.go | 3 +++ 2 files changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4e5cc5..22a877e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `review.Stats.LLMErrors` — surfaces non-fatal LLM provider errors + encountered during analysis (additive; findings may be partial when set). - `types.ParseSeverityStrict` and `sessions.ParsePhaseStrict` — error-reporting variants of the lenient parsers, for callers handling untrusted input. diff --git a/review/review.go b/review/review.go index a49992f..74c51f3 100644 --- a/review/review.go +++ b/review/review.go @@ -42,6 +42,9 @@ type Stats struct { AverageConfidence float64 `json:"average_confidence"` HighConfidenceCount int `json:"high_confidence_count"` LowConfidenceCount int `json:"low_confidence_count"` + // LLMErrors records non-fatal provider errors encountered during + // analysis; findings may be partial when it is non-empty. + LLMErrors []string `json:"llm_errors,omitempty"` } // ConfidenceBreakdown groups review findings by confidence band. From 6b3c66a53b96d92ada18eef474d833c9558183a1 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 00:37:54 +0530 Subject: [PATCH 4/6] feat(types,review): add Validate methods to Finding contracts Add Finding.Validate to both the types and review packages, enforcing the minimum contract invariants: non-blank Message, non-negative Line, and Confidence within [0, 1]. Returns a descriptive error naming the first violated field so producers can reject malformed findings before they reach consumers. --- CHANGELOG.md | 3 +++ review/review.go | 19 ++++++++++++++ review/review_test.go | 60 +++++++++++++++++++++++++++++++++++++++++++ types/finding.go | 18 +++++++++++++ types/finding_test.go | 58 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 158 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22a877e..839f1a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `types.Finding.Validate` and `review.Finding.Validate` — minimum contract + invariants (non-blank Message, non-negative Line, Confidence within + [0, 1]) with descriptive errors. - `review.Stats.LLMErrors` — surfaces non-fatal LLM provider errors encountered during analysis (additive; findings may be partial when set). - `types.ParseSeverityStrict` and `sessions.ParsePhaseStrict` — error-reporting diff --git a/review/review.go b/review/review.go index 74c51f3..5c85acf 100644 --- a/review/review.go +++ b/review/review.go @@ -1,6 +1,8 @@ package review import ( + "fmt" + "strings" "time" contracts "github.com/GrayCodeAI/hawk-core-contracts/types" @@ -21,6 +23,23 @@ type Finding struct { SASTSource bool `json:"sast_source,omitempty"` } +// Validate reports whether the finding satisfies the minimum contract +// invariants: a non-blank Message, a non-negative Line, and a Confidence +// within [0, 1]. It returns a descriptive error naming the first violated +// field. +func (f Finding) Validate() error { + if strings.TrimSpace(f.Message) == "" { + return fmt.Errorf("finding message is empty") + } + if f.Line < 0 { + return fmt.Errorf("finding line %d is negative", f.Line) + } + if f.Confidence < 0 || f.Confidence > 1 { + return fmt.Errorf("finding confidence %v is outside [0, 1]", f.Confidence) + } + return nil +} + // InlineComment is a review finding mapped to a concrete diff position. type InlineComment struct { Path string `json:"path"` diff --git a/review/review_test.go b/review/review_test.go index 6e3e962..7aa8220 100644 --- a/review/review_test.go +++ b/review/review_test.go @@ -103,6 +103,66 @@ func TestResultFailedUnsetThresholdDefaultsToCritical(t *testing.T) { } } +func TestFindingValidate(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + finding Finding + wantErr bool + }{ + { + name: "valid finding", + finding: Finding{Message: "unsanitized input", Line: 42, Confidence: 0.9}, + }, + { + name: "line zero is valid", + finding: Finding{Message: "whole-file finding", Line: 0, Confidence: 0.5}, + }, + { + name: "confidence bounds inclusive", + finding: Finding{Message: "m", Confidence: 1}, + }, + { + name: "empty message", + finding: Finding{Line: 10, Confidence: 0.5}, + wantErr: true, + }, + { + name: "blank message", + finding: Finding{Message: " ", Line: 10, Confidence: 0.5}, + wantErr: true, + }, + { + name: "negative line", + finding: Finding{Message: "m", Line: -3, Confidence: 0.5}, + wantErr: true, + }, + { + name: "confidence above one", + finding: Finding{Message: "m", Line: 10, Confidence: 1.01}, + wantErr: true, + }, + { + name: "confidence below zero", + finding: Finding{Message: "m", Line: 10, Confidence: -1}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.finding.Validate() + if tt.wantErr && err == nil { + t.Fatal("Validate() err = nil, want error") + } + if !tt.wantErr && err != nil { + t.Fatalf("Validate() unexpected error: %v", err) + } + }) + } +} + func TestNilResultMethods(t *testing.T) { t.Parallel() diff --git a/types/finding.go b/types/finding.go index d7aa928..a2a2482 100644 --- a/types/finding.go +++ b/types/finding.go @@ -2,6 +2,7 @@ package types import ( "fmt" + "strings" "time" ) @@ -25,6 +26,23 @@ type Finding struct { CreatedAt time.Time `json:"created_at"` } +// Validate reports whether the finding satisfies the minimum contract +// invariants: a non-blank Message, a non-negative Line, and a Confidence +// within [0, 1]. It returns a descriptive error naming the first violated +// field. +func (f Finding) Validate() error { + if strings.TrimSpace(f.Message) == "" { + return fmt.Errorf("finding message is empty") + } + if f.Line < 0 { + return fmt.Errorf("finding line %d is negative", f.Line) + } + if f.Confidence < 0 || f.Confidence > 1 { + return fmt.Errorf("finding confidence %v is outside [0, 1]", f.Confidence) + } + return nil +} + // FindingSlice is sortable by severity descending and confidence descending. type FindingSlice []Finding diff --git a/types/finding_test.go b/types/finding_test.go index 26e8586..5cc192a 100644 --- a/types/finding_test.go +++ b/types/finding_test.go @@ -113,6 +113,64 @@ func TestFindingSliceSummaryEmpty(t *testing.T) { } } +func TestFindingValidate(t *testing.T) { + tests := []struct { + name string + finding types.Finding + wantErr bool + }{ + { + name: "valid finding", + finding: types.Finding{Message: "unsanitized input", Line: 42, Confidence: 0.9}, + }, + { + name: "line zero is valid", + finding: types.Finding{Message: "whole-file finding", Line: 0, Confidence: 0.5}, + }, + { + name: "confidence bounds inclusive", + finding: types.Finding{Message: "m", Confidence: 0}, + }, + { + name: "empty message", + finding: types.Finding{Line: 10, Confidence: 0.5}, + wantErr: true, + }, + { + name: "blank message", + finding: types.Finding{Message: " ", Line: 10, Confidence: 0.5}, + wantErr: true, + }, + { + name: "negative line", + finding: types.Finding{Message: "m", Line: -1, Confidence: 0.5}, + wantErr: true, + }, + { + name: "confidence above one", + finding: types.Finding{Message: "m", Line: 10, Confidence: 1.5}, + wantErr: true, + }, + { + name: "confidence below zero", + finding: types.Finding{Message: "m", Line: 10, Confidence: -0.1}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.finding.Validate() + if tt.wantErr && err == nil { + t.Fatalf("Validate() err = nil, want error") + } + if !tt.wantErr && err != nil { + t.Fatalf("Validate() unexpected error: %v", err) + } + }) + } +} + func TestFindingFromSight(t *testing.T) { f := types.FindingFromSight("sql-injection", "db/query.go", 42, "unsanitized input", "CWE-89", types.SeverityCritical, 0.95) if f.Source != "sight" { From b796b1db1ce509cfe2ee0497f5c49fa0e53122f7 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 00:38:16 +0530 Subject: [PATCH 5/6] chore(ci): add OSSF scorecard workflow Weekly OSSF Scorecard analysis publishing SARIF to code scanning, pinned to the same action SHAs as the other hawk-eco repos (yaad, sight, inspect). --- .github/workflows/scorecard.yml | 42 +++++++++++++++++++++++++++++++++ CHANGELOG.md | 2 ++ 2 files changed, 44 insertions(+) create mode 100644 .github/workflows/scorecard.yml diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml new file mode 100644 index 0000000..b58802a --- /dev/null +++ b/.github/workflows/scorecard.yml @@ -0,0 +1,42 @@ +name: Scorecard + +on: + branch_protection_rule: + schedule: + - cron: '37 9 * * 1' + push: + branches: [main] + +permissions: + security-events: write + id-token: write + contents: read + +jobs: + analysis: + name: Scorecard analysis + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Run analysis + uses: ossf/scorecard-action@4eaacf0543bb3f2c246792bd56e8cdeffafb205a # v2.4.3 + with: + results_file: scorecard-results.sarif + results_format: sarif + publish_results: false + + - name: Upload artifact + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: SARIF file + path: scorecard-results.sarif + retention-days: 5 + + - name: Upload to code-scanning + uses: github/codeql-action/upload-sarif@8aad20d150bbac5944a9f9d289da16a4b0d87c1e # v4 + with: + sarif_file: scorecard-results.sarif diff --git a/CHANGELOG.md b/CHANGELOG.md index 839f1a3..9bdb076 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 encountered during analysis (additive; findings may be partial when set). - `types.ParseSeverityStrict` and `sessions.ParsePhaseStrict` — error-reporting variants of the lenient parsers, for callers handling untrusted input. +- OSSF Scorecard workflow (`.github/workflows/scorecard.yml`), matching the + other hawk-eco foundation/engine repos. ### Deprecated From 0f60bf0259c08d69c0bdac7c9a4810ffd41d1cda Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 02:02:43 +0530 Subject: [PATCH 6/6] fix(verify): default unset FailOn threshold to critical, not info Mirror the review.Result fix: Report.Failed now treats a threshold that was never set via SetFailOn as SeverityCritical instead of the SeverityInfo zero value, so a zero-value Report no longer fails on informational findings. --- CHANGELOG.md | 3 +++ verify/verify.go | 22 +++++++++++++++++++++- verify/verify_test.go | 29 ++++++++++++++++++++++++++++- 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bdb076..6c5846b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 review on informational findings. Producers that set `FailOn` by direct field assignment should migrate to `SetFailOn` so an explicitly configured threshold (including Info) keeps taking effect. +- `verify.Report.Failed` follows the same rule via `Report.SetFailOn` and + `Report.FailOnSet`: an unset threshold defaults to `SeverityCritical` + instead of failing the report on informational findings. ### Added diff --git a/verify/verify.go b/verify/verify.go index 0254d00..19bfec5 100644 --- a/verify/verify.go +++ b/verify/verify.go @@ -34,15 +34,35 @@ type Report struct { CrawledURLs int `json:"crawled_urls"` Duration time.Duration `json:"duration"` FailOn contracts.Severity `json:"fail_on"` + // FailOnSet reports whether FailOn was explicitly configured via + // SetFailOn. When it is false, Failed() treats SeverityCritical as the + // effective threshold: an unset FailOn must not fail the report on + // informational findings just because SeverityInfo is the zero value. + FailOnSet bool `json:"fail_on_set,omitempty"` +} + +// SetFailOn sets the fail threshold used by Failed. Set the threshold +// through this method rather than assigning FailOn directly, so that the +// threshold is recorded as explicitly configured. +func (r *Report) SetFailOn(sev contracts.Severity) { + r.FailOn = sev + r.FailOnSet = true } // Failed reports whether any finding meets or exceeds the configured fail threshold. +// When the threshold was never set — a zero Report, or a Report whose FailOn +// field was assigned directly — SeverityCritical is used as the effective +// threshold, mirroring review.Result. func (r *Report) Failed() bool { if r == nil { return false } + threshold := r.FailOn + if !r.FailOnSet { + threshold = contracts.SeverityCritical + } for _, f := range r.Findings { - if f.Severity.AtLeast(r.FailOn) { + if f.Severity.AtLeast(threshold) { return true } } diff --git a/verify/verify_test.go b/verify/verify_test.go index a8416f2..dd8a12d 100644 --- a/verify/verify_test.go +++ b/verify/verify_test.go @@ -10,12 +10,12 @@ func TestReportFailedAndMaxSeverity(t *testing.T) { t.Parallel() report := &Report{ - FailOn: contracts.SeverityMedium, Findings: []Finding{ {Severity: contracts.SeverityLow}, {Severity: contracts.SeverityHigh}, }, } + report.SetFailOn(contracts.SeverityMedium) if !report.Failed() { t.Fatal("expected report to fail at medium threshold") @@ -25,6 +25,33 @@ func TestReportFailedAndMaxSeverity(t *testing.T) { } } +func TestReportFailedUnsetThresholdDefaultsToCritical(t *testing.T) { + t.Parallel() + + // Zero-value Report: FailOn unset must not fail on info findings. + zero := &Report{ + Findings: []Finding{ + {Severity: contracts.SeverityInfo}, + {Severity: contracts.SeverityMedium}, + }, + } + if zero.Failed() { + t.Fatal("unset FailOn should default to critical threshold; info/medium findings must not fail") + } + + // Direct field assignment is also treated as unset. + direct := &Report{FailOn: contracts.SeverityMedium, Findings: []Finding{{Severity: contracts.SeverityMedium}}} + if direct.Failed() { + t.Fatal("directly assigned FailOn is treated as unset; medium finding must not fail at effective critical threshold") + } + + // A critical finding fails even when unset. + critical := &Report{Findings: []Finding{{Severity: contracts.SeverityCritical}}} + if !critical.Failed() { + t.Fatal("critical finding must fail at the default effective threshold") + } +} + func TestNilReportMethods(t *testing.T) { t.Parallel()