Refactor interactive/audit CLI large functions into focused helpers - #49799
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors oversized CLI functions into focused helpers while retaining existing interactive, audit, log-parsing, and action-build flows.
Changes:
- Decomposes interactive setup, engine selection, PR merging, and bootstrap orchestration.
- Extracts firewall diff, Squid log parsing, and JavaScript action-build helpers.
- Adds targeted tests for engine prioritization and merge options.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/audit_diff.go |
Extracts firewall diff classification helpers. |
pkg/cli/add_interactive_orchestrator.go |
Splits interactive orchestration into phases. |
pkg/cli/add_interactive_git.go |
Extracts PR merge and secret helpers. |
pkg/cli/add_interactive_git_test.go |
Tests merge-option construction. |
pkg/cli/add_interactive_engine.go |
Extracts engine selection helpers. |
pkg/cli/add_interactive_engine_test.go |
Tests engine-option prioritization. |
pkg/cli/actions_build_command.go |
Extracts JavaScript action building. |
pkg/cli/access_log.go |
Extracts Squid log-line processing. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
| for _, opt := range constants.EngineOptions { | ||
| if setutil.Contains(c.existingSecrets, opt.SecretName) { | ||
| addInteractiveLog.Printf("Found existing secret %s, recommending engine: %s", opt.SecretName, opt.Value) | ||
| return opt.Value | ||
| } | ||
| } |
| if err := c.promptAndEditPRTitle(prNumber); err == nil { | ||
| mergeFailed = false | ||
| } |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
🧪 Test Quality Sentinel Report
📊 Metrics (6 tests)
✅ Test Breakdown
🟢 Quality Signals
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Refactoring Review
The decomposition is clean and behavior-preserving overall. Two correctness issues have already been flagged by prior Copilot comments (engine-default priority shift in determineDefaultEngine, and silent error suppression in the promptAndEditPRTitle caller). Beyond those, a couple of structural observations:
processSquidAccessLogLine return type — the helper returns error but only ever returns nil. The unused error return adds noise and will mislead future callers if a real error path is added without testing. Consider returning bool (skip line?) or nothing, matching the original intent.
appendExistingFirewallDomainDiff append structure — the anomaly branches each do their own diff.StatusChanges = append(...) and return early, while a non-anomaly status change hits a separate append at the end. This is correct but fragile. Consistent with buildNewFirewallDomainEntry/buildRemovedFirewallDomainEntry, prefer: set anomaly fields conditionally, then append unconditionally.
No blocking issues beyond the two already filed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 42 AIC · ⌖ 12.1 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on three correctness/clarity issues before merge.
📋 Key Themes & Highlights
Key Issues
- Missing tests for
determineDefaultEngine— the 4-level priority chain is the most business-critical logic in this file and is entirely untested despite the PR adding tests for the simplerprioritizeEngineOptionhelper. - Silent error discard in the title-edit path —
runPRMergeLoopswallows all errors frompromptAndEditPRTitle, masking form cancellation and network failures. - Misleading error return signature in
processSquidAccessLogLine— the function always returnsnilbut the caller treats non-nil as a hard fatal abort, silently changing the originalcontinue-on-error behaviour.
Positive Highlights
- ✅ Clean decomposition throughout — each helper has a single clear responsibility
- ✅
buildMergeOptions/TestBuildMergeOptionsis an excellent example of the helper + test pattern to follow - ✅
appendExistingFirewallDomainDiffcorrectly preserves all anomaly-count semantics from the original megafunction - ✅
mergeActionconstants promoted to package scope — good move for testability
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 64.2 AIC · ⌖ 11 AIC · ⊞ 7.1K
Comment /matt to run again
| return "" | ||
| } | ||
|
|
||
| func (c *AddInteractiveConfig) determineDefaultEngine(workflowSpecifiedEngine string) string { |
There was a problem hiding this comment.
[/tdd] determineDefaultEngine implements a non-trivial 4-level priority chain (flag → secret → workflow frontmatter → env var) but has no unit tests. A wrong ordering silently selects the wrong model for every new user. Consider adding table-driven tests covering each priority level, similar to the TestPrioritizeEngineOption pattern already used in this PR.
@copilot please address this.
| mergeDone = done | ||
| mergeFailed = failed | ||
| case mergeActionEditTitle: | ||
| if err := c.promptAndEditPRTitle(prNumber); err == nil { |
There was a problem hiding this comment.
[/codebase-design] Error from promptAndEditPRTitle is silently dropped on the happy path: if err := c.promptAndEditPRTitle(prNumber); err == nil { mergeFailed = false }. A form-cancellation or network error is treated identically to an empty-title user decision, making the loop hard to reason about. Consider distinguishing user cancellation (return error and break loop) from empty-title (show warning, stay in loop) explicitly.
@copilot please address this.
| analysis.BlockedDomains = append(analysis.BlockedDomains, domain) | ||
| } | ||
| if err := processSquidAccessLogLine(strings.TrimSpace(scanner.Text()), verbose, analysis, allowedDomainsSet, blockedDomainsSet); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
[/codebase-design] The extracted processSquidAccessLogLine now returns a non-nil error only in an unreachable code path (it always returns nil), yet the caller propagates it as a fatal error with return nil, err. The original loop used continue for every error condition, so this changes observable behavior: any future addition of a real error return in the helper will suddenly abort log parsing. Either remove the error return and pass *DomainAnalysis mutations by pointer only, or document which conditions are fatal.
@copilot please address this.
| return entry, 0 | ||
| } | ||
|
|
||
| func appendExistingFirewallDomainDiff(diff *FirewallDiff, domain string, stats1, stats2 DomainRequestStats) int { |
There was a problem hiding this comment.
[/codebase-design] appendExistingFirewallDomainDiff returns an anomaly count that is 0 for volume changes, yet a volume change is appended to diff.VolumeChanges. The caller sums the return value into anomalyCount, which feeds diff.Summary.AnomalyCount. Volume changes are not anomalies, so the count is correct — but the asymmetry (new/removed return 1, status_changed returns 1, volume returns 0) is invisible from the call site and easy to misread or break. Consider returning a named struct or adding a brief comment above each return stating what it counts.
@copilot please address this.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (514 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 MatterADRs 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
|
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers:
Run details: https://github.com/github/gh-aw/actions/runs/30756126865
|
…rrors - determineDefaultEngine: when an existing secret maps to the default engine (Copilot), break rather than return early so that a workflow-specified engine or env-var credential can still override it. Adds TestDetermineDefaultEngine covering all six priority cases. - promptAndEditPRTitle: changed signature to (bool, error) so that a fatal form-input failure is propagated to the merge loop (matching original inline behavior), while a non-fatal empty-title or editPRTitle warning returns (false, nil) and continues the loop. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…appendExistingFirewallDomainDiff return semantics - access_log.go: drop the unused error return type from processSquidAccessLogLine — every path returned nil, so the caller's `return nil, err` was a latent misread trap. Removed the error return, updated the call site to not check it. - audit_diff.go: add inline comments to every return site in appendExistingFirewallDomainDiff explaining what the returned int counts (anomaly vs. non-anomaly), making the asymmetry between security-flip returns (1) and volume-change returns (0) explicit. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers:
Run details: https://github.com/github/gh-aw/actions/runs/30758413653
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers:
Failed checks:
Run details: https://github.com/github/gh-aw/actions/runs/30760589044
|
…fix-function-lengths-again Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please continue triage on this PR. Latest CI is green, but reviewer-visible follow-through is still needed. Please:
Run details: https://github.com/github/gh-aw/actions/runs/30763117443
|
This PR addresses the lint-monster
largefuncslice for CLI interactive/audit/entrypoint flows by decomposing oversized functions into focused internal helpers while preserving existing command behavior. The changes are intentionally scoped to the reported CLI files in this slice.Interactive flow decomposition (
pkg/cli/add_interactive_*)Audit diff decomposition (
pkg/cli/audit_diff.go)computeFirewallDiffinto domain-focused helpers:Access log and action build decomposition
parseSquidAccessLog: extracted line processing, status classification, and unique-domain append helpers.buildAction: extracted JavaScript action build path into a dedicated helper, keeping setup/composite branching intact.Targeted helper tests
add_interactive_engine_test.go(prioritizeEngineOption)add_interactive_git_test.go(buildMergeOptions)