Extract shared AnalysisBase from DomainAnalysis/FirewallAnalysis

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
 }

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

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

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

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

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