fix: add strict parsers, FailOn zero-value fix, Finding validation, scorecard

.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

CHANGELOG.md

@@ -7,6 +7,40 @@ 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.
+- `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
+
+- `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
+  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
+
+- `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

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"`
@@ -42,6 +61,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.
@@ -61,22 +83,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
 		}
 	}

review/review_test.go

@@ -10,21 +10,159 @@ 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 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()
 

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