refactor(workflow,cli): eliminate 680 function-length lint findings via targeted helper extraction#47061
Conversation
…pkg/workflow and pkg/cli Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Refactors large workflow and CLI functions into focused helpers while preserving public interfaces.
Changes:
- Splits workflow YAML generation and configuration parsing into helpers.
- Decomposes CLI workflow execution and logs command handling.
- Corrects repo-memory branch derivation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/side_repo_maintenance.go |
Decomposes side-repository workflow generation. |
pkg/workflow/repo_memory.go |
Extracts parsing and job-building helpers. |
pkg/workflow/maintenance_workflow_yaml.go |
Converts YAML generation into an orchestrator. |
pkg/workflow/maintenance_workflow_yaml_jobs.go |
Adds maintenance job builders. |
pkg/workflow/engine.go |
Decomposes engine configuration parsing. |
pkg/cli/run_workflow_execution.go |
Extracts workflow execution stages. |
pkg/cli/logs_command.go |
Extracts logs command setup and handling. |
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: 2
- Review effort level: Medium
| } | ||
| func validateLocalWorkflowForRun(workflowIdOrName string, inputs []string) error { | ||
| executionLog.Printf("Validating local workflow: %s", workflowIdOrName) | ||
| workflowFile, err := resolveWorkflowFile(workflowIdOrName, false) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: refactor(workflow,cli) — helper extraction
Overall the refactor is a solid structural improvement that achieves its lint goal. Two issues were found in the visible diff portion.
Blocking
-
Verbose flag regression (
pkg/cli/run_workflow_execution.goline 161) —resolveWorkflowFileis now called with a hardcodedfalseinstead ofopts.Verbose. Verbose-mode diagnostics during local workflow file resolution are silently dropped. Original code passedopts.Verbosecorrectly. -
Double-error return anti-pattern (
executeWorkflowRunline 392) — returning(string, *WorkflowRunInfo, error, error)is non-idiomatic Go and makes call-site reasoning error-prone. Prefer a result struct or encode the non-fatal fetch error inside*WorkflowRunInfo.
Non-blocking observations
handleWorkflowRunInfounusedoutputparameter: theoutput stringargument is accepted but never read inside the function body. Consider removing it.- The
runInfo != nilnil-guard added tohandleWorkflowRunInfois a correctness improvement (the old code would dereference a nil pointer ifgetLatestWorkflowRunWithRetryreturned(nil, nil)).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 57.1 AIC · ⌖ 4.62 AIC · ⊞ 4.9K
| } | ||
| func validateLocalWorkflowForRun(workflowIdOrName string, inputs []string) error { | ||
| executionLog.Printf("Validating local workflow: %s", workflowIdOrName) | ||
| workflowFile, err := resolveWorkflowFile(workflowIdOrName, false) |
There was a problem hiding this comment.
Verbose flag silently dropped in validateLocalWorkflowForRun
resolveWorkflowFile is called with a hardcoded false instead of forwarding the caller's verbose flag. The original code passed opts.Verbose, so any verbose-mode diagnostics printed during workflow file resolution are now silently suppressed when this extraction path is taken.
// Before (correct)
workflowFile, err := resolveWorkflowFile(workflowIdOrName, opts.Verbose)
// After (drops verbose)
workflowFile, err := resolveWorkflowFile(workflowIdOrName, false)Fix: add a verbose bool parameter to validateLocalWorkflowForRun (and its callers) and forward it here.
@copilot please address this.
|
|
||
| // Capture both stdout and stderr | ||
| stdout, err := cmd.Output() | ||
| func executeWorkflowRun(ctx context.Context, lockFileName string, args []string, opts RunOptions) (string, *WorkflowRunInfo, error, error) { |
There was a problem hiding this comment.
Unconventional double-error return signature
executeWorkflowRun returns (string, *WorkflowRunInfo, error, error) — two separate error values in the same return list. This is a Go anti-pattern that is easy to misread at the call site (callers must remember which error is fatal and which is a non-fatal run-info error).
Consider encoding the distinction with a named struct or by having the function return only the fatal error and communicating the non-fatal run-info error through the *WorkflowRunInfo itself (e.g., add a FetchErr error field). This keeps the public surface idiomatic and eliminates confusion about argument order.
@copilot please address this.
…a helper extraction
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (3,462 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
|
There was a problem hiding this comment.
REQUEST_CHANGES — one confirmed behavior regression
This refactor is mostly clean, but contains one correctness regression that must be fixed before merge: opts.Verbose is silently dropped when calling resolveWorkflowFile inside the new validateLocalWorkflowForRun helper (hard-coded false instead of passing opts.Verbose). Users running gh aw run --verbose will no longer get verbose output for local workflow resolution diagnostics.
Other observations (non-blocking)
validateWorkflowsForRunloop still callsresolveWorkflowFile(workflowName, opts.Verbose)correctly (line 1536), so the regression is isolated to the single-workflow path.- Dry-run verbose output format changed from
"gh workflow run ..."to"gh " + strings.Join(args, " ")— functionally equivalent but slightly different surface text. loadStdinLogsOptions/ StdinLogsOptions does not receivecacheBefore— but this was the same in the original code, not a regression introduced here.- The structural decomposition in
engine.go,repo_memory.go, andmaintenance_workflow_yaml_jobs.gois well-executed with no observable behaviour changes.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 125.7 AIC · ⌖ 4.57 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/run_workflow_execution.go:898
Behavior regression: opts.Verbose silently dropped in local workflow validation. The refactored helper hard-codes false, breaking verbose output for users of gh aw run --verbose.
<details>
<summary>💡 Fix</summary>
The original call was:
workflowFile, err := resolveWorkflowFile(workflowIdOrName, opts.Verbose)The refactored validateLocalWorkflowForRun hard-codes false instead:
workflowFile, err := resolveWorkflowFile(workflowIdOrName, false)Fix: pass `verb…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two correctness issues and one missing regression test.
📋 Key Themes & Highlights
Issues Found
-
Dropped error context (
run_workflow_execution.go:154) —validateWorkflowForRunsilently drops the"failed to validate remote workflow: "prefix on the remote-validation error path. This is a behavioral regression in user-visible error messages. -
MaxToolDenialsguard removed for inline engine path (engine.go:311) — the original code guarded top-levelmax-tool-denialsassignment withif topLevelMaxToolDenials != "". The newapplyInlineEngineFieldsunconditionally overwrites it. Verify this is intentional. -
Bug fix has no regression test (
repo_memory.go:196) — theBranchNamederivation bug fix (clearing pre-populatedBranchNamesoapplyRepoMemoryDefaultBranchre-derives from the custom ID) has no unit test. The existing integration tests always supply explicitbranch-name:fields, so the fixed path is untested.
Positive Highlights
- ✅ Excellent extraction discipline: all 680 lint findings eliminated with no public API changes
- ✅
applyReferencedEngineFieldsis a clean 12-helper orchestrator — easy to scan - ✅ Companion file pattern (
maintenance_workflow_yaml_jobs.go) keeps individual file sizes manageable - ✅ Bug fix is well-described in the PR body and the fix logic is clear
- ✅
workflowRunPreparationstruct groups relatedprepareWorkflowRunstate cleanly
Note on diff truncation
The pre-fetched diff was capped at 3000 lines (2 of 9 files visible). Findings from pkg/workflow/engine.go, repo_memory.go, and run_workflow_execution.go are based on reading the current file state in the repo. Files not visible in the diff (frontmatter_on_section_cleanup.go, maintenance_workflow_yaml*.go, mcp_config_custom.go, side_repo_maintenance.go) were not reviewed at line level.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 103.7 AIC · ⌖ 5.15 AIC · ⊞ 6.6K
Comment /matt to run again
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Consider pushing your changes before running the workflow")) | ||
| } | ||
| } | ||
| return validateRemoteWorkflow(workflowIdOrName, opts.RepoOverride, opts.Verbose) |
There was a problem hiding this comment.
[/codebase-design] Dropped error context: validateWorkflowForRun now returns validateRemoteWorkflow's error directly, silently removing the "failed to validate remote workflow: " prefix that existed before this refactor.
💡 Restore the wrapper
Before:
if err := validateRemoteWorkflow(workflowIdOrName, opts.RepoOverride, opts.Verbose); err != nil {
return fmt.Errorf("failed to validate remote workflow: %w", err)
}After (current):
return validateRemoteWorkflow(workflowIdOrName, opts.RepoOverride, opts.Verbose)The refactor inadvertently stripped the context message. Restore it:
if err := validateRemoteWorkflow(workflowIdOrName, opts.RepoOverride, opts.Verbose); err != nil {
return fmt.Errorf("failed to validate remote workflow: %w", err)
}
return nil@copilot please address this.
| } | ||
| } | ||
| } | ||
| func applyInlineEngineFields(config *EngineConfig, engineObj map[string]any, topLevel engineTopLevelConfig) { |
There was a problem hiding this comment.
[/codebase-design] applyInlineEngineFields unconditionally sets MaxToolDenials = topLevel.maxToolDenials (even when it is ""), whereas the original code guarded this with if topLevelMaxToolDenials != "" { ... }. This is a behavior change: if an inline engine had MaxToolDenials set through some other path, this now overwrites it with the empty top-level value.
💡 Original guard vs current code
Original (HEAD~1):
if topLevelMaxToolDenials != "" {
config.MaxToolDenials = topLevelMaxToolDenials
}Current refactor (line 315):
config.MaxToolDenials = topLevel.maxToolDenialsThis is consistent with how applyEngineTopLevelOverrides handles it for the referenced-engine path (applyReferencedEngineFields also unconditionally sets it via applyEngineTurnFields). But the inline path previously guarded it. Verify this is intentional and add a test if so.
@copilot please address this.
| entry.ID = "default" | ||
| } | ||
| if !explicitBranchName { | ||
| entry.BranchName = "" // let applyRepoMemoryDefaultBranch derive from ID |
There was a problem hiding this comment.
[/tdd] The bug fix (entry.BranchName = "" to let applyRepoMemoryDefaultBranch re-derive from the entry ID) has no dedicated unit test. The existing integration tests use explicit branch-name: fields, so they don't exercise the fixed code path.
💡 Missing test case
Add a unit test for parseRepoMemoryEntry with a custom id and no branch-name, verifying the branch name is derived from the custom ID rather than the workflow ID:
func TestParseRepoMemoryEntry_CustomIDDerivesBranchName(t *testing.T) {
entry, err := parseRepoMemoryEntry(
map[string]any{"id": "my-agent"},
"my-workflow", "memory", false,
)
require.NoError(t, err)
// Before the fix, this would produce memory/my-workflow instead of memory/my-agent
assert.Equal(t, "memory/my-agent", entry.BranchName)
}This pins the bug fix against regression.
@copilot please address this.
| config.UserAgent = userAgentStr | ||
| } | ||
| } | ||
| func applyReferencedEngineFields(config *EngineConfig, engineObj map[string]any, topLevel engineTopLevelConfig) { |
There was a problem hiding this comment.
[/codebase-design] applyReferencedEngineFields calls 12 sub-helpers sequentially but does not document the ordering contract. Some helpers likely depend on prior state (e.g., applyEngineProviderFields may need to run before applyEngineBooleanFields which guards on IsInlineDefinition). A brief comment listing the ordering rationale would make this safe to extend.
💡 Suggested doc comment
// applyReferencedEngineFields applies all optional fields from a referenced engine object.
// Order matters: provider fields must precede boolean fields (IsInlineDefinition guard),
// and top-level overrides must run last to ensure they take precedence.
func applyReferencedEngineFields(...) {@copilot please address this.
| config.LLMProvider = strings.ToLower(strings.TrimSpace(providerStr)) | ||
| } | ||
| } | ||
| func extractInlineEngineConfig(runtime any, engineObj map[string]any, topLevel engineTopLevelConfig) (string, *EngineConfig, string) { |
There was a problem hiding this comment.
[/codebase-design] extractObjectEngineConfig returns 3 values (engineID, config, model) from both branches, but extractInlineEngineConfig sets config.ID from runtimeObj["id"] while extractReferencedEngineConfig returns it as the first value and sets config.ID. The two paths have different conventions for how the engine ID surfaces — this asymmetry is subtle and worth documenting or unifying.
💡 The asymmetry
Inline path: config.ID set inside extractInlineEngineConfig, returned string is also config.ID.
Referenced path: config.ID set inside extractReferencedEngineConfig, returned string is also config.ID.
They're actually symmetric, but extractInlineEngineConfig additionally derives ID from runtimeObj["id"] (a nested key) while extractReferencedEngineConfig reads it from engineObj["id"] (a top-level key). A brief comment explaining the key path difference at the extractObjectEngineConfig decision point would aid future maintainers.
@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the PR finisher pass and pushed Local validation passed ( |
Triage
Rationale: Large mechanical refactor (3513 additions / 4109 deletions across 10 files) targeting lint compliance. No CI runs recorded yet. Behavior-preserving by design but scale warrants batch human review.
|
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, and /diagnosing-bugs — two actionable comments, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Missing regression test for the bug fix: The
applyRepoMemoryDefaultBranchearly-exit fix has no dedicated test; the bug can silently recur. - Global stderr mutation in test:
TestValidateLocalWorkflowForRun_PropagatesVerboseswapsos.Stderrwithout at.Cleanupguard — leaks state if the test panics.
Positive Highlights
- ✅ Excellent structural decomposition —
buildMaintenanceWorkflowYAML(961 → thin orchestrator) andExtractEngineConfig(monolith → layered helpers) are substantially easier to read. - ✅
parseNonEmptyAuthDefinitionis a clean, well-named extraction that makes the empty-auth guard intent explicit. - ✅ New companion file
maintenance_workflow_yaml_jobs.gokeeps job-builder logic cohesive and discoverable. - ✅ ADR-47061 documents the decision and trade-offs honestly — the negatives section is particularly useful.
- ✅
logsCommandExampleTemplatewithfmt.Sprintfreplaces fragile string concatenation — a nice readability win.
Diff Coverage Note
The patch was capped at 3000 lines; repo_memory.go, maintenance_workflow_yaml_jobs.go, and side_repo_maintenance.go were reviewed via direct file inspection rather than the diff.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 81.2 AIC · ⌖ 7.39 AIC · ⊞ 6.6K
Comment /matt to run again
|
Excellent work! 🎯 This refactoring systematically eliminates 680 long-function lint findings across Key strengths:
This is ready for merge. 🚀
|
|
🎉 This pull request is included in a new release. Release: |
make golint-customreported 680 long-function findings (>60 lines) acrosspkg/workflowandpkg/cli, with the worst offenders ranging from 200–961 lines. This extracts cohesive logical blocks into focused private helpers, reducing all findings to zero while preserving behavior and public interfaces.Changes
pkg/workflow/maintenance_workflow_yaml.go+ newmaintenance_workflow_yaml_jobs.gobuildMaintenanceWorkflowYAML(961 lines) → thin orchestrator delegating to per-job buildersbuildMaintenanceActivityReportJob,buildMaintenanceForecastReportJob, and their sub-helperspkg/workflow/frontmatter_on_section_cleanup.gocommentOutProcessedFieldsInOnSection(697 lines) split into focused per-field helperspkg/workflow/side_repo_maintenance.gogenerateSideRepoMaintenanceWorkflow(447 lines) decomposed into stage-specific builderspkg/workflow/mcp_config_custom.gogetMCPConfig(209 lines) →validateMCPKnownProperties,resolveMCPType,extractMCPStdioFields,extractMCPHTTPFields,postProcessMCPConfigpkg/workflow/repo_memory.gobuildPushRepoMemoryJob(204 lines) →buildPushRepoMemorySetupAndCheckoutSteps,buildPushRepoMemoryDownloadSteps,buildSinglePushRepoMemoryStep,buildPushRepoMemoryJobCondition,buildPushRepoMemoryOutputsBranchNamewas not correctly derived from entryIDbecausenewDefaultRepoMemoryEntrypre-populated it, causingapplyRepoMemoryDefaultBranchto always exit early. Now clearsBranchNamebefore branch derivation when no explicitbranch-namewas set.pkg/cli/run_workflow_execution.goRunWorkflowsOnGitHub(135 lines) →validateWorkflowsForRun,executeAllWorkflowsOnce,wrapRunWithJSONOutputpkg/cli/logs_command.goNewLogsCommand(376 lines) decomposed into flag-group and handler helpers