refactor: reduce largefunc backlog in pkg/workflow and pkg/cli#48553
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. This is a refactoring-only PR affecting production files in pkg/cli/ and pkg/workflow/. Test Quality Sentinel analysis not applicable. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Refactors long workflow/compiler and interactive CLI functions into focused helpers to reduce largefunc findings.
Changes:
- Extracts prompt, AWF, checkout, Antigravity, and safe-output helpers.
- Splits interactive PR/run handling and firewall diff collection.
- Regenerates one compiled workflow lock file.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/unified_prompt_step.go |
Splits prompt and safe-output generation. |
pkg/workflow/compiler_safe_outputs_job.go |
Extracts safe-output job-building phases. |
pkg/workflow/checkout_config_parser.go |
Separates checkout field parsers. |
pkg/workflow/awf_helpers.go |
Extracts AWF command and argument helpers. |
pkg/workflow/antigravity_engine.go |
Splits Antigravity execution construction. |
pkg/cli/audit_diff.go |
Introduces a firewall diff collector. |
pkg/cli/add_interactive_workflow.go |
Splits workflow status/run flow. |
pkg/cli/add_interactive_git.go |
Splits PR merge and secret configuration. |
.github/workflows/smoke-copilot-auto.lock.yml |
Removes model-cost metadata. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Medium
| if !awfSupportsDockerHostPathPrefix(firewallConfig) { | ||
| return prefixProbe, dockerHostProbe, dockerHostRef |
| if err := c.ensureWorkflowPRMerged(result); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return c.configureRepositorySecret(secretName, secretValue) |
| @@ -149,7 +149,6 @@ jobs: | |||
| GH_AW_INFO_FIREWALL_TYPE: "squid" | |||
| GH_AW_INFO_FRONTMATTER_EMOJI: "🌸" | |||
| GH_AW_COMPILED_STRICT: "true" | |||
🧪 Test Quality Sentinel Reporti️ No test files were added or modified in this PR. This PR consists of refactoring changes to production code in
✅ Skipped — No action needed.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean refactor — no logic changes, well-structured extractions, and zero lock-file drift confirmed across 264 workflows. The code is easier to follow post-split.
One minor note: buildGitHubContextPromptSection silently returns nil when ExtractExpressions returns an error or no mappings, dropping the GitHub context prompt section entirely. This was pre-existing behavior preserved by the refactor, but worth a follow-up comment or log line to make the silent drop visible.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 52.4 AIC · ⌖ 5.63 AIC · ⊞ 5K
ADR-48553 documents the decision to reduce the function-length lint backlog by extracting focused private helpers (no logic changes). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,384 new lines across 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 run pr-finisher skill |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — the extractions are clean and the PR is well-structured. Two minor observations flagged as inline comments.
📋 Key Themes & Highlights
Key Themes
- Silent error swallow lost its context:
promptToRunWorkflowNowreturns a realerrornow, but its caller still swallows it without the original// Not criticalcomment — intent is invisible to the next reader. - Mutually exclusive anomaly conditions:
addStatusChangeuses two sequentialifchecks fordenied→allowed/allowed→denied. These are logically correct but not structurally exclusive — aswitchwould make the intent clearer and guard against future double-marking.
Positive Highlights
- ✅ The
firewallDiffCollectorstruct +markAnomalyhelper is a significant readability win — anomaly bookkeeping is now centralised. - ✅
prMergeState+mergeActionenum cleanly replaces three loose booleans — the state machine intent is now explicit. - ✅
defer spinner.Stop()inwaitForWorkflowAvailabilitycorrectly handles early-exit paths that previously needed manual cleanup. - ✅
promptAndUpdatePRTitlemakes the title-edit flow testable in isolation. - ✅ Zero lock-file drift across 264 workflows is strong evidence of correctness.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 83.1 AIC · ⌖ 4.84 AIC · ⊞ 6.7K
Comment /matt to run again
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Could not verify workflow status.")) | ||
| fmt.Fprintf(os.Stderr, "You can check status with: %s status\n", string(constants.CLIExtensionPrefix)) | ||
| runNow, err := c.promptToRunWorkflowNow(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
[/codebase-design] promptToRunWorkflowNow now returns a proper error but this caller silently discards it (return nil on line 45). The original swallowing was co-located with // Not critical, just skip; after the refactor the intent is invisible and context-cancellation errors are masked at a distance.
💡 Suggestion
Add a comment explaining the intentional swallow:
runNow, err := c.promptToRunWorkflowNow(ctx)
if err != nil {
// Not critical — user interaction prompt failed; skip the run offer.
c.showFinalInstructions()
return nil
}This preserves the original intent while making it legible to the next reader.
@copilot please address this.
| Run1Status: status1, | ||
| Run2Status: status2, | ||
| } | ||
| if status1 == "denied" && status2 == "allowed" { |
There was a problem hiding this comment.
[/codebase-design] Two independent if checks for mutually exclusive conditions — denied→allowed and allowed→denied. While logically safe today, using else if (or a switch) would make the mutual exclusivity explicit and prevent double-marking if a future classifyFirewallDomainStatus change introduces a third status that matches both conditions.
💡 Suggestion
switch {
case status1 == "denied" && status2 == "allowed":
c.markAnomaly(&entry, "previously denied, now allowed")
case status1 == "allowed" && status2 == "denied":
c.markAnomaly(&entry, "previously allowed, now denied")
}This makes the intent explicit and guards against future markAnomaly being called twice on a single entry.
@copilot please address this.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot run pr-finisher skill |
Incrementally addresses the function-length lint backlog (675 findings → 664) across
pkg/workflowandpkg/cliby extracting focused private helpers from the longest functions, with no logic changes.pkg/cli
audit_diff.go— splitcomputeFirewallDiffinto phase helpersadd_interactive_git.go— splitcreateWorkflowPRAndConfigureSecretintoaddResolvedWorkflowsWithPR,ensureWorkflowPRMerged,configureRepositorySecretadd_interactive_workflow.go— splitcheckStatusAndOfferRuninto phase helperspkg/workflow
awf_helpers.go— splitBuildAWFCommand(377 lines) andBuildAWFArgs(168 lines) into arc/dind probe, tool-cache mount probe, and config file setup helpersunified_prompt_step.go— splitgenerateUnifiedPromptCreationStep(259 lines),buildSafeOutputsSections(226 lines), andcollectPromptSections(196 lines) intocollectEnvVarsAndMappings, apromptYAMLWriterstruct, and per-category tool-append helpers (appendPullRequestSafeOutputTools,appendAdministrativeSafeOutputTools,appendWorkflowAndProjectSafeOutputTools)checkout_config_parser.go— splitcheckoutConfigFromMap(199 lines) into field-group parserscompiler_safe_outputs_job.go— splitbuildSafeOutputsHandlerOutputsAndActionSteps(220 lines)antigravity_engine.go— splitGetExecutionSteps(204 lines)Correctness notes
The refactoring of
buildSafeOutputsSectionsaccidentally reordered the tool list in the safe-output-tools prompt section (push_to_pull_request_branchmoved beforeadd_labels/add_reviewer). This was caught viamake recompileand corrected — the append order now exactly matches the original, confirmed by zero lock-file drift across all 264 workflows.