From 887e5e2f50b95d99986989435a0acebf468acf47 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 11 Nov 2025 02:29:12 +0000 Subject: [PATCH 1/2] Initial plan From 111cfc50c50f73d0922e4dc7b68f7143ff7591e2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 11 Nov 2025 02:43:28 +0000 Subject: [PATCH 2/2] Refactor: Extract shared domain aggregation logic - Created aggregateDomainStats() helper to centralize domain aggregation - Created convertDomainsToSortedSlices() helper for domain map conversion - Refactored buildAccessLogSummary() to use shared helpers - Refactored buildFirewallLogSummary() to use shared helpers - Added comprehensive tests for all new helper functions - All existing tests pass, functionality unchanged Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/logs_report.go | 172 +++++++++++--------- pkg/cli/logs_report_test.go | 312 ++++++++++++++++++++++++++++++++++++ 2 files changed, 407 insertions(+), 77 deletions(-) diff --git a/pkg/cli/logs_report.go b/pkg/cli/logs_report.go index 771a9b6fbce..da41a561182 100644 --- a/pkg/cli/logs_report.go +++ b/pkg/cli/logs_report.go @@ -368,52 +368,87 @@ func buildMCPFailuresSummary(processedRuns []ProcessedRun) []MCPFailureSummary { return result } -// buildAccessLogSummary aggregates access log data across all runs -func buildAccessLogSummary(processedRuns []ProcessedRun) *AccessLogSummary { - allAllowedDomains := make(map[string]bool) - allDeniedDomains := make(map[string]bool) - byWorkflow := make(map[string]*DomainAnalysis) - totalRequests := 0 - allowedCount := 0 - deniedCount := 0 +// domainAggregation holds the result of aggregating domain statistics +type domainAggregation struct { + allAllowedDomains map[string]bool + allDeniedDomains map[string]bool + totalRequests int + allowedCount int + deniedCount int +} + +// aggregateDomainStats aggregates domain statistics across runs +// This is a shared helper for both access log and firewall log summaries +func aggregateDomainStats(processedRuns []ProcessedRun, getAnalysis func(*ProcessedRun) (allowedDomains, deniedDomains []string, totalRequests, allowedCount, deniedCount int, exists bool)) *domainAggregation { + agg := &domainAggregation{ + allAllowedDomains: make(map[string]bool), + allDeniedDomains: make(map[string]bool), + } for _, pr := range processedRuns { - if pr.AccessAnalysis != nil { - totalRequests += pr.AccessAnalysis.TotalRequests - allowedCount += pr.AccessAnalysis.AllowedCount - deniedCount += pr.AccessAnalysis.DeniedCount - byWorkflow[pr.Run.WorkflowName] = pr.AccessAnalysis - - for _, domain := range pr.AccessAnalysis.AllowedDomains { - allAllowedDomains[domain] = true - } - for _, domain := range pr.AccessAnalysis.DeniedDomains { - allDeniedDomains[domain] = true - } + allowedDomains, deniedDomains, totalRequests, allowedCount, deniedCount, exists := getAnalysis(&pr) + if !exists { + continue + } + + agg.totalRequests += totalRequests + agg.allowedCount += allowedCount + agg.deniedCount += deniedCount + + for _, domain := range allowedDomains { + agg.allAllowedDomains[domain] = true + } + for _, domain := range deniedDomains { + agg.allDeniedDomains[domain] = true } } - if totalRequests == 0 { - return nil + return agg +} + +// convertDomainsToSortedSlices converts domain maps to sorted slices +func convertDomainsToSortedSlices(allowedMap, deniedMap map[string]bool) (allowed, denied []string) { + for domain := range allowedMap { + allowed = append(allowed, domain) } + sort.Strings(allowed) - // Convert maps to slices - var allowedDomains []string - for domain := range allAllowedDomains { - allowedDomains = append(allowedDomains, domain) + for domain := range deniedMap { + denied = append(denied, domain) } - sort.Strings(allowedDomains) + sort.Strings(denied) + + return allowed, denied +} + +// buildAccessLogSummary aggregates access log data across all runs +func buildAccessLogSummary(processedRuns []ProcessedRun) *AccessLogSummary { + byWorkflow := make(map[string]*DomainAnalysis) + + // Use shared aggregation helper + agg := aggregateDomainStats(processedRuns, func(pr *ProcessedRun) ([]string, []string, int, int, int, bool) { + if pr.AccessAnalysis == nil { + return nil, nil, 0, 0, 0, false + } + byWorkflow[pr.Run.WorkflowName] = pr.AccessAnalysis + return pr.AccessAnalysis.AllowedDomains, + pr.AccessAnalysis.DeniedDomains, + pr.AccessAnalysis.TotalRequests, + pr.AccessAnalysis.AllowedCount, + pr.AccessAnalysis.DeniedCount, + true + }) - var deniedDomains []string - for domain := range allDeniedDomains { - deniedDomains = append(deniedDomains, domain) + if agg.totalRequests == 0 { + return nil } - sort.Strings(deniedDomains) + + allowedDomains, deniedDomains := convertDomainsToSortedSlices(agg.allAllowedDomains, agg.allDeniedDomains) return &AccessLogSummary{ - TotalRequests: totalRequests, - AllowedCount: allowedCount, - DeniedCount: deniedCount, + TotalRequests: agg.totalRequests, + AllowedCount: agg.allowedCount, + DeniedCount: agg.deniedCount, AllowedDomains: allowedDomains, DeniedDomains: deniedDomains, ByWorkflow: byWorkflow, @@ -422,59 +457,42 @@ func buildAccessLogSummary(processedRuns []ProcessedRun) *AccessLogSummary { // buildFirewallLogSummary aggregates firewall log data across all runs func buildFirewallLogSummary(processedRuns []ProcessedRun) *FirewallLogSummary { - allAllowedDomains := make(map[string]bool) - allDeniedDomains := make(map[string]bool) allRequestsByDomain := make(map[string]DomainRequestStats) byWorkflow := make(map[string]*FirewallAnalysis) - totalRequests := 0 - allowedRequests := 0 - deniedRequests := 0 - for _, pr := range processedRuns { - if pr.FirewallAnalysis != nil { - totalRequests += pr.FirewallAnalysis.TotalRequests - allowedRequests += pr.FirewallAnalysis.AllowedRequests - deniedRequests += pr.FirewallAnalysis.DeniedRequests - byWorkflow[pr.Run.WorkflowName] = pr.FirewallAnalysis - - for _, domain := range pr.FirewallAnalysis.AllowedDomains { - allAllowedDomains[domain] = true - } - for _, domain := range pr.FirewallAnalysis.DeniedDomains { - allDeniedDomains[domain] = true - } - - // Aggregate request stats by domain - for domain, stats := range pr.FirewallAnalysis.RequestsByDomain { - existing := allRequestsByDomain[domain] - existing.Allowed += stats.Allowed - existing.Denied += stats.Denied - allRequestsByDomain[domain] = existing - } + // Use shared aggregation helper + agg := aggregateDomainStats(processedRuns, func(pr *ProcessedRun) ([]string, []string, int, int, int, bool) { + if pr.FirewallAnalysis == nil { + return nil, nil, 0, 0, 0, false + } + byWorkflow[pr.Run.WorkflowName] = pr.FirewallAnalysis + + // Aggregate request stats by domain (firewall-specific) + for domain, stats := range pr.FirewallAnalysis.RequestsByDomain { + existing := allRequestsByDomain[domain] + existing.Allowed += stats.Allowed + existing.Denied += stats.Denied + allRequestsByDomain[domain] = existing } - } - if totalRequests == 0 { - return nil - } + return pr.FirewallAnalysis.AllowedDomains, + pr.FirewallAnalysis.DeniedDomains, + pr.FirewallAnalysis.TotalRequests, + pr.FirewallAnalysis.AllowedRequests, + pr.FirewallAnalysis.DeniedRequests, + true + }) - // Convert maps to slices - var allowedDomains []string - for domain := range allAllowedDomains { - allowedDomains = append(allowedDomains, domain) + if agg.totalRequests == 0 { + return nil } - sort.Strings(allowedDomains) - var deniedDomains []string - for domain := range allDeniedDomains { - deniedDomains = append(deniedDomains, domain) - } - sort.Strings(deniedDomains) + allowedDomains, deniedDomains := convertDomainsToSortedSlices(agg.allAllowedDomains, agg.allDeniedDomains) return &FirewallLogSummary{ - TotalRequests: totalRequests, - AllowedRequests: allowedRequests, - DeniedRequests: deniedRequests, + TotalRequests: agg.totalRequests, + AllowedRequests: agg.allowedCount, + DeniedRequests: agg.deniedCount, AllowedDomains: allowedDomains, DeniedDomains: deniedDomains, RequestsByDomain: allRequestsByDomain, diff --git a/pkg/cli/logs_report_test.go b/pkg/cli/logs_report_test.go index 609f5b56f49..da49283cb29 100644 --- a/pkg/cli/logs_report_test.go +++ b/pkg/cli/logs_report_test.go @@ -374,3 +374,315 @@ func TestBuildMCPFailuresSummaryDeduplication(t *testing.T) { } } } + +// TestAggregateDomainStats tests the shared domain aggregation helper +func TestAggregateDomainStats(t *testing.T) { + t.Run("aggregates domains correctly", func(t *testing.T) { + processedRuns := []ProcessedRun{ + { + AccessAnalysis: &DomainAnalysis{ + AllowedDomains: []string{"example.com", "api.github.com"}, + DeniedDomains: []string{"blocked.com"}, + TotalRequests: 10, + AllowedCount: 8, + DeniedCount: 2, + }, + }, + { + AccessAnalysis: &DomainAnalysis{ + AllowedDomains: []string{"api.github.com", "docs.github.com"}, + DeniedDomains: []string{"spam.com"}, + TotalRequests: 5, + AllowedCount: 4, + DeniedCount: 1, + }, + }, + } + + agg := aggregateDomainStats(processedRuns, func(pr *ProcessedRun) ([]string, []string, int, int, int, bool) { + if pr.AccessAnalysis == nil { + return nil, nil, 0, 0, 0, false + } + return pr.AccessAnalysis.AllowedDomains, + pr.AccessAnalysis.DeniedDomains, + pr.AccessAnalysis.TotalRequests, + pr.AccessAnalysis.AllowedCount, + pr.AccessAnalysis.DeniedCount, + true + }) + + if agg.totalRequests != 15 { + t.Errorf("Expected totalRequests = 15, got %d", agg.totalRequests) + } + if agg.allowedCount != 12 { + t.Errorf("Expected allowedCount = 12, got %d", agg.allowedCount) + } + if agg.deniedCount != 3 { + t.Errorf("Expected deniedCount = 3, got %d", agg.deniedCount) + } + + // Check unique domains + if len(agg.allAllowedDomains) != 3 { + t.Errorf("Expected 3 unique allowed domains, got %d", len(agg.allAllowedDomains)) + } + if len(agg.allDeniedDomains) != 2 { + t.Errorf("Expected 2 unique denied domains, got %d", len(agg.allDeniedDomains)) + } + + // Verify specific domains + if !agg.allAllowedDomains["example.com"] { + t.Error("Expected example.com in allowed domains") + } + if !agg.allAllowedDomains["api.github.com"] { + t.Error("Expected api.github.com in allowed domains") + } + if !agg.allDeniedDomains["blocked.com"] { + t.Error("Expected blocked.com in denied domains") + } + }) + + t.Run("handles nil analysis", func(t *testing.T) { + processedRuns := []ProcessedRun{ + { + AccessAnalysis: nil, + }, + { + AccessAnalysis: &DomainAnalysis{ + AllowedDomains: []string{"example.com"}, + TotalRequests: 5, + AllowedCount: 5, + DeniedCount: 0, + }, + }, + } + + agg := aggregateDomainStats(processedRuns, func(pr *ProcessedRun) ([]string, []string, int, int, int, bool) { + if pr.AccessAnalysis == nil { + return nil, nil, 0, 0, 0, false + } + return pr.AccessAnalysis.AllowedDomains, + pr.AccessAnalysis.DeniedDomains, + pr.AccessAnalysis.TotalRequests, + pr.AccessAnalysis.AllowedCount, + pr.AccessAnalysis.DeniedCount, + true + }) + + if agg.totalRequests != 5 { + t.Errorf("Expected totalRequests = 5, got %d", agg.totalRequests) + } + if len(agg.allAllowedDomains) != 1 { + t.Errorf("Expected 1 allowed domain, got %d", len(agg.allAllowedDomains)) + } + }) + + t.Run("handles empty runs", func(t *testing.T) { + processedRuns := []ProcessedRun{} + + agg := aggregateDomainStats(processedRuns, func(pr *ProcessedRun) ([]string, []string, int, int, int, bool) { + return nil, nil, 0, 0, 0, false + }) + + if agg.totalRequests != 0 { + t.Errorf("Expected totalRequests = 0, got %d", agg.totalRequests) + } + if len(agg.allAllowedDomains) != 0 { + t.Errorf("Expected 0 allowed domains, got %d", len(agg.allAllowedDomains)) + } + }) +} + +// TestConvertDomainsToSortedSlices tests the domain conversion helper +func TestConvertDomainsToSortedSlices(t *testing.T) { + t.Run("converts and sorts domains", func(t *testing.T) { + allowedMap := map[string]bool{ + "z.com": true, + "a.com": true, + "m.com": true, + } + deniedMap := map[string]bool{ + "y.com": true, + "b.com": true, + } + + allowed, denied := convertDomainsToSortedSlices(allowedMap, deniedMap) + + // Check sorted order + expectedAllowed := []string{"a.com", "m.com", "z.com"} + if len(allowed) != len(expectedAllowed) { + t.Errorf("Expected %d allowed domains, got %d", len(expectedAllowed), len(allowed)) + } + for i, domain := range expectedAllowed { + if allowed[i] != domain { + t.Errorf("Expected allowed[%d] = %s, got %s", i, domain, allowed[i]) + } + } + + expectedDenied := []string{"b.com", "y.com"} + if len(denied) != len(expectedDenied) { + t.Errorf("Expected %d denied domains, got %d", len(expectedDenied), len(denied)) + } + for i, domain := range expectedDenied { + if denied[i] != domain { + t.Errorf("Expected denied[%d] = %s, got %s", i, domain, denied[i]) + } + } + }) + + t.Run("handles empty maps", func(t *testing.T) { + allowedMap := map[string]bool{} + deniedMap := map[string]bool{} + + allowed, denied := convertDomainsToSortedSlices(allowedMap, deniedMap) + + if len(allowed) != 0 { + t.Errorf("Expected 0 allowed domains, got %d", len(allowed)) + } + if len(denied) != 0 { + t.Errorf("Expected 0 denied domains, got %d", len(denied)) + } + }) +} + +// TestBuildAccessLogSummaryWithSharedHelper tests access log summary with shared helper +func TestBuildAccessLogSummaryWithSharedHelper(t *testing.T) { + processedRuns := []ProcessedRun{ + { + Run: WorkflowRun{ + WorkflowName: "workflow-a", + }, + AccessAnalysis: &DomainAnalysis{ + AllowedDomains: []string{"example.com", "api.github.com"}, + DeniedDomains: []string{"blocked.com"}, + TotalRequests: 10, + AllowedCount: 8, + DeniedCount: 2, + }, + }, + { + Run: WorkflowRun{ + WorkflowName: "workflow-b", + }, + AccessAnalysis: &DomainAnalysis{ + AllowedDomains: []string{"docs.github.com"}, + DeniedDomains: []string{}, + TotalRequests: 5, + AllowedCount: 5, + DeniedCount: 0, + }, + }, + } + + summary := buildAccessLogSummary(processedRuns) + + if summary == nil { + t.Fatal("Expected non-nil summary") + } + + if summary.TotalRequests != 15 { + t.Errorf("Expected TotalRequests = 15, got %d", summary.TotalRequests) + } + if summary.AllowedCount != 13 { + t.Errorf("Expected AllowedCount = 13, got %d", summary.AllowedCount) + } + if summary.DeniedCount != 2 { + t.Errorf("Expected DeniedCount = 2, got %d", summary.DeniedCount) + } + + // Check sorted domains + expectedAllowed := []string{"api.github.com", "docs.github.com", "example.com"} + if len(summary.AllowedDomains) != len(expectedAllowed) { + t.Errorf("Expected %d allowed domains, got %d", len(expectedAllowed), len(summary.AllowedDomains)) + } + for i, domain := range expectedAllowed { + if summary.AllowedDomains[i] != domain { + t.Errorf("Expected AllowedDomains[%d] = %s, got %s", i, domain, summary.AllowedDomains[i]) + } + } + + if len(summary.DeniedDomains) != 1 || summary.DeniedDomains[0] != "blocked.com" { + t.Errorf("Expected DeniedDomains = [blocked.com], got %v", summary.DeniedDomains) + } + + // Check by workflow + if len(summary.ByWorkflow) != 2 { + t.Errorf("Expected 2 workflows, got %d", len(summary.ByWorkflow)) + } +} + +// TestBuildFirewallLogSummaryWithSharedHelper tests firewall log summary with shared helper +func TestBuildFirewallLogSummaryWithSharedHelper(t *testing.T) { + processedRuns := []ProcessedRun{ + { + Run: WorkflowRun{ + WorkflowName: "workflow-a", + }, + FirewallAnalysis: &FirewallAnalysis{ + AllowedDomains: []string{"example.com"}, + DeniedDomains: []string{"blocked.com"}, + TotalRequests: 10, + AllowedRequests: 8, + DeniedRequests: 2, + RequestsByDomain: map[string]DomainRequestStats{ + "example.com": {Allowed: 8, Denied: 0}, + "blocked.com": {Allowed: 0, Denied: 2}, + }, + }, + }, + { + Run: WorkflowRun{ + WorkflowName: "workflow-b", + }, + FirewallAnalysis: &FirewallAnalysis{ + AllowedDomains: []string{"example.com", "api.github.com"}, + DeniedDomains: []string{}, + TotalRequests: 5, + AllowedRequests: 5, + DeniedRequests: 0, + RequestsByDomain: map[string]DomainRequestStats{ + "example.com": {Allowed: 3, Denied: 0}, + "api.github.com": {Allowed: 2, Denied: 0}, + }, + }, + }, + } + + summary := buildFirewallLogSummary(processedRuns) + + if summary == nil { + t.Fatal("Expected non-nil summary") + } + + if summary.TotalRequests != 15 { + t.Errorf("Expected TotalRequests = 15, got %d", summary.TotalRequests) + } + if summary.AllowedRequests != 13 { + t.Errorf("Expected AllowedRequests = 13, got %d", summary.AllowedRequests) + } + if summary.DeniedRequests != 2 { + t.Errorf("Expected DeniedRequests = 2, got %d", summary.DeniedRequests) + } + + // Check RequestsByDomain aggregation (firewall-specific) + if stats, ok := summary.RequestsByDomain["example.com"]; !ok { + t.Error("Expected example.com in RequestsByDomain") + } else { + if stats.Allowed != 11 { + t.Errorf("Expected example.com Allowed = 11, got %d", stats.Allowed) + } + if stats.Denied != 0 { + t.Errorf("Expected example.com Denied = 0, got %d", stats.Denied) + } + } + + if stats, ok := summary.RequestsByDomain["blocked.com"]; !ok { + t.Error("Expected blocked.com in RequestsByDomain") + } else { + if stats.Allowed != 0 { + t.Errorf("Expected blocked.com Allowed = 0, got %d", stats.Allowed) + } + if stats.Denied != 2 { + t.Errorf("Expected blocked.com Denied = 2, got %d", stats.Denied) + } + } +}