Refactor audit report largefunc slice in pkg/cli - #47262
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pkg/cli
There was a problem hiding this comment.
Pull request overview
Refactors audit reporting into focused helpers while preserving report assembly, findings, recommendations, and console output behavior.
Changes:
- Split audit data assembly and metric fallback logic.
- Decomposed findings, recommendations, and console rendering.
- Added focused formatting and ordering tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/audit_report.go |
Extracts audit assembly helpers. |
pkg/cli/audit_report_analysis.go |
Decomposes findings and recommendations. |
pkg/cli/audit_report_render.go |
Splits console rendering by section. |
pkg/cli/audit_report_test.go |
Adds formatting regression tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Medium
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
@copilot please run the Failed checks:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with improvement suggestions; no blocking correctness issues.
📋 Key Themes & Highlights
Key Themes
- Thin pass-through wrappers:
buildAuditAssessmentsandappendToolingRecommendationsgroup calls without adding depth — opportunities to either inline or deepen remain. auditDataInputscarries forwarding fields:metricsandmcpToolUsageare in the struct only to thread them intobuildSessionAnalysis/buildMCPServerHealth; pre-computing those outsideassembleAuditDatawould let the struct hold only results.- Test scope: the new
TestRenderConsoleExperimentsSortedgoes through the fullrenderConsolepath and swapsos.Stderrglobally — testing the extractedrenderConsoleExperimentshelper directly would be tighter and parallel-safe. appendToolingRecommendationsnaming mismatch: it handles missing tools, MCP failures and firewall blocks — inconsistent with the single-concern pattern of the rest of the family.
Positive Highlights
- ✅ The
renderConsoledecomposition is clean and the naming is consistent — each helper does exactly what its name says. - ✅
buildFailureFindingDescriptionis a genuine improvement: early-return structure and named constants make the branching readable. - ✅
generateErrorVolumeFindingsandgenerateFirewallFindingsconverting to direct[]Finding{{...}}returns is idiomatic Go. - ✅
TestBuildBlockedNetworkFindingDescriptioncovers all four switch branches — solid table-driven coverage. - ✅
lookupFallbackMetricscorrectly moves thelogsPath == ""guard to the top, eliminating a repeated condition.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 78.5 AIC · ⌖ 8.1 AIC · ⊞ 6.7K
Comment /matt to run again
Investigated the failure: run |
|
🎉 This pull request is included in a new release. Release: |
This PR addresses the
lint-monsterfunction-length slice inpkg/cliby breaking up the highest-impact audit-report functions without changing their external behavior. The refactor targets the audit reporting path specifically and reduces the assignedlargefuncfindings by turning long mixed-responsibility functions into small coordinators over focused helpers.Audit report assembly
buildAuditDatainto narrow helpers for:AuditDatacompositionFindings and recommendations
generateFindingsinto focused helpers for:generateRecommendationsinto helper-driven branches for:Console rendering
renderConsoleinto a thin top-level renderer that delegates to section-specific helpers:Focused regression coverage
Example of the new structure: