diff --git a/pkg/cli/access_log.go b/pkg/cli/access_log.go index 01e8ae9229b..2e66f38d9d0 100644 --- a/pkg/cli/access_log.go +++ b/pkg/cli/access_log.go @@ -2,6 +2,7 @@ package cli import ( "bufio" + "encoding/json" "fmt" "os" "path/filepath" @@ -32,18 +33,53 @@ type AccessLogEntry struct { // DomainAnalysis represents analysis of domains from access logs type DomainAnalysis struct { - DomainBuckets - TotalRequests int `json:"total_requests"` - AllowedCount int `json:"allowed_count"` - BlockedCount int `json:"blocked_count"` + AnalysisBase +} + +// domainAnalysisWire is the stable JSON schema for DomainAnalysis. +// It preserves the original "allowed_count"/"blocked_count" field names so that +// cached RunSummary.access_analysis JSON and AccessLogSummary.by_workflow values +// remain backward-compatible after the AnalysisBase refactor, which renamed those +// fields to AllowedRequests/BlockedRequests internally. +type domainAnalysisWire struct { + TotalRequests int `json:"total_requests"` + AllowedCount int `json:"allowed_count"` + BlockedCount int `json:"blocked_count"` + AllowedDomains []string `json:"allowed_domains,omitempty"` + BlockedDomains []string `json:"blocked_domains,omitempty"` +} + +// MarshalJSON emits the original "allowed_count"/"blocked_count" wire names so +// existing consumers of the access-analysis JSON do not see a silent field rename. +func (d DomainAnalysis) MarshalJSON() ([]byte, error) { + return json.Marshal(domainAnalysisWire{ + TotalRequests: d.TotalRequests, + AllowedCount: d.AllowedRequests, + BlockedCount: d.BlockedRequests, + AllowedDomains: d.AllowedDomains, + BlockedDomains: d.BlockedDomains, + }) +} + +// UnmarshalJSON accepts the original "allowed_count"/"blocked_count" wire names, +// keeping round-trip compatibility with cached JSON produced before the refactor. +func (d *DomainAnalysis) UnmarshalJSON(data []byte) error { + var wire domainAnalysisWire + if err := json.Unmarshal(data, &wire); err != nil { + return err + } + d.TotalRequests = wire.TotalRequests + d.AllowedRequests = wire.AllowedCount + d.BlockedRequests = wire.BlockedCount + d.AllowedDomains = wire.AllowedDomains + d.BlockedDomains = wire.BlockedDomains + return nil } // AddMetrics adds metrics from another analysis func (d *DomainAnalysis) AddMetrics(other LogAnalysis) { if otherDomain, ok := other.(*DomainAnalysis); ok { - d.TotalRequests += otherDomain.TotalRequests - d.AllowedCount += otherDomain.AllowedCount - d.BlockedCount += otherDomain.BlockedCount + d.addBaseMetrics(&otherDomain.AnalysisBase) } } @@ -101,14 +137,14 @@ func parseSquidAccessLog(logPath string, verbose bool) (*DomainAnalysis, error) strings.Contains(statusCode, "/304") if isAllowed { - analysis.AllowedCount++ + analysis.AllowedRequests++ if !setutil.Contains(allowedDomainsSet, domain) { allowedDomainsSet[domain] = struct { }{} analysis.AllowedDomains = append(analysis.AllowedDomains, domain) } } else { - analysis.BlockedCount++ + analysis.BlockedRequests++ if !setutil.Contains(blockedDomainsSet, domain) { blockedDomainsSet[domain] = struct { }{} @@ -126,7 +162,7 @@ func parseSquidAccessLog(logPath string, verbose bool) (*DomainAnalysis, error) sort.Strings(analysis.BlockedDomains) accessLogLog.Printf("Parsed access log: total_requests=%d, allowed=%d, blocked=%d, unique_allowed_domains=%d, unique_blocked_domains=%d", - analysis.TotalRequests, analysis.AllowedCount, analysis.BlockedCount, len(analysis.AllowedDomains), len(analysis.BlockedDomains)) + analysis.TotalRequests, analysis.AllowedRequests, analysis.BlockedRequests, len(analysis.AllowedDomains), len(analysis.BlockedDomains)) return analysis, nil } diff --git a/pkg/cli/access_log_test.go b/pkg/cli/access_log_test.go index 682acf1247d..822cfc2775a 100644 --- a/pkg/cli/access_log_test.go +++ b/pkg/cli/access_log_test.go @@ -3,6 +3,7 @@ package cli import ( + "encoding/json" "os" "path/filepath" "testing" @@ -35,8 +36,8 @@ func TestAccessLogParsing(t *testing.T) { // Verify results assert.Equal(t, 4, analysis.TotalRequests, "should count all log entries") - assert.Equal(t, 2, analysis.AllowedCount, "should count allowed requests") - assert.Equal(t, 2, analysis.BlockedCount, "should count blocked requests") + assert.Equal(t, 2, analysis.AllowedRequests, "should count allowed requests") + assert.Equal(t, 2, analysis.BlockedRequests, "should count blocked requests") // Check allowed domains expectedAllowed := []string{"api.github.com", "example.com"} @@ -73,8 +74,8 @@ func TestMultipleAccessLogAnalysis(t *testing.T) { // Verify aggregated results assert.Equal(t, 4, analysis.TotalRequests, "should count all requests from multiple logs") - assert.Equal(t, 2, analysis.AllowedCount, "should count allowed requests") - assert.Equal(t, 2, analysis.BlockedCount, "should count blocked requests") + assert.Equal(t, 2, analysis.AllowedRequests, "should count allowed requests") + assert.Equal(t, 2, analysis.BlockedRequests, "should count blocked requests") // Check allowed domains expectedAllowed := []string{"api.github.com", "example.com"} @@ -249,55 +250,37 @@ func TestAddMetrics(t *testing.T) { { name: "add valid domain analysis", base: &DomainAnalysis{ - TotalRequests: 10, - AllowedCount: 8, - BlockedCount: 2, + AnalysisBase: AnalysisBase{TotalRequests: 10, AllowedRequests: 8, BlockedRequests: 2}, }, toAdd: &DomainAnalysis{ - TotalRequests: 5, - AllowedCount: 4, - BlockedCount: 1, + AnalysisBase: AnalysisBase{TotalRequests: 5, AllowedRequests: 4, BlockedRequests: 1}, }, expected: &DomainAnalysis{ - TotalRequests: 15, - AllowedCount: 12, - BlockedCount: 3, + AnalysisBase: AnalysisBase{TotalRequests: 15, AllowedRequests: 12, BlockedRequests: 3}, }, }, { name: "add zero values", base: &DomainAnalysis{ - TotalRequests: 10, - AllowedCount: 8, - BlockedCount: 2, + AnalysisBase: AnalysisBase{TotalRequests: 10, AllowedRequests: 8, BlockedRequests: 2}, }, toAdd: &DomainAnalysis{ - TotalRequests: 0, - AllowedCount: 0, - BlockedCount: 0, + AnalysisBase: AnalysisBase{TotalRequests: 0, AllowedRequests: 0, BlockedRequests: 0}, }, expected: &DomainAnalysis{ - TotalRequests: 10, - AllowedCount: 8, - BlockedCount: 2, + AnalysisBase: AnalysisBase{TotalRequests: 10, AllowedRequests: 8, BlockedRequests: 2}, }, }, { name: "add to empty base", base: &DomainAnalysis{ - TotalRequests: 0, - AllowedCount: 0, - BlockedCount: 0, + AnalysisBase: AnalysisBase{TotalRequests: 0, AllowedRequests: 0, BlockedRequests: 0}, }, toAdd: &DomainAnalysis{ - TotalRequests: 5, - AllowedCount: 3, - BlockedCount: 2, + AnalysisBase: AnalysisBase{TotalRequests: 5, AllowedRequests: 3, BlockedRequests: 2}, }, expected: &DomainAnalysis{ - TotalRequests: 5, - AllowedCount: 3, - BlockedCount: 2, + AnalysisBase: AnalysisBase{TotalRequests: 5, AllowedRequests: 3, BlockedRequests: 2}, }, }, } @@ -306,8 +289,46 @@ func TestAddMetrics(t *testing.T) { t.Run(tt.name, func(t *testing.T) { tt.base.AddMetrics(tt.toAdd) assert.Equal(t, tt.expected.TotalRequests, tt.base.TotalRequests, "total requests should match") - assert.Equal(t, tt.expected.AllowedCount, tt.base.AllowedCount, "allowed count should match") - assert.Equal(t, tt.expected.BlockedCount, tt.base.BlockedCount, "blocked count should match") + assert.Equal(t, tt.expected.AllowedRequests, tt.base.AllowedRequests, "allowed requests should match") + assert.Equal(t, tt.expected.BlockedRequests, tt.base.BlockedRequests, "blocked requests should match") }) } } + +// TestDomainAnalysisJSONWireNames verifies that DomainAnalysis serializes with the +// original "allowed_count"/"blocked_count" JSON keys (not "allowed_requests"/ +// "blocked_requests") so that cached access-analysis JSON remains backward-compatible. +func TestDomainAnalysisJSONWireNames(t *testing.T) { + d := DomainAnalysis{ + AnalysisBase: AnalysisBase{ + TotalRequests: 10, + AllowedRequests: 7, + BlockedRequests: 3, + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"example.com"}, + BlockedDomains: []string{"blocked.com"}, + }, + }, + } + + data, err := json.Marshal(d) + require.NoError(t, err) + + var raw map[string]any + require.NoError(t, json.Unmarshal(data, &raw)) + + assert.EqualValues(t, 7, raw["allowed_count"], "should use legacy key allowed_count") + assert.EqualValues(t, 3, raw["blocked_count"], "should use legacy key blocked_count") + assert.Nil(t, raw["allowed_requests"], "should not emit allowed_requests") + assert.Nil(t, raw["blocked_requests"], "should not emit blocked_requests") + assert.EqualValues(t, 10, raw["total_requests"]) + + // Round-trip: unmarshal back should restore fields correctly. + var d2 DomainAnalysis + require.NoError(t, json.Unmarshal(data, &d2)) + assert.Equal(t, d.TotalRequests, d2.TotalRequests) + assert.Equal(t, d.AllowedRequests, d2.AllowedRequests) + assert.Equal(t, d.BlockedRequests, d2.BlockedRequests) + assert.Equal(t, d.AllowedDomains, d2.AllowedDomains) + assert.Equal(t, d.BlockedDomains, d2.BlockedDomains) +} diff --git a/pkg/cli/audit_agent_example_test.go b/pkg/cli/audit_agent_example_test.go index c9aac098111..60af9c04eac 100644 --- a/pkg/cli/audit_agent_example_test.go +++ b/pkg/cli/audit_agent_example_test.go @@ -66,19 +66,21 @@ func TestAgentFriendlyOutputExample(t *testing.T) { } firewallAnalysis := &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{ - "api.github.com:443", - "search.brave.com:443", - "npmjs.org:443", - }, - BlockedDomains: []string{ - "tracking.example.com:443", + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{ + "api.github.com:443", + "search.brave.com:443", + "npmjs.org:443", + }, + BlockedDomains: []string{ + "tracking.example.com:443", + }, }, + TotalRequests: 42, + AllowedRequests: 40, + BlockedRequests: 2, }, - TotalRequests: 42, - AllowedRequests: 40, - BlockedRequests: 2, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 25, Blocked: 0}, "search.brave.com:443": {Allowed: 10, Blocked: 0}, diff --git a/pkg/cli/audit_agent_output_test.go b/pkg/cli/audit_agent_output_test.go index 6dd19f8adde..82e347adb0a 100644 --- a/pkg/cli/audit_agent_output_test.go +++ b/pkg/cli/audit_agent_output_test.go @@ -282,7 +282,7 @@ func TestPerformanceMetricsGeneration(t *testing.T) { Duration: 5 * time.Minute, }, firewallAnalysis: &FirewallAnalysis{ - TotalRequests: 25, + AnalysisBase: AnalysisBase{TotalRequests: 25}, }, expectNetworkRequests: true, }, diff --git a/pkg/cli/audit_cross_run_test.go b/pkg/cli/audit_cross_run_test.go index ec438ca28ad..addb6185e46 100644 --- a/pkg/cli/audit_cross_run_test.go +++ b/pkg/cli/audit_cross_run_test.go @@ -31,9 +31,11 @@ func TestBuildCrossRunAuditReport_SingleRunWithData(t *testing.T) { WorkflowName: "test-workflow", Conclusion: "success", FirewallAnalysis: &FirewallAnalysis{ - TotalRequests: 10, - AllowedRequests: 8, - BlockedRequests: 2, + AnalysisBase: AnalysisBase{ + TotalRequests: 10, + AllowedRequests: 8, + BlockedRequests: 2, + }, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 5, Blocked: 0}, "evil.example.com:443": {Allowed: 0, Blocked: 2}, @@ -72,9 +74,11 @@ func TestBuildCrossRunAuditReport_MultipleRuns(t *testing.T) { WorkflowName: "workflow-a", Conclusion: "success", FirewallAnalysis: &FirewallAnalysis{ - TotalRequests: 5, - AllowedRequests: 5, - BlockedRequests: 0, + AnalysisBase: AnalysisBase{ + TotalRequests: 5, + AllowedRequests: 5, + BlockedRequests: 0, + }, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 3, Blocked: 0}, "npm.pkg.github.com:443": {Allowed: 2, Blocked: 0}, @@ -86,9 +90,11 @@ func TestBuildCrossRunAuditReport_MultipleRuns(t *testing.T) { WorkflowName: "workflow-a", Conclusion: "failure", FirewallAnalysis: &FirewallAnalysis{ - TotalRequests: 8, - AllowedRequests: 5, - BlockedRequests: 3, + AnalysisBase: AnalysisBase{ + TotalRequests: 8, + AllowedRequests: 5, + BlockedRequests: 3, + }, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 3, Blocked: 0}, "evil.example.com:443": {Allowed: 0, Blocked: 3}, @@ -174,9 +180,11 @@ func TestBuildCrossRunAuditReport_DomainInventorySorted(t *testing.T) { WorkflowName: "wf", Conclusion: "success", FirewallAnalysis: &FirewallAnalysis{ - TotalRequests: 6, - AllowedRequests: 6, - BlockedRequests: 0, + AnalysisBase: AnalysisBase{ + TotalRequests: 6, + AllowedRequests: 6, + BlockedRequests: 0, + }, RequestsByDomain: map[string]DomainRequestStats{ "z-domain.com:443": {Allowed: 2}, "a-domain.com:443": {Allowed: 2}, diff --git a/pkg/cli/audit_diff_test.go b/pkg/cli/audit_diff_test.go index dbdd8c6c1c6..03c26f64058 100644 --- a/pkg/cli/audit_diff_test.go +++ b/pkg/cli/audit_diff_test.go @@ -13,16 +13,13 @@ import ( func TestComputeFirewallDiff_NewDomains(t *testing.T) { run1 := &FirewallAnalysis{ - TotalRequests: 5, - AllowedRequests: 5, + AnalysisBase: AnalysisBase{TotalRequests: 5, AllowedRequests: 5}, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 5, Blocked: 0}, }, } run2 := &FirewallAnalysis{ - TotalRequests: 20, - AllowedRequests: 17, - BlockedRequests: 3, + AnalysisBase: AnalysisBase{TotalRequests: 20, AllowedRequests: 17, BlockedRequests: 3}, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 5, Blocked: 0}, "registry.npmjs.org:443": {Allowed: 15, Blocked: 0}, @@ -228,9 +225,7 @@ func TestComputeFirewallDiff_NoChanges(t *testing.T) { func TestComputeFirewallDiff_CompleteScenario(t *testing.T) { run1 := &FirewallAnalysis{ - TotalRequests: 46, - AllowedRequests: 38, - BlockedRequests: 8, + AnalysisBase: AnalysisBase{TotalRequests: 46, AllowedRequests: 38, BlockedRequests: 8}, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 23, Blocked: 0}, "old-api.internal.com:443": {Allowed: 8, Blocked: 0}, @@ -239,9 +234,7 @@ func TestComputeFirewallDiff_CompleteScenario(t *testing.T) { }, } run2 := &FirewallAnalysis{ - TotalRequests: 108, - AllowedRequests: 106, - BlockedRequests: 2, + AnalysisBase: AnalysisBase{TotalRequests: 108, AllowedRequests: 106, BlockedRequests: 2}, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 89, Blocked: 0}, "registry.npmjs.org:443": {Allowed: 15, Blocked: 0}, diff --git a/pkg/cli/audit_report_test.go b/pkg/cli/audit_report_test.go index fd480da7f2e..9a35a0062d4 100644 --- a/pkg/cli/audit_report_test.go +++ b/pkg/cli/audit_report_test.go @@ -382,9 +382,11 @@ func TestGenerateFindings(t *testing.T) { processedRun: func() ProcessedRun { pr := createTestProcessedRun() pr.FirewallAnalysis = &FirewallAnalysis{ - TotalRequests: 10, - BlockedRequests: 5, - AllowedRequests: 5, + AnalysisBase: AnalysisBase{ + TotalRequests: 10, + BlockedRequests: 5, + AllowedRequests: 5, + }, } return pr }(), @@ -503,7 +505,7 @@ func TestGenerateRecommendations(t *testing.T) { processedRun: func() ProcessedRun { pr := createTestProcessedRun() pr.FirewallAnalysis = &FirewallAnalysis{ - BlockedRequests: 15, // > 10 threshold + AnalysisBase: AnalysisBase{BlockedRequests: 15}, // > 10 threshold } return pr }(), @@ -610,7 +612,7 @@ func TestGeneratePerformanceMetrics(t *testing.T) { processedRun: func() ProcessedRun { pr := createTestProcessedRun() pr.FirewallAnalysis = &FirewallAnalysis{ - TotalRequests: 50, + AnalysisBase: AnalysisBase{TotalRequests: 50}, } return pr }(), @@ -718,13 +720,15 @@ func TestBuildAuditDataComplete(t *testing.T) { {ServerName: "test-mcp", Status: "connection_error"}, }, FirewallAnalysis: &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"api.github.com"}, - BlockedDomains: []string{"blocked.example.com"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"api.github.com"}, + BlockedDomains: []string{"blocked.example.com"}, + }, + TotalRequests: 15, + AllowedRequests: 10, + BlockedRequests: 5, }, - TotalRequests: 15, - AllowedRequests: 10, - BlockedRequests: 5, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com": {Allowed: 10, Blocked: 0}, "blocked.example.com": {Allowed: 0, Blocked: 5}, @@ -1103,7 +1107,7 @@ func TestRecommendationPriorityOrdering(t *testing.T) { {Tool: "missing", Reason: "Not available"}, }, FirewallAnalysis: &FirewallAnalysis{ - BlockedRequests: 20, // Many blocked requests + AnalysisBase: AnalysisBase{BlockedRequests: 20}, // Many blocked requests }, } @@ -1634,9 +1638,11 @@ func TestGenerateFindingsFirewallWithBlockedDomains(t *testing.T) { // A single blocked domain should produce a finding naming the domain. pr := createTestProcessedRun() fw := &FirewallAnalysis{ - TotalRequests: 1, - BlockedRequests: 1, - AllowedRequests: 0, + AnalysisBase: AnalysisBase{ + TotalRequests: 1, + BlockedRequests: 1, + AllowedRequests: 0, + }, RequestsByDomain: map[string]DomainRequestStats{}, } fw.SetBlockedDomains([]string{"chatgpt.com"}) @@ -1661,9 +1667,11 @@ func TestGenerateRecommendationsFirewallSingleBlock(t *testing.T) { // should generate a recommendation with the domain name in the example. pr := createTestProcessedRun() fw := &FirewallAnalysis{ - TotalRequests: 1, - BlockedRequests: 1, - AllowedRequests: 0, + AnalysisBase: AnalysisBase{ + TotalRequests: 1, + BlockedRequests: 1, + AllowedRequests: 0, + }, RequestsByDomain: map[string]DomainRequestStats{}, } fw.SetBlockedDomains([]string{"chatgpt.com"}) @@ -1692,9 +1700,11 @@ func TestGenerateRecommendationsFiltersDashPlaceholder(t *testing.T) { // still produce "-" entries. pr := createTestProcessedRun() fw := &FirewallAnalysis{ - TotalRequests: 1, - BlockedRequests: 1, - AllowedRequests: 0, + AnalysisBase: AnalysisBase{ + TotalRequests: 1, + BlockedRequests: 1, + AllowedRequests: 0, + }, RequestsByDomain: map[string]DomainRequestStats{"-": {Blocked: 1}}, } fw.SetBlockedDomains([]string{"-"}) @@ -1721,9 +1731,11 @@ func TestGenerateRecommendationsFiltersUnknownSentinel(t *testing.T) { // sentinel in the allow-list example. pr := createTestProcessedRun() fw := &FirewallAnalysis{ - TotalRequests: 1, - BlockedRequests: 1, - AllowedRequests: 0, + AnalysisBase: AnalysisBase{ + TotalRequests: 1, + BlockedRequests: 1, + AllowedRequests: 0, + }, RequestsByDomain: map[string]DomainRequestStats{unknownDomain: {Blocked: 1}}, } fw.SetBlockedDomains([]string{unknownDomain}) diff --git a/pkg/cli/audit_test.go b/pkg/cli/audit_test.go index 033a3c79841..827cd395661 100644 --- a/pkg/cli/audit_test.go +++ b/pkg/cli/audit_test.go @@ -725,13 +725,15 @@ func TestBuildAuditDataWithFirewall(t *testing.T) { } firewallAnalysis := &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"api.github.com:443", "npmjs.org:443"}, - BlockedDomains: []string{"blocked.example.com:443"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"api.github.com:443", "npmjs.org:443"}, + BlockedDomains: []string{"blocked.example.com:443"}, + }, + TotalRequests: 10, + AllowedRequests: 7, + BlockedRequests: 3, }, - TotalRequests: 10, - AllowedRequests: 7, - BlockedRequests: 3, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 5, Blocked: 0}, "npmjs.org:443": {Allowed: 2, Blocked: 0}, @@ -775,13 +777,15 @@ func TestBuildAuditDataWithFirewall(t *testing.T) { func TestRenderJSONWithFirewall(t *testing.T) { // Create test audit data with firewall analysis firewallAnalysis := &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"api.github.com:443"}, - BlockedDomains: []string{"blocked.example.com:443"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"api.github.com:443"}, + BlockedDomains: []string{"blocked.example.com:443"}, + }, + TotalRequests: 10, + AllowedRequests: 7, + BlockedRequests: 3, }, - TotalRequests: 10, - AllowedRequests: 7, - BlockedRequests: 3, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 7, Blocked: 0}, "blocked.example.com:443": {Allowed: 0, Blocked: 3}, diff --git a/pkg/cli/domain_buckets.go b/pkg/cli/domain_buckets.go index 8f3ed52ca05..dc6d0e9f9be 100644 --- a/pkg/cli/domain_buckets.go +++ b/pkg/cli/domain_buckets.go @@ -1,5 +1,7 @@ package cli +import "github.com/github/gh-aw/pkg/sliceutil" + // DomainBuckets holds allowed and blocked domain lists with accessor methods. // This struct is embedded by DomainAnalysis and FirewallAnalysis to share // domain management functionality and eliminate code duplication. @@ -27,3 +29,40 @@ func (d *DomainBuckets) SetAllowedDomains(domains []string) { func (d *DomainBuckets) SetBlockedDomains(domains []string) { d.BlockedDomains = domains } + +// AnalysisBase is the shared base embedded by DomainAnalysis and FirewallAnalysis. +// It holds the common counters and domain lists that both analysis types share, +// and provides a single AddMetrics implementation for the shared fields. +type AnalysisBase struct { + DomainBuckets + TotalRequests int `json:"total_requests"` + AllowedRequests int `json:"allowed_requests"` + BlockedRequests int `json:"blocked_requests"` +} + +// addBaseMetrics merges TotalRequests, AllowedRequests, BlockedRequests and domain +// lists from other into a. It is called by DomainAnalysis.AddMetrics and +// FirewallAnalysis.AddMetrics to eliminate the shared accumulation logic. +func (a *AnalysisBase) addBaseMetrics(other *AnalysisBase) { + a.TotalRequests += other.TotalRequests + a.AllowedRequests += other.AllowedRequests + a.BlockedRequests += other.BlockedRequests + a.BlockedDomains = mergeDomainList(a.BlockedDomains, other.BlockedDomains) + a.AllowedDomains = mergeDomainList(a.AllowedDomains, other.AllowedDomains) +} + +// mergeDomainList returns a sorted, deduplicated union of existing and incoming domain lists. +// If incoming is empty, existing is returned unchanged. +func mergeDomainList(existing, incoming []string) []string { + if len(incoming) == 0 { + return existing + } + domainSet := make(map[string]struct{}, len(existing)+len(incoming)) + for _, d := range existing { + domainSet[d] = struct{}{} + } + for _, d := range incoming { + domainSet[d] = struct{}{} + } + return sliceutil.SortedKeys(domainSet) +} diff --git a/pkg/cli/firewall_log.go b/pkg/cli/firewall_log.go index bf5704656d4..c2cadb62922 100644 --- a/pkg/cli/firewall_log.go +++ b/pkg/cli/firewall_log.go @@ -131,47 +131,16 @@ type FirewallLogEntry struct { // FirewallAnalysis represents analysis of firewall logs // This mirrors the structure from the JavaScript parser type FirewallAnalysis struct { - DomainBuckets - TotalRequests int `json:"total_requests"` - AllowedRequests int `json:"allowed_requests"` - BlockedRequests int `json:"blocked_requests"` + AnalysisBase RequestsByDomain map[string]DomainRequestStats `json:"requests_by_domain,omitempty"` } // AddMetrics adds metrics from another analysis func (f *FirewallAnalysis) AddMetrics(other LogAnalysis) { if otherFirewall, ok := other.(*FirewallAnalysis); ok { - f.TotalRequests += otherFirewall.TotalRequests - f.AllowedRequests += otherFirewall.AllowedRequests - f.BlockedRequests += otherFirewall.BlockedRequests - - // Merge blocked domain lists - if len(otherFirewall.BlockedDomains) > 0 { - domainSet := make(map[string]struct{}, len(f.BlockedDomains)+len(otherFirewall.BlockedDomains)) - for _, d := range f.BlockedDomains { - domainSet[d] = struct{}{} - } - for _, d := range otherFirewall.BlockedDomains { - domainSet[d] = struct{}{} - } - merged := sliceutil.SortedKeys(domainSet) - f.SetBlockedDomains(merged) - } - - // Merge allowed domain lists - if len(otherFirewall.AllowedDomains) > 0 { - domainSet := make(map[string]struct{}, len(f.AllowedDomains)+len(otherFirewall.AllowedDomains)) - for _, d := range f.AllowedDomains { - domainSet[d] = struct{}{} - } - for _, d := range otherFirewall.AllowedDomains { - domainSet[d] = struct{}{} - } - merged := sliceutil.SortedKeys(domainSet) - f.SetAllowedDomains(merged) - } + f.addBaseMetrics(&otherFirewall.AnalysisBase) - // Merge request stats by domain + // Merge request stats by domain (firewall-specific) if f.RequestsByDomain == nil { f.RequestsByDomain = make(map[string]DomainRequestStats) } @@ -512,9 +481,10 @@ func extractFirewallFromAgentLog(logsPath string, verbose bool) *FirewallAnalysi blockedDomains := sliceutil.SortedKeys(blockedDomainsSet) analysis := &FirewallAnalysis{ - TotalRequests: len(blockedDomains), - AllowedRequests: 0, - BlockedRequests: len(blockedDomains), + AnalysisBase: AnalysisBase{ + TotalRequests: len(blockedDomains), + BlockedRequests: len(blockedDomains), + }, RequestsByDomain: make(map[string]DomainRequestStats), } analysis.SetBlockedDomains(blockedDomains) diff --git a/pkg/cli/firewall_log_integration_test.go b/pkg/cli/firewall_log_integration_test.go index 4e5dcbf7b4d..78c6c603fad 100644 --- a/pkg/cli/firewall_log_integration_test.go +++ b/pkg/cli/firewall_log_integration_test.go @@ -146,13 +146,15 @@ func TestFirewallLogSummaryBuilding(t *testing.T) { WorkflowName: "workflow-1", }, FirewallAnalysis: &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"api.github.com:443", "api.npmjs.org:443"}, - BlockedDomains: []string{"blocked.example.com:443"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"api.github.com:443", "api.npmjs.org:443"}, + BlockedDomains: []string{"blocked.example.com:443"}, + }, + TotalRequests: 10, + AllowedRequests: 8, + BlockedRequests: 2, }, - TotalRequests: 10, - AllowedRequests: 8, - BlockedRequests: 2, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 5, Blocked: 0}, "api.npmjs.org:443": {Allowed: 3, Blocked: 0}, @@ -165,13 +167,15 @@ func TestFirewallLogSummaryBuilding(t *testing.T) { WorkflowName: "workflow-2", }, FirewallAnalysis: &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"api.github.com:443"}, - BlockedDomains: []string{"denied.site:443"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"api.github.com:443"}, + BlockedDomains: []string{"denied.site:443"}, + }, + TotalRequests: 5, + AllowedRequests: 3, + BlockedRequests: 2, }, - TotalRequests: 5, - AllowedRequests: 3, - BlockedRequests: 2, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 3, Blocked: 0}, "denied.site:443": {Allowed: 0, Blocked: 2}, diff --git a/pkg/cli/firewall_log_test.go b/pkg/cli/firewall_log_test.go index 91958137a3f..bb44e6c6370 100644 --- a/pkg/cli/firewall_log_test.go +++ b/pkg/cli/firewall_log_test.go @@ -969,18 +969,14 @@ func TestExtractFirewallFromAgentLogNoFile(t *testing.T) { func TestFirewallAnalysisAddMetricsMergesDomains(t *testing.T) { base := &FirewallAnalysis{ - TotalRequests: 2, - AllowedRequests: 1, - BlockedRequests: 1, + AnalysisBase: AnalysisBase{TotalRequests: 2, AllowedRequests: 1, BlockedRequests: 1}, RequestsByDomain: map[string]DomainRequestStats{}, } base.SetBlockedDomains([]string{"blocked-a.com"}) base.SetAllowedDomains([]string{"allowed-a.com"}) other := &FirewallAnalysis{ - TotalRequests: 2, - AllowedRequests: 1, - BlockedRequests: 1, + AnalysisBase: AnalysisBase{TotalRequests: 2, AllowedRequests: 1, BlockedRequests: 1}, RequestsByDomain: map[string]DomainRequestStats{}, } other.SetBlockedDomains([]string{"blocked-b.com"}) @@ -1109,16 +1105,14 @@ func TestAnalyzeFirewallLogsSandboxEmptySquidSubdirFallsBackToTopLevel(t *testin func TestFirewallAnalysisAddMetricsDeduplicatesDomains(t *testing.T) { base := &FirewallAnalysis{ - TotalRequests: 1, - BlockedRequests: 1, + AnalysisBase: AnalysisBase{TotalRequests: 1, BlockedRequests: 1}, RequestsByDomain: map[string]DomainRequestStats{}, } base.SetBlockedDomains([]string{"chatgpt.com"}) // Same domain added again (e.g. from two different log sources) other := &FirewallAnalysis{ - TotalRequests: 1, - BlockedRequests: 1, + AnalysisBase: AnalysisBase{TotalRequests: 1, BlockedRequests: 1}, RequestsByDomain: map[string]DomainRequestStats{}, } other.SetBlockedDomains([]string{"chatgpt.com"}) diff --git a/pkg/cli/log_aggregation_test.go b/pkg/cli/log_aggregation_test.go index b1c348d7898..eefcad2e358 100644 --- a/pkg/cli/log_aggregation_test.go +++ b/pkg/cli/log_aggregation_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" ) // Test that DomainAnalysis implements LogAnalysis interface @@ -32,9 +33,11 @@ func TestFirewallAnalysisImplementsMutableLogAnalysis(t *testing.T) { func TestDomainAnalysisGettersSetters(t *testing.T) { analysis := &DomainAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"example.com", "test.com"}, - BlockedDomains: []string{"blocked.com"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"example.com", "test.com"}, + BlockedDomains: []string{"blocked.com"}, + }, }, } @@ -65,15 +68,25 @@ func TestDomainAnalysisGettersSetters(t *testing.T) { func TestDomainAnalysisAddMetrics(t *testing.T) { analysis1 := &DomainAnalysis{ - TotalRequests: 10, - AllowedCount: 6, - BlockedCount: 4, + AnalysisBase: AnalysisBase{ + TotalRequests: 10, AllowedRequests: 6, BlockedRequests: 4, + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"api.github.com", "example.com"}, + BlockedDomains: []string{"blocked.com"}, + }, + }, } analysis2 := &DomainAnalysis{ - TotalRequests: 5, - AllowedCount: 3, - BlockedCount: 2, + AnalysisBase: AnalysisBase{ + TotalRequests: 5, AllowedRequests: 3, BlockedRequests: 2, + DomainBuckets: DomainBuckets{ + // "example.com" is a duplicate; "new.com" is new + AllowedDomains: []string{"example.com", "new.com"}, + // "extra-blocked.com" is new + BlockedDomains: []string{"blocked.com", "extra-blocked.com"}, + }, + }, } analysis1.AddMetrics(analysis2) @@ -81,19 +94,25 @@ func TestDomainAnalysisAddMetrics(t *testing.T) { if analysis1.TotalRequests != 15 { t.Errorf("Expected TotalRequests 15, got %d", analysis1.TotalRequests) } - if analysis1.AllowedCount != 9 { - t.Errorf("Expected AllowedCount 9, got %d", analysis1.AllowedCount) + if analysis1.AllowedRequests != 9 { + t.Errorf("Expected AllowedRequests 9, got %d", analysis1.AllowedRequests) } - if analysis1.BlockedCount != 6 { - t.Errorf("Expected BlockedCount 6, got %d", analysis1.BlockedCount) + if analysis1.BlockedRequests != 6 { + t.Errorf("Expected BlockedRequests 6, got %d", analysis1.BlockedRequests) } + + // Domain lists must be deduplicated and sorted. + assert.Equal(t, []string{"api.github.com", "example.com", "new.com"}, analysis1.AllowedDomains) + assert.Equal(t, []string{"blocked.com", "extra-blocked.com"}, analysis1.BlockedDomains) } func TestFirewallAnalysisGettersSetters(t *testing.T) { analysis := &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"api.github.com:443", "api.npmjs.org:443"}, - BlockedDomains: []string{"blocked.example.com:443"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"api.github.com:443", "api.npmjs.org:443"}, + BlockedDomains: []string{"blocked.example.com:443"}, + }, }, RequestsByDomain: make(map[string]DomainRequestStats), } @@ -125,18 +144,14 @@ func TestFirewallAnalysisGettersSetters(t *testing.T) { func TestFirewallAnalysisAddMetrics(t *testing.T) { analysis1 := &FirewallAnalysis{ - TotalRequests: 10, - AllowedRequests: 6, - BlockedRequests: 4, + AnalysisBase: AnalysisBase{TotalRequests: 10, AllowedRequests: 6, BlockedRequests: 4}, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 3, Blocked: 1}, }, } analysis2 := &FirewallAnalysis{ - TotalRequests: 5, - AllowedRequests: 3, - BlockedRequests: 2, + AnalysisBase: AnalysisBase{TotalRequests: 5, AllowedRequests: 3, BlockedRequests: 2}, RequestsByDomain: map[string]DomainRequestStats{ "api.github.com:443": {Allowed: 2, Blocked: 0}, "api.npmjs.org:443": {Allowed: 1, Blocked: 2}, @@ -209,12 +224,7 @@ func TestAggregateLogFilesWithAccessLogs(t *testing.T) { false, parseSquidAccessLog, func() *DomainAnalysis { - return &DomainAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{}, - BlockedDomains: []string{}, - }, - } + return &DomainAnalysis{} }, ) @@ -227,12 +237,12 @@ func TestAggregateLogFilesWithAccessLogs(t *testing.T) { t.Errorf("Expected 4 total requests, got %d", analysis.TotalRequests) } - if analysis.AllowedCount != 2 { - t.Errorf("Expected 2 allowed requests, got %d", analysis.AllowedCount) + if analysis.AllowedRequests != 2 { + t.Errorf("Expected 2 allowed requests, got %d", analysis.AllowedRequests) } - if analysis.BlockedCount != 2 { - t.Errorf("Expected 2 denied requests, got %d", analysis.BlockedCount) + if analysis.BlockedRequests != 2 { + t.Errorf("Expected 2 denied requests, got %d", analysis.BlockedRequests) } // Check allowed domains @@ -285,10 +295,6 @@ func TestAggregateLogFilesWithFirewallLogs(t *testing.T) { parseFirewallLog, func() *FirewallAnalysis { return &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{}, - BlockedDomains: []string{}, - }, RequestsByDomain: make(map[string]DomainRequestStats), } }, @@ -334,12 +340,7 @@ func TestAggregateLogFilesNoFiles(t *testing.T) { false, parseSquidAccessLog, func() *DomainAnalysis { - return &DomainAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{}, - BlockedDomains: []string{}, - }, - } + return &DomainAnalysis{} }, ) @@ -383,12 +384,7 @@ func TestAggregateLogFilesWithParseErrors(t *testing.T) { false, parseSquidAccessLog, func() *DomainAnalysis { - return &DomainAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{}, - BlockedDomains: []string{}, - }, - } + return &DomainAnalysis{} }, ) diff --git a/pkg/cli/logs_episode_test.go b/pkg/cli/logs_episode_test.go index 5da7ea8fcec..d2a24a33315 100644 --- a/pkg/cli/logs_episode_test.go +++ b/pkg/cli/logs_episode_test.go @@ -95,9 +95,11 @@ func TestBuildEpisodeDataSetsBlockedAtCapWhenFirewallCountHitsCap(t *testing.T) WorkflowName: "firewall-heavy", }, FirewallAnalysis: &FirewallAnalysis{ - TotalRequests: 100, - BlockedRequests: firewallBlockedRequestCap, // exactly at the proxy cap - AllowedRequests: 50, + AnalysisBase: AnalysisBase{ + TotalRequests: 100, + BlockedRequests: firewallBlockedRequestCap, // exactly at the proxy cap + AllowedRequests: 50, + }, }, }, } @@ -126,9 +128,11 @@ func TestBuildEpisodeDataDoesNotSetBlockedAtCapBelowThreshold(t *testing.T) { WorkflowName: "low-block", }, FirewallAnalysis: &FirewallAnalysis{ - TotalRequests: 100, - BlockedRequests: 8, - AllowedRequests: 92, + AnalysisBase: AnalysisBase{ + TotalRequests: 100, + BlockedRequests: 8, + AllowedRequests: 92, + }, }, }, } diff --git a/pkg/cli/logs_report_firewall.go b/pkg/cli/logs_report_firewall.go index fd47d250e48..87d86d6cd67 100644 --- a/pkg/cli/logs_report_firewall.go +++ b/pkg/cli/logs_report_firewall.go @@ -87,8 +87,8 @@ func buildAccessLogSummary(processedRuns []ProcessedRun) *AccessLogSummary { return pr.AccessAnalysis.AllowedDomains, pr.AccessAnalysis.BlockedDomains, pr.AccessAnalysis.TotalRequests, - pr.AccessAnalysis.AllowedCount, - pr.AccessAnalysis.BlockedCount, + pr.AccessAnalysis.AllowedRequests, + pr.AccessAnalysis.BlockedRequests, true }) diff --git a/pkg/cli/logs_report_test.go b/pkg/cli/logs_report_test.go index f9d28daaeed..3d6f9088c6f 100644 --- a/pkg/cli/logs_report_test.go +++ b/pkg/cli/logs_report_test.go @@ -505,24 +505,28 @@ func TestAggregateDomainStats(t *testing.T) { processedRuns := []ProcessedRun{ { AccessAnalysis: &DomainAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"example.com", "api.github.com"}, - BlockedDomains: []string{"blocked.com"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"example.com", "api.github.com"}, + BlockedDomains: []string{"blocked.com"}, + }, + TotalRequests: 10, + AllowedRequests: 8, + BlockedRequests: 2, }, - TotalRequests: 10, - AllowedCount: 8, - BlockedCount: 2, }, }, { AccessAnalysis: &DomainAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"api.github.com", "docs.github.com"}, - BlockedDomains: []string{"spam.com"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"api.github.com", "docs.github.com"}, + BlockedDomains: []string{"spam.com"}, + }, + TotalRequests: 5, + AllowedRequests: 4, + BlockedRequests: 1, }, - TotalRequests: 5, - AllowedCount: 4, - BlockedCount: 1, }, }, } @@ -534,8 +538,8 @@ func TestAggregateDomainStats(t *testing.T) { return pr.AccessAnalysis.AllowedDomains, pr.AccessAnalysis.BlockedDomains, pr.AccessAnalysis.TotalRequests, - pr.AccessAnalysis.AllowedCount, - pr.AccessAnalysis.BlockedCount, + pr.AccessAnalysis.AllowedRequests, + pr.AccessAnalysis.BlockedRequests, true }) @@ -576,12 +580,14 @@ func TestAggregateDomainStats(t *testing.T) { }, { AccessAnalysis: &DomainAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"example.com"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"example.com"}, + }, + TotalRequests: 5, + AllowedRequests: 5, + BlockedRequests: 0, }, - TotalRequests: 5, - AllowedCount: 5, - BlockedCount: 0, }, }, } @@ -593,8 +599,8 @@ func TestAggregateDomainStats(t *testing.T) { return pr.AccessAnalysis.AllowedDomains, pr.AccessAnalysis.BlockedDomains, pr.AccessAnalysis.TotalRequests, - pr.AccessAnalysis.AllowedCount, - pr.AccessAnalysis.BlockedCount, + pr.AccessAnalysis.AllowedRequests, + pr.AccessAnalysis.BlockedRequests, true }) @@ -686,13 +692,15 @@ func TestBuildAccessLogSummaryWithSharedHelper(t *testing.T) { WorkflowName: "workflow-a", }, AccessAnalysis: &DomainAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"example.com", "api.github.com"}, - BlockedDomains: []string{"blocked.com"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"example.com", "api.github.com"}, + BlockedDomains: []string{"blocked.com"}, + }, + TotalRequests: 10, + AllowedRequests: 8, + BlockedRequests: 2, }, - TotalRequests: 10, - AllowedCount: 8, - BlockedCount: 2, }, }, { @@ -700,13 +708,15 @@ func TestBuildAccessLogSummaryWithSharedHelper(t *testing.T) { WorkflowName: "workflow-b", }, AccessAnalysis: &DomainAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"docs.github.com"}, - BlockedDomains: []string{}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"docs.github.com"}, + BlockedDomains: []string{}, + }, + TotalRequests: 5, + AllowedRequests: 5, + BlockedRequests: 0, }, - TotalRequests: 5, - AllowedCount: 5, - BlockedCount: 0, }, }, } @@ -756,13 +766,15 @@ func TestBuildFirewallLogSummaryWithSharedHelper(t *testing.T) { WorkflowName: "workflow-a", }, FirewallAnalysis: &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"example.com"}, - BlockedDomains: []string{"blocked.com"}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"example.com"}, + BlockedDomains: []string{"blocked.com"}, + }, + TotalRequests: 10, + AllowedRequests: 8, + BlockedRequests: 2, }, - TotalRequests: 10, - AllowedRequests: 8, - BlockedRequests: 2, RequestsByDomain: map[string]DomainRequestStats{ "example.com": {Allowed: 8, Blocked: 0}, "blocked.com": {Allowed: 0, Blocked: 2}, @@ -774,13 +786,15 @@ func TestBuildFirewallLogSummaryWithSharedHelper(t *testing.T) { WorkflowName: "workflow-b", }, FirewallAnalysis: &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"example.com", "api.github.com"}, - BlockedDomains: []string{}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"example.com", "api.github.com"}, + BlockedDomains: []string{}, + }, + TotalRequests: 5, + AllowedRequests: 5, + BlockedRequests: 0, }, - TotalRequests: 5, - AllowedRequests: 5, - BlockedRequests: 0, RequestsByDomain: map[string]DomainRequestStats{ "example.com": {Allowed: 3, Blocked: 0}, "api.github.com": {Allowed: 2, Blocked: 0}, diff --git a/pkg/cli/logs_summary_test.go b/pkg/cli/logs_summary_test.go index 247c19ad7be..02876d4761c 100644 --- a/pkg/cli/logs_summary_test.go +++ b/pkg/cli/logs_summary_test.go @@ -283,13 +283,15 @@ func TestRunSummaryJSONStructure(t *testing.T) { Turns: 5, }, AccessAnalysis: &DomainAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: []string{"github.com", "api.github.com"}, - BlockedDomains: []string{}, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: []string{"github.com", "api.github.com"}, + BlockedDomains: []string{}, + }, + TotalRequests: 10, + AllowedRequests: 10, + BlockedRequests: 0, }, - TotalRequests: 10, - AllowedCount: 10, - BlockedCount: 0, }, MissingTools: []MissingToolReport{ { diff --git a/pkg/cli/logs_usage_activity.go b/pkg/cli/logs_usage_activity.go index ace34e98921..ee06e977c82 100644 --- a/pkg/cli/logs_usage_activity.go +++ b/pkg/cli/logs_usage_activity.go @@ -130,13 +130,15 @@ func applyUsageActivitySummaryToResult(summary *usageActivitySummary, result *Do blockedDomains := sliceutil.SortedKeys(blockedSet) result.FirewallAnalysis = &FirewallAnalysis{ - DomainBuckets: DomainBuckets{ - AllowedDomains: allowedDomains, - BlockedDomains: blockedDomains, + AnalysisBase: AnalysisBase{ + DomainBuckets: DomainBuckets{ + AllowedDomains: allowedDomains, + BlockedDomains: blockedDomains, + }, + TotalRequests: summary.Firewall.TotalRequests, + AllowedRequests: summary.Firewall.AllowedRequests, + BlockedRequests: summary.Firewall.BlockedRequests, }, - TotalRequests: summary.Firewall.TotalRequests, - AllowedRequests: summary.Firewall.AllowedRequests, - BlockedRequests: summary.Firewall.BlockedRequests, RequestsByDomain: requestsByDomain, } } diff --git a/pkg/cli/logs_usage_activity_test.go b/pkg/cli/logs_usage_activity_test.go index 470f7f49fd7..30402a46c26 100644 --- a/pkg/cli/logs_usage_activity_test.go +++ b/pkg/cli/logs_usage_activity_test.go @@ -177,7 +177,7 @@ func TestLoadUsageActivitySummaryRejectsUnsupportedSchema(t *testing.T) { func TestApplyUsageActivitySummaryDoesNotOverwriteExistingData(t *testing.T) { t.Parallel() - existingFirewall := &FirewallAnalysis{TotalRequests: 100} + existingFirewall := &FirewallAnalysis{AnalysisBase: AnalysisBase{TotalRequests: 100}} existingMCP := &MCPToolUsageData{ Summary: []MCPToolSummary{}, ToolCalls: []MCPToolCall{}, diff --git a/pkg/cli/observability_insights_test.go b/pkg/cli/observability_insights_test.go index a9e0ba6ffb5..3ac8175bb94 100644 --- a/pkg/cli/observability_insights_test.go +++ b/pkg/cli/observability_insights_test.go @@ -24,9 +24,11 @@ func TestBuildAuditObservabilityInsights(t *testing.T) { MCPFailures: []MCPFailureReport{{ServerName: "github"}}, MissingData: []MissingDataReport{{DataType: "issue_body"}}, FirewallAnalysis: &FirewallAnalysis{ - TotalRequests: 20, - BlockedRequests: 8, - AllowedRequests: 12, + AnalysisBase: AnalysisBase{ + TotalRequests: 20, + BlockedRequests: 8, + AllowedRequests: 12, + }, }, RedactedDomainsAnalysis: &RedactedDomainsAnalysis{TotalDomains: 3}, } @@ -60,12 +62,12 @@ func TestBuildLogsObservabilityInsights(t *testing.T) { { Run: WorkflowRun{WorkflowName: "triage", Conclusion: "failure", Turns: 3, SafeItemsCount: 0}, MissingTools: []MissingToolReport{{Tool: "terraform"}}, - FirewallAnalysis: &FirewallAnalysis{TotalRequests: 10, BlockedRequests: 1}, + FirewallAnalysis: &FirewallAnalysis{AnalysisBase: AnalysisBase{TotalRequests: 10, BlockedRequests: 1}}, }, { Run: WorkflowRun{WorkflowName: "triage", Conclusion: "failure", Turns: 9, SafeItemsCount: 1}, MCPFailures: []MCPFailureReport{{ServerName: "github"}}, - FirewallAnalysis: &FirewallAnalysis{TotalRequests: 10, BlockedRequests: 7}, + FirewallAnalysis: &FirewallAnalysis{AnalysisBase: AnalysisBase{TotalRequests: 10, BlockedRequests: 7}}, }, { Run: WorkflowRun{WorkflowName: "docs", Conclusion: "success", Turns: 2, SafeItemsCount: 1}, @@ -100,9 +102,11 @@ func TestBuildAuditObservabilityInsightsSuppressesHighSeverityAtFirewallCap(t *t processedRun := ProcessedRun{ Run: WorkflowRun{Turns: 3}, FirewallAnalysis: &FirewallAnalysis{ - TotalRequests: 200, - BlockedRequests: firewallBlockedRequestCap, // 50 — at cap - AllowedRequests: 150, + AnalysisBase: AnalysisBase{ + TotalRequests: 200, + BlockedRequests: firewallBlockedRequestCap, // 50 — at cap + AllowedRequests: 150, + }, }, } @@ -124,9 +128,11 @@ func TestBuildAuditObservabilityInsightsHighSeverityWhenHighBlockRate(t *testing processedRun := ProcessedRun{ Run: WorkflowRun{Turns: 3}, FirewallAnalysis: &FirewallAnalysis{ - TotalRequests: 60, - BlockedRequests: firewallBlockedRequestCap, // 50 out of 60 → 83% block rate - AllowedRequests: 10, + AnalysisBase: AnalysisBase{ + TotalRequests: 60, + BlockedRequests: firewallBlockedRequestCap, // 50 out of 60 → 83% block rate + AllowedRequests: 10, + }, }, } @@ -150,9 +156,11 @@ func TestBuildLogsObservabilityInsightsSuppressesHighSeverityAtFirewallCap(t *te { Run: WorkflowRun{WorkflowName: "w1", Conclusion: "success", Turns: 5}, FirewallAnalysis: &FirewallAnalysis{ - TotalRequests: 500, - BlockedRequests: firewallBlockedRequestCap, // 50/500 = 10% → low rate but >=10 - AllowedRequests: 450, + AnalysisBase: AnalysisBase{ + TotalRequests: 500, + BlockedRequests: firewallBlockedRequestCap, // 50/500 = 10% → low rate but >=10 + AllowedRequests: 450, + }, }, }, }