From 4b7646b47c34e6e9fd464fef6e7d2eff3563bee9 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 01:12:32 +0530 Subject: [PATCH 1/7] refactor!: remove dead audit and graph API surface The WithGraph, WithAuditMode, and WithAuditTargets options configured Reviewer fields (g, audit) that were never read outside NewReviewer, and the internal/graph and internal/audit packages had zero importers. The graph/audit .sight.toml keys fed the same dead chain via ApplyFileConfig. Remove the options, the AuditMode/AuditTarget/AuditTargetType/ AuditOption/ParseAuditMode types, both internal packages, the unused Reviewer fields, the config-file keys, and the stale docs sections. The keys were effectively no-ops, so user-visible behavior is unchanged apart from the removed API. BREAKING CHANGE: public options and types listed above are removed. --- CHANGELOG.md | 14 ++ config.go | 13 -- docs/architecture.md | 57 ----- internal/audit/audit.go | 407 -------------------------------- internal/audit/audit_test.go | 317 ------------------------- internal/graph/graph.go | 433 ----------------------------------- internal/graph/graph_test.go | 275 ---------------------- options.go | 69 ------ reviewer.go | 20 +- 9 files changed, 16 insertions(+), 1589 deletions(-) delete mode 100644 internal/audit/audit.go delete mode 100644 internal/audit/audit_test.go delete mode 100644 internal/graph/graph.go delete mode 100644 internal/graph/graph_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bb9689..92fb05f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm --- +## [Unreleased] + +### Removed +- **Dead audit/graph API surface** (breaking, pre-1.0). The + `WithGraph`, `WithAuditMode`, and `WithAuditTargets` options and the + `AuditMode`, `AuditTarget`, `AuditTargetType`, `AuditOption`, and + `ParseAuditMode` types configured `Reviewer` fields that were never + read, and the `internal/graph` and `internal/audit` packages had no + callers. Removed the options, types, packages, the unused `Reviewer` + fields, and the `graph`/`audit` `.sight.toml` keys (the keys were + silently ignored in effect; they now remain unparsed, which is the + same behavior). Consumers should use `qualitygraph` for graph + projection and `inspect` for deployed-surface auditing. + ## [0.1.2] - 2026-07-04 ### Changed diff --git a/config.go b/config.go index 58536e3..0157c5a 100644 --- a/config.go +++ b/config.go @@ -19,8 +19,6 @@ type FileConfig struct { Reflection *bool `json:"reflection"` Parallel *bool `json:"parallel"` Prompts map[string]string `json:"prompts"` - Graph *bool `json:"graph"` - Audit *string `json:"audit"` // "none", "hooks", "mcp", "full" } // LoadConfigFile reads .sight.toml from the given directory (or parents). @@ -80,12 +78,6 @@ func ApplyFileConfig(fc *FileConfig) []Option { if len(fc.Exclude) > 0 { opts = append(opts, WithExclude(fc.Exclude...)) } - if fc.Graph != nil { - opts = append(opts, WithGraph(*fc.Graph)) - } - if fc.Audit != nil { - opts = append(opts, WithAuditMode(ParseAuditMode(*fc.Audit))) - } return opts } @@ -160,11 +152,6 @@ func parseTOMLConfig(content string) (*FileConfig, error) { case "parallel": b := value == "true" cfg.Parallel = &b - case "graph": - b := value == "true" - cfg.Graph = &b - case "audit": - cfg.Audit = &value } } diff --git a/docs/architecture.md b/docs/architecture.md index 2958c35..2981f38 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -134,63 +134,6 @@ srv.ServeHTTP("127.0.0.1:8080") // 🌐 streamable HTTP transport, served at /mc --- -## 🔗 Structural Dependency Graph - -The `internal/graph` package provides **optional structural dependency analysis** for code reviews: - -**Key capabilities:** -- Parse Go AST to build structural dependency graph -- Blast-radius analysis: identify all files affected by changes -- Transitive dependency tracking -- Impact scoring based on depth and number of dependents -- SQLite persistence for incremental updates - -**Usage:** -```go -// Enable graph-backed review -g := graph.New() -// ... build graph or load from SQLite ... - -// Run blast radius analysis -result := g.GetBlastRadius([]string{"file.go"}) -fmt.Printf("Direct: %d, Transitive: %d\n", result.Direct, result.Transitive) -``` - -**Tools exposed via MCP:** `sight_graph_blastRadius`, `sight_graph_query`, `sight_graph_stats` - ---- - -## 🔒 Security Auditing - -The `internal/audit` package provides **agent surface security auditing**: - -**Key capabilities:** -- Concurrent security scanning of multiple targets -- Filter findings by severity (critical, high, medium, low) or category -- Integration with MCP hooks, permissions, and endpoints -- Webhook validation and test requests -- JSON-serializable audit findings - -**Usage:** -```go -// Create audit scope -auditScope := &audit.AuditScope{ - Targets: []audit.AuditTarget{ - {Type: audit.AuditTargetHooks, Path: "/hooks/webhook"}, - {Type: audit.AuditTargetEndpoints, Path: "/api/endpoint"}, - }, - Rules: []string{"detect_unauthenticated_endpoints", "detect_hardcoded_secrets"}, -} - -// Run audit -report, err := audit.Audit(ctx, auditScope) -if report.Count() > 0 { - fmt.Printf("Found %d security issues\n", report.Count()) -} -``` - ---- - ## 🌐 Browser Automation Tools The `internal/tool` package provides **HTTP-based browser automation** capabilities: diff --git a/internal/audit/audit.go b/internal/audit/audit.go deleted file mode 100644 index 9abcd25..0000000 --- a/internal/audit/audit.go +++ /dev/null @@ -1,407 +0,0 @@ -// Package audit provides agent surface security auditing for hawk-eco. -// It scans MCP hooks, permissions, and integration points for security issues. -package audit - -import ( - "context" - "errors" - "fmt" - "io" - "net/http" - "regexp" - "strings" - "sync" - "time" -) - -// ErrNotImplemented indicates that an audit check has not yet been implemented. -var ErrNotImplemented = errors.New("audit check not implemented") - -// AuditTargetType represents a type of audit target. -type AuditTargetType int - -const ( - // AuditTargetHooks audits MCP hooks and webhooks. - AuditTargetHooks AuditTargetType = iota - // AuditTargetMCP audits MCP server configurations. - AuditTargetMCP - // AuditTargetPermissions audits permission configurations. - AuditTargetPermissions - // AuditTargetSecrets audits secret storage and access. - AuditTargetSecrets - // AuditTargetEndpoints audits external endpoints and APIs. - AuditTargetEndpoints -) - -// AuditTarget represents a target to audit in the codebase. -type AuditTarget struct { - Type AuditTargetType - Path string - Recurse bool - Depth int -} - -// AuditFinding represents a security finding from an audit. -type AuditFinding struct { - Severity string `json:"severity"` - Category string `json:"category"` - File string `json:"file"` - Line int `json:"line"` - Description string `json:"description"` - Recommendation string `json:"recommendation"` - Evidence string `json:"evidence,omitempty"` - Source string `json:"source"` -} - -// AuditReport contains the results of an audit. -type AuditReport struct { - mu sync.Mutex - findings []AuditFinding - stats AuditStats -} - -// AuditStats contains statistics about the audit. -type AuditStats struct { - TotalTargets int - TargetsScanned int - FindingsBySeverity map[string]int - Categories map[string]int - DurationMs int64 -} - -// AuditScope defines the scope of an audit. -type AuditScope struct { - Targets []AuditTarget - Rules []string - Timeout time.Duration - MaxDepth int - Concurrency int -} - -// NewAuditReport creates a new empty audit report. -func NewAuditReport() *AuditReport { - return &AuditReport{ - findings: make([]AuditFinding, 0), - stats: AuditStats{ - FindingsBySeverity: make(map[string]int), - Categories: make(map[string]int), - }, - } -} - -// AddFinding adds a finding to the report. -func (r *AuditReport) AddFinding(f AuditFinding) { - r.mu.Lock() - defer r.mu.Unlock() - - r.findings = append(r.findings, f) - r.stats.TargetsScanned++ - - if r.stats.FindingsBySeverity[f.Severity] == 0 { - r.stats.FindingsBySeverity[f.Severity] = 0 - } - r.stats.FindingsBySeverity[f.Severity]++ - - if r.stats.Categories[f.Category] == 0 { - r.stats.Categories[f.Category] = 0 - } - r.stats.Categories[f.Category]++ -} - -// Findings returns all findings. -func (r *AuditReport) Findings() []AuditFinding { - r.mu.Lock() - defer r.mu.Unlock() - - return r.findings -} - -// Stats returns audit statistics. -func (r *AuditReport) Stats() AuditStats { - r.mu.Lock() - defer r.mu.Unlock() - - return r.stats -} - -// Count returns the total number of findings. -func (r *AuditReport) Count() int { - r.mu.Lock() - defer r.mu.Unlock() - - return len(r.findings) -} - -// FilterBySeverity filters findings by severity. -func (r *AuditReport) FilterBySeverity(severity string) []AuditFinding { - r.mu.Lock() - defer r.mu.Unlock() - - var result []AuditFinding - for _, f := range r.findings { - if f.Severity == severity { - result = append(result, f) - } - } - return result -} - -// FilterByCategory filters findings by category. -func (r *AuditReport) FilterByCategory(category string) []AuditFinding { - r.mu.Lock() - defer r.mu.Unlock() - - var result []AuditFinding - for _, f := range r.findings { - if f.Category == category { - result = append(result, f) - } - } - return result -} - -// Summary returns a summary of the report. -func (r *AuditReport) Summary() string { - r.mu.Lock() - defer r.mu.Unlock() - - count := len(r.findings) - if count == 0 { - return "No security findings detected." - } - - var sb strings.Builder - sb.WriteString(fmt.Sprintf("Security audit complete: %d findings detected\n", count)) - sb.WriteString(fmt.Sprintf(" - Critical: %d\n", r.stats.FindingsBySeverity["critical"])) - sb.WriteString(fmt.Sprintf(" - High: %d\n", r.stats.FindingsBySeverity["high"])) - sb.WriteString(fmt.Sprintf(" - Medium: %d\n", r.stats.FindingsBySeverity["medium"])) - sb.WriteString(fmt.Sprintf(" - Low: %d\n", r.stats.FindingsBySeverity["low"])) - sb.WriteString("\nCategories:\n") - for cat, cnt := range r.stats.Categories { - sb.WriteString(fmt.Sprintf(" - %s: %d\n", cat, cnt)) - } - - return sb.String() -} - -// Audit performs a security audit on the given targets. -// It returns a report with all findings. -func Audit(ctx context.Context, scope *AuditScope) (*AuditReport, error) { - if scope == nil { - return nil, fmt.Errorf("scope cannot be nil") - } - - report := NewAuditReport() - report.stats.TotalTargets = len(scope.Targets) - - // Apply default rules if none specified - if len(scope.Rules) == 0 { - scope.Rules = DefaultRules() - } - - // Run concurrent audits - var wg sync.WaitGroup - sem := make(chan struct{}, scope.Concurrency) - - for _, target := range scope.Targets { - wg.Add(1) - go func(t AuditTarget) { - defer wg.Done() - - sem <- struct{}{} // Acquire - defer func() { <-sem }() // Release - - if err := auditTarget(ctx, t, scope, report); err != nil { - report.AddFinding(AuditFinding{ - Severity: "medium", - Category: "audit_error", - File: t.Path, - Description: fmt.Sprintf("Audit failed: %v", err), - Source: "audit", - }) - } - }(target) - - // Respect timeout - select { - case <-ctx.Done(): - report.AddFinding(AuditFinding{ - Severity: "medium", - Category: "timeout", - Description: "Audit timed out", - Source: "audit", - }) - return report, ctx.Err() - default: - } - } - - wg.Wait() - return report, nil -} - -// auditTarget performs audit on a single target. -func auditTarget(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { - switch target.Type { - case AuditTargetHooks: - return auditHooks(ctx, target, scope, report) - case AuditTargetMCP: - return auditMCPServer(ctx, target, scope, report) - case AuditTargetPermissions: - return auditPermissions(ctx, target, scope, report) - case AuditTargetSecrets: - return auditSecrets(ctx, target, scope, report) - case AuditTargetEndpoints: - return auditEndpoints(ctx, target, scope, report) - default: - return fmt.Errorf("unknown audit target type: %d", target.Type) - } -} - -// DefaultRules returns the default security audit rules. -func DefaultRules() []string { - return []string{ - "detect_unauthenticated_endpoints", - "detect_hardcoded_secrets", - "detect_missing_input_validation", - "detect_excessive_permissions", - "detect_insecure_mcp_config", - "detect_webhook_validation", - } -} - -// auditHooks performs audit on MCP hooks. -func auditHooks(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { - // Placeholder: implement hook auditing - return ErrNotImplemented -} - -// auditMCPServer performs audit on MCP server configurations. -func auditMCPServer(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { - // Placeholder: implement MCP config auditing - return ErrNotImplemented -} - -// auditPermissions performs audit on permission configurations. -func auditPermissions(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { - // Placeholder: implement permission auditing - return ErrNotImplemented -} - -// auditSecrets performs audit on secret storage and access. -func auditSecrets(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { - // Placeholder: implement secret auditing - return ErrNotImplemented -} - -// auditEndpoints performs audit on external endpoints and APIs. -func auditEndpoints(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { - // Placeholder: implement endpoint auditing - return ErrNotImplemented -} - -// ValidateRule checks if a rule is valid. -func ValidateRule(rule string) bool { - validRules := map[string]bool{ - "detect_unauthenticated_endpoints": true, - "detect_hardcoded_secrets": true, - "detect_missing_input_validation": true, - "detect_excessive_permissions": true, - "detect_insecure_mcp_config": true, - "detect_webhook_validation": true, - } - return validRules[rule] -} - -// Category represents common vulnerability categories. -type Category struct { - Name string - Description string - Severity string -} - -// Common categories -var ( - CriticalCategories = []Category{ - {Name: "RCE", Description: "Remote Code Execution", Severity: "critical"}, - {Name: "SQL Injection", Description: "SQL Injection vulnerability", Severity: "critical"}, - {Name: "Auth Bypass", Description: "Authentication bypass", Severity: "critical"}, - } - - HighCategories = []Category{ - {Name: "SSRF", Description: "Server-Side Request Forgery", Severity: "high"}, - {Name: "Path Traversal", Description: "Path traversal vulnerability", Severity: "high"}, - {Name: "Command Injection", Description: "OS command injection", Severity: "high"}, - } - - MediumCategories = []Category{ - {Name: "XSS", Description: "Cross-Site Scripting", Severity: "medium"}, - {Name: "IDOR", Description: "Insecure Direct Object Reference", Severity: "medium"}, - {Name: "Missing Encryption", Description: "Missing TLS/encryption", Severity: "medium"}, - } - - LowCategories = []Category{ - {Name: "Info Exposure", Description: "Information exposure", Severity: "low"}, - {Name: "Missing Headers", Description: "Missing security headers", Severity: "low"}, - {Name: "Verbose Errors", Description: "Verbose error messages", Severity: "low"}, - } -) - -// Common patterns for security detection -var ( - SecretPatterns = []*regexp.Regexp{ - regexp.MustCompile(`(?i)(password|passwd|pwd)\s*[:=]\s*["'][^"']{8,}`), - regexp.MustCompile(`(?i)api[_-]?key\s*[:=]\s*["'][^"']{16,}`), - regexp.MustCompile(`(?i)secret[_-]?key\s*[:=]\s*["'][^"']{16,}`), - regexp.MustCompile(`(?i)token\s*[:=]\s*["'][^"']{20,}`), - regexp.MustCompile(`(?i)aws[_-]?access[_-]?key[_-]?id\s*[:=]\s*["'][A-Z0-9]{16}`), - regexp.MustCompile(`(?i)hardcoded.*secret`), - regexp.MustCompile(`(?i)(sk|pk)_[a-zA-Z0-9]{20,}`), - } - - AuthBypassPatterns = []*regexp.Regexp{ - regexp.MustCompile(`(?i)(bypass|disable|skip|ignore|no.*auth|without.*auth)`), - regexp.MustCompile(`(?i)(admin.*true|superuser|root.*access)`), - } -) - -// HTTPClient provides HTTP access for auditing endpoints. -type HTTPClient struct { - client *http.Client -} - -// NewHTTPClient creates a new HTTP client. -func NewHTTPClient(timeout time.Duration) *HTTPClient { - return &HTTPClient{ - client: &http.Client{ - Timeout: timeout, - }, - } -} - -// Get performs a GET request to an endpoint. -func (h *HTTPClient) Get(ctx context.Context, url string) (*HTTPResponse, error) { - req, err := http.NewRequestWithContext(ctx, "GET", url, nil) - if err != nil { - return nil, err - } - - resp, err := h.client.Do(req) - if err != nil { - return nil, err - } - - bodyBytes, err := io.ReadAll(resp.Body) - _ = resp.Body.Close() - if err != nil { - return nil, err - } - - return &HTTPResponse{StatusCode: resp.StatusCode, Body: bodyBytes}, nil -} - -// HTTPResponse represents an HTTP response. -type HTTPResponse struct { - StatusCode int - Body []byte -} diff --git a/internal/audit/audit_test.go b/internal/audit/audit_test.go deleted file mode 100644 index a6f8161..0000000 --- a/internal/audit/audit_test.go +++ /dev/null @@ -1,317 +0,0 @@ -package audit - -import ( - "encoding/json" - "errors" - "sync" - "testing" -) - -// Test basic audit report operations - -func TestNewAuditReport(t *testing.T) { - report := NewAuditReport() - if report == nil { - t.Fatal("expected non-nil report") - } - if report.Count() != 0 { - t.Errorf("expected 0 findings, got %d", report.Count()) - } -} - -func TestAddFinding(t *testing.T) { - report := NewAuditReport() - - finding := AuditFinding{ - Severity: "high", - Category: "injection", - File: "main.go", - Line: 10, - Description: "SQL injection detected", - Recommendation: "Use parameterized queries", - Source: "static_analysis", - } - - report.AddFinding(finding) - - if report.Count() != 1 { - t.Errorf("expected 1 finding, got %d", report.Count()) - } - - findings := report.Findings() - if len(findings) != 1 { - t.Errorf("expected 1 finding, got %d", len(findings)) - } - if findings[0].Severity != "high" { - t.Errorf("expected severity high, got %s", findings[0].Severity) - } -} - -func TestAddMultipleFindings(t *testing.T) { - report := NewAuditReport() - - findings := []AuditFinding{ - {Severity: "critical", Category: "rce", Description: "Remote code execution"}, - {Severity: "high", Category: "injection", Description: "SQL injection"}, - {Severity: "medium", Category: "xss", Description: "Cross-site scripting"}, - {Severity: "low", Category: "info", Description: "Information exposure"}, - } - - for _, f := range findings { - report.AddFinding(f) - } - - if report.Count() != 4 { - t.Errorf("expected 4 findings, got %d", report.Count()) - } - - // Check severity counts - critical := report.FilterBySeverity("critical") - if len(critical) != 1 { - t.Errorf("expected 1 critical finding, got %d", len(critical)) - } - - high := report.FilterBySeverity("high") - if len(high) != 1 { - t.Errorf("expected 1 high finding, got %d", len(high)) - } - - medium := report.FilterBySeverity("medium") - if len(medium) != 1 { - t.Errorf("expected 1 medium finding, got %d", len(medium)) - } - - low := report.FilterBySeverity("low") - if len(low) != 1 { - t.Errorf("expected 1 low finding, got %d", len(low)) - } -} - -// Test filtering operations - -func TestFilterByCategory(t *testing.T) { - report := NewAuditReport() - - findings := []AuditFinding{ - {Severity: "high", Category: "injection", Description: "SQL injection"}, - {Severity: "high", Category: "injection", Description: "Another injection"}, - {Severity: "medium", Category: "xss", Description: "XSS found"}, - {Severity: "low", Category: "info", Description: "Info exposure"}, - } - - for _, f := range findings { - report.AddFinding(f) - } - - injections := report.FilterByCategory("injection") - if len(injections) != 2 { - t.Errorf("expected 2 injection findings, got %d", len(injections)) - } - - xss := report.FilterByCategory("xss") - if len(xss) != 1 { - t.Errorf("expected 1 xss finding, got %d", len(xss)) - } - - info := report.FilterByCategory("info") - if len(info) != 1 { - t.Errorf("expected 1 info finding, got %d", len(info)) - } -} - -// Test summary generation - -func TestSummary(t *testing.T) { - report := NewAuditReport() - - findings := []AuditFinding{ - {Severity: "critical", Category: "rce", Description: "Remote code execution"}, - {Severity: "critical", Category: "rce", Description: "Another RCE"}, - {Severity: "high", Category: "injection", Description: "SQL injection"}, - {Severity: "medium", Category: "xss", Description: "XSS found"}, - } - - for _, f := range findings { - report.AddFinding(f) - } - - summary := report.Summary() - if summary == "" { - t.Error("expected non-empty summary") - } - - // Check that summary contains expected information - if summary == "No security findings detected." { - t.Error("expected findings in summary") - } -} - -// Test JSON serialization - -func TestAuditFindingJSON(t *testing.T) { - finding := AuditFinding{ - Severity: "high", - Category: "injection", - File: "main.go", - Line: 10, - Description: "SQL injection detected", - Recommendation: "Use parameterized queries", - Source: "static_analysis", - } - - data, err := json.Marshal(finding) - if err != nil { - t.Fatalf("failed to marshal finding: %v", err) - } - - var decoded AuditFinding - err = json.Unmarshal(data, &decoded) - if err != nil { - t.Fatalf("failed to unmarshal finding: %v", err) - } - - if decoded.Severity != finding.Severity { - t.Errorf("expected severity %s, got %s", finding.Severity, decoded.Severity) - } - if decoded.Category != finding.Category { - t.Errorf("expected category %s, got %s", finding.Category, decoded.Category) - } - if decoded.File != finding.File { - t.Errorf("expected file %s, got %s", finding.File, decoded.File) - } - if decoded.Source != finding.Source { - t.Errorf("expected source %s, got %s", finding.Source, decoded.Source) - } -} - -// Test AuditScope - -func TestAuditScope(t *testing.T) { - scope := &AuditScope{ - Targets: []AuditTarget{ - {Type: AuditTargetHooks, Path: "/hooks/webhook"}, - {Type: AuditTargetMCP, Path: "/mcp/server"}, - {Type: AuditTargetEndpoints, Path: "/api/endpoint"}, - }, - Rules: []string{ - "detect_unauthenticated_endpoints", - "detect_hardcoded_secrets", - }, - } - - if len(scope.Targets) != 3 { - t.Errorf("expected 3 targets, got %d", len(scope.Targets)) - } - if len(scope.Rules) != 2 { - t.Errorf("expected 2 rules, got %d", len(scope.Rules)) - } -} - -// Test ValidateRule - -func TestValidateRule(t *testing.T) { - tests := []struct { - rule string - expected bool - }{ - {"detect_unauthenticated_endpoints", true}, - {"detect_hardcoded_secrets", true}, - {"detect_missing_input_validation", true}, - {"detect_excessive_permissions", true}, - {"detect_insecure_mcp_config", true}, - {"detect_webhook_validation", true}, - {"invalid_rule", false}, - {"", false}, - } - - for _, tt := range tests { - got := ValidateRule(tt.rule) - if got != tt.expected { - t.Errorf("ValidateRule(%q) = %v, want %v", tt.rule, got, tt.expected) - } - } -} - -// Test helper functions - -func TestMustJSONMarshal(t *testing.T) { - // Test with valid data - the function exists but is unexported - // We just test the JSON marshaling works - data := map[string]string{"key": "value"} - result, err := json.Marshal(data) - if err != nil { - t.Fatalf("failed to marshal: %v", err) - } - if string(result) == "" { - t.Error("expected non-empty JSON") - } -} - -// Test ParseRules - -func TestDefaultRules(t *testing.T) { - rules := DefaultRules() - if len(rules) == 0 { - t.Error("expected non-empty default rules") - } - - // Check expected rules are present - expectedRules := map[string]bool{ - "detect_unauthenticated_endpoints": true, - "detect_hardcoded_secrets": true, - "detect_missing_input_validation": true, - "detect_excessive_permissions": true, - "detect_insecure_mcp_config": true, - "detect_webhook_validation": true, - } - - for _, rule := range rules { - if !expectedRules[rule] { - t.Errorf("unexpected rule: %s", rule) - } - } -} - -// Test that auditTarget correctly dispatches - -func TestAuditTargetDispatch(t *testing.T) { - // AuditTargetDispatch is tested indirectly through other tests -} - -// Test concurrent additions - -func TestConcurrentAddFinding(t *testing.T) { - report := NewAuditReport() - - err := errors.New("test error") - var wg sync.WaitGroup - - // Add findings concurrently - for i := 0; i < 100; i++ { - wg.Add(1) - go func(sev string) { - defer wg.Done() - if sev == "error" { - report.AddFinding(AuditFinding{ - Severity: "high", - Category: "error", - Description: err.Error(), - Source: "test", - }) - } else { - report.AddFinding(AuditFinding{ - Severity: sev, - Category: "test", - Description: "Test finding", - Source: "test", - }) - } - }([]string{"high", "medium", "low"}[i%3]) - } - - wg.Wait() - - if report.Count() != 100 { - t.Errorf("expected 100 findings after concurrent add, got %d", report.Count()) - } -} diff --git a/internal/graph/graph.go b/internal/graph/graph.go deleted file mode 100644 index d75b3c6..0000000 --- a/internal/graph/graph.go +++ /dev/null @@ -1,433 +0,0 @@ -// Package graph provides structural dependency analysis for code reviews. -// It builds and queries a graph of code units to identify blast-radius -// impacts and minimal review sets. -package graph - -import ( - "context" - "fmt" - "go/ast" - "go/parser" - "go/token" - "path/filepath" - "sort" - "sync" -) - -// Node represents a code unit in the dependency graph. -type Node struct { - URI string - Name string - Type NodeType -} - -// NodeType represents the type of node. -type NodeType int - -const ( - NodeTypeFunction NodeType = iota - NodeTypeMethod - NodeTypeType - NodeTypeStruct - NodeTypeInterface - NodeTypePackage - NodeTypeFile -) - -// String returns a string representation of the node type. -func (n NodeType) String() string { - switch n { - case NodeTypeFunction: - return "function" - case NodeTypeMethod: - return "method" - case NodeTypeType: - return "type" - case NodeTypeStruct: - return "struct" - case NodeTypeInterface: - return "interface" - case NodeTypePackage: - return "package" - case NodeTypeFile: - return "file" - default: - return "unknown" - } -} - -// EdgeType represents the type of dependency between nodes. -type EdgeType int - -const ( - EdgeCalls EdgeType = iota - EdgeReturns - EdgeParameter - EdgeReceiver - EdgeImport -) - -// String returns a string representation of the edge type. -func (e EdgeType) String() string { - switch e { - case EdgeCalls: - return "calls" - default: - return "unknown" - } -} - -// BlastRadiusResult represents the results of a blast-radius analysis. -type BlastRadiusResult struct { - Files []string `json:"files"` - Direct int `json:"direct"` - Transitive int `json:"transitive"` - MaxDepth int `json:"max_depth"` - ImpactScore float64 `json:"impact_score"` -} - -// Score computes an overall impact score. -func (b *BlastRadiusResult) Score() float64 { - if len(b.Files) == 0 { - return 0 - } - score := 0.0 - for i := range b.Files { - if i < b.Direct { - score += 1.0 - } else { - depth := b.MaxDepth - int(float64(b.MaxDepth)*(float64(i)/float64(len(b.Files)))) - switch depth { - case 2: - score += 0.8 - case 3: - score += 0.6 - case 4: - score += 0.4 - default: - score += 0.2 - } - } - } - return score / float64(len(b.Files)) -} - -// DependencyGraph represents structural dependencies between code units. -type DependencyGraph struct { - mu sync.RWMutex - nodes map[string]*Node - edges map[string][]string -} - -// New creates a new DependencyGraph. -func New() *DependencyGraph { - return &DependencyGraph{ - nodes: make(map[string]*Node), - edges: make(map[string][]string), - } -} - -// AddNode adds a node to the graph. -func (g *DependencyGraph) AddNode(node *Node) { - g.mu.Lock() - defer g.mu.Unlock() - g.nodes[node.URI] = node -} - -// AddEdge adds a dependency edge between two nodes. -func (g *DependencyGraph) AddEdge(from, to string, _ EdgeType) { - g.mu.Lock() - defer g.mu.Unlock() - - if g.edges[from] == nil { - g.edges[from] = []string{} - } - for _, target := range g.edges[from] { - if target == to { - return - } - } - g.edges[from] = append(g.edges[from], to) - - if g.edges[to] == nil { - g.edges[to] = []string{} - } -} - -// GetNode returns a node by URI. -func (g *DependencyGraph) GetNode(uri string) *Node { - g.mu.RLock() - defer g.mu.RUnlock() - return g.nodes[uri] -} - -// GetDirectDependents returns direct dependents (outgoing edges). -func (g *DependencyGraph) GetDirectDependents(uri string) []string { - g.mu.RLock() - defer g.mu.RUnlock() - return g.edges[uri] -} - -// MaxTraversalDepth is the maximum depth for graph traversal. -const MaxTraversalDepth = 64 - -// GetAllDependents returns all transitive dependents using iterative BFS. -func (g *DependencyGraph) GetAllDependents(uri string) []string { - g.mu.RLock() - defer g.mu.RUnlock() - - visited := make(map[string]bool) - visited[uri] = true - var result []string - - // Iterative BFS with explicit queue and depth tracking. - type entry struct { - node string - depth int - } - queue := []entry{{node: uri, depth: 0}} - head := 0 - - for head < len(queue) { - cur := queue[head] - head++ - - if cur.depth >= MaxTraversalDepth { - continue - } - - for _, child := range g.edges[cur.node] { - if visited[child] { - continue - } - visited[child] = true - result = append(result, child) - queue = append(queue, entry{node: child, depth: cur.depth + 1}) - } - } - return result -} - -// Build parses a Go module and builds the dependency graph. -func (g *DependencyGraph) Build(ctx context.Context, modulePath string) error { - fset := token.NewFileSet() - packages, err := parser.ParseDir(fset, modulePath, nil, parser.ParseComments) - if err != nil { - return fmt.Errorf("failed to parse module %s: %w", modulePath, err) - } - - var wg sync.WaitGroup - sem := make(chan struct{}, 50) - - for pkgName, pkg := range packages { - pkgURI := fmt.Sprintf("pkg://%s", pkgName) - g.AddNode(&Node{URI: pkgURI, Name: pkgName, Type: NodeTypePackage}) - - for filename, f := range pkg.Files { - fileURI := fmt.Sprintf("file://%s", filename) - g.AddNode(&Node{URI: fileURI, Name: filepath.Base(filename), Type: NodeTypeFile}) - g.AddEdge(pkgURI, fileURI, EdgeCalls) - - for _, decl := range f.Decls { - wg.Add(1) - sem <- struct{}{} - go func(d ast.Decl, fURI string) { - defer func() { <-sem }() - defer wg.Done() - g.processDecl(d, fURI) - }(decl, fileURI) - } - } - } - - wg.Wait() - close(sem) - return nil -} - -// processDecl processes an AST declaration. -func (g *DependencyGraph) processDecl(decl ast.Decl, fileURI string) { - switch d := decl.(type) { - case *ast.FuncDecl: - g.processFuncDecl(d, fileURI) - case *ast.GenDecl: - for _, spec := range d.Specs { - if ts, ok := spec.(*ast.TypeSpec); ok { - g.processTypeSpec(ts, fileURI) - } - } - } -} - -// processFuncDecl processes a function declaration. -func (g *DependencyGraph) processFuncDecl(d *ast.FuncDecl, fileURI string) { - funcName := d.Name.Name - methodName := funcName - typName := "" - - if d.Recv != nil { - for _, r := range d.Recv.List { - recvType := r.Type - if star, ok := recvType.(*ast.StarExpr); ok { - recvType = star.X - } - if ident, ok := recvType.(*ast.Ident); ok { - typName = ident.Name - } - } - methodName = fmt.Sprintf("%s.%s", typName, funcName) - } - - methodURI := fmt.Sprintf("method://%s/%s", fileURI, methodName) - g.AddNode(&Node{URI: methodURI, Name: methodName, Type: NodeTypeMethod}) - g.AddEdge(fileURI, methodURI, EdgeCalls) - - if d.Body != nil { - g.extractCalls(d.Body, methodURI) - } - g.extractReturns(d.Type, methodURI, methodName) -} - -// processTypeSpec processes a type specification. -func (g *DependencyGraph) processTypeSpec(ts *ast.TypeSpec, fileURI string) { - typeName := ts.Name.Name - typeURI := fmt.Sprintf("type://%s/%s", fileURI, typeName) - - switch ts.Type.(type) { - case *ast.StructType: - g.AddNode(&Node{URI: typeURI, Name: typeName, Type: NodeTypeStruct}) - case *ast.InterfaceType: - g.AddNode(&Node{URI: typeURI, Name: typeName, Type: NodeTypeInterface}) - default: - g.AddNode(&Node{URI: typeURI, Name: typeName, Type: NodeTypeType}) - } - - g.AddEdge(fileURI, typeURI, EdgeCalls) - - if structType, ok := ts.Type.(*ast.StructType); ok { - for _, field := range structType.Fields.List { - if field.Names != nil { - for _, name := range field.Names { - embeddedURI := fmt.Sprintf("type://%s/%s", fileURI, name.Name) - g.AddNode(&Node{URI: embeddedURI, Name: name.Name, Type: NodeTypeType}) - g.AddEdge(typeURI, embeddedURI, EdgeCalls) - } - } else if ident, ok := field.Type.(*ast.Ident); ok { - embeddedURI := fmt.Sprintf("type://%s/%s", fileURI, ident.Name) - g.AddNode(&Node{URI: embeddedURI, Name: ident.Name, Type: NodeTypeType}) - g.AddEdge(typeURI, embeddedURI, EdgeCalls) - } - } - } -} - -// extractCalls extracts function/method calls from a body. -func (g *DependencyGraph) extractCalls(body *ast.BlockStmt, fromURI string) { - ast.Inspect(body, func(n ast.Node) bool { - call, ok := n.(*ast.CallExpr) - if !ok { - return true - } - - callName := "" - callType := NodeTypeFunction - - if ident, ok := call.Fun.(*ast.Ident); ok { - callName = ident.Name - } else if sel, ok := call.Fun.(*ast.SelectorExpr); ok { - callName = sel.Sel.Name - callType = NodeTypeMethod - } - - if callName == "" { - return true - } - - callURI := fmt.Sprintf("func://%s/%s", fromURI, callName) - g.AddNode(&Node{URI: callURI, Name: callName, Type: callType}) - g.AddEdge(fromURI, callURI, EdgeCalls) - return true - }) -} - -// extractReturns extracts return value dependencies. -func (g *DependencyGraph) extractReturns(t *ast.FuncType, fromURI, methodName string) { - if t == nil || t.Results == nil { - return - } - - for _, field := range t.Results.List { - for _, name := range field.Names { - returnURI := fmt.Sprintf("return://%s/%s", fromURI, name.Name) - g.AddNode(&Node{URI: returnURI, Name: name.Name, Type: NodeTypeFunction}) - g.AddEdge(fromURI, returnURI, EdgeCalls) - } - } -} - -// GetBlastRadius returns files affected by changing the given files. -func (g *DependencyGraph) GetBlastRadius(files []string) *BlastRadiusResult { - g.mu.RLock() - defer g.mu.RUnlock() - - result := &BlastRadiusResult{ - Files: make([]string, 0, len(files)*10), - Direct: len(files), - Transitive: 0, - MaxDepth: 0, - } - - fileScores := make(map[string]float64) - - for _, file := range files { - if g.nodes[file] == nil { - continue - } - - directDeps := g.edges[file] - result.Transitive += len(directDeps) - - if len(directDeps) > 0 { - result.MaxDepth = max(result.MaxDepth, 2) - } - - for _, dep := range directDeps { - if g.nodes[dep] == nil { - continue - } - result.Files = append(result.Files, dep) - fileScores[dep] += 0.8 - - for _, transDep := range g.edges[dep] { - if g.nodes[transDep] == nil { - continue - } - fileScores[transDep] += 0.6 - result.MaxDepth = max(result.MaxDepth, 3) - - for _, grand := range g.edges[transDep] { - if g.nodes[grand] == nil { - continue - } - fileScores[grand] += 0.4 - result.MaxDepth = max(result.MaxDepth, 4) - } - } - } - } - - for _, file := range files { - fileScores[file] = 1.0 - } - result.Files = append(result.Files, files...) - - sortByScore(result.Files, fileScores) - result.ImpactScore = result.Score() - return result -} - -func sortByScore(strs []string, scores map[string]float64) { - sort.Slice(strs, func(i, j int) bool { - return scores[strs[i]] > scores[strs[j]] - }) -} diff --git a/internal/graph/graph_test.go b/internal/graph/graph_test.go deleted file mode 100644 index d83115f..0000000 --- a/internal/graph/graph_test.go +++ /dev/null @@ -1,275 +0,0 @@ -package graph - -import ( - "context" - "fmt" - "os" - "path/filepath" - "testing" -) - -func TestNew(t *testing.T) { - g := New() - if g == nil { - t.Fatal("expected non-nil graph") - } - if len(g.nodes) != 0 { - t.Errorf("expected 0 nodes, got %d", len(g.nodes)) - } -} - -func TestAddNode(t *testing.T) { - g := New() - - node := &Node{ - URI: "file:///path/to/main.go", - Name: "main.go", - Type: NodeTypeFile, - } - - g.AddNode(node) - - if len(g.nodes) != 1 { - t.Errorf("expected 1 node, got %d", len(g.nodes)) - } - - got := g.nodes["file:///path/to/main.go"] - if got != node { - t.Error("expected same node reference") - } -} - -func TestAddEdge(t *testing.T) { - g := New() - - g.AddEdge("file:///a.go", "type:///a.go/MyType", EdgeCalls) - - if len(g.edges) != 2 { - t.Errorf("expected 2 edge entries, got %d", len(g.edges)) - } - - edges := g.edges["file:///a.go"] - if len(edges) != 1 { - t.Errorf("expected 1 edge target, got %d", len(edges)) - } - if edges[0] != "type:///a.go/MyType" { - t.Errorf("expected edge target to be type:///a.go/MyType, got %s", edges[0]) - } -} - -func TestGetNode(t *testing.T) { - g := New() - - node := &Node{ - URI: "file:///main.go", - Name: "main.go", - Type: NodeTypeFile, - } - g.AddNode(node) - - got := g.GetNode("file:///main.go") - if got != node { - t.Error("expected same node reference") - } - - got = g.GetNode("file:///nonexistent.go") - if got != nil { - t.Error("expected nil for non-existent node") - } -} - -func TestGetBlastRadius(t *testing.T) { - g := New() - - // Setup: main.go -> helper.go -> db.go - g.AddEdge("file:///main.go", "file:///helper.go", EdgeCalls) - g.AddEdge("file:///helper.go", "file:///db.go", EdgeCalls) - - result := g.GetBlastRadius([]string{"file:///main.go"}) - - if len(result.Files) == 0 { - t.Fatal("expected at least main.go in blast radius") - } - - t.Logf("Blast radius: %d files, score %.2f", len(result.Files), result.Score()) -} - -func TestGetDirectDependents(t *testing.T) { - g := New() - - // a.go -> b.go, c.go - g.AddEdge("file:///a.go", "file:///b.go", EdgeCalls) - g.AddEdge("file:///a.go", "file:///c.go", EdgeCalls) - - dependents := g.GetDirectDependents("file:///a.go") - if len(dependents) != 2 { - t.Errorf("expected 2 direct dependents, got %d: %v", len(dependents), dependents) - } -} - -func TestGetAllDependents(t *testing.T) { - g := New() - - // a.go -> b.go -> c.go - g.AddEdge("file:///a.go", "file:///b.go", EdgeCalls) - g.AddEdge("file:///b.go", "file:///c.go", EdgeCalls) - - dependents := g.GetAllDependents("file:///b.go") - if len(dependents) != 1 { - t.Errorf("expected 1 dependent, got %d: %v", len(dependents), dependents) - } - if dependents[0] != "file:///c.go" { - t.Errorf("expected c.go, got %s", dependents[0]) - } -} - -func TestNodeTypeString(t *testing.T) { - tests := []struct { - nodeType NodeType - expected string - }{ - {NodeTypeFile, "file"}, - {NodeTypePackage, "package"}, - {NodeTypeFunction, "function"}, - {NodeTypeMethod, "method"}, - {NodeTypeType, "type"}, - {NodeTypeStruct, "struct"}, - {NodeTypeInterface, "interface"}, - {NodeType(99), "unknown"}, - } - - for _, tt := range tests { - got := tt.nodeType.String() - if got != tt.expected { - t.Errorf("NodeType(%d).String() = %q, want %q", tt.nodeType, got, tt.expected) - } - } -} - -func TestEdgeTypeString(t *testing.T) { - tests := []struct { - edgeType EdgeType - expected string - }{ - {EdgeCalls, "calls"}, - {EdgeReturns, "unknown"}, - {EdgeType(99), "unknown"}, - } - for _, tt := range tests { - if got := tt.edgeType.String(); got != tt.expected { - t.Errorf("EdgeType(%d).String() = %q, want %q", tt.edgeType, got, tt.expected) - } - } -} - -func TestBlastRadiusResultScore(t *testing.T) { - empty := &BlastRadiusResult{} - if got := empty.Score(); got != 0 { - t.Errorf("Score() on empty result = %v, want 0", got) - } - - r := &BlastRadiusResult{ - Files: []string{"a", "b", "c", "d"}, - Direct: 1, - MaxDepth: 4, - } - if got := r.Score(); got <= 0 { - t.Errorf("Score() = %v, want > 0", got) - } -} - -func TestBuild(t *testing.T) { - dir := t.TempDir() - - src := `package example - -type Greeter struct { - Name string -} - -type Speaker interface { - Speak() string -} - -func (g *Greeter) Speak() string { - return greet(g.Name) -} - -func greet(name string) string { - return "hello " + name -} - -func compute() (result int) { - result = 1 - return -} -` - if err := os.WriteFile(filepath.Join(dir, "example.go"), []byte(src), 0o600); err != nil { - t.Fatalf("WriteFile: %v", err) - } - - g := New() - if err := g.Build(context.Background(), dir); err != nil { - t.Fatalf("Build returned err: %v", err) - } - - foundStruct, foundInterface, foundMethod, foundFunc := false, false, false, false - for _, n := range g.nodes { - switch { - case n.Type == NodeTypeStruct && n.Name == "Greeter": - foundStruct = true - case n.Type == NodeTypeInterface && n.Name == "Speaker": - foundInterface = true - case n.Type == NodeTypeMethod && n.Name == "Greeter.Speak": - foundMethod = true - case n.Name == "greet": - foundFunc = true - } - } - if !foundStruct { - t.Error("Build did not register the Greeter struct") - } - if !foundInterface { - t.Error("Build did not register the Speaker interface") - } - if !foundMethod { - t.Error("Build did not register the Greeter.Speak method") - } - if !foundFunc { - t.Error("Build did not register a call to greet") - } -} - -func TestBuildInvalidDir(t *testing.T) { - g := New() - if err := g.Build(context.Background(), filepath.Join(t.TempDir(), "does-not-exist")); err == nil { - t.Fatal("Build on a non-existent directory should return an error") - } -} - -func BenchmarkGetBlastRadius(b *testing.B) { - for i := 0; i < b.N; i++ { - g := New() - - for j := 0; j < 100; j++ { - fileURI := fmt.Sprintf("file:///pkg%d/file.go", j) - g.AddNode(&Node{URI: fileURI, Name: fmt.Sprintf("file%d.go", j), Type: NodeTypeFile}) - for k := 0; k < 10; k++ { - depURI := fmt.Sprintf("file:///pkg%d/dep%d.go", j, k) - g.AddNode(&Node{URI: depURI, Name: fmt.Sprintf("dep%d.go", k), Type: NodeTypeFile}) - g.AddEdge(fileURI, depURI, EdgeCalls) - } - } - - files := make([]string, 100) - for j := range files { - files[j] = fmt.Sprintf("file:///pkg%d/file.go", j) - } - - result := g.GetBlastRadius(files) - if result == nil { - b.Fatal("expected non-nil result") - } - _ = g // avoid unused variable - } -} diff --git a/options.go b/options.go index 9b47094..c972895 100644 --- a/options.go +++ b/options.go @@ -43,9 +43,6 @@ type config struct { exclude []string minScore int projectRules string - graphEnabled bool - auditMode AuditMode - auditTargets []AuditTarget } // defaultExclude is the default set of file patterns excluded from review. @@ -119,59 +116,12 @@ var CI Option = optFunc(func(c *config) { c.failOn = SeverityHigh }) -// AuditMode represents the audit mode for code review. -type AuditMode int - -const ( - // AuditModeNone disables security audit. - AuditModeNone AuditMode = iota - // AuditModeHooks audits hooks only. - AuditModeHooks - // AuditModeMCP audits MCP servers only. - AuditModeMCP - // AuditModeFull performs comprehensive audit. - AuditModeFull -) - -// AuditTargetType represents a type of audit target. -type AuditTargetType int - -const ( - AuditTargetHooks AuditTargetType = iota - AuditTargetMCP - AuditTargetPermissions - AuditTargetSecrets -) - -// AuditTarget represents a target to audit in the codebase. -type AuditTarget struct { - Type AuditTargetType - Path string - Recurse bool -} - -// AuditOption configures security audit options. -type AuditOption struct { - Mode AuditMode - Targets []AuditTarget -} - // Configuration functions func WithProvider(p Provider) Option { return optFunc(func(c *config) { c.provider = p }) } -// WithAuditTargets specifies audit targets for security auditing. -func WithAuditTargets(targets ...AuditTarget) Option { - return optFunc(func(c *config) { c.auditTargets = targets }) -} - -// WithAuditMode sets the audit mode. -func WithAuditMode(mode AuditMode) Option { - return optFunc(func(c *config) { c.auditMode = mode }) -} - func WithModel(model string) Option { return optFunc(func(c *config) { c.model = model }) } @@ -251,22 +201,3 @@ func WithProjectRules(rules string) Option { func WithFilterMode(mode FilterMode) Option { return optFunc(func(c *config) { c.filterMode = mode }) } - -// WithGraph enables structural dependency graph for blast-radius analysis. -func WithGraph(enabled bool) Option { - return optFunc(func(c *config) { c.graphEnabled = enabled }) -} - -// ParseAuditMode converts a string audit mode to AuditMode. -func ParseAuditMode(s string) AuditMode { - switch s { - case "full": - return AuditModeFull - case "mcp": - return AuditModeMCP - case "hooks": - return AuditModeHooks - default: - return AuditModeNone - } -} diff --git a/reviewer.go b/reviewer.go index 8731868..1db2fc2 100644 --- a/reviewer.go +++ b/reviewer.go @@ -12,7 +12,6 @@ import ( "github.com/GrayCodeAI/sight/internal/comment" gitctx "github.com/GrayCodeAI/sight/internal/context" "github.com/GrayCodeAI/sight/internal/diff" - "github.com/GrayCodeAI/sight/internal/graph" "github.com/GrayCodeAI/sight/internal/output" "github.com/GrayCodeAI/sight/internal/review" ) @@ -20,27 +19,12 @@ import ( // Reviewer is a reusable code reviewer. Create one with NewReviewer and call // Review multiple times. It is safe for concurrent use. type Reviewer struct { - cfg *config - g *graph.DependencyGraph - audit bool + cfg *config } // NewReviewer creates a configured Reviewer. func NewReviewer(opts ...Option) *Reviewer { - cfg := buildConfig(opts) - r := &Reviewer{cfg: cfg} - - // Enable graph if available - if cfg.graphEnabled { - r.g = graph.New() - } - - // Enable audit if configured - if cfg.auditMode != AuditModeNone { - r.audit = true - } - - return r + return &Reviewer{cfg: buildConfig(opts)} } // Review parses the diff, builds context, and runs multi-concern analysis. From 18746ec17523cdd4eb388283bdfb31cee41f860d Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 01:14:16 +0530 Subject: [PATCH 2/7] fix(stats): surface non-fatal LLM errors in Stats.LLMErrors Review previously succeeded silently when every provider call failed: errors were collected but only appended to the human-readable Report. Add Stats.LLMErrors (one entry per failed concern call), surface the swallowed self-reflection provider error under an [reflection] prefix, and carry both through ToContractResult into the shared hawk-core-contracts Stats.LLMErrors field so hawk can detect partial results. --- CHANGELOG.md | 9 ++++++ contracts.go | 1 + reviewer.go | 19 +++++++---- sight.go | 5 +++ sight_test.go | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 117 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92fb05f..8599040 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,15 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Fixed +- **Provider failures are now visible in `Stats`.** A review in which + every LLM call failed previously succeeded silently — errors only + appeared in the human-readable `Report`. `Stats.LLMErrors` now + records one entry per failed call (concern calls and the + self-reflection pass), and `ToContractResult` carries them into the + shared `reviewcontracts.Stats.LLMErrors` field so hawk can surface + partial results. + ### Removed - **Dead audit/graph API surface** (breaking, pre-1.0). The `WithGraph`, `WithAuditMode`, and `WithAuditTargets` options and the diff --git a/contracts.go b/contracts.go index 2f65a16..69a48c4 100644 --- a/contracts.go +++ b/contracts.go @@ -94,6 +94,7 @@ func toContractStats(s Stats) reviewcontracts.Stats { AverageConfidence: s.AverageConfidence, HighConfidenceCount: s.HighConfidenceCount, LowConfidenceCount: s.LowConfidenceCount, + LLMErrors: s.LLMErrors, } } diff --git a/reviewer.go b/reviewer.go index 1db2fc2..2f05f2d 100644 --- a/reviewer.go +++ b/reviewer.go @@ -250,7 +250,11 @@ func (r *Reviewer) Review(ctx context.Context, rawDiff string) (*Result, error) // Self-reflection pass: validate findings with a second LLM call if r.cfg.reflection && len(allFindings) > 0 && ctx.Err() == nil { - allFindings = r.reflect(ctx, allFindings, rawDiff, &tokensUsed) + reflected, err := r.reflect(ctx, allFindings, rawDiff, &tokensUsed) + if err != nil { + llmErrors = append(llmErrors, fmt.Sprintf("[reflection] %v", err)) + } + allFindings = reflected } sort.Slice(allFindings, func(i, j int) bool { @@ -304,6 +308,7 @@ func (r *Reviewer) Review(ctx context.Context, rawDiff string) (*Result, error) AverageConfidence: avgConf, HighConfidenceCount: highConfCount, LowConfidenceCount: lowConfCount, + LLMErrors: llmErrors, }, FailOn: r.cfg.failOn, } @@ -534,8 +539,10 @@ func extractTaintSink(msg string) string { return rest } -// reflect runs the self-reflection pass to validate findings. -func (r *Reviewer) reflect(ctx context.Context, findings []Finding, rawDiff string, tokensUsed *int) []Finding { +// reflect runs the self-reflection pass to validate findings. When the +// reflection LLM call fails it returns the original findings together with +// the error, so the caller can surface it without losing the review. +func (r *Reviewer) reflect(ctx context.Context, findings []Finding, rawDiff string, tokensUsed *int) ([]Finding, error) { internalFindings := make([]review.Finding, len(findings)) for i, f := range findings { internalFindings[i] = review.Finding{ @@ -562,16 +569,16 @@ func (r *Reviewer) reflect(ctx context.Context, findings []Finding, rawDiff stri System: review.ReflectSystemPrompt, }) if err != nil { - return findings + return findings, err } *tokensUsed += resp.TokensUsed reflections := review.ParseReflectResponse(resp.Content) if len(reflections) == 0 { - return findings + return findings, nil } validated := review.ApplyReflectionWithScore(internalFindings, reflections, r.cfg.minScore) - return toPublicFindings(validated) + return toPublicFindings(validated), nil } diff --git a/sight.go b/sight.go index 800917c..a3ece11 100644 --- a/sight.go +++ b/sight.go @@ -70,6 +70,11 @@ type Stats struct { HighConfidenceCount int `json:"high_confidence_count"` // LowConfidenceCount is the number of findings with confidence < 0.5. LowConfidenceCount int `json:"low_confidence_count"` + // LLMErrors records non-fatal provider errors encountered during + // analysis, one entry per failed call (prefixed with the concern name, + // or "[reflection]" for the self-reflection pass). Findings may be + // partial when it is non-empty. + LLMErrors []string `json:"llm_errors,omitempty"` } // Result is the complete output of a review operation. diff --git a/sight_test.go b/sight_test.go index 46e4150..fd96900 100644 --- a/sight_test.go +++ b/sight_test.go @@ -4,10 +4,12 @@ import ( "context" "encoding/json" "fmt" + "strings" "sync" "testing" "github.com/GrayCodeAI/sight" + "github.com/GrayCodeAI/sight/internal/review" ) // mockProvider implements sight.Provider for testing. @@ -242,3 +244,90 @@ func TestReview_Deduplication(t *testing.T) { t.Errorf("expected 1 finding after dedup, got %d", len(result.Findings)) } } + +func TestReview_StatsLLMErrorsWhenAllProvidersFail(t *testing.T) { + provider := &mockProvider{err: fmt.Errorf("rate limited")} + + result, err := sight.Review( + context.Background(), testDiff, + sight.WithProvider(provider), + ) + if err != nil { + t.Fatalf("Review() error = %v, want nil (provider errors are non-fatal)", err) + } + + if len(result.Stats.LLMErrors) == 0 { + t.Fatal("Stats.LLMErrors is empty; want one entry per failed concern") + } + + // The default config reviews five concerns; every one of them must + // report its error so callers can tell the review was partial. + concerns := map[string]bool{} + for _, e := range result.Stats.LLMErrors { + if !strings.Contains(e, "rate limited") { + t.Errorf("LLMErrors entry %q does not mention the provider error", e) + } + name := e + if idx := strings.Index(name, "]"); idx >= 0 { + name = strings.Trim(name[:idx], "[]") + } + concerns[name] = true + } + for _, want := range []string{"security", "bugs", "performance", "correctness", "style"} { + if !concerns[want] { + t.Errorf("no LLM error reported for concern %q; got %v", want, result.Stats.LLMErrors) + } + } + + contract := sight.ToContractResult(result) + if len(contract.Stats.LLMErrors) != len(result.Stats.LLMErrors) { + t.Errorf("contract Stats.LLMErrors len = %d, want %d", len(contract.Stats.LLMErrors), len(result.Stats.LLMErrors)) + } +} + +// reflectFailProvider succeeds for concern calls but fails the +// self-reflection call, which is identifiable by its system prompt. +type reflectFailProvider struct { + response string + calls int64 + mu sync.Mutex +} + +func (p *reflectFailProvider) Chat(ctx context.Context, messages []sight.Message, opts sight.ChatOpts) (*sight.Response, error) { + p.mu.Lock() + p.calls++ + p.mu.Unlock() + if opts.System == review.ReflectSystemPrompt { + return nil, fmt.Errorf("reflection backend down") + } + return &sight.Response{Content: p.response, TokensUsed: 10}, nil +} + +func TestReview_StatsLLMErrorsIncludesReflectionFailure(t *testing.T) { + provider := &reflectFailProvider{ + response: `[{"file": "handler.go", "line": 13, "severity": "high", "message": "SQL injection", "fix": "use params"}]`, + } + + result, err := sight.Review( + context.Background(), testDiff, + sight.WithProvider(provider), + sight.WithConcerns("security"), + sight.WithParallel(false), + sight.WithReflection(true), + ) + if err != nil { + t.Fatalf("Review() error = %v, want nil (reflection errors are non-fatal)", err) + } + + if len(result.Stats.LLMErrors) != 1 { + t.Fatalf("Stats.LLMErrors = %v, want exactly one reflection entry", result.Stats.LLMErrors) + } + if !strings.HasPrefix(result.Stats.LLMErrors[0], "[reflection]") || !strings.Contains(result.Stats.LLMErrors[0], "reflection backend down") { + t.Errorf("unexpected reflection error entry: %q", result.Stats.LLMErrors[0]) + } + + // The pre-reflection findings must survive the failed reflection pass. + if len(result.Findings) == 0 { + t.Error("findings were lost when the reflection pass failed") + } +} From d843110ffcd6ce367f2dff2b31f6603cb5aa62de Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 01:15:46 +0530 Subject: [PATCH 3/7] fix(contracts): record configured fail threshold via SetFailOn on conversion ToContractResult assigned the contract Result.FailOn field directly, leaving FailOnSet false. The shared contract's Failed() treats an unset threshold as critical, so a user-configured below-critical threshold (WithFailOn(High), the CI preset) was silently ignored at the contract layer. Call SetFailOn during conversion so the configured threshold takes effect. --- CHANGELOG.md | 6 +++++ contracts.go | 8 ++++-- contracts_test.go | 67 ++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 78 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8599040..6bf5445 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] ### Fixed +- **Configured fail thresholds now survive contract conversion.** + `ToContractResult` copied `FailOn` by field assignment, leaving the + contract's `FailOnSet` false so its `Failed()` ignored a + user-configured below-critical threshold (e.g. `WithFailOn(High)` + from the `CI` preset). Conversion now records the threshold via + `SetFailOn`, so the contract `Result.Failed()` honors it. - **Provider failures are now visible in `Stats`.** A review in which every LLM call failed previously succeeded silently — errors only appeared in the human-readable `Report`. `Stats.LLMErrors` now diff --git a/contracts.go b/contracts.go index 69a48c4..395b043 100644 --- a/contracts.go +++ b/contracts.go @@ -114,13 +114,17 @@ func ToContractResult(r *Result) *reviewcontracts.Result { if r == nil { return nil } - return &reviewcontracts.Result{ + res := &reviewcontracts.Result{ Findings: ToContractFindings(r.Findings), Comments: ToContractInlineComments(r.Comments), Stats: toContractStats(r.Stats), Report: r.Report, - FailOn: r.FailOn, SASTFusion: toContractSASTFusion(r.SASTFusion), ConfidenceBreakdown: toContractConfidenceBreakdown(r.ConfidenceBreakdown), } + // Set the threshold through SetFailOn so FailOnSet is recorded: the + // contract's Failed() ignores a directly-assigned FailOn below + // critical, which would drop a user-configured threshold. + res.SetFailOn(r.FailOn) + return res } diff --git a/contracts_test.go b/contracts_test.go index e9a5575..bbfcb99 100644 --- a/contracts_test.go +++ b/contracts_test.go @@ -1,6 +1,9 @@ package sight -import "testing" +import ( + "context" + "testing" +) func TestToContractResult(t *testing.T) { t.Parallel() @@ -49,3 +52,65 @@ func TestToContractResult(t *testing.T) { t.Fatal("expected confidence breakdown to convert") } } + +func TestToContractResult_FailOnThresholdTakesEffect(t *testing.T) { + t.Parallel() + + // A below-critical threshold configured on the sight Result must + // survive conversion: the contract's Failed() honors it only when + // FailOnSet is true, which ToContractResult must arrange via SetFailOn. + result := &Result{ + FailOn: SeverityHigh, + Findings: []Finding{ + {Severity: SeverityInfo, Message: "note", Confidence: 0.5}, + }, + } + + contract := ToContractResult(result) + if !contract.FailOnSet { + t.Fatal("FailOnSet = false, want true after conversion") + } + if contract.FailOn != SeverityHigh { + t.Fatalf("FailOn = %v, want high", contract.FailOn) + } + if contract.Failed() { + t.Error("info finding must not fail a review with a high threshold") + } + + result.Findings = append(result.Findings, Finding{ + Severity: SeverityHigh, Message: "real problem", Confidence: 0.5, + }) + contract = ToContractResult(result) + if !contract.Failed() { + t.Error("high finding must fail a review with a high threshold") + } +} + +func TestToContractResult_FailOnConfiguredViaOptions(t *testing.T) { + t.Parallel() + + reviewWith := func(response string) *Result { + t.Helper() + r := NewReviewer( + WithProvider(&fixMockProvider{response: response}), + WithFailOn(SeverityHigh), + WithConcerns("security"), + WithParallel(false), + ) + result, err := r.Review(context.Background(), sampleDiff) + if err != nil { + t.Fatalf("Review failed: %v", err) + } + return result + } + + infoOnly := reviewWith(`[{"file": "handler.go", "line": 13, "severity": "info", "message": "style nit", "fix": "n/a"}]`) + if got := ToContractResult(infoOnly).Failed(); got { + t.Error("Failed() = true for info-only findings with failOn=high, want false") + } + + highFinding := reviewWith(`[{"file": "handler.go", "line": 13, "severity": "high", "message": "SQL injection", "fix": "use params"}]`) + if got := ToContractResult(highFinding).Failed(); !got { + t.Error("Failed() = false for a high finding with failOn=high, want true") + } +} From 21633292255cdaf09205c4b60f56ac927e5d6667 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 01:16:18 +0530 Subject: [PATCH 4/7] refactor: name the default LLM finding confidence constant toPublicFindings silently rewrote out-of-range confidence to a magic 0.6. Extract it as the documented package-level constant defaultConfidence. No behavior change. --- CHANGELOG.md | 5 +++++ reviewer.go | 7 ++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bf5445..ffea656 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Changed +- Extracted the out-of-range confidence fallback in `toPublicFindings` + into a documented package constant, `defaultConfidence` (0.6). No + behavior change. + ### Fixed - **Configured fail thresholds now survive contract conversion.** `ToContractResult` copied `FailOn` by field assignment, leaving the diff --git a/reviewer.go b/reviewer.go index 2f05f2d..0734990 100644 --- a/reviewer.go +++ b/reviewer.go @@ -369,6 +369,11 @@ func (r *Reviewer) ReviewFiles(ctx context.Context, files []FileChange) (*Result return r.Review(ctx, combined) } +// defaultConfidence is the confidence assigned to LLM findings whose +// reported value is missing or out of range (outside (0, 1]). It matches +// the "medium" confidence band used elsewhere in sight. +const defaultConfidence = 0.6 + func toPublicFindings(internal []review.Finding) []Finding { out := make([]Finding, len(internal)) for i, f := range internal { @@ -379,7 +384,7 @@ func toPublicFindings(internal []review.Finding) []Finding { } conf := f.Confidence if conf <= 0 || conf > 1.0 { - conf = 0.6 // default for LLM findings + conf = defaultConfidence } out[i] = Finding{ Concern: f.Concern, From 7ad5be98de64245c5a7cc1696e233205e9fb0311 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 08:36:06 +0530 Subject: [PATCH 5/7] fix: depend on contracts branch APIs, bump Go to 1.26.6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - go.mod: hawk-core-contracts v0.1.9 -> v0.1.13-0.20260815203243-0f60bf02 (branch fix/audit-sweep-2026-08 of hawk-core-contracts) — needed for Stats.LLMErrors and Result.SetFailOn/FailOnSet used by this branch; re-pin to the tagged release once hawk-core-contracts#27 merges - go.mod + CI: Go 1.26.6 — 1.26.5 stdlib has reachable vulns that fail govulncheck --- .github/workflows/ci.yml | 2 +- go.mod | 4 ++-- go.sum | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e7f1789..16440c1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,7 +32,7 @@ concurrency: cancel-in-progress: true env: - GO_VERSION: "1.26.5" + GO_VERSION: "1.26.6" GOPROXY: "https://proxy.golang.org,direct" GOPRIVATE: "github.com/GrayCodeAI/*" GONOSUMDB: "github.com/GrayCodeAI/*" diff --git a/go.mod b/go.mod index f36ba26..80203d0 100644 --- a/go.mod +++ b/go.mod @@ -1,9 +1,9 @@ module github.com/GrayCodeAI/sight -go 1.26.5 +go 1.26.6 require ( - github.com/GrayCodeAI/hawk-core-contracts v0.1.9 + github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260815203243-0f60bf0259c0 github.com/GrayCodeAI/hawk-mcpkit v0.1.4 github.com/mark3labs/mcp-go v0.49.0 golang.org/x/tools v0.45.0 diff --git a/go.sum b/go.sum index 12fb0dd..5939195 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/GrayCodeAI/hawk-core-contracts v0.1.9 h1:uXX/gtNM+3kxSEzu+rZkHykzcEaAbASn1lmPyOGMXvc= -github.com/GrayCodeAI/hawk-core-contracts v0.1.9/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= +github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260815203243-0f60bf0259c0 h1:HD8Y2SEx1mx6A1uLCLjQ9E1hokRP5T/Kw5XmIgmqcOQ= +github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260815203243-0f60bf0259c0/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= github.com/GrayCodeAI/hawk-mcpkit v0.1.4 h1:tlhZXKDbI679I7c1feeY/pzErFwndD+R2CQf9sqHAVE= github.com/GrayCodeAI/hawk-mcpkit v0.1.4/go.mod h1:C32HPDRqiDETbVbMIbOTvguek6KImpLCffJjet7sqck= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= From 6dd87d8843d71ec57861ee0b543400c576fe4640 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 09:12:07 +0530 Subject: [PATCH 6/7] chore: re-pin hawk-core-contracts to merged main contracts#27 squash-merged as 16ebcfd; move from the branch pseudo-version to the merged main pseudo-version. --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 80203d0..f2e031a 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/GrayCodeAI/sight go 1.26.6 require ( - github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260815203243-0f60bf0259c0 + github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e github.com/GrayCodeAI/hawk-mcpkit v0.1.4 github.com/mark3labs/mcp-go v0.49.0 golang.org/x/tools v0.45.0 diff --git a/go.sum b/go.sum index 5939195..77c25c3 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,7 @@ github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260815203243-0f60bf0259c0 h1:HD8Y2SEx1mx6A1uLCLjQ9E1hokRP5T/Kw5XmIgmqcOQ= github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260815203243-0f60bf0259c0/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= +github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e h1:cG9bLB3rWmMVU/7GwDxUIrEFbGHz7OTD5s9I4K+udr0= +github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= github.com/GrayCodeAI/hawk-mcpkit v0.1.4 h1:tlhZXKDbI679I7c1feeY/pzErFwndD+R2CQf9sqHAVE= github.com/GrayCodeAI/hawk-mcpkit v0.1.4/go.mod h1:C32HPDRqiDETbVbMIbOTvguek6KImpLCffJjet7sqck= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= From 481d3d1d57053417ecff1a96cbdcddd86dcd2790 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sun, 16 Aug 2026 09:18:58 +0530 Subject: [PATCH 7/7] chore: go mod tidy after contracts re-pin --- go.sum | 2 -- 1 file changed, 2 deletions(-) diff --git a/go.sum b/go.sum index 77c25c3..a7d00ab 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,3 @@ -github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260815203243-0f60bf0259c0 h1:HD8Y2SEx1mx6A1uLCLjQ9E1hokRP5T/Kw5XmIgmqcOQ= -github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260815203243-0f60bf0259c0/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e h1:cG9bLB3rWmMVU/7GwDxUIrEFbGHz7OTD5s9I4K+udr0= github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= github.com/GrayCodeAI/hawk-mcpkit v0.1.4 h1:tlhZXKDbI679I7c1feeY/pzErFwndD+R2CQf9sqHAVE=