diff --git a/docs/adr/52218-extract-helper-functions-to-reduce-largefunc-backlog.md b/docs/adr/52218-extract-helper-functions-to-reduce-largefunc-backlog.md new file mode 100644 index 00000000000..33127d76785 --- /dev/null +++ b/docs/adr/52218-extract-helper-functions-to-reduce-largefunc-backlog.md @@ -0,0 +1,44 @@ +# ADR-52218: Extract Helper Functions to Reduce pkg/cli largefunc Backlog + +**Date**: 2026-08-13 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +A daily `make golint-custom` run surfaces `largefunc` violations in `pkg/cli/add_package_manifest.go`. Three functions exceeded the line-length threshold: `resolveRepositoryPackage`, `parseRepositoryPackageManifest`, and `resolvePackageSkillFiles`. Each function mixed multiple responsibilities (slug parsing, ref resolution, YAML parsing, version checking, metadata population, skill-directory scanning) in a single body, making individual steps hard to test or reason about in isolation. Issue #52208 tracks the full `pkg/cli` largefunc backlog; this PR addresses the first slice. + +### Decision + +We will decompose the three oversized functions into focused, single-responsibility helper functions using the Extract Function refactoring pattern. The helpers remain package-level functions in `pkg/cli/add_package_manifest.go` and are named with the `repositoryPackage*` prefix to be self-documenting. No observable behavior changes; the refactoring is a pure structural improvement to eliminate lint violations. + +### Alternatives Considered + +#### Alternative 1: Suppress lint warnings with `//nolint:largefunc` directives + +Add per-function suppression comments and leave the functions as-is. This is the lowest-effort option. It was rejected because silencing the linter removes the signal without fixing the underlying maintainability concern; the functions remain difficult to read and individually test. + +#### Alternative 2: Reorganize as method receivers on a new struct + +Introduce a `repositoryPackageResolver` struct and convert the pipeline into chained method calls (e.g., `r.splitSlug()`, `r.resolveRef()`, …). This would co-locate state and reduce parameter threading. It was not chosen for this PR because it represents a larger semantic restructuring that goes beyond the targeted lint-reduction scope; the struct shape would need agreement across the team before adoption, and the issue specifically asks for minimal helper extractions. + +### Consequences + +#### Positive +- `largefunc` lint violations in `add_package_manifest.go` are eliminated, keeping the `make golint-custom` baseline clean. +- Each extracted helper has a single, named responsibility and can be tested and reasoned about independently. +- The request-struct pattern (`repositoryPackageExtensionFilesRequest`) avoids long parameter lists and makes callsites readable. + +#### Negative +- The `pkg/cli` package namespace grows with several new `resolveRepositoryPackage*` and `parseRepositoryPackageManifest*` top-level functions, which can feel cluttered when browsing the file. +- The newly introduced `repositoryPackageExtensionFilesRequest` and `repositoryPackageExtensionFiles` types are additional concepts callers must learn, even though their scope is intentionally local. + +#### Neutral +- All existing behavior is preserved; this is a zero-semantic-change refactoring. +- The extracted-function pattern established here will be replicated by follow-on PRs that address the remaining `pkg/cli` largefunc findings tracked in issue #52208. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/add_package_manifest.go b/pkg/cli/add_package_manifest.go index 95ad8c3498c..719a136b669 100644 --- a/pkg/cli/add_package_manifest.go +++ b/pkg/cli/add_package_manifest.go @@ -77,32 +77,11 @@ func (e packageRemoteNotFoundError) Unwrap() []error { } func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host string) (*resolvedRepositoryPackage, error) { - parts := strings.SplitN(repoSpec.RepoSlug, "/", 2) - if len(parts) != 2 { - return nil, fmt.Errorf("repository slug %q is not in 'owner/repo' format. Example: owner/repo", repoSpec.RepoSlug) - } - - owner := parts[0] - repo := parts[1] - // At manifest-fetch time there is no resolved package metadata yet. - ref := repositoryPackageEffectiveRef(repoSpec, nil) - if ref == "" { - if isGhAwRepository(repoSpec.RepoSlug) { - if latestRelease, err := getRepositoryPackageLatestRelease(ctx, repoSpec.RepoSlug, host); err == nil { - ref = latestRelease - } else { - addPackageManifestLog.Printf("failed to resolve latest release for %s (host=%q): %v", repoSpec.RepoSlug, host, err) - } - } - if ref == "" { - ref = "main" - if defaultBranch, err := getRepositoryPackageDefaultBranch(ctx, repoSpec.RepoSlug, host); err == nil { - ref = defaultBranch - } else { - addPackageManifestLog.Printf("failed to resolve default branch for %s (host=%q), falling back to %q: %v", repoSpec.RepoSlug, host, ref, err) - } - } + owner, repo, err := splitRepositoryPackageSlug(repoSpec.RepoSlug) + if err != nil { + return nil, err } + ref := resolveRepositoryPackageRef(ctx, repoSpec, host) packagePath := strings.Trim(repoSpec.PackagePath, "/") manifestPath, manifestContent, err := loadRepositoryPackageManifestFile(ctx, owner, repo, packagePath, ref, host) @@ -115,25 +94,84 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri return nil, err } + installationSources, includeSkillDirs, includeAgentFiles, err := resolveRepositoryPackageInstallablePaths(ctx, owner, repo, packagePath, ref, host, manifest, manifestPath) + if err != nil { + return nil, err + } + + docsPath, err := resolveRepositoryPackageDocsPath(ctx, owner, repo, packagePath, ref, host) + if err != nil { + return nil, err + } + + extensionFiles, err := resolveRepositoryPackageExtensionFiles(ctx, owner, repo, packagePath, ref, host, manifest, includeSkillDirs, includeAgentFiles) + if err != nil { + return nil, err + } + warnings = append(warnings, extensionFiles.warnings...) + + if len(installationSources) == 0 && len(extensionFiles.skillFiles) == 0 && len(extensionFiles.agentFiles) == 0 { + return nil, fmt.Errorf("repository %q does not contain any installable workflows, skills, or agents (either explicitly declared or auto-discovered). Add workflows under 'workflows/', skills under 'skills/', or agents under 'agents/', or declare them explicitly in aw.yml", repositoryPackageIdentifier(repoSpec.RepoSlug, packagePath)) + } + + return newResolvedRepositoryPackage(manifestPath, ref, docsPath, manifest, installationSources, extensionFiles, warnings), nil +} + +func splitRepositoryPackageSlug(repoSlug string) (string, string, error) { + parts := strings.SplitN(repoSlug, "/", 2) + if len(parts) != 2 { + return "", "", fmt.Errorf("repository slug %q is not in 'owner/repo' format. Example: owner/repo", repoSlug) + } + return parts[0], parts[1], nil +} + +func resolveRepositoryPackageRef(ctx context.Context, repoSpec *RepoSpec, host string) string { + // At manifest-fetch time there is no resolved package metadata yet. + ref := repositoryPackageEffectiveRef(repoSpec, nil) + if ref != "" { + return ref + } + if isGhAwRepository(repoSpec.RepoSlug) { + if latestRelease, err := getRepositoryPackageLatestRelease(ctx, repoSpec.RepoSlug, host); err == nil { + return latestRelease + } else { + addPackageManifestLog.Printf("failed to resolve latest release for %s (host=%q): %v", repoSpec.RepoSlug, host, err) + } + } + ref = "main" + if defaultBranch, err := getRepositoryPackageDefaultBranch(ctx, repoSpec.RepoSlug, host); err == nil { + ref = defaultBranch + } else { + addPackageManifestLog.Printf("failed to resolve default branch for %s (host=%q), falling back to %q: %v", repoSpec.RepoSlug, host, ref, err) + } + return ref +} + +func resolveRepositoryPackageInstallablePaths(ctx context.Context, owner, repo, packagePath, ref, host string, manifest *repositoryPackageManifest, manifestPath string) ([]string, []string, []string, error) { includeInstallablePaths, includeSkillDirs, includeAgentFiles := splitManifestIncludePaths(manifest.Includes) includeInstallablePaths = append(includeInstallablePaths, manifest.Files...) installationSources := normalizePackageInstallablePaths(includeInstallablePaths, packagePath) if len(installationSources) == 0 { + var err error installationSources, err = scanRepositoryPackageInstallablePaths(ctx, owner, repo, packagePath, ref, host) if err != nil { - return nil, err + return nil, nil, nil, err } } if err := validateUniqueManifestWorkflowFilenames(installationSources, manifestPath); err != nil { - return nil, err + return nil, nil, nil, err } + return installationSources, includeSkillDirs, includeAgentFiles, nil +} - docsPath, err := resolveRepositoryPackageDocsPath(ctx, owner, repo, packagePath, ref, host) - if err != nil { - return nil, err - } +type repositoryPackageExtensionFiles struct { + skillFiles []resolvedPackageSkillFile + agentFiles []string + warnings []string +} +func resolveRepositoryPackageExtensionFiles(ctx context.Context, owner, repo, packagePath, ref, host string, manifest *repositoryPackageManifest, includeSkillDirs, includeAgentFiles []string) (*repositoryPackageExtensionFiles, error) { // Resolve skill files: explicit from manifest or auto-scanned. explicitSkillDirs := append([]string{}, manifest.Skills...) explicitSkillDirs = append(explicitSkillDirs, includeSkillDirs...) @@ -141,7 +179,6 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri if err != nil { return nil, err } - warnings = append(warnings, skillWarnings...) // Resolve agent files: explicit from manifest or auto-scanned. explicitAgentFiles := append([]string{}, manifest.Agents...) @@ -150,12 +187,16 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri if err != nil { return nil, err } - warnings = append(warnings, agentWarnings...) - if len(installationSources) == 0 && len(skillFiles) == 0 && len(agentFiles) == 0 { - return nil, fmt.Errorf("repository %q does not contain any installable workflows, skills, or agents (either explicitly declared or auto-discovered). Add workflows under 'workflows/', skills under 'skills/', or agents under 'agents/', or declare them explicitly in aw.yml", repositoryPackageIdentifier(repoSpec.RepoSlug, packagePath)) - } + warnings := append(skillWarnings, agentWarnings...) + return &repositoryPackageExtensionFiles{ + skillFiles: skillFiles, + agentFiles: agentFiles, + warnings: warnings, + }, nil +} +func newResolvedRepositoryPackage(manifestPath, ref, docsPath string, manifest *repositoryPackageManifest, installationSources []string, extensionFiles *repositoryPackageExtensionFiles, warnings []string) *resolvedRepositoryPackage { return &resolvedRepositoryPackage{ ManifestPath: manifestPath, ResolvedRef: ref, @@ -166,10 +207,10 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri DocsPath: docsPath, InstallationSource: installationSources, Bootstrap: manifest.Bootstrap, - SkillFiles: skillFiles, - AgentFiles: agentFiles, + SkillFiles: extensionFiles.skillFiles, + AgentFiles: extensionFiles.agentFiles, Warnings: warnings, - }, nil + } } func loadRepositoryPackageManifestFile(ctx context.Context, owner, repo, packagePath, ref, host string) (string, []byte, error) { @@ -205,32 +246,60 @@ type repositoryPackageManifest struct { } func parseRepositoryPackageManifest(manifestPath string, content []byte) (*repositoryPackageManifest, []string, error) { + root, name, err := parseRepositoryPackageManifestRoot(manifestPath, content) + if err != nil { + return nil, nil, err + } + + manifest := &repositoryPackageManifest{ + Name: strings.TrimSpace(name), + } + warnings, err := populateRepositoryPackageManifest(manifest, root, manifestPath) + if err != nil { + return nil, nil, err + } + return manifest, warnings, nil +} + +func parseRepositoryPackageManifestRoot(manifestPath string, content []byte) (map[string]any, string, error) { var raw any if err := yaml.Unmarshal(content, &raw); err != nil { - return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: %s. Ensure the manifest is valid YAML. Example:\nname: My Package", manifestPath, parser.FormatYAMLError(err, 1, string(content))) + return nil, "", fmt.Errorf("invalid Agentic Workflow manifest %q: %s. Ensure the manifest is valid YAML. Example:\nname: My Package", manifestPath, parser.FormatYAMLError(err, 1, string(content))) } root, ok := raw.(map[string]any) if !ok { - return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: top-level document must be a mapping, not a list or scalar. Example:\nname: My Package", manifestPath) + return nil, "", fmt.Errorf("invalid Agentic Workflow manifest %q: top-level document must be a mapping, not a list or scalar. Example:\nname: My Package", manifestPath) } // Validate name before schema validation to provide a clear error message for // the most common manifest authoring error (missing or empty name). name, ok := stringValue(root["name"]) if !ok || strings.TrimSpace(name) == "" { - return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: name must be a non-empty string. Example:\nname: My Package", manifestPath) + return nil, "", fmt.Errorf("invalid Agentic Workflow manifest %q: name must be a non-empty string. Example:\nname: My Package", manifestPath) } if err := parser.ValidateRepositoryPackageManifestWithSchemaAndLocation(root, manifestPath); err != nil { - return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: %w", manifestPath, err) + return nil, "", fmt.Errorf("invalid Agentic Workflow manifest %q: %w", manifestPath, err) } - manifest := &repositoryPackageManifest{ - Name: strings.TrimSpace(name), - } + return root, name, nil +} + +func populateRepositoryPackageManifest(manifest *repositoryPackageManifest, root map[string]any, manifestPath string) ([]string, error) { var warnings []string + if err := populateRepositoryPackageManifestVersions(manifest, root, manifestPath); err != nil { + return nil, err + } + metadataWarnings, err := populateRepositoryPackageManifestMetadata(manifest, root, manifestPath) + if err != nil { + return nil, err + } + warnings = append(warnings, metadataWarnings...) + return warnings, nil +} +func populateRepositoryPackageManifestVersions(manifest *repositoryPackageManifest, root map[string]any, manifestPath string) error { if manifestVersion, ok := stringValue(root["manifest-version"]); ok { manifest.ManifestVersion = strings.TrimSpace(manifestVersion) } else { @@ -240,18 +309,22 @@ func parseRepositoryPackageManifest(manifestPath string, content []byte) (*repos if minVersion, ok := stringValue(root["min-version"]); ok { manifest.MinVersion = strings.TrimSpace(minVersion) if !isSupportedManifestMinVersion(manifest.MinVersion) { - return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version must use vMAJOR.minor.patch, got %q. Example:\nmin-version: v1.2.3", manifestPath, minVersion) + return fmt.Errorf("invalid Agentic Workflow manifest %q: min-version must use vMAJOR.minor.patch, got %q. Example:\nmin-version: v1.2.3", manifestPath, minVersion) } currentVersion := GetVersion() if !semverutil.IsValid(currentVersion) { - return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version validation requires a semantic-versioned compiler, but the current compiler version %q is not a valid semantic version. This indicates a build issue; rebuild gh-aw with a proper version tag. Example: v1.2.3", manifestPath, currentVersion) + return fmt.Errorf("invalid Agentic Workflow manifest %q: min-version validation requires a semantic-versioned compiler, but the current compiler version %q is not a valid semantic version. This indicates a build issue; rebuild gh-aw with a proper version tag. Example: v1.2.3", manifestPath, currentVersion) } currentVersion = semverutil.NormalizeGitDescribeSemver(currentVersion) if semverutil.Compare(currentVersion, manifest.MinVersion) < 0 { - return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version %q requires gh-aw %s or newer (current: %s). Upgrade gh-aw, or lower min-version in aw.yml to a version at or below the current one. Example:\nmin-version: %s", manifestPath, manifest.MinVersion, manifest.MinVersion, currentVersion, currentVersion) + return fmt.Errorf("invalid Agentic Workflow manifest %q: min-version %q requires gh-aw %s or newer (current: %s). Upgrade gh-aw, or lower min-version in aw.yml to a version at or below the current one. Example:\nmin-version: %s", manifestPath, manifest.MinVersion, manifest.MinVersion, currentVersion, currentVersion) } } + return nil +} +func populateRepositoryPackageManifestMetadata(manifest *repositoryPackageManifest, root map[string]any, manifestPath string) ([]string, error) { + var warnings []string if description, ok := stringValue(root["description"]); ok { manifest.Description = description if len(description) > 255 { @@ -299,12 +372,12 @@ func parseRepositoryPackageManifest(manifestPath string, content []byte) (*repos warnings = append(warnings, "Using experimental feature: config") bootstrap, err := extractManifestConfig(configValue, manifestPath) if err != nil { - return nil, nil, err + return nil, err } manifest.Bootstrap = bootstrap } - return manifest, warnings, nil + return warnings, nil } func extractManifestIncludes(value any, manifestPath string) ([]string, []string) { @@ -542,81 +615,97 @@ func agentDirectoryRoot(cleaned string) string { // contain a SKILL.md file but are not already covered by the manifest. Each skill folder // is traversed recursively so that all nested files are included. func resolvePackageSkillFiles(ctx context.Context, owner, repo, packagePath, ref, host string, explicitSkillDirs []string) ([]resolvedPackageSkillFile, []string, error) { - // seenSkillDirs tracks full skill directories already added so that auto-scanned - // duplicates of manifest-specified skills are not added a second time. - seenSkillDirs := make(map[string]struct{}) - var warnings []string - // Step 1: resolve manifest skills first (explicit dirs). + manifestSkillDirs := normalizeManifestSkillDirs(explicitSkillDirs, packagePath) + skillDirs, warnings, err := resolvePackageSkillDirs(ctx, owner, repo, packagePath, ref, host, manifestSkillDirs) + if err != nil { + return nil, nil, err + } + + // manifestSkillDirSet is used to know which dirs require a SKILL.md marker check. + manifestSkillDirSet := make(map[string]struct{}, len(manifestSkillDirs)) + for _, d := range manifestSkillDirs { + manifestSkillDirSet[d] = struct{}{} + } + + var skillFiles []resolvedPackageSkillFile + for _, skillDir := range skillDirs { + files, fileWarnings, err := resolvePackageSkillDirFiles(ctx, owner, repo, ref, host, skillDir, manifestSkillDirSet) + if err != nil { + return nil, nil, err + } + warnings = append(warnings, fileWarnings...) + skillFiles = append(skillFiles, files...) + } + return skillFiles, warnings, nil +} + +func normalizeManifestSkillDirs(explicitSkillDirs []string, packagePath string) []string { var manifestSkillDirs []string for _, dir := range explicitSkillDirs { manifestSkillDirs = append(manifestSkillDirs, joinRepositoryPackagePath(packagePath, dir)) } + return manifestSkillDirs +} +func resolvePackageSkillDirs(ctx context.Context, owner, repo, packagePath, ref, host string, manifestSkillDirs []string) ([]string, []string, error) { + var warnings []string // Step 2: always auto-scan and append any skills not already in the manifest. autoScanned, err := scanPackageSkillDirs(ctx, owner, repo, packagePath, ref, host) if err != nil { // Auto-scan is supplementary for manifest-declared skills; preserve manifest // resolution even when scan fails transiently. - if len(manifestSkillDirs) > 0 { - warnings = append(warnings, fmt.Sprintf("failed to auto-scan skills directory, proceeding with manifest skills only: %v", err)) - } else { + if len(manifestSkillDirs) == 0 { return nil, nil, err } + warnings = append(warnings, fmt.Sprintf("failed to auto-scan skills directory, proceeding with manifest skills only: %v", err)) } // Build the final ordered list: manifest skills first, then auto-scanned extras. var skillDirs []string - appendIfNew := func(dir string) { - if _, exists := seenSkillDirs[dir]; !exists { - seenSkillDirs[dir] = struct{}{} - skillDirs = append(skillDirs, dir) + seenSkillDirs := make(map[string]struct{}) + for _, dir := range append(manifestSkillDirs, autoScanned...) { + if _, exists := seenSkillDirs[dir]; exists { + continue } + seenSkillDirs[dir] = struct{}{} + skillDirs = append(skillDirs, dir) } - for _, dir := range manifestSkillDirs { - appendIfNew(dir) - } - for _, dir := range autoScanned { - appendIfNew(dir) - } - - // manifestSkillDirSet is used to know which dirs require a SKILL.md marker check. - manifestSkillDirSet := make(map[string]struct{}, len(manifestSkillDirs)) - for _, d := range manifestSkillDirs { - manifestSkillDirSet[d] = struct{}{} - } + return skillDirs, warnings, nil +} - var skillFiles []resolvedPackageSkillFile - for _, skillDir := range skillDirs { - // For skills that came from the manifest, validate that the SKILL.md marker - // exists so that typos in the manifest surface as clear warnings. - if _, fromManifest := manifestSkillDirSet[skillDir]; fromManifest { - markerPath := joinRepositoryPackagePath(skillDir, packageSkillMarkerFile) - if _, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, markerPath, ref, host); err != nil { - if isRepositoryFileNotFound(err) { - warnings = append(warnings, fmt.Sprintf("Skill directory %q is missing required %s marker file", skillDir, packageSkillMarkerFile)) - continue - } - return nil, nil, fmt.Errorf("failed to validate skill marker %q (check the repository, ref, and network connectivity): %w", markerPath, err) - } - } - skillName := filepath.Base(skillDir) - // Use recursive listing so that the entire skill folder (including any - // subdirectories) is copied, not just the top-level files. - files, err := listPackageDirFilesRecursivelyForHost(ctx, owner, repo, ref, skillDir, host) - if err != nil { +func resolvePackageSkillDirFiles(ctx context.Context, owner, repo, ref, host, skillDir string, manifestSkillDirSet map[string]struct{}) ([]resolvedPackageSkillFile, []string, error) { + var warnings []string + // For skills that came from the manifest, validate that the SKILL.md marker + // exists so that typos in the manifest surface as clear warnings. + if _, fromManifest := manifestSkillDirSet[skillDir]; fromManifest { + markerPath := joinRepositoryPackagePath(skillDir, packageSkillMarkerFile) + if _, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, markerPath, ref, host); err != nil { if isRepositoryFileNotFound(err) { - warnings = append(warnings, fmt.Sprintf("Skill directory %q not found in package, skipping", skillDir)) - continue + return nil, []string{fmt.Sprintf("Skill directory %q is missing required %s marker file", skillDir, packageSkillMarkerFile)}, nil } - return nil, nil, fmt.Errorf("failed to list files in skill directory %q (check the repository, ref, and network connectivity): %w", skillDir, err) + return nil, nil, fmt.Errorf("failed to validate skill marker %q (check the repository, ref, and network connectivity): %w", markerPath, err) } - for _, file := range files { - skillFiles = append(skillFiles, resolvedPackageSkillFile{ - SourcePath: file, - SkillName: skillName, - }) + } + + // Use recursive listing so that the entire skill folder (including any + // subdirectories) is copied, not just the top-level files. + files, err := listPackageDirFilesRecursivelyForHost(ctx, owner, repo, ref, skillDir, host) + if err != nil { + if isRepositoryFileNotFound(err) { + warnings = append(warnings, fmt.Sprintf("Skill directory %q not found in package, skipping", skillDir)) + return nil, warnings, nil } + return nil, nil, fmt.Errorf("failed to list files in skill directory %q (check the repository, ref, and network connectivity): %w", skillDir, err) + } + + skillFiles := make([]resolvedPackageSkillFile, 0, len(files)) + skillName := filepath.Base(skillDir) + for _, file := range files { + skillFiles = append(skillFiles, resolvedPackageSkillFile{ + SourcePath: file, + SkillName: skillName, + }) } return skillFiles, warnings, nil }