Reduce lint-monster function-length backlog in pkg/cli by extracting audit diff helpers#47987
pkg/cli by extracting audit diff helpers#47987Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pkg/cli by extracting audit diff helpers
There was a problem hiding this comment.
Pull request overview
Refactors audit diff logic into focused helpers while preserving behavior.
Changes:
- Decomposes firewall diff processing and summary generation.
- Extracts MCP tool indexing, sorting, and classification helpers.
- Preserves deterministic ordering and anomaly detection.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/audit_diff.go |
Extracts firewall and MCP diff helpers to reduce function length. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
| for _, key := range mcpSortedKeys(run1Tools, run2Tools) { | ||
| anomalyCount += appendMCPToolDiff(diff, key, run1Tools, run2Tools) |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The refactoring is behaviorally equivalent and clean. All extracted helpers correctly preserve the original semantics. The PR description inaccuracy (claiming computeMCPToolsDiff remains above the linter limit) is already flagged in a prior review comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.3 AIC · ⌖ 5.13 AIC · ⊞ 5K
Documents the decision to extract focused helper functions from computeFirewallDiff and computeMCPToolsDiff to satisfy the project's 60-line function-length linter constraint.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (200 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on three actionable issues.
📋 Key Themes & Highlights
Key Themes
- Duplicated union-key sort pattern:
mcpSortedKeysandfirewallSortedDomainsare identical in structure; a generic helper would close this gap before more diff types are added. - Append-and-count side-effect coupling: helpers both mutate
diffand return an anomaly count, making them stateful rather than pure — a missed opportunity now that they are small enough to be pure. - No unit tests for newly isolated helpers: the refactor makes helpers independently testable for the first time; leaving them untested is the main regression risk.
Positive Highlights
- ✅ Clean extraction — each helper has a clear, single-phrase name that maps to the domain language.
- ✅ Anomaly rules and status/volume thresholds are faithfully preserved across all branching paths.
- ✅ Deterministic sort order is maintained via
sliceutil.SortedKeys. - ✅
firewallStatsByRunandmcpSummaryMapare genuinely deep — nil safety handled once, not scattered across callers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 51.6 AIC · ⌖ 5.11 AIC · ⊞ 6.7K
Comment /matt to run again
| for key := range run1Tools { | ||
| allKeys[key] = struct{}{} | ||
| } | ||
| for key := range run2Tools { |
There was a problem hiding this comment.
[/codebase-design] mcpSortedKeys and firewallSortedDomains are structurally identical — both union two maps into a sorted key slice. This duplication will compound as more diff helpers are extracted.
💡 Suggested unification
A small generic helper could serve both callers:
// sortedUnionKeys returns sorted keys present in either map.
func sortedUnionKeys[V any](m1, m2 map[string]V) []string {
set := make(map[string]struct{}, len(m1)+len(m2))
for k := range m1 { set[k] = struct{}{} }
for k := range m2 { set[k] = struct{}{} }
return sliceutil.SortedKeys(set)
}Then each call-site becomes a single line, and the pattern is no longer duplicated.
@copilot please address this.
| Domain: domain, | ||
| DiffEntryBase: DiffEntryBase{Status: "removed"}, | ||
| Run1Allowed: stats1.Allowed, | ||
| Run1Blocked: stats1.Blocked, |
There was a problem hiding this comment.
[/codebase-design] appendFirewallDomainDiff appends to diff and returns an anomaly count, giving it two responsibilities. The append-and-count pattern is already established in appendMCPToolDiff and appendFirewallExistingDomainDiff, but mixing side effects with a count return makes the functions harder to test in isolation.
💡 Alternative approach
Consider returning the entries instead of appending, letting the caller own both accumulation and the anomaly count:
func firewallDomainDiff(domain string, run1Stats, run2Stats map[string]DomainRequestStats) (entry DomainDiffEntry, isAnomaly bool) { ... }This makes each helper a pure function with predictable output, which is the deeper-module sweet spot /codebase-design points to.
@copilot please address this.
| stats1, inRun1 := run1Stats[domain] | ||
| stats2, inRun2 := run2Stats[domain] | ||
|
|
||
| switch { |
There was a problem hiding this comment.
[/codebase-design] The new anomaly path for a !inRun1 && inRun2 domain appends to diff.NewDomains twice — once inside the if stats2.Blocked > 0 block and once just after it. This is a behavior change from the original code, which only appended once.
💡 The original logic
Original (single append):
if stats2.Blocked > 0 {
entry.IsAnomaly = true
entry.AnomalyNote = "new denied domain"
anomalyCount++
}
diff.NewDomains = append(diff.NewDomains, entry)New code appends inside the anomaly branch and then falls through to append again. The anomaly entry is appended, then the function returns 1 — but the non-anomaly path appends and returns 0, which is correct. However, the anomaly entry is appended at line ~130 and the subsequent diff.NewDomains = append(...) at line ~135 still executes in the non-anomaly path. Double-check that the early return 1 in the anomaly branch prevents the second append — if it does, this is fine, but the structure is fragile.
@copilot please address this.
| Run1ErrorCount: s1.ErrorCount, | ||
| Run2ErrorCount: s2.ErrorCount, | ||
| CallCountChange: formatCountChange(s1.CallCount, s2.CallCount), | ||
| } |
There was a problem hiding this comment.
[/tdd] The extracted helpers (firewallStatsByRun, appendFirewallDomainDiff, appendFirewallExistingDomainDiff, mcpSummaryMap, appendMCPToolDiff) are now individually testable — but no tests were added. This is the primary opportunity the refactor opens up.
💡 High-value test targets
Suggested table-driven tests for the riskiest helpers:
func TestAppendFirewallDomainDiff_NewDenied(t *testing.T) {
diff := &FirewallDiff{}
run2Stats := map[string]DomainRequestStats{"example.com": {Allowed: 0, Blocked: 5}}
n := appendFirewallDomainDiff(diff, "example.com", nil, run2Stats)
require.Equal(t, 1, n) // anomaly
require.Len(t, diff.NewDomains, 1)
assert.True(t, diff.NewDomains[0].IsAnomaly)
}Similarly for appendFirewallExistingDomainDiff covering status flips (allowed→denied, denied→allowed) and the volume change threshold boundary.
@copilot please address this.
There was a problem hiding this comment.
Non-blocking refactor. Behavior is fully preserved — anomaly counts, entry placement, and summary fields match the original code exactly.
### Review findings
One low-priority observation posted inline (firewallStatsByRun allocates two maps it then discards on the non-nil path — use var instead of make()).
All other potential concerns raised during analysis (nil-map reads, default branch for impossible !inRun1&&!inRun2, volume-change anomaly count) are either safe by construction, pre-existing behavior not introduced by this PR, or non-issues in Go.
The PR description's claim that computeMCPToolsDiff remains above the 60-line linter limit is inaccurate (as noted in the existing review comment); this refactor brings that function well below the threshold too.
No blocking issues found.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 46.6 AIC · ⌖ 4.68 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/audit_diff.go:29
Unnecessary allocation: make() is called then immediately discarded on the non-nil path, allocating two maps that go unused.
<details>
<summary>💡 Suggested simplification</summary>
func firewallStatsByRun(run1, run2 *FirewallAnalysis) (map[string]DomainRequestStats, map[string]DomainRequestStats) {
var run1Stats, run2Stats map[string]DomainRequestStats
if run1 != nil {
run1Stats = run1.RequestsByDomain
}
if run2 != nil {
run2Stats = run2.RequestsByDomain
}
return run1St…
</details>
|
@copilot please run the
|
… claim computeMCPToolsDiff was also reduced to 26 body lines by the helper extraction, bringing it below the 60-line linter threshold. The ADR previously claimed only computeFirewallDiff was fixed (662→661); the correct reduction is 662→660 (net -2). Remove the erroneous claim that computeMCPToolsDiff remains a follow-up candidate and update the Neutral consequences to name the actual remaining violators in the file. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. The reviewer was correct — Local validation: |
This PR takes one focused slice of the shared long-function backlog in
pkg/workflowandpkg/cliby refactoringpkg/cli/audit_diff.gowith behavior-preserving helper extraction. It reduces the reported function-length findings by one (662 → 661) without changing external diff semantics.Firewall diff decomposition (
computeFirewallDiff)firewallStatsByRunfirewallSortedDomainsappendFirewallDomainDiffappendFirewallExistingDomainDiffbuildFirewallDiffSummaryMCP tool diff decomposition (
computeMCPToolsDiff)mcpSummaryMapmcpSortedKeysappendMCPToolDiffNet effect
computeFirewallDiffis no longer flagged by the 60-line function-length linter.computeMCPToolsDiffremains as a separate follow-up candidate in the same file.run: https://github.com/github/gh-aw/actions/runs/30159569186