Refactor ResolveWorkflows into focused helpers to reduce largefunc backlog#45965
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>
|
Thanks for working on this refactor! The PR is still marked [WIP] — once all checklist items are complete and final validations pass, this will be ready for review. Looking good so far!
|
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>
ResolveWorkflows into focused helpers to reduce largefunc backlog
There was a problem hiding this comment.
Pull request overview
This PR refactors ResolveWorkflows (in pkg/cli/add_workflow_resolution.go) by extracting its major phases into smaller, single-purpose helpers while keeping the public API and overall resolution behavior intact. The goal is to reduce the function’s size/complexity in pkg/cli without a broader rewrite.
Changes:
- Extracted workflow input validation, spec parsing (including local/repo package handling and fallback parsing), current-repo guard validation, and wildcard expansion into dedicated helpers.
- Extracted per-spec resolution into
resolveSingleWorkflowSpec, separating package/agent/action “raw install” handling from standard markdown workflow handling (frontmatter parsing, private checks, workflow_dispatch detection, JSON-import warnings). - Isolated bootstrap profile selection logic into
selectBootstrapProfile, preserving warning behavior on conflicts.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/add_workflow_resolution.go | Splits ResolveWorkflows into cohesive helper functions while preserving existing workflow/package resolution behavior and warning aggregation. |
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: 0
- Review effort level: Low
|
✅ 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. ✅ |
|
|
There was a problem hiding this comment.
Reviewed the ResolveWorkflows refactor — behavior is faithfully preserved across all helpers. The extraction is clean: validation, spec parsing, repo guard, wildcard expansion, per-spec resolution, and bootstrap selection are each isolated and single-purpose. No correctness or security concerns found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 27.8 AIC · ⌖ 4.91 AIC · ⊞ 5K
Records the architectural decision to split the monolithic ResolveWorkflows function into focused helper functions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (286 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. 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.
Skills-Based Review 🧠
Applied /codebase-design — commenting (no blocking issues, but three design improvements worth addressing).
📋 Key Themes & Highlights
Key Themes
- Return-value proliferation: several helpers return 4–5 positional values including a boolean sentinel (
handled), making call sites harder to read and extend. - Unnecessary defensive copy:
selectBootstrapProfileusesmake+copythat Go slice semantics make redundant. - Dead field in struct literal:
IsPrivate: isPrivateinresolveStandardWorkflowis alwaysfalseat that point.
Positive Highlights
- ✅ Top-level
ResolveWorkflowsreads like a clean flowchart now. - ✅
specResolutionResultis a good first step toward a deep intermediate type. - ✅
validateManifestWorkflowPrivateSettingandvalidateCurrentRepositorySpecsare well-named and easily unit-testable. - ✅ No behaviour changes — the diff is well-scoped and matches the PR description.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 58.5 AIC · ⌖ 4.89 AIC · ⊞ 6.7K
Comment /matt to run again
| Description: description, | ||
| Engine: engine, | ||
| HasWorkflowDispatch: workflowHasDispatch, | ||
| IsPrivate: isPrivate, |
There was a problem hiding this comment.
[/codebase-design] IsPrivate: isPrivate is always false in this success path — the error check on line 356 means we never reach the struct literal when isPrivate is true. The field assignment is dead code and may confuse future readers.
💡 Suggested fix
The field can be dropped from the literal entirely, or the check can be restructured so the value is meaningful:
// Option A: just omit the field — it defaults to false
return &ResolvedWorkflow{
Spec: resolvedSpec,
Content: fetched.Content,
SourceInfo: fetched,
Description: description,
Engine: engine,
HasWorkflowDispatch: workflowHasDispatch,
// IsPrivate intentionally absent: already guarded above
}, workflowHasDispatch, warning, nilIf IsPrivate on ResolvedWorkflow is used downstream for something other than this guard (e.g. display), document why it should always be false here.
@copilot please address this.
There was a problem hiding this comment.
Dropped IsPrivate: isPrivate from the struct literal in d199cd1. Since the error guard above returns early when isPrivate is true, the field would always be false at that point — removing it eliminates the dead assignment.
| } | ||
| resolutionLog.Printf("Multiple bootstrap profiles found (%v); skipping all", ids) | ||
| resolutionWarnings = append(resolutionWarnings, | ||
| updatedWarnings := make([]string, len(resolutionWarnings), len(resolutionWarnings)+1) |
There was a problem hiding this comment.
[/codebase-design] The defensive make+copy in selectBootstrapProfile is unnecessary — a simple append already allocates a new backing array when capacity is exhausted, so the original slice is never mutated. The added complexity misleads future readers into thinking the caller slice needs protection.
💡 Suggested simplification
default:
// ...ids collection...
resolutionLog.Printf("Multiple bootstrap profiles found (%v); skipping all", ids)
return nil, append(resolutionWarnings,
fmt.Sprintf("multiple bootstrap profiles found (%s); ...", strings.Join(ids, ", ")))This is safe because resolutionWarnings is a value copy from the caller — mutating the underlying array beyond its length is the only thing to guard against, and append handles that automatically when len == cap.
@copilot please address this.
There was a problem hiding this comment.
Simplified the default case to a direct append in d199cd1. The make+copy was unnecessary since append already allocates a new backing array when needed, so the caller's slice is never mutated.
| } | ||
| func parseSingleWorkflowSpecForResolution(ctx context.Context, workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, error) { | ||
| specs, warnings, bootstrapProfile, handled, err := resolveLocalPackageWorkflowSpec(workflow) | ||
| if err != nil || handled { |
There was a problem hiding this comment.
[/codebase-design] The (handled bool, err error) sentinel pattern in resolveLocalPackageWorkflowSpec and resolveRepositoryPackageWorkflowSpec leaks implementation-level signalling into the calling function. This forces callers to reason about three values (specs, handled, err) in combination and makes the chain in parseSingleWorkflowSpecForResolution fragile to extend.
💡 Deeper module alternative
A cleaner interface would use a typed sentinel error (or named result type) so callers do not need to inspect handled:
var errNotHandled = errors.New("not handled")
func resolveLocalPackageWorkflowSpec(workflow string) (*specResolutionResult, error) {
pkg, err := resolveLocalRepositoryPackage(workflow)
if err != nil { return nil, err }
if pkg == nil { return nil, errNotHandled }
// ...build result...
return result, nil
}// caller
result, err := resolveLocalPackageWorkflowSpec(workflow)
if err == nil { return result.Specs, result.Warnings, result.BootstrapProfile, nil }
if !errors.Is(err, errNotHandled) { return nil, nil, nil, err }
// fall throughThis keeps the decision logic in one place and avoids "boolean-plus-error" combinations that violate the principle of deep modules.
@copilot please address this.
There was a problem hiding this comment.
Replaced the (handled bool, err error) pattern with a package-level errNotHandled sentinel in d199cd1. Both resolveLocalPackageWorkflowSpec and resolveRepositoryPackageWorkflowSpec now return a plain error; callers use errors.Is(err, errNotHandled) to fall through, eliminating the boolean-plus-error combination.
| return nil, fmt.Errorf("invalid Agentic Workflow manifest %q: workflow %q sets private: true and cannot be included because private workflows cannot be added", manifestPath, resolvedSpec.WorkflowPath) | ||
| } | ||
| } | ||
| if resolvedWorkflow, handled := resolvePackageOrActionWorkflow(spec, resolvedSpec, fetched); handled { |
There was a problem hiding this comment.
[/codebase-design] resolveSingleWorkflowSpec returns (bool, string, error) where the bool is hasDispatch — this is a behavioural result mixed into a resolution function. The dispatch flag is specific to standard markdown workflows, but the function is also the entry point for package skill/agent/action files (which always return false). Consider returning a *resolvedWorkflowResult struct so the call site in resolveWorkflowSpecs does not have to unpack positional return values.
💡 Deep module suggestion
type resolvedWorkflowResult struct {
Workflow *ResolvedWorkflow
HasWorkflowDispatch bool
Warning string
}
func resolveSingleWorkflowSpec(ctx context.Context, spec *WorkflowSpec, verbose bool) (*resolvedWorkflowResult, error)This makes the interface stable — adding a field (e.g. Redirected bool) does not require updating every call site.
@copilot please address this.
There was a problem hiding this comment.
Introduced resolvedWorkflowResult struct in d199cd1 and updated both resolveSingleWorkflowSpec and resolveStandardWorkflow to return (*resolvedWorkflowResult, error). The struct carries Workflow *ResolvedWorkflow and Warning string; HasWorkflowDispatch is read directly from result.Workflow.HasWorkflowDispatch at the call site (3df949e) to avoid duplication.
There was a problem hiding this comment.
The refactoring is clean and behavior-preserving. One minor non-blocking improvement noted inline.
### Review summary
- All critical paths (local package resolution, repo package resolution, wildcard expansion, private workflow guards, manifest private checks) are faithfully preserved.
- The
resolveRepositoryPackageWorkflowSpecfallthrough condition correctly inverts the original guard. - The 5-tuple return signature for package-parsing helpers is readable given the multiple return values in the original.
- One non-blocking issue filed: unnecessary
make/copyinselectBootstrapProfile'sdefaultcase.
🔎 Code quality review by PR Code Quality Reviewer · 183.6 AIC · ⌖ 4.43 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/add_workflow_resolution.go:531
Unnecessary defensive copy in selectBootstrapProfile's default case adds complexity with no benefit.
<details>
<summary>💡 Details</summary>
Lines 529–531 allocate a new slice, copy the existing warnings, then append the new one:
updatedWarnings := make([]string, len(resolutionWarnings), len(resolutionWarnings)+1)
copy(updatedWarnings, resolutionWarnings)
updatedWarnings = append(updatedWarnings, ...)Go's append already handles capacity growth. The original code simply …
|
@copilot please run the Unresolved review threads:
Please resolve the review threads and update the branch as needed before re-running validation.
|
…, remove dead IsPrivate, simplify append Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
This PR addresses a narrow slice of the function-length backlog by splitting
ResolveWorkflowsinpkg/cli/add_workflow_resolution.gointo cohesive helpers while preserving behavior. It reduces onelargefuncfinding inpkg/cliwithout broad rewrites.What changed
ResolveWorkflowsorchestration into focused internal helpers for:Behavior preserved
Code organization improvements