Extract shared base for access and firewall log summaries

[Copilot]

pkg/cli/logs_report_firewall.go had two near-duplicate summary structs that carried the same request/domain aggregate fields with inconsistent counter names. This refactor consolidates the shared shape into one embedded base and aligns access-log summary counters with the firewall summary.

• Shared summary base

  • Introduces FirewallSummaryBase for:
    • TotalRequests
    • AllowedRequests
    • BlockedRequests
    • AllowedDomains
    • BlockedDomains
  • Embeds the base in both AccessLogSummary and FirewallLogSummary

• Counter name alignment

  • Renames access summary counters from AllowedCount / BlockedCount to AllowedRequests / BlockedRequests
  • Keeps firewall-specific data (RequestsByDomain) only on FirewallLogSummary
  • Keeps per-workflow payloads specific to each summary type

• Construction updates

  • Updates both summary builders to populate the embedded base from the shared aggregation helper instead of duplicating field assignment

• Focused coverage

  • Updates access-summary expectations to use the aligned field names
  • Adds a JSON serialization check to confirm embedded fields stay flattened and emit allowed_requests / blocked_requests

type FirewallSummaryBase struct {
	TotalRequests   int      `json:"total_requests"`
	AllowedRequests int      `json:"allowed_requests"`
	BlockedRequests int      `json:"blocked_requests"`
	AllowedDomains  []string `json:"allowed_domains"`
	BlockedDomains  []string `json:"blocked_domains"`
}

type AccessLogSummary struct {
	FirewallSummaryBase
	ByWorkflow map[string]*DomainAnalysis `json:"by_workflow,omitempty"`
}

[Copilot]

Pull request overview

Consolidates shared access and firewall summary fields while aligning counter names.

Changes:

• Adds the embedded FirewallSummaryBase.
• Renames access counters and verifies flattened JSON serialization.
• Updates both summary builders to initialize the shared base.

Show a summary per file

File	Description
pkg/cli/logs_report_firewall.go	Defines and populates the shared summary base.
pkg/cli/logs_report_test.go	Updates assertions and tests JSON field names.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Medium

[github-actions]

Hey @copilot-swe-agent -- great work refactoring the duplicate summary structs in pkg/cli/logs_report_firewall.go! Introducing FirewallSummaryBase as a shared embedded base and aligning the counter names is a clean improvement. The PR is focused, well-described, and includes test coverage. Looks ready for review!

Generated by ✅ Contribution Check · 155.6 AIC · ⌖ 8.97 AIC · ⊞ 6.2K · ◷

[pelikhan]

/matt

[pelikhan]

/review

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 88/100 — Excellent

Analyzed 5 test(s): 5 design, 0 implementation, 0 violation(s).

📊 Metrics (5 tests)

Metric	Value
Analyzed	5 (Go: 5, JS: 0)
✅ Design	5 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	3 (60%)
Duplicate clusters	0
Inflation	No (1.48:1)
🚨 Violations	0

Test	File	Classification	Issues
TestAggregateDomainStats	logs_report_test.go:504	design_test / high_value	None
TestConvertDomainsToSortedSlices	logs_report_test.go:627	design_test / high_value	None
TestBuildAccessLogSummaryWithSharedHelper	logs_report_test.go:683	design_test / high_value	None
TestAccessLogSummaryJSONUsesEmbeddedBaseFields	logs_report_test.go:752	design_test / high_value	None
TestBuildFirewallLogSummaryWithSharedHelper	logs_report_test.go:788	design_test / high_value	None

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). All 5 new tests are behavioral contract tests covering the refactored shared aggregateDomainStats helper. Build tag present (//go:build !integration). No mock violations. Test inflation ratio 1.48:1 (below 2:1 threshold).

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 34.3 AIC · ⌖ 10.2 AIC · ⊞ 6.8K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 88/100. 0% implementation tests (threshold: 30%). All 5 new tests cover behavioral contracts of the refactored shared aggregation helper.

[github-actions]

Skills-Based Review 🧠

Applied /tdd and /codebase-design — one minor suggestion on test coverage symmetry.

📋 Key Themes & Highlights

Key Themes

• Good deduplication: FirewallSummaryBase cleanly eliminates the duplicate struct fields and aligns counter names across both summary types.
• Test coverage gap: TestAccessLogSummaryJSONUsesEmbeddedBaseFields covers only AccessLogSummary JSON serialization; FirewallLogSummary lacks a parallel test for the same embedded fields.

Positive Highlights

• ✅ FirewallSummaryBase embedding is the right abstraction — single source of truth for shared fields.
• ✅ Counter name alignment (AllowedRequests / BlockedRequests) improves consistency across the API surface.
• ✅ JSON serialization test correctly asserts that legacy field names are absent from the output.
• ✅ Constructor functions cleanly populate the base literal rather than scattering assignments.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 19.6 AIC · ⌖ 4.57 AIC · ⊞ 6.7K
_{Comment /matt to run again}

[github-actions]

Two issues require attention before merging

The refactor cleanly eliminates duplication, but has a breaking wire-format change and a naming problem.

1. Silent breaking JSON rename (high) — AccessLogSummary previously serialized AllowedCount as allowed_count and BlockedCount as blocked_count. After this PR those keys become allowed_requests and blocked_requests. Any consumer parsing the old keys will silently see zeros. No migration path or deprecation notice is provided.

2. Misleading shared-base name (medium) — FirewallSummaryBase is now embedded in AccessLogSummary, which is not a firewall type. The name implies firewall ownership. Rename to something neutral like RequestSummaryBase.

🔎 Code quality review by PR Code Quality Reviewer · 56.2 AIC · ⌖ 4.61 AIC · ⊞ 5.6K
_{Comment /review to run again}

Done

[github-actions]

🤖 PR Triage

Field	Value
Category	refactor
Risk	🟢 Low
Score	43/100
Action	✅ auto_merge

Score breakdown: Impact 18 + Urgency 10 + Quality 15

Rationale: Consolidates near-duplicate structs in pkg/cli/logs_report_firewall.go into a shared base. No behavior change. CI passes (30 checks, all green). Safe to auto-merge.

Run §29545908133

Generated by 🔧 PR Triage Agent · 181.7 AIC · ⌖ 5.04 AIC · ⊞ 5.6K · ◷

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.12