From 0b25bbca151ae0be30b5dd11410591ebf8fed517 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:03:08 +0000 Subject: [PATCH 01/10] Initial plan From 2252f7dd36f0b84fe20ec302c71cf6efbefd6a67 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:27:26 +0000 Subject: [PATCH 02/10] refactor ResolveWorkflows into focused helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_workflow_resolution.go | 474 +++++++++++++++++------------ 1 file changed, 284 insertions(+), 190 deletions(-) diff --git a/pkg/cli/add_workflow_resolution.go b/pkg/cli/add_workflow_resolution.go index 47c7119f6b8..4af89d9337b 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -71,254 +71,348 @@ type ResolvedWorkflows struct { func ResolveWorkflows(ctx context.Context, workflows []string, verbose bool) (*ResolvedWorkflows, error) { resolutionLog.Printf("Resolving workflows: count=%d", len(workflows)) - if len(workflows) == 0 { - return nil, errors.New("at least one workflow name is required") + if err := validateResolveWorkflowsInput(workflows); err != nil { + return nil, err + } + + specResolution, err := parseWorkflowSpecsForResolution(ctx, workflows) + if err != nil { + return nil, err + } + if err := validateCurrentRepositorySpecs(specResolution.ParsedSpecs); err != nil { + return nil, err + } + + parsedSpecs, hasWildcard, err := expandWorkflowSpecsIfNeeded(specResolution.ParsedSpecs, verbose) + if err != nil { + return nil, err + } + + resolvedWorkflows, hasWorkflowDispatch, resolutionWarnings, err := resolveWorkflowSpecs( + ctx, + parsedSpecs, + specResolution.Warnings, + verbose, + ) + if err != nil { + return nil, err } + bootstrapProfile, resolutionWarnings := selectBootstrapProfile(specResolution.BootstrapProfiles, resolutionWarnings) + + resolutionLog.Printf("Resolution complete: resolved=%d workflows, has_wildcard=%t, has_dispatch=%t", + len(resolvedWorkflows), hasWildcard, hasWorkflowDispatch) + + return &ResolvedWorkflows{ + Workflows: resolvedWorkflows, + HasWildcard: hasWildcard, + HasWorkflowDispatch: hasWorkflowDispatch, + Warnings: resolutionWarnings, + BootstrapProfile: bootstrapProfile, + }, nil +} + +type workflowSpecResolution struct { + ParsedSpecs []*WorkflowSpec + Warnings []string + BootstrapProfiles []*resolvedBootstrapProfile +} + +func validateResolveWorkflowsInput(workflows []string) error { + if len(workflows) == 0 { + return errors.New("at least one workflow name is required") + } for i, workflow := range workflows { if workflow == "" { - return nil, fmt.Errorf("workflow name cannot be empty (workflow %d)", i+1) + return fmt.Errorf("workflow name cannot be empty (workflow %d)", i+1) } } + return nil +} - // Parse workflow specifications - parsedSpecs := make([]*WorkflowSpec, 0, len(workflows)) - var resolutionWarnings []string - var bootstrapProfiles []*resolvedBootstrapProfile - +func parseWorkflowSpecsForResolution(ctx context.Context, workflows []string) (*workflowSpecResolution, error) { + result := &workflowSpecResolution{ + ParsedSpecs: make([]*WorkflowSpec, 0, len(workflows)), + } for _, workflow := range workflows { - if pkg, pkgErr := resolveLocalRepositoryPackage(workflow); pkgErr != nil { - return nil, pkgErr - } else if pkg != nil { - resolutionWarnings = append(resolutionWarnings, pkg.Warnings...) - parsedSpecs = appendLocalRepositoryPackageWorkflowSpecs(parsedSpecs, pkg) - if pkg.Bootstrap != nil { - bootstrapProfiles = append(bootstrapProfiles, &resolvedBootstrapProfile{ - PackageID: pkg.ManifestPath, - Source: workflow, - Profile: pkg.Bootstrap, - }) - } - continue + specs, warnings, bootstrapProfile, err := parseSingleWorkflowSpecForResolution(ctx, workflow) + if err != nil { + return nil, err + } + result.ParsedSpecs = append(result.ParsedSpecs, specs...) + result.Warnings = append(result.Warnings, warnings...) + if bootstrapProfile != nil { + result.BootstrapProfiles = append(result.BootstrapProfiles, bootstrapProfile) } + } + return result, nil +} - if repoSpec, ok, repoErr := parseRepositoryPackageSpec(workflow); ok { - if repoErr != nil { - return nil, repoErr - } +func parseSingleWorkflowSpecForResolution(ctx context.Context, workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, error) { + specs, warnings, bootstrapProfile, handled, err := resolveLocalPackageWorkflowSpec(workflow) + if err != nil || handled { + return specs, warnings, bootstrapProfile, err + } - pkg, pkgErr := resolveRepositoryPackage(ctx, repoSpec, explicitHostForRepo(repoSpec.RepoSlug)) - if pkgErr == nil { - resolutionWarnings = append(resolutionWarnings, pkg.Warnings...) - parsedSpecs = appendRepositoryPackageWorkflowSpecs(parsedSpecs, repoSpec, pkg) - if pkg.Bootstrap != nil { - packageID := repositoryPackageIdentifier(repoSpec.RepoSlug, repoSpec.PackagePath) - bootstrapProfiles = append(bootstrapProfiles, &resolvedBootstrapProfile{ - PackageID: packageID, - Source: workflow, - Profile: pkg.Bootstrap, - }) - } - continue - } - if repoSpec.PackagePath == "" || !isRepositoryPackageManifestNotFound(pkgErr) { - return nil, pkgErr - } + specs, warnings, bootstrapProfile, handled, err = resolveRepositoryPackageWorkflowSpec(ctx, workflow) + if err != nil || handled { + return specs, warnings, bootstrapProfile, err + } + + spec, err := parseWorkflowSpec(workflow) + if err == nil { + if spec.IsWildcard && !isLocalWorkflowPath(spec.WorkflowPath) { + return nil, nil, nil, fmt.Errorf("wildcards are only supported for local workflows, not remote repositories: %s", workflow) } + return []*WorkflowSpec{spec}, nil, nil, nil + } - spec, err := parseWorkflowSpec(workflow) - if err != nil { - repoSpec, repoErr := parseRepoSpec(workflow) - if repoErr != nil { - return nil, fmt.Errorf("invalid specification '%s': not a valid workflow path or repository package: %w", workflow, repoErr) - } + specs, warnings, bootstrapProfile, err = resolveRepositoryPackageFallback(ctx, workflow) + return specs, warnings, bootstrapProfile, err +} - pkg, pkgErr := resolveRepositoryPackage(ctx, repoSpec, explicitHostForRepo(repoSpec.RepoSlug)) - if pkgErr != nil { - return nil, pkgErr - } - resolutionWarnings = append(resolutionWarnings, pkg.Warnings...) - parsedSpecs = appendRepositoryPackageWorkflowSpecs(parsedSpecs, repoSpec, pkg) - if pkg.Bootstrap != nil { - packageID := repositoryPackageIdentifier(repoSpec.RepoSlug, repoSpec.PackagePath) - bootstrapProfiles = append(bootstrapProfiles, &resolvedBootstrapProfile{ - PackageID: packageID, - Source: workflow, - Profile: pkg.Bootstrap, - }) - } - continue - } +func resolveLocalPackageWorkflowSpec(workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, bool, error) { + pkg, err := resolveLocalRepositoryPackage(workflow) + if err != nil { + return nil, nil, nil, true, err + } + if pkg == nil { + return nil, nil, nil, false, nil + } - // Wildcards are only supported for local workflows - if spec.IsWildcard && !isLocalWorkflowPath(spec.WorkflowPath) { - return nil, fmt.Errorf("wildcards are only supported for local workflows, not remote repositories: %s", workflow) + var bootstrapProfile *resolvedBootstrapProfile + if pkg.Bootstrap != nil { + bootstrapProfile = &resolvedBootstrapProfile{ + PackageID: pkg.ManifestPath, + Source: workflow, + Profile: pkg.Bootstrap, } + } + specs := appendLocalRepositoryPackageWorkflowSpecs(nil, pkg) + return specs, pkg.Warnings, bootstrapProfile, true, nil +} - parsedSpecs = append(parsedSpecs, spec) +func resolveRepositoryPackageWorkflowSpec(ctx context.Context, workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, bool, error) { + repoSpec, ok, err := parseRepositoryPackageSpec(workflow) + if !ok { + return nil, nil, nil, false, nil + } + if err != nil { + return nil, nil, nil, true, err + } + specs, warnings, bootstrapProfile, err := resolveRepositoryPackageSpecs(ctx, workflow, repoSpec) + if err == nil { + return specs, warnings, bootstrapProfile, true, nil + } + if repoSpec.PackagePath != "" && isRepositoryPackageManifestNotFound(err) { + return nil, nil, nil, false, nil } + return nil, nil, nil, true, err +} - // Check if any workflow is from the current repository - // Skip this check if we can't determine the current repository (e.g., not in a git repo) - currentRepoSlug, repoErr := GetCurrentRepoSlug() - if repoErr == nil { - resolutionLog.Printf("Current repository: %s", currentRepoSlug) - // We successfully determined the current repository, check all workflow specs - for _, spec := range parsedSpecs { - // Skip local workflow specs - if isLocalWorkflowPath(spec.WorkflowPath) { - continue - } +func resolveRepositoryPackageFallback(ctx context.Context, workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, error) { + repoSpec, repoErr := parseRepoSpec(workflow) + if repoErr != nil { + return nil, nil, nil, fmt.Errorf("invalid specification '%s': not a valid workflow path or repository package: %w", workflow, repoErr) + } + return resolveRepositoryPackageSpecs(ctx, workflow, repoSpec) +} - if spec.RepoSlug == currentRepoSlug { - return nil, fmt.Errorf("cannot add workflows from the current repository (%s). The 'add' command is for installing workflows from other repositories", currentRepoSlug) - } +func resolveRepositoryPackageSpecs(ctx context.Context, workflow string, repoSpec *RepoSpec) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, error) { + pkg, err := resolveRepositoryPackage(ctx, repoSpec, explicitHostForRepo(repoSpec.RepoSlug)) + if err != nil { + return nil, nil, nil, err + } + specs := appendRepositoryPackageWorkflowSpecs(nil, repoSpec, pkg) + + var bootstrapProfile *resolvedBootstrapProfile + if pkg.Bootstrap != nil { + bootstrapProfile = &resolvedBootstrapProfile{ + PackageID: repositoryPackageIdentifier(repoSpec.RepoSlug, repoSpec.PackagePath), + Source: workflow, + Profile: pkg.Bootstrap, } - } else { - resolutionLog.Printf("Could not determine current repository: %v", repoErr) } - // If we can't determine the current repository, proceed without the check + return specs, pkg.Warnings, bootstrapProfile, nil +} - // Check if any workflow specs contain wildcards (local only) +func validateCurrentRepositorySpecs(parsedSpecs []*WorkflowSpec) error { + currentRepoSlug, err := GetCurrentRepoSlug() + if err != nil { + resolutionLog.Printf("Could not determine current repository: %v", err) + return nil + } + resolutionLog.Printf("Current repository: %s", currentRepoSlug) + for _, spec := range parsedSpecs { + if isLocalWorkflowPath(spec.WorkflowPath) { + continue + } + if spec.RepoSlug == currentRepoSlug { + return fmt.Errorf("cannot add workflows from the current repository (%s). The 'add' command is for installing workflows from other repositories", currentRepoSlug) + } + } + return nil +} + +func expandWorkflowSpecsIfNeeded(parsedSpecs []*WorkflowSpec, verbose bool) ([]*WorkflowSpec, bool, error) { hasWildcard := sliceutil.Any(parsedSpecs, func(spec *WorkflowSpec) bool { return spec.IsWildcard }) - - // Expand wildcards for local workflows only - if hasWildcard { - var err error - parsedSpecs, err = expandLocalWildcardWorkflows(parsedSpecs, verbose) - if err != nil { - return nil, err - } + if !hasWildcard { + return parsedSpecs, false, nil + } + expandedSpecs, err := expandLocalWildcardWorkflows(parsedSpecs, verbose) + if err != nil { + return nil, false, err } + return expandedSpecs, true, nil +} - // Fetch workflow content and metadata for each workflow +func resolveWorkflowSpecs(ctx context.Context, parsedSpecs []*WorkflowSpec, warnings []string, verbose bool) ([]*ResolvedWorkflow, bool, []string, error) { resolvedWorkflows := make([]*ResolvedWorkflow, 0, len(parsedSpecs)) + resolutionWarnings := append([]string{}, warnings...) hasWorkflowDispatch := false for _, spec := range parsedSpecs { - // Fetch workflow content (including redirect resolution for remote workflows) - resolvedSpec, fetched, err := resolveAddWorkflowSpecAndContent(ctx, spec, verbose) + resolvedWorkflow, workflowHasDispatch, warning, err := resolveSingleWorkflowSpec(ctx, spec, verbose) if err != nil { - return nil, fmt.Errorf("workflow '%s' not found: %w", spec.String(), err) - } - - // Package skill files are installed as-is to the engine's skill directory. - if spec.IsPackageSkillFile { - resolutionLog.Printf("Resolved package skill file: spec=%s, skill=%s, content_size=%d bytes", - spec.String(), spec.SkillName, len(fetched.Content)) - resolvedWorkflows = append(resolvedWorkflows, &ResolvedWorkflow{ - Spec: resolvedSpec, - Content: fetched.Content, - SourceInfo: fetched, - IsPackageSkillFile: true, - SkillName: spec.SkillName, - }) - continue + return nil, false, nil, err } - - // Package agent files are installed as-is to the engine's agents directory. - if spec.IsPackageAgentFile { - resolutionLog.Printf("Resolved package agent file: spec=%s, content_size=%d bytes", - spec.String(), len(fetched.Content)) - resolvedWorkflows = append(resolvedWorkflows, &ResolvedWorkflow{ - Spec: resolvedSpec, - Content: fetched.Content, - SourceInfo: fetched, - IsPackageAgentFile: true, - }) - continue + if workflowHasDispatch { + hasWorkflowDispatch = true } - - // Action workflow files (.yml) are raw GitHub Actions YAML — skip all markdown - // frontmatter processing and install them as-is. - if isActionWorkflowPath(resolvedSpec.WorkflowPath) { - resolutionLog.Printf("Resolved action workflow: spec=%s, content_size=%d bytes", - spec.String(), len(fetched.Content)) - resolvedWorkflows = append(resolvedWorkflows, &ResolvedWorkflow{ - Spec: resolvedSpec, - Content: fetched.Content, - SourceInfo: fetched, - IsActionWorkflow: true, - }) - continue + if warning != "" { + resolutionWarnings = append(resolutionWarnings, warning) } + resolvedWorkflows = append(resolvedWorkflows, resolvedWorkflow) + } - // Extract description from content - description := ExtractWorkflowDescription(string(fetched.Content)) + return resolvedWorkflows, hasWorkflowDispatch, resolutionWarnings, nil +} - // Extract engine from content (if specified in frontmatter) - engine := ExtractWorkflowEngine(string(fetched.Content)) +func resolveSingleWorkflowSpec(ctx context.Context, spec *WorkflowSpec, verbose bool) (*ResolvedWorkflow, bool, string, error) { + resolvedSpec, fetched, err := resolveAddWorkflowSpecAndContent(ctx, spec, verbose) + if err != nil { + return nil, false, "", fmt.Errorf("workflow '%s' not found: %w", spec.String(), err) + } - if spec.FromRepositoryManifest { - privateValue, hasPrivate := ExtractWorkflowPrivateSetting(string(fetched.Content)) - if hasPrivate && privateValue { - manifestPath := joinRepositoryPackagePath(spec.PackagePath, repositoryPackageManifestFileName) - 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 { + return resolvedWorkflow, false, "", nil + } - // Check if workflow is private - private workflows cannot be added to other repositories - isPrivate := ExtractWorkflowPrivate(string(fetched.Content)) - if isPrivate { - return nil, fmt.Errorf("workflow '%s' is private and cannot be added to other repositories", spec.String()) - } + return resolveStandardWorkflow(spec, resolvedSpec, fetched) +} - // Check for workflow_dispatch trigger in content - workflowHasDispatch := checkWorkflowHasDispatchFromContent(string(fetched.Content)) - if workflowHasDispatch { - hasWorkflowDispatch = true - } +func resolvePackageOrActionWorkflow(spec, resolvedSpec *WorkflowSpec, fetched *FetchedWorkflow) (*ResolvedWorkflow, bool) { + if spec.IsPackageSkillFile { + resolutionLog.Printf("Resolved package skill file: spec=%s, skill=%s, content_size=%d bytes", + spec.String(), spec.SkillName, len(fetched.Content)) + return &ResolvedWorkflow{ + Spec: resolvedSpec, + Content: fetched.Content, + SourceInfo: fetched, + IsPackageSkillFile: true, + SkillName: spec.SkillName, + }, true + } - if fetched.ConvertedFromJSON { - resolutionWarnings = append(resolutionWarnings, - fmt.Sprintf("JSON workflow import for %q was best-effort; run an agentic prompt to refine .github/workflows/%s.md", resolvedSpec.WorkflowName, resolvedSpec.WorkflowName)) - } + if spec.IsPackageAgentFile { + resolutionLog.Printf("Resolved package agent file: spec=%s, content_size=%d bytes", + spec.String(), len(fetched.Content)) + return &ResolvedWorkflow{ + Spec: resolvedSpec, + Content: fetched.Content, + SourceInfo: fetched, + IsPackageAgentFile: true, + }, true + } - resolutionLog.Printf("Resolved workflow: spec=%s, engine=%s, has_dispatch=%t, content_size=%d bytes", - spec.String(), engine, workflowHasDispatch, len(fetched.Content)) + if isActionWorkflowPath(resolvedSpec.WorkflowPath) { + resolutionLog.Printf("Resolved action workflow: spec=%s, content_size=%d bytes", + spec.String(), len(fetched.Content)) + return &ResolvedWorkflow{ + Spec: resolvedSpec, + Content: fetched.Content, + SourceInfo: fetched, + IsActionWorkflow: true, + }, true + } - resolvedWorkflows = append(resolvedWorkflows, &ResolvedWorkflow{ - Spec: resolvedSpec, - Content: fetched.Content, - SourceInfo: fetched, - Description: description, - Engine: engine, - HasWorkflowDispatch: workflowHasDispatch, - IsPrivate: isPrivate, - }) + return nil, false +} + +func resolveStandardWorkflow(spec, resolvedSpec *WorkflowSpec, fetched *FetchedWorkflow) (*ResolvedWorkflow, bool, string, error) { + content := string(fetched.Content) + description := ExtractWorkflowDescription(content) + engine := ExtractWorkflowEngine(content) + + if err := validateManifestWorkflowPrivateSetting(spec, resolvedSpec, content); err != nil { + return nil, false, "", err } - resolutionLog.Printf("Resolution complete: resolved=%d workflows, has_wildcard=%t, has_dispatch=%t", - len(resolvedWorkflows), hasWildcard, hasWorkflowDispatch) + isPrivate := ExtractWorkflowPrivate(content) + if isPrivate { + return nil, false, "", fmt.Errorf("workflow '%s' is private and cannot be added to other repositories", spec.String()) + } - // Collect the single bootstrap profile if exactly one package declared one. - // Multiple conflicting profiles produce a warning; the caller gets nil. - var bootstrapProfile *resolvedBootstrapProfile + workflowHasDispatch := checkWorkflowHasDispatchFromContent(content) + resolutionLog.Printf("Resolved workflow: spec=%s, engine=%s, has_dispatch=%t, content_size=%d bytes", + spec.String(), engine, workflowHasDispatch, len(fetched.Content)) + + var warning string + if fetched.ConvertedFromJSON { + warning = fmt.Sprintf( + "JSON workflow import for %q was best-effort; run an agentic prompt to refine .github/workflows/%s.md", + resolvedSpec.WorkflowName, + resolvedSpec.WorkflowName, + ) + } + + return &ResolvedWorkflow{ + Spec: resolvedSpec, + Content: fetched.Content, + SourceInfo: fetched, + Description: description, + Engine: engine, + HasWorkflowDispatch: workflowHasDispatch, + IsPrivate: isPrivate, + }, workflowHasDispatch, warning, nil +} + +func validateManifestWorkflowPrivateSetting(spec, resolvedSpec *WorkflowSpec, content string) error { + if !spec.FromRepositoryManifest { + return nil + } + privateValue, hasPrivate := ExtractWorkflowPrivateSetting(content) + if !hasPrivate || !privateValue { + return nil + } + manifestPath := joinRepositoryPackagePath(spec.PackagePath, repositoryPackageManifestFileName) + return 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, + ) +} + +func selectBootstrapProfile(bootstrapProfiles []*resolvedBootstrapProfile, resolutionWarnings []string) (*resolvedBootstrapProfile, []string) { switch len(bootstrapProfiles) { case 0: - // nothing to do + return nil, resolutionWarnings case 1: - bootstrapProfile = bootstrapProfiles[0] + bootstrapProfile := bootstrapProfiles[0] resolutionLog.Printf("Bootstrap profile found: packageID=%s", bootstrapProfile.PackageID) + return bootstrapProfile, resolutionWarnings default: ids := make([]string, 0, len(bootstrapProfiles)) - for _, p := range bootstrapProfiles { - ids = append(ids, p.PackageID) + for _, profile := range bootstrapProfiles { + ids = append(ids, profile.PackageID) } resolutionLog.Printf("Multiple bootstrap profiles found (%v); skipping all", ids) resolutionWarnings = append(resolutionWarnings, fmt.Sprintf("multiple bootstrap profiles found (%s); bootstrap config will be skipped — run each package separately to apply its config", strings.Join(ids, ", "))) + return nil, resolutionWarnings } - - return &ResolvedWorkflows{ - Workflows: resolvedWorkflows, - HasWildcard: hasWildcard, - HasWorkflowDispatch: hasWorkflowDispatch, - Warnings: resolutionWarnings, - BootstrapProfile: bootstrapProfile, - }, nil } func resolveLocalRepositoryPackage(source string) (*resolvedRepositoryPackage, error) { From 957753326885e1e9e701adc8cd03ece1af036c3e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:33:19 +0000 Subject: [PATCH 03/10] address review note in bootstrap profile helper Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_workflow_resolution.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cli/add_workflow_resolution.go b/pkg/cli/add_workflow_resolution.go index 4af89d9337b..042f51135ab 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -405,8 +405,8 @@ func selectBootstrapProfile(bootstrapProfiles []*resolvedBootstrapProfile, resol return bootstrapProfile, resolutionWarnings default: ids := make([]string, 0, len(bootstrapProfiles)) - for _, profile := range bootstrapProfiles { - ids = append(ids, profile.PackageID) + for _, p := range bootstrapProfiles { + ids = append(ids, p.PackageID) } resolutionLog.Printf("Multiple bootstrap profiles found (%v); skipping all", ids) resolutionWarnings = append(resolutionWarnings, From 6d3515368225b62daf7b6104ef6fa317a8ea0c5d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:39:45 +0000 Subject: [PATCH 04/10] polish helper naming and warning assignment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_workflow_resolution.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/cli/add_workflow_resolution.go b/pkg/cli/add_workflow_resolution.go index 042f51135ab..85bcc3acf8a 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -98,7 +98,8 @@ func ResolveWorkflows(ctx context.Context, workflows []string, verbose bool) (*R return nil, err } - bootstrapProfile, resolutionWarnings := selectBootstrapProfile(specResolution.BootstrapProfiles, resolutionWarnings) + bootstrapProfile, updatedWarnings := selectBootstrapProfile(specResolution.BootstrapProfiles, resolutionWarnings) + resolutionWarnings = updatedWarnings resolutionLog.Printf("Resolution complete: resolved=%d workflows, has_wildcard=%t, has_dispatch=%t", len(resolvedWorkflows), hasWildcard, hasWorkflowDispatch) @@ -112,7 +113,7 @@ func ResolveWorkflows(ctx context.Context, workflows []string, verbose bool) (*R }, nil } -type workflowSpecResolution struct { +type specResolutionResult struct { ParsedSpecs []*WorkflowSpec Warnings []string BootstrapProfiles []*resolvedBootstrapProfile @@ -130,8 +131,8 @@ func validateResolveWorkflowsInput(workflows []string) error { return nil } -func parseWorkflowSpecsForResolution(ctx context.Context, workflows []string) (*workflowSpecResolution, error) { - result := &workflowSpecResolution{ +func parseWorkflowSpecsForResolution(ctx context.Context, workflows []string) (*specResolutionResult, error) { + result := &specResolutionResult{ ParsedSpecs: make([]*WorkflowSpec, 0, len(workflows)), } for _, workflow := range workflows { From 83b83b80c78397f0bb706babff5dfe166c10e196 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:45:41 +0000 Subject: [PATCH 05/10] use slices.Clone for warning copy clarity Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_workflow_resolution.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cli/add_workflow_resolution.go b/pkg/cli/add_workflow_resolution.go index 85bcc3acf8a..2c45a565249 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "slices" "strings" "github.com/github/gh-aw/pkg/console" @@ -271,7 +272,7 @@ func expandWorkflowSpecsIfNeeded(parsedSpecs []*WorkflowSpec, verbose bool) ([]* func resolveWorkflowSpecs(ctx context.Context, parsedSpecs []*WorkflowSpec, warnings []string, verbose bool) ([]*ResolvedWorkflow, bool, []string, error) { resolvedWorkflows := make([]*ResolvedWorkflow, 0, len(parsedSpecs)) - resolutionWarnings := append([]string{}, warnings...) + resolutionWarnings := slices.Clone(warnings) hasWorkflowDispatch := false for _, spec := range parsedSpecs { From dd747194ae1ffd42db1e2e4adf12043cf2273a3b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:51:56 +0000 Subject: [PATCH 06/10] clarify warning slice handling in helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_workflow_resolution.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cli/add_workflow_resolution.go b/pkg/cli/add_workflow_resolution.go index 2c45a565249..772d24c310b 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "path/filepath" - "slices" "strings" "github.com/github/gh-aw/pkg/console" @@ -272,7 +271,7 @@ func expandWorkflowSpecsIfNeeded(parsedSpecs []*WorkflowSpec, verbose bool) ([]* func resolveWorkflowSpecs(ctx context.Context, parsedSpecs []*WorkflowSpec, warnings []string, verbose bool) ([]*ResolvedWorkflow, bool, []string, error) { resolvedWorkflows := make([]*ResolvedWorkflow, 0, len(parsedSpecs)) - resolutionWarnings := slices.Clone(warnings) + resolutionWarnings := warnings hasWorkflowDispatch := false for _, spec := range parsedSpecs { @@ -411,9 +410,10 @@ func selectBootstrapProfile(bootstrapProfiles []*resolvedBootstrapProfile, resol ids = append(ids, p.PackageID) } resolutionLog.Printf("Multiple bootstrap profiles found (%v); skipping all", ids) - resolutionWarnings = append(resolutionWarnings, + updatedWarnings := append([]string{}, resolutionWarnings...) + updatedWarnings = append(updatedWarnings, fmt.Sprintf("multiple bootstrap profiles found (%s); bootstrap config will be skipped — run each package separately to apply its config", strings.Join(ids, ", "))) - return nil, resolutionWarnings + return nil, updatedWarnings } } From 9a8cf3124c0324c2723924445e81600ea326bc44 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:55:33 +0000 Subject: [PATCH 07/10] optimize bootstrap warning slice copy Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_workflow_resolution.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cli/add_workflow_resolution.go b/pkg/cli/add_workflow_resolution.go index 772d24c310b..8b0fc587d01 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -410,7 +410,8 @@ func selectBootstrapProfile(bootstrapProfiles []*resolvedBootstrapProfile, resol ids = append(ids, p.PackageID) } resolutionLog.Printf("Multiple bootstrap profiles found (%v); skipping all", ids) - updatedWarnings := append([]string{}, resolutionWarnings...) + updatedWarnings := make([]string, len(resolutionWarnings), len(resolutionWarnings)+1) + copy(updatedWarnings, resolutionWarnings) updatedWarnings = append(updatedWarnings, fmt.Sprintf("multiple bootstrap profiles found (%s); bootstrap config will be skipped — run each package separately to apply its config", strings.Join(ids, ", "))) return nil, updatedWarnings From 9bc2bf8f61ddcff1e803b732ce3b02a0358c4167 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 16 Jul 2026 10:56:50 +0000 Subject: [PATCH 08/10] docs(adr): add draft ADR-45965 for ResolveWorkflows decomposition Records the architectural decision to split the monolithic ResolveWorkflows function into focused helper functions. Co-Authored-By: Claude Sonnet 4.6 --- ...decompose-resolveworkflows-into-helpers.md | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 docs/adr/45965-decompose-resolveworkflows-into-helpers.md diff --git a/docs/adr/45965-decompose-resolveworkflows-into-helpers.md b/docs/adr/45965-decompose-resolveworkflows-into-helpers.md new file mode 100644 index 00000000000..5776827cd46 --- /dev/null +++ b/docs/adr/45965-decompose-resolveworkflows-into-helpers.md @@ -0,0 +1,51 @@ +# ADR-45965: Decompose `ResolveWorkflows` into Focused Helper Functions + +**Date**: 2026-07-16 +**Status**: Draft +**Deciders**: Unknown (AI-generated from PR diff; authored by copilot-swe-agent) + +--- + +### Context + +`ResolveWorkflows` in `pkg/cli/add_workflow_resolution.go` grew into a single function exceeding 250 lines of code. It combined six distinct concerns in one body: input validation, workflow spec parsing (local packages, repository packages, and plain specs), current-repository guard checks, wildcard expansion, per-spec content fetching and metadata extraction, and bootstrap profile selection. The `largefunc` linter flagged this as a maintainability problem. Functions of this size are difficult to read, review, and test in isolation. The codebase uses the lint-enforced `largefunc` policy as a code-quality gate, so the violation needed to be resolved. + +### Decision + +We will decompose `ResolveWorkflows` into a pipeline of single-responsibility internal helper functions, keeping the public signature and external call sites unchanged. Each phase (validation, parsing, guard-checking, wildcard expansion, resolution, bootstrap selection) becomes its own named function, with an intermediate `specResolutionResult` struct carrying state between phases. + +### Alternatives Considered + +#### Alternative 1: Maintain the Monolithic Function + +Accept the `largefunc` lint finding and disable or suppress the lint rule for this function. This avoids any risk of behavioral regression from restructuring. + +Not chosen because the project uses `largefunc` as an active quality gate; suppressing it incurs ongoing lint debt and leaves the function difficult to test and review. + +#### Alternative 2: Struct-Based Resolver with Methods + +Introduce a `workflowResolver` struct and convert each phase into a method, using fields to share state between phases instead of a pass-through result struct. + +Not chosen because it would require changing the call site signature or wrapping the public function in a factory, adding abstraction overhead. For a single entrypoint that is not instantiated in different configurations, a free-function pipeline is simpler and the struct carries no meaningful object lifetime. + +### Consequences + +#### Positive +- Resolves one `largefunc` lint violation in `pkg/cli`, keeping the lint gate clean. +- Each helper function has a clear, testable boundary — input validation, spec parsing, wildcard expansion, and bootstrap selection can each be unit-tested independently. +- The `ResolveWorkflows` orchestrator reads as a high-level pipeline, making control flow auditable without tracing a 250-line monolith. +- `specResolutionResult` makes intermediate state explicit and typed instead of implicitly carried in three parallel local variables. + +#### Negative +- More function call sites across the file increase the surface area to navigate when debugging end-to-end behavior. +- The five-return-value signatures on dispatch helpers (`specs, warnings, bootstrapProfile, handled, error`) are non-idiomatic Go and may be confusing to new contributors. +- Each new helper is unexported; if future tests need finer-grained coverage they must be in the same package, which is already the case but limits test organization options. + +#### Neutral +- Public interface and all external call sites are unchanged; no callers need to be updated. +- The refactor is strictly a code organization change — no behavioral changes to validation logic, error messages, or warning aggregation are introduced. +- The `specResolutionResult` struct is local to the file; it is not part of the exported API. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From d199cd123db1c1c2c5b04120d501a77326e224e8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 11:37:59 +0000 Subject: [PATCH 09/10] address review: errNotHandled sentinel, resolvedWorkflowResult struct, remove dead IsPrivate, simplify append Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/add_workflow_resolution.go | 99 +++++++++++++++++------------- 1 file changed, 58 insertions(+), 41 deletions(-) diff --git a/pkg/cli/add_workflow_resolution.go b/pkg/cli/add_workflow_resolution.go index 8b0fc587d01..81fcf3378c7 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -18,6 +18,11 @@ import ( var resolutionLog = logger.New("cli:add_workflow_resolution") var fetchWorkflowFromSourceWithContextFn = FetchWorkflowFromSourceWithContext +// errNotHandled is a sentinel used by package-spec parse helpers to signal +// that a workflow string was not recognized as their type; callers should +// fall through to the next parser. +var errNotHandled = errors.New("not handled") + // ResolvedWorkflow contains metadata about a workflow that has been resolved and is ready to add type ResolvedWorkflow struct { // Spec is the parsed workflow specification @@ -150,14 +155,20 @@ func parseWorkflowSpecsForResolution(ctx context.Context, workflows []string) (* } func parseSingleWorkflowSpecForResolution(ctx context.Context, workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, error) { - specs, warnings, bootstrapProfile, handled, err := resolveLocalPackageWorkflowSpec(workflow) - if err != nil || handled { - return specs, warnings, bootstrapProfile, err + specs, warnings, bootstrapProfile, err := resolveLocalPackageWorkflowSpec(workflow) + if err == nil { + return specs, warnings, bootstrapProfile, nil + } + if !errors.Is(err, errNotHandled) { + return nil, nil, nil, err } - specs, warnings, bootstrapProfile, handled, err = resolveRepositoryPackageWorkflowSpec(ctx, workflow) - if err != nil || handled { - return specs, warnings, bootstrapProfile, err + specs, warnings, bootstrapProfile, err = resolveRepositoryPackageWorkflowSpec(ctx, workflow) + if err == nil { + return specs, warnings, bootstrapProfile, nil + } + if !errors.Is(err, errNotHandled) { + return nil, nil, nil, err } spec, err := parseWorkflowSpec(workflow) @@ -172,13 +183,13 @@ func parseSingleWorkflowSpecForResolution(ctx context.Context, workflow string) return specs, warnings, bootstrapProfile, err } -func resolveLocalPackageWorkflowSpec(workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, bool, error) { +func resolveLocalPackageWorkflowSpec(workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, error) { pkg, err := resolveLocalRepositoryPackage(workflow) if err != nil { - return nil, nil, nil, true, err + return nil, nil, nil, err } if pkg == nil { - return nil, nil, nil, false, nil + return nil, nil, nil, errNotHandled } var bootstrapProfile *resolvedBootstrapProfile @@ -190,25 +201,25 @@ func resolveLocalPackageWorkflowSpec(workflow string) ([]*WorkflowSpec, []string } } specs := appendLocalRepositoryPackageWorkflowSpecs(nil, pkg) - return specs, pkg.Warnings, bootstrapProfile, true, nil + return specs, pkg.Warnings, bootstrapProfile, nil } -func resolveRepositoryPackageWorkflowSpec(ctx context.Context, workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, bool, error) { +func resolveRepositoryPackageWorkflowSpec(ctx context.Context, workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, error) { repoSpec, ok, err := parseRepositoryPackageSpec(workflow) if !ok { - return nil, nil, nil, false, nil + return nil, nil, nil, errNotHandled } if err != nil { - return nil, nil, nil, true, err + return nil, nil, nil, err } specs, warnings, bootstrapProfile, err := resolveRepositoryPackageSpecs(ctx, workflow, repoSpec) if err == nil { - return specs, warnings, bootstrapProfile, true, nil + return specs, warnings, bootstrapProfile, nil } if repoSpec.PackagePath != "" && isRepositoryPackageManifestNotFound(err) { - return nil, nil, nil, false, nil + return nil, nil, nil, errNotHandled } - return nil, nil, nil, true, err + return nil, nil, nil, err } func resolveRepositoryPackageFallback(ctx context.Context, workflow string) ([]*WorkflowSpec, []string, *resolvedBootstrapProfile, error) { @@ -275,30 +286,37 @@ func resolveWorkflowSpecs(ctx context.Context, parsedSpecs []*WorkflowSpec, warn hasWorkflowDispatch := false for _, spec := range parsedSpecs { - resolvedWorkflow, workflowHasDispatch, warning, err := resolveSingleWorkflowSpec(ctx, spec, verbose) + result, err := resolveSingleWorkflowSpec(ctx, spec, verbose) if err != nil { return nil, false, nil, err } - if workflowHasDispatch { + if result.HasWorkflowDispatch { hasWorkflowDispatch = true } - if warning != "" { - resolutionWarnings = append(resolutionWarnings, warning) + if result.Warning != "" { + resolutionWarnings = append(resolutionWarnings, result.Warning) } - resolvedWorkflows = append(resolvedWorkflows, resolvedWorkflow) + resolvedWorkflows = append(resolvedWorkflows, result.Workflow) } return resolvedWorkflows, hasWorkflowDispatch, resolutionWarnings, nil } -func resolveSingleWorkflowSpec(ctx context.Context, spec *WorkflowSpec, verbose bool) (*ResolvedWorkflow, bool, string, error) { +// resolvedWorkflowResult holds the outcome of resolving a single workflow spec. +type resolvedWorkflowResult struct { + Workflow *ResolvedWorkflow + HasWorkflowDispatch bool + Warning string +} + +func resolveSingleWorkflowSpec(ctx context.Context, spec *WorkflowSpec, verbose bool) (*resolvedWorkflowResult, error) { resolvedSpec, fetched, err := resolveAddWorkflowSpecAndContent(ctx, spec, verbose) if err != nil { - return nil, false, "", fmt.Errorf("workflow '%s' not found: %w", spec.String(), err) + return nil, fmt.Errorf("workflow '%s' not found: %w", spec.String(), err) } if resolvedWorkflow, handled := resolvePackageOrActionWorkflow(spec, resolvedSpec, fetched); handled { - return resolvedWorkflow, false, "", nil + return &resolvedWorkflowResult{Workflow: resolvedWorkflow}, nil } return resolveStandardWorkflow(spec, resolvedSpec, fetched) @@ -342,18 +360,17 @@ func resolvePackageOrActionWorkflow(spec, resolvedSpec *WorkflowSpec, fetched *F return nil, false } -func resolveStandardWorkflow(spec, resolvedSpec *WorkflowSpec, fetched *FetchedWorkflow) (*ResolvedWorkflow, bool, string, error) { +func resolveStandardWorkflow(spec, resolvedSpec *WorkflowSpec, fetched *FetchedWorkflow) (*resolvedWorkflowResult, error) { content := string(fetched.Content) description := ExtractWorkflowDescription(content) engine := ExtractWorkflowEngine(content) if err := validateManifestWorkflowPrivateSetting(spec, resolvedSpec, content); err != nil { - return nil, false, "", err + return nil, err } - isPrivate := ExtractWorkflowPrivate(content) - if isPrivate { - return nil, false, "", fmt.Errorf("workflow '%s' is private and cannot be added to other repositories", spec.String()) + if ExtractWorkflowPrivate(content) { + return nil, fmt.Errorf("workflow '%s' is private and cannot be added to other repositories", spec.String()) } workflowHasDispatch := checkWorkflowHasDispatchFromContent(content) @@ -369,15 +386,18 @@ func resolveStandardWorkflow(spec, resolvedSpec *WorkflowSpec, fetched *FetchedW ) } - return &ResolvedWorkflow{ - Spec: resolvedSpec, - Content: fetched.Content, - SourceInfo: fetched, - Description: description, - Engine: engine, + return &resolvedWorkflowResult{ + Workflow: &ResolvedWorkflow{ + Spec: resolvedSpec, + Content: fetched.Content, + SourceInfo: fetched, + Description: description, + Engine: engine, + HasWorkflowDispatch: workflowHasDispatch, + }, HasWorkflowDispatch: workflowHasDispatch, - IsPrivate: isPrivate, - }, workflowHasDispatch, warning, nil + Warning: warning, + }, nil } func validateManifestWorkflowPrivateSetting(spec, resolvedSpec *WorkflowSpec, content string) error { @@ -410,11 +430,8 @@ func selectBootstrapProfile(bootstrapProfiles []*resolvedBootstrapProfile, resol ids = append(ids, p.PackageID) } resolutionLog.Printf("Multiple bootstrap profiles found (%v); skipping all", ids) - updatedWarnings := make([]string, len(resolutionWarnings), len(resolutionWarnings)+1) - copy(updatedWarnings, resolutionWarnings) - updatedWarnings = append(updatedWarnings, + return nil, append(resolutionWarnings, fmt.Sprintf("multiple bootstrap profiles found (%s); bootstrap config will be skipped — run each package separately to apply its config", strings.Join(ids, ", "))) - return nil, updatedWarnings } } From 3df949e2faa1178b7ba72747eeafd7848ab21872 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 11:39:31 +0000 Subject: [PATCH 10/10] remove HasWorkflowDispatch duplication from resolvedWorkflowResult Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/add_workflow_resolution.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/cli/add_workflow_resolution.go b/pkg/cli/add_workflow_resolution.go index 81fcf3378c7..900d357f3ba 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -290,7 +290,7 @@ func resolveWorkflowSpecs(ctx context.Context, parsedSpecs []*WorkflowSpec, warn if err != nil { return nil, false, nil, err } - if result.HasWorkflowDispatch { + if result.Workflow.HasWorkflowDispatch { hasWorkflowDispatch = true } if result.Warning != "" { @@ -304,9 +304,8 @@ func resolveWorkflowSpecs(ctx context.Context, parsedSpecs []*WorkflowSpec, warn // resolvedWorkflowResult holds the outcome of resolving a single workflow spec. type resolvedWorkflowResult struct { - Workflow *ResolvedWorkflow - HasWorkflowDispatch bool - Warning string + Workflow *ResolvedWorkflow + Warning string } func resolveSingleWorkflowSpec(ctx context.Context, spec *WorkflowSpec, verbose bool) (*resolvedWorkflowResult, error) { @@ -395,8 +394,7 @@ func resolveStandardWorkflow(spec, resolvedSpec *WorkflowSpec, fetched *FetchedW Engine: engine, HasWorkflowDispatch: workflowHasDispatch, }, - HasWorkflowDispatch: workflowHasDispatch, - Warning: warning, + Warning: warning, }, nil }