diff --git a/.github/workflows/daily-cli-performance.md b/.github/workflows/daily-cli-performance.md index 93b573dcf8..2bf6a000c8 100644 --- a/.github/workflows/daily-cli-performance.md +++ b/.github/workflows/daily-cli-performance.md @@ -79,7 +79,7 @@ This will execute `make bench-performance` which runs targeted performance bench The targeted benchmarks include: - **Workflow compilation**: CompileSimpleWorkflow, CompileComplexWorkflow, CompileMCPWorkflow, CompileMemoryUsage - **Workflow phases**: ParseWorkflow, Validation, YAMLGeneration -- **CLI helpers**: ExtractWorkflowNameFromFile, UpdateWorkflowTitle, FindIncludesInContent +- **CLI helpers**: ExtractWorkflowNameFromFile, FindIncludesInContent **Step 3**: Copy results to our tracking directory diff --git a/Makefile b/Makefile index f312edea2d..2fea7550b0 100644 --- a/Makefile +++ b/Makefile @@ -110,7 +110,7 @@ bench-performance: -benchmem -benchtime=3x -run=^$$ ./pkg/workflow | tee bench_performance.txt @echo "" @echo "Also running CLI helper benchmarks..." - @go test -bench='Benchmark(ExtractWorkflowNameFromFile|UpdateWorkflowTitle|FindIncludesInContent)$$' \ + @go test -bench='Benchmark(ExtractWorkflowNameFromFile|FindIncludesInContent)$$' \ -benchmem -benchtime=3x -run=^$$ ./pkg/cli >> bench_performance.txt @echo "" @echo "Performance benchmark results saved to bench_performance.txt" diff --git a/docs/src/content/docs/guides/packaging-imports.md b/docs/src/content/docs/guides/packaging-imports.md index 23445989e7..d88fd62a61 100644 --- a/docs/src/content/docs/guides/packaging-imports.md +++ b/docs/src/content/docs/guides/packaging-imports.md @@ -15,7 +15,7 @@ gh aw add githubnext/agentics/ci-doctor@v1.0.0 # with version gh aw add githubnext/agentics/workflows/ci-doctor.md # explicit path ``` -Use `--name`, `--pr`, `--force`, `--number`, `--engine`, or `--verbose` flags to customize installation. The `source` field is automatically added to workflow frontmatter for tracking origin and enabling updates. +Use `--name`, `--pr`, `--force`, `--engine`, or `--verbose` flags to customize installation. The `source` field is automatically added to workflow frontmatter for tracking origin and enabling updates. ## Updating Workflows diff --git a/docs/src/content/docs/setup/cli.md b/docs/src/content/docs/setup/cli.md index 7140e07334..a04fd020b2 100644 --- a/docs/src/content/docs/setup/cli.md +++ b/docs/src/content/docs/setup/cli.md @@ -139,11 +139,11 @@ Add workflows from The Agentics collection or other repositories to `.github/wor ```bash wrap gh aw add githubnext/agentics/ci-doctor # Add single workflow gh aw add "githubnext/agentics/ci-*" # Add multiple with wildcards -gh aw add ci-doctor --dir shared --number 3 # Organize in subdirectories with copies +gh aw add ci-doctor --dir shared # Organize in subdirectory gh aw add ci-doctor --create-pull-request # Create PR instead of commit ``` -**Options:** `--dir`, `--number`, `--create-pull-request` (or `--pr`), `--no-gitattributes` +**Options:** `--dir`, `--create-pull-request` (or `--pr`), `--no-gitattributes` #### `new` diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index d4af1261be..7cec82ed17 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -18,7 +18,6 @@ var addLog = logger.New("cli:add_command") // AddOptions contains all configuration options for adding workflows type AddOptions struct { - Number int Verbose bool Quiet bool EngineOverride string @@ -63,24 +62,23 @@ Use --non-interactive to skip the guided setup and add workflows directly. Examples: ` + string(constants.CLIExtensionPrefix) + ` add githubnext/agentics/daily-repo-status # Interactive setup (recommended) - ` + string(constants.CLIExtensionPrefix) + ` add githubnext/agentics # List available workflows ` + string(constants.CLIExtensionPrefix) + ` add githubnext/agentics/ci-doctor --non-interactive # Skip interactive mode ` + string(constants.CLIExtensionPrefix) + ` add githubnext/agentics/ci-doctor@v1.0.0 # Add with version ` + string(constants.CLIExtensionPrefix) + ` add githubnext/agentics/workflows/ci-doctor.md@main ` + string(constants.CLIExtensionPrefix) + ` add https://github.com/githubnext/agentics/blob/main/workflows/ci-doctor.md ` + string(constants.CLIExtensionPrefix) + ` add githubnext/agentics/ci-doctor --create-pull-request --force ` + string(constants.CLIExtensionPrefix) + ` add githubnext/agentics/ci-doctor --push # Add and push changes - ` + string(constants.CLIExtensionPrefix) + ` add githubnext/agentics/* - ` + string(constants.CLIExtensionPrefix) + ` add githubnext/agentics/*@v1.0.0 + ` + string(constants.CLIExtensionPrefix) + ` add ./my-workflow.md # Add local workflow + ` + string(constants.CLIExtensionPrefix) + ` add ./*.md # Add all local workflows ` + string(constants.CLIExtensionPrefix) + ` add githubnext/agentics/ci-doctor --dir shared # Add to .github/workflows/shared/ Workflow specifications: - - Two parts: "owner/repo[@version]" (lists available workflows in the repository) - Three parts: "owner/repo/workflow-name[@version]" (implicitly looks in workflows/ directory) - Four+ parts: "owner/repo/workflows/workflow-name.md[@version]" (requires explicit .md extension) - GitHub URL: "https://github.com/owner/repo/blob/branch/path/to/workflow.md" - - Wildcard: "owner/repo/*[@version]" (adds all workflows from the repository) - - Version can be tag, branch, or SHA + - Local file: "./path/to/workflow.md" (adds a workflow from local filesystem) + - Local wildcard: "./*.md" or "./dir/*.md" (adds all .md files matching pattern) + - Version can be tag, branch, or SHA (for remote workflows) The -n flag allows you to specify a custom name for the workflow file (only applies to the first workflow when adding multiple). The --dir flag allows you to specify a subdirectory under .github/workflows/ where the workflow will be added. @@ -93,7 +91,6 @@ Note: To create a new workflow from scratch, use the 'new' command instead.`, Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { workflows := args - numberFlag, _ := cmd.Flags().GetInt("number") engineOverride, _ := cmd.Flags().GetString("engine") nameFlag, _ := cmd.Flags().GetString("name") createPRFlag, _ := cmd.Flags().GetBool("create-pull-request") @@ -116,20 +113,17 @@ Note: To create a new workflow from scratch, use the 'new' command instead.`, // Determine if we should use interactive mode // Interactive mode is the default for TTY unless: // - --non-interactive flag is set - // - Any of the batch/automation flags are set (--create-pull-request, --force, --name, --number > 1, --append) + // - Any of the batch/automation flags are set (--create-pull-request, --force, --name, --append) // - Not a TTY (piped input/output) // - In CI environment - // - This is a repo-only spec (listing workflows) useInteractive := !nonInteractive && !prFlag && !forceFlag && nameFlag == "" && - numberFlag == 1 && appendText == "" && tty.IsStdoutTerminal() && os.Getenv("CI") == "" && - os.Getenv("GO_TEST_MODE") != "true" && - !isRepoOnlySpec(workflows[0]) + os.Getenv("GO_TEST_MODE") != "true" if useInteractive { addLog.Print("Using interactive mode") @@ -138,7 +132,6 @@ Note: To create a new workflow from scratch, use the 'new' command instead.`, // Handle normal (non-interactive) mode opts := AddOptions{ - Number: numberFlag, Verbose: verbose, EngineOverride: engineOverride, Name: nameFlag, @@ -157,9 +150,6 @@ Note: To create a new workflow from scratch, use the 'new' command instead.`, }, } - // Add number flag to add command - cmd.Flags().Int("number", 1, "Create multiple numbered copies") - // Add name flag to add command cmd.Flags().StringP("name", "n", "", "Specify name for the added workflow (without .md extension)") @@ -212,13 +202,7 @@ Note: To create a new workflow from scratch, use the 'new' command instead.`, // with optional repository installation and PR creation. // Returns AddWorkflowsResult containing PR number (if created) and other metadata. func AddWorkflows(workflows []string, opts AddOptions) (*AddWorkflowsResult, error) { - // Check if this is a repo-only specification (owner/repo instead of owner/repo/workflow) - // If so, list available workflows and exit - if len(workflows) == 1 && isRepoOnlySpec(workflows[0]) { - return &AddWorkflowsResult{}, handleRepoOnlySpec(workflows[0], opts.Verbose) - } - - // Resolve workflows first + // Resolve workflows first - fetches content directly from GitHub resolved, err := ResolveWorkflows(workflows, opts.Verbose) if err != nil { return nil, err @@ -253,12 +237,6 @@ func AddResolvedWorkflows(workflowStrings []string, resolved *ResolvedWorkflows, } } - // Extract the workflow specs for processing - processedWorkflows := make([]*WorkflowSpec, len(resolved.Workflows)) - for i, rw := range resolved.Workflows { - processedWorkflows[i] = rw.Spec - } - // Set workflow_dispatch result result.HasWorkflowDispatch = resolved.HasWorkflowDispatch @@ -268,7 +246,7 @@ func AddResolvedWorkflows(workflowStrings []string, resolved *ResolvedWorkflows, // Handle PR creation workflow if opts.CreatePR { addLog.Print("Creating workflow with PR") - prNumber, prURL, err := addWorkflowsWithPR(processedWorkflows, opts) + prNumber, prURL, err := addWorkflowsWithPR(resolved.Workflows, opts) if err != nil { return nil, err } @@ -277,13 +255,13 @@ func AddResolvedWorkflows(workflowStrings []string, resolved *ResolvedWorkflows, return result, nil } - // Handle normal workflow addition + // Handle normal workflow addition - pass resolved workflows with content addLog.Print("Adding workflows normally without PR") - return result, addWorkflowsNormal(processedWorkflows, opts) + return result, addWorkflows(resolved.Workflows, opts) } -// addWorkflowsNormal handles normal workflow addition without PR creation -func addWorkflowsNormal(workflows []*WorkflowSpec, opts AddOptions) error { +// addWorkflows handles workflow addition using pre-fetched content +func addWorkflows(workflows []*ResolvedWorkflow, opts AddOptions) error { // Create file tracker for all operations tracker, err := NewFileTracker() if err != nil { @@ -293,7 +271,11 @@ func addWorkflowsNormal(workflows []*WorkflowSpec, opts AddOptions) error { } tracker = nil } + return addWorkflowsWithTracking(workflows, tracker, opts) +} +// addWorkflows handles workflow addition using pre-fetched content +func addWorkflowsWithTracking(workflows []*ResolvedWorkflow, tracker *FileTracker, opts AddOptions) error { // Ensure .gitattributes is configured unless flag is set if !opts.NoGitattributes { addLog.Print("Configuring .gitattributes") @@ -312,14 +294,14 @@ func addWorkflowsNormal(workflows []*WorkflowSpec, opts AddOptions) error { fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Adding %d workflow(s)...", len(workflows)))) } - // Add each workflow - for i, workflow := range workflows { + // Add each workflow using pre-fetched content + for i, resolved := range workflows { if !opts.Quiet && len(workflows) > 1 { - fmt.Fprintln(os.Stderr, console.FormatProgressMessage(fmt.Sprintf("Adding workflow %d/%d: %s", i+1, len(workflows), workflow.WorkflowName))) + fmt.Fprintln(os.Stderr, console.FormatProgressMessage(fmt.Sprintf("Adding workflow %d/%d: %s", i+1, len(workflows), resolved.Spec.WorkflowName))) } - if err := addWorkflowWithTracking(workflow, tracker, opts); err != nil { - return fmt.Errorf("failed to add workflow '%s': %w", workflow.String(), err) + if err := addWorkflowWithTracking(resolved, tracker, opts); err != nil { + return fmt.Errorf("failed to add workflow '%s': %w", resolved.Spec.String(), err) } } @@ -350,7 +332,7 @@ func addWorkflowsNormal(workflows []*WorkflowSpec, opts AddOptions) error { // Create commit message var commitMessage string if len(workflows) == 1 { - commitMessage = fmt.Sprintf("chore: add workflow %s", workflows[0].WorkflowName) + commitMessage = fmt.Sprintf("chore: add workflow %s", workflows[0].Spec.WorkflowName) } else { commitMessage = fmt.Sprintf("chore: add %d workflows", len(workflows)) } @@ -376,58 +358,31 @@ func addWorkflowsNormal(workflows []*WorkflowSpec, opts AddOptions) error { } } + // Stage tracked files to git if in a git repository + if isGitRepo() && tracker != nil { + if err := tracker.StageAllFiles(opts.Verbose); err != nil { + return fmt.Errorf("failed to stage workflow files: %w", err) + } + } + return nil } -// addWorkflowWithTracking adds a workflow from components to .github/workflows with file tracking -func addWorkflowWithTracking(workflowSpec *WorkflowSpec, tracker *FileTracker, opts AddOptions) error { +// addWorkflowWithTracking adds a workflow using pre-fetched content with file tracking +func addWorkflowWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, opts AddOptions) error { + workflowSpec := resolved.Spec + sourceContent := resolved.Content + sourceInfo := resolved.SourceInfo + if opts.Verbose { fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Adding workflow: %s", workflowSpec.String()))) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Number of copies: %d", opts.Number))) if opts.Force { fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Force flag enabled: will overwrite existing files")) } } - // Validate number of copies - if opts.Number < 1 { - return fmt.Errorf("number of copies must be a positive integer") - } - - if opts.Verbose { - fmt.Fprintln(os.Stderr, "Locating workflow components...") - } - - workflowPath := workflowSpec.WorkflowPath - - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Looking for workflow file: %s", workflowPath))) - } - - // Try to read the workflow content from multiple sources - sourceContent, sourceInfo, err := findWorkflowInPackageForRepo(workflowSpec, opts.Verbose) - if err != nil { - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(fmt.Sprintf("Workflow '%s' not found.", workflowPath))) - - // Try to list available workflows from the installed package - if err := displayAvailableWorkflows(workflowSpec.RepoSlug, workflowSpec.Version, opts.Verbose); err != nil { - // If we can't list workflows, provide generic help - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("To add workflows to your project:")) - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Use the 'add' command with repository/workflow specifications:")) - fmt.Fprintf(os.Stderr, " %s add owner/repo/workflow-name\n", string(constants.CLIExtensionPrefix)) - fmt.Fprintf(os.Stderr, " %s add owner/repo/workflow-name@version\n", string(constants.CLIExtensionPrefix)) - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Example:")) - fmt.Fprintf(os.Stderr, " %s add githubnext/agentics/ci-doctor\n", string(constants.CLIExtensionPrefix)) - fmt.Fprintf(os.Stderr, " %s add githubnext/agentics/daily-plan@main\n", string(constants.CLIExtensionPrefix)) - } - - return fmt.Errorf("workflow not found: %s", workflowPath) - } - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Read workflow content (%d bytes)", len(sourceContent)))) + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Using pre-fetched workflow content (%d bytes)", len(sourceContent)))) } // Security scan: reject workflows containing malicious or dangerous content @@ -435,7 +390,7 @@ func addWorkflowWithTracking(workflowSpec *WorkflowSpec, tracker *FileTracker, o if findings := workflow.ScanMarkdownSecurity(string(sourceContent)); len(findings) > 0 { fmt.Fprintln(os.Stderr, console.FormatErrorMessage("Security scan failed for workflow")) fmt.Fprintln(os.Stderr, workflow.FormatSecurityFindings(findings)) - return fmt.Errorf("workflow '%s' failed security scan: %d issue(s) detected", workflowPath, len(findings)) + return fmt.Errorf("workflow '%s' failed security scan: %d issue(s) detected", workflowSpec.WorkflowPath, len(findings)) } if opts.Verbose { fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Security scan passed")) @@ -453,21 +408,16 @@ func addWorkflowWithTracking(workflowSpec *WorkflowSpec, tracker *FileTracker, o // Determine the target workflow directory var githubWorkflowsDir string if opts.WorkflowDir != "" { - // Validate that the path is relative if filepath.IsAbs(opts.WorkflowDir) { return fmt.Errorf("workflow directory must be a relative path, got: %s", opts.WorkflowDir) } - // Clean the path to avoid issues with ".." or other problematic elements opts.WorkflowDir = filepath.Clean(opts.WorkflowDir) - // Ensure the path is under .github/workflows if !strings.HasPrefix(opts.WorkflowDir, ".github/workflows") { - // If user provided a subdirectory opts.Name, prepend .github/workflows/ githubWorkflowsDir = filepath.Join(gitRoot, ".github/workflows", opts.WorkflowDir) } else { githubWorkflowsDir = filepath.Join(gitRoot, opts.WorkflowDir) } } else { - // Use default .github/workflows directory githubWorkflowsDir = filepath.Join(gitRoot, ".github/workflows") } @@ -479,177 +429,181 @@ func addWorkflowWithTracking(workflowSpec *WorkflowSpec, tracker *FileTracker, o // Determine the workflowName to use var workflowName string if opts.Name != "" { - // Use the explicitly provided name workflowName = opts.Name } else { - // Extract filename from workflow path and remove .md extension for processing workflowName = workflowSpec.WorkflowName } // Check if a workflow with this name already exists existingFile := filepath.Join(githubWorkflowsDir, workflowName+".md") if _, err := os.Stat(existingFile); err == nil && !opts.Force { - // When adding with wildcard, emit warning and skip instead of error if opts.FromWildcard { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Workflow '%s' already exists in .github/workflows/. Skipping.", workflowName))) return nil } - return fmt.Errorf("workflow '%s' already exists in .github/workflows/. Use a different name with -n flag, remove the existing workflow first, or use --opts.Force to overwrite", workflowName) + return fmt.Errorf("workflow '%s' already exists in .github/workflows/. Use a different name with -n flag, remove the existing workflow first, or use --force to overwrite", workflowName) } - // Collect all @include dependencies from the workflow file - includeDeps, err := collectPackageIncludeDependencies(string(sourceContent), sourceInfo.PackagePath, opts.Verbose) - if err != nil { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to collect include dependencies: %v", err))) + // For remote workflows, fetch and save include dependencies directly from the source + if !isLocalWorkflowPath(workflowSpec.WorkflowPath) { + if err := fetchAndSaveRemoteIncludes(string(sourceContent), workflowSpec, githubWorkflowsDir, opts.Verbose, opts.Force, tracker); err != nil { + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch include dependencies: %v", err))) + } + } + } else if sourceInfo != nil && sourceInfo.IsLocal { + // For local workflows, collect and copy include dependencies from local paths + // The source directory is derived from the workflow's path + sourceDir := filepath.Dir(workflowSpec.WorkflowPath) + includeDeps, err := collectLocalIncludeDependencies(string(sourceContent), sourceDir, opts.Verbose) + if err != nil { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to collect include dependencies: %v", err))) + } + if err := copyIncludeDependenciesFromPackageWithForce(includeDeps, githubWorkflowsDir, opts.Verbose, opts.Force, tracker); err != nil { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to copy include dependencies: %v", err))) + } } - // Copy all @include dependencies to .github/workflows maintaining relative paths - if err := copyIncludeDependenciesFromPackageWithForce(includeDeps, githubWorkflowsDir, opts.Verbose, opts.Force, tracker); err != nil { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to copy include dependencies: %v", err))) + // Process the workflow + destFile := filepath.Join(githubWorkflowsDir, workflowName+".md") + + fileExists := false + if _, err := os.Stat(destFile); err == nil { + fileExists = true + if !opts.Force { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Destination file '%s' already exists, skipping.", destFile))) + return nil + } + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Overwriting existing file: %s", destFile))) } - // Process each copy - for i := 1; i <= opts.Number; i++ { - // Construct the destination file path with numbering in .github/workflows - var destFile string - if opts.Number == 1 { - destFile = filepath.Join(githubWorkflowsDir, workflowName+".md") + content := string(sourceContent) + + // Add source field to frontmatter + commitSHA := "" + if sourceInfo != nil { + commitSHA = sourceInfo.CommitSHA + } + sourceString := buildSourceStringWithCommitSHA(workflowSpec, commitSHA) + if sourceString != "" { + updatedContent, err := addSourceToWorkflow(content, sourceString) + if err != nil { + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to add source field: %v", err))) + } } else { - destFile = filepath.Join(githubWorkflowsDir, fmt.Sprintf("%s-%d.md", workflowName, i)) + content = updatedContent } - // Check if destination file already exists - fileExists := false - if _, err := os.Stat(destFile); err == nil { - fileExists = true - if !opts.Force { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Destination file '%s' already exists, skipping.", destFile))) - continue + // Process imports field and replace with workflowspec + processedImportsContent, err := processImportsWithWorkflowSpec(content, workflowSpec, commitSHA, opts.Verbose) + if err != nil { + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to process imports: %v", err))) } - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Overwriting existing file: %s", destFile))) + } else { + content = processedImportsContent } - // Process content for numbered workflows - content := string(sourceContent) - if opts.Number > 1 { - // Update H1 title to include opts.Number - content = updateWorkflowTitle(content, i) + // Process @include directives and replace with workflowspec + // For local workflows, use the workflow's directory as the base path + includeSourceDir := "" + if sourceInfo != nil && sourceInfo.IsLocal { + includeSourceDir = filepath.Dir(workflowSpec.WorkflowPath) } - - // Add source field to frontmatter - sourceString := buildSourceStringWithCommitSHA(workflowSpec, sourceInfo.CommitSHA) - if sourceString != "" { - updatedContent, err := addSourceToWorkflow(content, sourceString) - if err != nil { - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to add source field: %v", err))) - } - } else { - content = updatedContent + processedContent, err := processIncludesWithWorkflowSpec(content, workflowSpec, commitSHA, includeSourceDir, opts.Verbose) + if err != nil { + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to process includes: %v", err))) } + } else { + content = processedContent + } + } - // Process imports field and replace with workflowspec - processedImportsContent, err := processImportsWithWorkflowSpec(content, workflowSpec, sourceInfo.CommitSHA, opts.Verbose) - if err != nil { - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to process imports: %v", err))) - } - } else { - content = processedImportsContent + // Handle stop-after field modifications + if opts.NoStopAfter { + cleanedContent, err := RemoveFieldFromOnTrigger(content, "stop-after") + if err != nil { + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to remove stop-after field: %v", err))) } - - // Process @include directives and replace with workflowspec - processedContent, err := processIncludesWithWorkflowSpec(content, workflowSpec, sourceInfo.CommitSHA, sourceInfo.PackagePath, opts.Verbose) - if err != nil { - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to process includes: %v", err))) - } - } else { - content = processedContent + } else { + content = cleanedContent + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Removed stop-after field from workflow")) } } - - // Handle stop-after field modifications - if opts.NoStopAfter { - // Remove stop-after field if requested - cleanedContent, err := RemoveFieldFromOnTrigger(content, "stop-after") - if err != nil { - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to remove stop-after field: %v", err))) - } - } else { - content = cleanedContent - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Removed stop-after field from workflow")) - } + } else if opts.StopAfter != "" { + updatedContent, err := SetFieldInOnTrigger(content, "stop-after", opts.StopAfter) + if err != nil { + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to set stop-after field: %v", err))) } - } else if opts.StopAfter != "" { - // Set custom stop-after value if provided - updatedContent, err := SetFieldInOnTrigger(content, "stop-after", opts.StopAfter) - if err != nil { - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to set stop-after field: %v", err))) - } - } else { - content = updatedContent - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Set stop-after field to: %s", opts.StopAfter))) - } + } else { + content = updatedContent + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Set stop-after field to: %s", opts.StopAfter))) } } + } - // Append text if provided - if opts.AppendText != "" { - // Ensure we have a newline before appending - if !strings.HasSuffix(content, "\n") { - content += "\n" + // Handle engine override - add/update the engine field in frontmatter + if opts.EngineOverride != "" { + updatedContent, err := addEngineToWorkflow(content, opts.EngineOverride) + if err != nil { + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to set engine field: %v", err))) + } + } else { + content = updatedContent + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Set engine field to: %s", opts.EngineOverride))) } - content += "\n" + opts.AppendText } + } - // Track the file based on whether it existed before (if tracker is available) - if tracker != nil { - if fileExists { - tracker.TrackModified(destFile) - } else { - tracker.TrackCreated(destFile) - } + // Append text if provided + if opts.AppendText != "" { + if !strings.HasSuffix(content, "\n") { + content += "\n" } + content += "\n" + opts.AppendText + } - // Write the file with restrictive permissions (0600) to follow security best practices - if err := os.WriteFile(destFile, []byte(content), 0600); err != nil { - return fmt.Errorf("failed to write destination file '%s': %w", destFile, err) + // Track the file + if tracker != nil { + if fileExists { + tracker.TrackModified(destFile) + } else { + tracker.TrackCreated(destFile) } + } - // Show detailed output only when not in opts.Quiet mode - if !opts.Quiet { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Added workflow: %s", destFile))) + // Write the file + if err := os.WriteFile(destFile, []byte(content), 0600); err != nil { + return fmt.Errorf("failed to write destination file '%s': %w", destFile, err) + } - // Extract and display description if present - if description := ExtractWorkflowDescription(content); description != "" { - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(description)) - fmt.Fprintln(os.Stderr, "") - } - } + // Show output + if !opts.Quiet { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Added workflow: %s", destFile))) - // Try to compile the workflow and track generated files - if tracker != nil { - if err := compileWorkflowWithTracking(destFile, opts.Verbose, opts.Quiet, opts.EngineOverride, tracker); err != nil { - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(err.Error())) - } - } else { - // Fall back to basic compilation without tracking - if err := compileWorkflow(destFile, opts.Verbose, opts.Quiet, opts.EngineOverride); err != nil { - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(err.Error())) - } + if description := ExtractWorkflowDescription(content); description != "" { + fmt.Fprintln(os.Stderr, "") + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(description)) + fmt.Fprintln(os.Stderr, "") } } - // Stage tracked files to git if in a git repository - if isGitRepo() && tracker != nil { - if err := tracker.StageAllFiles(opts.Verbose); err != nil { - return fmt.Errorf("failed to stage workflow files: %w", err) + // Compile the workflow + if tracker != nil { + if err := compileWorkflowWithTracking(destFile, opts.Verbose, opts.Quiet, opts.EngineOverride, tracker); err != nil { + fmt.Fprintln(os.Stderr, console.FormatErrorMessage(err.Error())) + } + } else { + if err := compileWorkflow(destFile, opts.Verbose, opts.Quiet, opts.EngineOverride); err != nil { + fmt.Fprintln(os.Stderr, console.FormatErrorMessage(err.Error())) } } diff --git a/pkg/cli/add_command_table_test.go b/pkg/cli/add_command_table_test.go deleted file mode 100644 index 811e9bca9c..0000000000 --- a/pkg/cli/add_command_table_test.go +++ /dev/null @@ -1,213 +0,0 @@ -//go:build !integration - -package cli - -import ( - "bytes" - "os" - "path/filepath" - "strings" - "testing" - - "github.com/github/gh-aw/pkg/testutil" -) - -// TestListWorkflowsWithMetadata tests that listWorkflowsWithMetadata extracts workflow metadata correctly -func TestListWorkflowsWithMetadata(t *testing.T) { - // Create a temporary packages directory structure - tempDir := testutil.TempDir(t, "test-*") - - // Override packages directory for testing - t.Setenv("HOME", tempDir) - - // Create a mock package structure - packagePath := filepath.Join(tempDir, ".aw", "packages", "test-owner", "test-repo") - workflowsDir := filepath.Join(packagePath, "workflows") - if err := os.MkdirAll(workflowsDir, 0755); err != nil { - t.Fatalf("Failed to create test directories: %v", err) - } - - // Create workflow files with different metadata configurations - testWorkflows := []struct { - filename string - content string - expectedID string - expectedName string - expectedDesc string - }{ - { - filename: "ci-doctor.md", - content: `--- -on: push -name: CI Doctor -description: Diagnoses and fixes CI/CD pipeline issues ---- - -# CI Doctor Workflow - -This workflow analyzes your CI/CD pipeline. -`, - expectedID: "ci-doctor", - expectedName: "CI Doctor", - expectedDesc: "Diagnoses and fixes CI/CD pipeline issues", - }, - { - filename: "daily-plan.md", - content: `--- -on: - schedule: - - cron: "0 9 * * *" -description: Creates a daily plan based on repository activity ---- - -# Daily Planning Assistant - -This workflow creates a daily plan. -`, - expectedID: "daily-plan", - expectedName: "Daily Planning Assistant", - expectedDesc: "Creates a daily plan based on repository activity", - }, - { - filename: "weekly-summary.md", - content: `--- -on: workflow_dispatch ---- - -# Weekly Summary Report - -This workflow generates a weekly summary. -`, - expectedID: "weekly-summary", - expectedName: "Weekly Summary Report", - expectedDesc: "", - }, - } - - for _, wf := range testWorkflows { - wfPath := filepath.Join(workflowsDir, wf.filename) - if err := os.WriteFile(wfPath, []byte(wf.content), 0644); err != nil { - t.Fatalf("Failed to create workflow file %s: %v", wf.filename, err) - } - } - - // Call listWorkflowsWithMetadata - workflows, err := listWorkflowsWithMetadata("test-owner/test-repo", false) - if err != nil { - t.Fatalf("listWorkflowsWithMetadata failed: %v", err) - } - - // Verify the results - if len(workflows) != len(testWorkflows) { - t.Fatalf("Expected %d workflows, got %d", len(testWorkflows), len(workflows)) - } - - // Create a map for easier verification - workflowMap := make(map[string]WorkflowInfo) - for _, wf := range workflows { - workflowMap[wf.ID] = wf - } - - // Verify each workflow - for _, expected := range testWorkflows { - wf, exists := workflowMap[expected.expectedID] - if !exists { - t.Errorf("Workflow with ID %s not found", expected.expectedID) - continue - } - - if wf.Name != expected.expectedName { - t.Errorf("Workflow %s: expected name %q, got %q", expected.expectedID, expected.expectedName, wf.Name) - } - - if wf.Description != expected.expectedDesc { - t.Errorf("Workflow %s: expected description %q, got %q", expected.expectedID, expected.expectedDesc, wf.Description) - } - } -} - -// TestHandleRepoOnlySpecTableDisplay tests that handleRepoOnlySpec displays workflows as a table -func TestHandleRepoOnlySpecTableDisplay(t *testing.T) { - // Create a temporary packages directory structure - tempDir := testutil.TempDir(t, "test-*") - - // Override packages directory for testing - t.Setenv("HOME", tempDir) - - // Create a mock package structure - packagePath := filepath.Join(tempDir, ".aw", "packages", "test-owner", "test-repo") - workflowsDir := filepath.Join(packagePath, "workflows") - if err := os.MkdirAll(workflowsDir, 0755); err != nil { - t.Fatalf("Failed to create test directories: %v", err) - } - - // Create a workflow file with metadata - workflowContent := `--- -on: push -name: CI Doctor -description: Diagnoses and fixes CI/CD pipeline issues ---- - -# CI Doctor Workflow -` - - wfPath := filepath.Join(workflowsDir, "ci-doctor.md") - if err := os.WriteFile(wfPath, []byte(workflowContent), 0644); err != nil { - t.Fatalf("Failed to create workflow file: %v", err) - } - - // Capture stderr output - oldStderr := os.Stderr - r, w, _ := os.Pipe() - os.Stderr = w - - // Call displayAvailableWorkflows (used by handleRepoOnlySpec) - err := displayAvailableWorkflows("test-owner/test-repo", "", false) - - // Restore stderr and capture output - w.Close() - os.Stderr = oldStderr - var buf bytes.Buffer - buf.ReadFrom(r) - output := buf.String() - - if err != nil { - t.Fatalf("displayAvailableWorkflows failed: %v", err) - } - - // Verify table-like output is present - // Should contain header row with ID, Name, Description - if !strings.Contains(output, "ID") { - t.Errorf("Output should contain 'ID' header, got:\n%s", output) - } - - if !strings.Contains(output, "Name") { - t.Errorf("Output should contain 'Name' header, got:\n%s", output) - } - - if !strings.Contains(output, "Description") { - t.Errorf("Output should contain 'Description' header, got:\n%s", output) - } - - // Should contain workflow data - if !strings.Contains(output, "ci-doctor") { - t.Errorf("Output should contain workflow ID 'ci-doctor', got:\n%s", output) - } - - if !strings.Contains(output, "CI Doctor") { - t.Errorf("Output should contain workflow name 'CI Doctor', got:\n%s", output) - } - - if !strings.Contains(output, "Diagnoses and fixes CI/CD pipeline issues") { - t.Errorf("Output should contain workflow description, got:\n%s", output) - } - - // Should contain example command - if !strings.Contains(output, "Example:") { - t.Errorf("Output should contain 'Example:' section, got:\n%s", output) - } - - if !strings.Contains(output, "gh aw add test-owner/test-repo/ci-doctor") { - t.Errorf("Output should contain example command with workflow ID, got:\n%s", output) - } -} diff --git a/pkg/cli/add_command_test.go b/pkg/cli/add_command_test.go index a0db141ce3..8a836900e0 100644 --- a/pkg/cli/add_command_test.go +++ b/pkg/cli/add_command_test.go @@ -29,11 +29,6 @@ func TestNewAddCommand(t *testing.T) { // Verify flags are registered flags := cmd.Flags() - // Check number flag - numberFlag := flags.Lookup("number") - assert.NotNil(t, numberFlag, "Should have 'number' flag") - assert.Empty(t, numberFlag.Shorthand, "Number flag should not have shorthand (conflicts with logs -c)") - // Check name flag nameFlag := flags.Lookup("name") assert.NotNil(t, nameFlag, "Should have 'name' flag") @@ -88,30 +83,26 @@ func TestAddWorkflows(t *testing.T) { tests := []struct { name string workflows []string - number int expectError bool errorContains string }{ { name: "empty workflows list", workflows: []string{}, - number: 1, expectError: true, errorContains: "at least one workflow", }, { - name: "repo-only spec (should list workflows)", - workflows: []string{"owner/repo"}, - number: 1, - expectError: true, // Will error on workflow listing, but tests repo-only detection + name: "repo-only spec (requires workflow path)", + workflows: []string{"owner/repo"}, + expectError: true, + errorContains: "workflow specification must be in format", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - opts := AddOptions{ - Number: tt.number, - } + opts := AddOptions{} _, err := AddWorkflows(tt.workflows, opts) if tt.expectError { @@ -126,34 +117,17 @@ func TestAddWorkflows(t *testing.T) { } } -// TestUpdateWorkflowTitle is tested in commands_utils_test.go -// Removed duplicate test to avoid redeclaration - // TestAddCommandStructure removed - redundant with TestNewAddCommand func TestAddResolvedWorkflows(t *testing.T) { tests := []struct { name string - number int expectError bool errorContains string }{ { - name: "invalid number - zero", - number: 0, - expectError: true, - errorContains: "number of copies must be a positive integer", - }, - { - name: "invalid number - negative", - number: -1, - expectError: true, - errorContains: "number of copies must be a positive integer", - }, - { - name: "valid number - one", - number: 1, - expectError: true, // Will still error due to missing workflow, but validates number check + name: "valid workflow", + expectError: true, // Will still error due to missing git repo, but validates basic flow }, } @@ -174,9 +148,7 @@ func TestAddResolvedWorkflows(t *testing.T) { }, } - opts := AddOptions{ - Number: tt.number, - } + opts := AddOptions{} _, err := AddResolvedWorkflows( []string{"test/repo/test-workflow"}, resolved, @@ -320,7 +292,6 @@ func TestAddCommandFlagDefaults(t *testing.T) { flagName string defaultValue string }{ - {"number", "1"}, {"name", ""}, {"engine", ""}, {"repo", ""}, diff --git a/pkg/cli/add_current_repo_test.go b/pkg/cli/add_current_repo_test.go index 89f0d0db09..d98dc92016 100644 --- a/pkg/cli/add_current_repo_test.go +++ b/pkg/cli/add_current_repo_test.go @@ -68,7 +68,7 @@ func TestAddWorkflowsFromCurrentRepository(t *testing.T) { // Clear cache before each test ClearCurrentRepoSlugCache() - opts := AddOptions{Number: 1} + opts := AddOptions{} _, err := AddWorkflows(tt.workflowSpecs, opts) if tt.expectError { @@ -152,7 +152,7 @@ func TestAddWorkflowsFromCurrentRepositoryMultiple(t *testing.T) { // Clear cache before each test ClearCurrentRepoSlugCache() - opts := AddOptions{Number: 1} + opts := AddOptions{} _, err := AddWorkflows(tt.workflowSpecs, opts) if tt.expectError { @@ -194,7 +194,7 @@ func TestAddWorkflowsFromCurrentRepositoryNotInGitRepo(t *testing.T) { // When not in a git repo, the check should be skipped (can't determine current repo) // The function should proceed and fail for other reasons (e.g., workflow not found) - opts := AddOptions{Number: 1} + opts := AddOptions{} _, err = AddWorkflows([]string{"some-owner/some-repo/workflow"}, opts) // Should NOT get the "cannot add workflows from the current repository" error diff --git a/pkg/cli/add_gitattributes_test.go b/pkg/cli/add_gitattributes_test.go index 4d9837d403..c4c80c7f0a 100644 --- a/pkg/cli/add_gitattributes_test.go +++ b/pkg/cli/add_gitattributes_test.go @@ -52,7 +52,7 @@ func TestAddCommandUpdatesGitAttributes(t *testing.T) { t.Fatalf("Failed to create .github/workflows directory: %v", err) } - // Create a minimal workflow file to simulate a workflow being added + // Create a minimal workflow content for testing workflowContent := `--- on: push permissions: @@ -64,32 +64,35 @@ engine: copilot This is a test workflow.` - workflowPath := filepath.Join(workflowsDir, "test.md") - if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { - t.Fatalf("Failed to create workflow file: %v", err) - } - - // Create a minimal WorkflowSpec for testing - spec := &WorkflowSpec{ - RepoSpec: RepoSpec{ - RepoSlug: "test/repo", - Version: "", + // Create a ResolvedWorkflow for testing + resolved := &ResolvedWorkflow{ + Spec: &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "test/repo", + Version: "", + }, + WorkflowPath: "./test.md", + WorkflowName: "test", + }, + Content: []byte(workflowContent), + SourceInfo: &FetchedWorkflow{ + Content: []byte(workflowContent), + IsLocal: true, + SourcePath: "./test.md", + CommitSHA: "", }, - WorkflowPath: "test.md", - WorkflowName: "test", } t.Run("gitattributes_updated_by_default", func(t *testing.T) { // Remove any existing .gitattributes os.Remove(".gitattributes") - // Call addWorkflowsNormal with noGitattributes=false - opts := AddOptions{Number: 1} - err := addWorkflowsNormal([]*WorkflowSpec{spec}, opts) + // Call addWorkflows with noGitattributes=false + opts := AddOptions{} + err := addWorkflows([]*ResolvedWorkflow{resolved}, opts) if err != nil { - // We expect this to fail because we don't have a full workflow setup, - // but gitattributes should still be updated before the error - t.Logf("Expected error during workflow addition: %v", err) + // Log any error but don't fail - we're testing gitattributes behavior + t.Logf("Note: workflow addition returned: %v", err) } // Check that .gitattributes was created @@ -114,12 +117,12 @@ This is a test workflow.` // Remove any existing .gitattributes os.Remove(".gitattributes") - opts := AddOptions{Number: 1, NoGitattributes: true} - // Call addWorkflowsNormal with noGitattributes=true - err := addWorkflowsNormal([]*WorkflowSpec{spec}, opts) + opts := AddOptions{NoGitattributes: true} + // Call addWorkflows with noGitattributes=true + err := addWorkflows([]*ResolvedWorkflow{resolved}, opts) if err != nil { - // We expect this to fail because we don't have a full workflow setup - t.Logf("Expected error during workflow addition: %v", err) + // Log any error but don't fail - we're testing gitattributes behavior + t.Logf("Note: workflow addition returned: %v", err) } // Check that .gitattributes was NOT created @@ -137,12 +140,12 @@ This is a test workflow.` t.Fatalf("Failed to create .gitattributes: %v", err) } - opts := AddOptions{Number: 1, NoGitattributes: true} - // Call addWorkflowsNormal with noGitattributes=true - err := addWorkflowsNormal([]*WorkflowSpec{spec}, opts) + opts := AddOptions{NoGitattributes: true} + // Call addWorkflows with noGitattributes=true + err := addWorkflows([]*ResolvedWorkflow{resolved}, opts) if err != nil { - // We expect this to fail because we don't have a full workflow setup - t.Logf("Expected error during workflow addition: %v", err) + // Log any error but don't fail - we're testing gitattributes behavior + t.Logf("Note: workflow addition returned: %v", err) } // Check that .gitattributes was NOT modified diff --git a/pkg/cli/add_integration_test.go b/pkg/cli/add_integration_test.go new file mode 100644 index 0000000000..90cdd4f5d9 --- /dev/null +++ b/pkg/cli/add_integration_test.go @@ -0,0 +1,703 @@ +//go:build integration + +package cli + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/github/gh-aw/pkg/cli/fileutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// addIntegrationTestSetup holds the setup state for add integration tests +type addIntegrationTestSetup struct { + tempDir string + originalWd string + binaryPath string + cleanup func() +} + +// setupAddIntegrationTest creates a minimal test environment for add command: +// - temporary directory +// - git init (required by add command) +// - pre-built gh-aw binary +// Does NOT create .github/workflows - the add command should create it +func setupAddIntegrationTest(t *testing.T) *addIntegrationTestSetup { + t.Helper() + + // Create a temporary directory for the test + tempDir, err := os.MkdirTemp("", "gh-aw-add-integration-*") + require.NoError(t, err, "Failed to create temp directory") + + // Save current working directory and change to temp directory + originalWd, err := os.Getwd() + require.NoError(t, err, "Failed to get current working directory") + + err = os.Chdir(tempDir) + require.NoError(t, err, "Failed to change to temp directory") + + // Initialize git repository (required by add command) + gitInitCmd := exec.Command("git", "init") + gitInitCmd.Dir = tempDir + output, err := gitInitCmd.CombinedOutput() + require.NoError(t, err, "Failed to run git init: %s", string(output)) + + // Configure git user for commits (required for some operations) + gitConfigName := exec.Command("git", "config", "user.name", "Test User") + gitConfigName.Dir = tempDir + _ = gitConfigName.Run() // Ignore errors - may already be configured globally + + gitConfigEmail := exec.Command("git", "config", "user.email", "test@example.com") + gitConfigEmail.Dir = tempDir + _ = gitConfigEmail.Run() // Ignore errors - may already be configured globally + + // Copy the pre-built binary to this test's temp directory + binaryPath := filepath.Join(tempDir, "gh-aw") + err = fileutil.CopyFile(globalBinaryPath, binaryPath) + require.NoError(t, err, "Failed to copy gh-aw binary to temp directory") + + // Make the binary executable + err = os.Chmod(binaryPath, 0755) + require.NoError(t, err, "Failed to make binary executable") + + // Setup cleanup function + cleanup := func() { + _ = os.Chdir(originalWd) + _ = os.RemoveAll(tempDir) + } + + return &addIntegrationTestSetup{ + tempDir: tempDir, + originalWd: originalWd, + binaryPath: binaryPath, + cleanup: cleanup, + } +} + +// TestAddRemoteWorkflowFromURL tests adding a remote workflow via GitHub URL +// This test requires GitHub authentication +func TestAddRemoteWorkflowFromURL(t *testing.T) { + // Skip if GitHub authentication is not available + // Check by running `gh auth status` - if it fails, skip + authCmd := exec.Command("gh", "auth", "status") + if err := authCmd.Run(); err != nil { + t.Skip("Skipping test: GitHub authentication not available (gh auth status failed)") + } + + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Add a workflow from a GitHub URL using the non-interactive flag + // Using a workflow from the gh-aw repo itself for reliability + workflowURL := "https://github.com/github/gh-aw/blob/v0.42.13/.github/workflows/github-mcp-tools-report.md" + + cmd := exec.Command(setup.binaryPath, "add", workflowURL, "--non-interactive", "--verbose") + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + // Log output for debugging + t.Logf("Command output:\n%s", outputStr) + + require.NoError(t, err, "add command should succeed: %s", outputStr) + + // Verify .github/workflows directory was created + workflowsDir := filepath.Join(setup.tempDir, ".github", "workflows") + info, err := os.Stat(workflowsDir) + require.NoError(t, err, ".github/workflows directory should exist") + assert.True(t, info.IsDir(), ".github/workflows should be a directory") + + // Verify the workflow file was created + workflowFile := filepath.Join(workflowsDir, "github-mcp-tools-report.md") + _, err = os.Stat(workflowFile) + require.NoError(t, err, "workflow file should exist: %s", workflowFile) + + // Read and verify the workflow content + content, err := os.ReadFile(workflowFile) + require.NoError(t, err, "should be able to read workflow file") + contentStr := string(content) + + // Verify the workflow has expected content + assert.Contains(t, contentStr, "---", "workflow should have frontmatter delimiters") + assert.Contains(t, contentStr, "on:", "workflow should have trigger definition") + + // Verify source field was added with commit pinning + assert.Contains(t, contentStr, "source:", "workflow should have source field added") + assert.Contains(t, contentStr, "github/gh-aw", "source should reference the source repo") + + // Verify the compiled .lock.yml file was created + lockFile := filepath.Join(workflowsDir, "github-mcp-tools-report.lock.yml") + _, err = os.Stat(lockFile) + require.NoError(t, err, "lock file should exist: %s", lockFile) + + // Verify the lock file contains expected GitHub Actions content + lockContent, err := os.ReadFile(lockFile) + require.NoError(t, err, "should be able to read lock file") + lockContentStr := string(lockContent) + + assert.Contains(t, lockContentStr, "name:", "lock file should have workflow name") + assert.Contains(t, lockContentStr, "jobs:", "lock file should have jobs section") +} + +// TestAddLocalWorkflow tests adding a local workflow file +func TestAddLocalWorkflow(t *testing.T) { + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Create a local workflow file in a separate "source" directory + sourceDir := filepath.Join(setup.tempDir, "source-workflows") + err := os.MkdirAll(sourceDir, 0755) + require.NoError(t, err, "should create source directory") + + localWorkflowPath := filepath.Join(sourceDir, "test-local-workflow.md") + localWorkflowContent := `--- +name: Test Local Workflow +on: + workflow_dispatch: +permissions: + contents: read +engine: copilot +--- + +# Test Local Workflow + +This is a test workflow for integration testing. + +Please analyze the repository and provide a summary. +` + err = os.WriteFile(localWorkflowPath, []byte(localWorkflowContent), 0644) + require.NoError(t, err, "should write local workflow file") + + // Add the local workflow using non-interactive mode + cmd := exec.Command(setup.binaryPath, "add", localWorkflowPath, "--non-interactive", "--verbose") + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + // Log output for debugging + t.Logf("Command output:\n%s", outputStr) + + require.NoError(t, err, "add command should succeed: %s", outputStr) + + // Verify .github/workflows directory was created + workflowsDir := filepath.Join(setup.tempDir, ".github", "workflows") + info, err := os.Stat(workflowsDir) + require.NoError(t, err, ".github/workflows directory should exist") + assert.True(t, info.IsDir(), ".github/workflows should be a directory") + + // Verify the workflow file was copied + destWorkflowFile := filepath.Join(workflowsDir, "test-local-workflow.md") + _, err = os.Stat(destWorkflowFile) + require.NoError(t, err, "workflow file should exist: %s", destWorkflowFile) + + // Read and verify the workflow content + content, err := os.ReadFile(destWorkflowFile) + require.NoError(t, err, "should be able to read workflow file") + contentStr := string(content) + + // Verify the workflow has expected content (original content preserved) + assert.Contains(t, contentStr, "name: Test Local Workflow", "workflow should have original name") + assert.Contains(t, contentStr, "workflow_dispatch", "workflow should have original trigger") + assert.Contains(t, contentStr, "engine: copilot", "workflow should have original engine") + assert.Contains(t, contentStr, "Please analyze the repository", "workflow should have original prompt") + + // Note: For local workflows without a git remote, source field is NOT added + // since we can't determine the repo slug + + // Verify the compiled .lock.yml file was created + lockFile := filepath.Join(workflowsDir, "test-local-workflow.lock.yml") + _, err = os.Stat(lockFile) + require.NoError(t, err, "lock file should exist: %s", lockFile) + + // Verify the lock file contains expected GitHub Actions content + lockContent, err := os.ReadFile(lockFile) + require.NoError(t, err, "should be able to read lock file") + lockContentStr := string(lockContent) + + assert.Contains(t, lockContentStr, "name: \"Test Local Workflow\"", "lock file should have workflow name") + assert.Contains(t, lockContentStr, "workflow_dispatch", "lock file should have trigger") + assert.Contains(t, lockContentStr, "jobs:", "lock file should have jobs section") +} + +// TestAddWorkflowWithCustomName tests adding a workflow with a custom name +func TestAddWorkflowWithCustomName(t *testing.T) { + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Create a local workflow file + sourceDir := filepath.Join(setup.tempDir, "source-workflows") + err := os.MkdirAll(sourceDir, 0755) + require.NoError(t, err, "should create source directory") + + localWorkflowPath := filepath.Join(sourceDir, "original-name.md") + localWorkflowContent := `--- +name: Original Workflow +on: workflow_dispatch +permissions: + contents: read +engine: copilot +--- + +# Original Workflow + +Test content. +` + err = os.WriteFile(localWorkflowPath, []byte(localWorkflowContent), 0644) + require.NoError(t, err, "should write local workflow file") + + // Add with a custom name + cmd := exec.Command(setup.binaryPath, "add", localWorkflowPath, "--non-interactive", "--name", "custom-workflow-name") + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + t.Logf("Command output:\n%s", outputStr) + + require.NoError(t, err, "add command should succeed: %s", outputStr) + + // Verify the workflow file was created with custom name + workflowsDir := filepath.Join(setup.tempDir, ".github", "workflows") + customWorkflowFile := filepath.Join(workflowsDir, "custom-workflow-name.md") + _, err = os.Stat(customWorkflowFile) + require.NoError(t, err, "workflow file with custom name should exist: %s", customWorkflowFile) + + // Verify original name file does NOT exist + originalNameFile := filepath.Join(workflowsDir, "original-name.md") + _, err = os.Stat(originalNameFile) + assert.True(t, os.IsNotExist(err), "original name file should NOT exist") +} + +// TestAddWorkflowToCustomDir tests adding a workflow to a custom subdirectory +func TestAddWorkflowToCustomDir(t *testing.T) { + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Create a local workflow file + sourceDir := filepath.Join(setup.tempDir, "source-workflows") + err := os.MkdirAll(sourceDir, 0755) + require.NoError(t, err, "should create source directory") + + localWorkflowPath := filepath.Join(sourceDir, "test-workflow.md") + localWorkflowContent := `--- +name: Test Workflow +on: workflow_dispatch +permissions: + contents: read +engine: copilot +--- + +# Test Workflow + +Test content. +` + err = os.WriteFile(localWorkflowPath, []byte(localWorkflowContent), 0644) + require.NoError(t, err, "should write local workflow file") + + // Add to a custom directory + cmd := exec.Command(setup.binaryPath, "add", localWorkflowPath, "--non-interactive", "--dir", "experimental") + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + t.Logf("Command output:\n%s", outputStr) + + require.NoError(t, err, "add command should succeed: %s", outputStr) + + // Verify the workflow file was created in the custom subdirectory + customDir := filepath.Join(setup.tempDir, ".github", "workflows", "experimental") + info, err := os.Stat(customDir) + require.NoError(t, err, "custom workflows subdirectory should exist") + assert.True(t, info.IsDir(), "should be a directory") + + workflowFile := filepath.Join(customDir, "test-workflow.md") + _, err = os.Stat(workflowFile) + require.NoError(t, err, "workflow file should exist in custom directory: %s", workflowFile) + + // Verify the lock file is also in the custom directory + lockFile := filepath.Join(customDir, "test-workflow.lock.yml") + _, err = os.Stat(lockFile) + require.NoError(t, err, "lock file should exist in custom directory: %s", lockFile) +} + +// TestAddWorkflowForce tests the --force flag to overwrite existing workflows +func TestAddWorkflowForce(t *testing.T) { + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Create .github/workflows directory with an existing workflow + workflowsDir := filepath.Join(setup.tempDir, ".github", "workflows") + err := os.MkdirAll(workflowsDir, 0755) + require.NoError(t, err, "should create workflows directory") + + existingWorkflowPath := filepath.Join(workflowsDir, "existing-workflow.md") + existingContent := `--- +name: Old Workflow +on: push +permissions: + contents: read +engine: copilot +--- + +# Old Workflow + +This is the OLD content that should be replaced. +` + err = os.WriteFile(existingWorkflowPath, []byte(existingContent), 0644) + require.NoError(t, err, "should write existing workflow file") + + // Create a new workflow file with same name in source directory + sourceDir := filepath.Join(setup.tempDir, "source-workflows") + err = os.MkdirAll(sourceDir, 0755) + require.NoError(t, err, "should create source directory") + + newWorkflowPath := filepath.Join(sourceDir, "existing-workflow.md") + newContent := `--- +name: New Workflow +on: workflow_dispatch +permissions: + contents: read +engine: copilot +--- + +# New Workflow + +This is the NEW content that should replace the old. +` + err = os.WriteFile(newWorkflowPath, []byte(newContent), 0644) + require.NoError(t, err, "should write new workflow file") + + // First try without --force (should fail) + cmdNoForce := exec.Command(setup.binaryPath, "add", newWorkflowPath, "--non-interactive") + cmdNoForce.Dir = setup.tempDir + outputNoForce, errNoForce := cmdNoForce.CombinedOutput() + outputNoForceStr := string(outputNoForce) + + t.Logf("Command output (without --force):\n%s", outputNoForceStr) + + assert.Error(t, errNoForce, "add without --force should fail when file exists") + assert.Contains(t, outputNoForceStr, "already exists", "error should mention file exists") + + // Verify original content is still there + content, err := os.ReadFile(existingWorkflowPath) + require.NoError(t, err, "should read existing workflow") + assert.Contains(t, string(content), "OLD content", "original content should remain") + + // Now try with --force (should succeed) + cmdForce := exec.Command(setup.binaryPath, "add", newWorkflowPath, "--non-interactive", "--force") + cmdForce.Dir = setup.tempDir + outputForce, errForce := cmdForce.CombinedOutput() + outputForceStr := string(outputForce) + + t.Logf("Command output (with --force):\n%s", outputForceStr) + + require.NoError(t, errForce, "add with --force should succeed: %s", outputForceStr) + + // Verify new content replaced old + newContentRead, err := os.ReadFile(existingWorkflowPath) + require.NoError(t, err, "should read updated workflow") + assert.Contains(t, string(newContentRead), "NEW content", "new content should replace old") + assert.NotContains(t, string(newContentRead), "OLD content", "old content should be gone") +} + +// TestAddWorkflowCreatesGitattributes tests that .gitattributes is properly configured +func TestAddWorkflowCreatesGitattributes(t *testing.T) { + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Create a local workflow file + sourceDir := filepath.Join(setup.tempDir, "source-workflows") + err := os.MkdirAll(sourceDir, 0755) + require.NoError(t, err, "should create source directory") + + localWorkflowPath := filepath.Join(sourceDir, "test.md") + localWorkflowContent := `--- +name: Test +on: workflow_dispatch +permissions: + contents: read +engine: copilot +--- + +# Test + +Content. +` + err = os.WriteFile(localWorkflowPath, []byte(localWorkflowContent), 0644) + require.NoError(t, err, "should write local workflow file") + + // Add the workflow + cmd := exec.Command(setup.binaryPath, "add", localWorkflowPath, "--non-interactive") + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + t.Logf("Command output:\n%s", outputStr) + + require.NoError(t, err, "add command should succeed: %s", outputStr) + + // Verify .gitattributes was created + gitattributesPath := filepath.Join(setup.tempDir, ".gitattributes") + _, err = os.Stat(gitattributesPath) + require.NoError(t, err, ".gitattributes file should exist") + + // Verify .gitattributes has the lock file pattern + gitattributesContent, err := os.ReadFile(gitattributesPath) + require.NoError(t, err, "should read .gitattributes") + gitattributesStr := string(gitattributesContent) + + assert.Contains(t, gitattributesStr, ".lock.yml", ".gitattributes should contain lock file pattern") +} + +// TestAddWorkflowNoGitattributes tests that --no-gitattributes skips .gitattributes configuration in the add step +// Note: The compile step may still create .gitattributes, so we check the verbose output instead +func TestAddWorkflowNoGitattributes(t *testing.T) { + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Create a local workflow file + sourceDir := filepath.Join(setup.tempDir, "source-workflows") + err := os.MkdirAll(sourceDir, 0755) + require.NoError(t, err, "should create source directory") + + localWorkflowPath := filepath.Join(sourceDir, "test.md") + localWorkflowContent := `--- +name: Test +on: workflow_dispatch +permissions: + contents: read +engine: copilot +--- + +# Test + +Content. +` + err = os.WriteFile(localWorkflowPath, []byte(localWorkflowContent), 0644) + require.NoError(t, err, "should write local workflow file") + + // Add the workflow with --no-gitattributes and --verbose to see output + cmd := exec.Command(setup.binaryPath, "add", localWorkflowPath, "--non-interactive", "--no-gitattributes", "--verbose") + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + t.Logf("Command output:\n%s", outputStr) + + require.NoError(t, err, "add command should succeed: %s", outputStr) + + // Verify the "Configured .gitattributes" message is NOT in the add step output + // Note: The compile step may still create .gitattributes, but the add step should skip it + assert.NotContains(t, outputStr, "Configured .gitattributes", + "add step should NOT configure .gitattributes when --no-gitattributes is set") +} + +// TestAddRemoteWorkflowWithVersion tests adding a remote workflow with a specific version tag +// Uses the 4+ part format with explicit path since the workflow is in .github/workflows/ +// This test requires GitHub authentication +func TestAddRemoteWorkflowWithVersion(t *testing.T) { + // Skip if GitHub authentication is not available + authCmd := exec.Command("gh", "auth", "status") + if err := authCmd.Run(); err != nil { + t.Skip("Skipping test: GitHub authentication not available (gh auth status failed)") + } + + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Use a workflow spec with explicit path (owner/repo/path/workflow.md@version format) + // The 3-part format (owner/repo/workflow@version) looks in workflows/ directory, + // but this workflow is in .github/workflows/, so we need the explicit path + workflowSpec := "github/gh-aw/.github/workflows/github-mcp-tools-report.md@v0.42.13" + + cmd := exec.Command(setup.binaryPath, "add", workflowSpec, "--non-interactive", "--verbose") + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + t.Logf("Command output:\n%s", outputStr) + + require.NoError(t, err, "add command should succeed: %s", outputStr) + + // Verify the workflow file was created + workflowsDir := filepath.Join(setup.tempDir, ".github", "workflows") + workflowFile := filepath.Join(workflowsDir, "github-mcp-tools-report.md") + _, err = os.Stat(workflowFile) + require.NoError(t, err, "workflow file should exist: %s", workflowFile) + + // Read and verify source pinning + content, err := os.ReadFile(workflowFile) + require.NoError(t, err, "should be able to read workflow file") + contentStr := string(content) + + // Should have source with version pinning + assert.Contains(t, contentStr, "source:", "workflow should have source field") + // The source should reference the commit SHA (not the tag directly) + // This ensures reproducible builds + assert.True(t, + strings.Contains(contentStr, "@") && strings.Contains(contentStr, "github/gh-aw"), + "source should have commit pinning") +} + +// TestAddWorkflowWithEngineOverride tests that --engine flag adds/updates the engine field in the workflow +func TestAddWorkflowWithEngineOverride(t *testing.T) { + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Create a local workflow file WITHOUT an engine specified + sourceDir := filepath.Join(setup.tempDir, "source-workflows") + err := os.MkdirAll(sourceDir, 0755) + require.NoError(t, err, "should create source directory") + + localWorkflowPath := filepath.Join(sourceDir, "no-engine-workflow.md") + localWorkflowContent := `--- +name: Workflow Without Engine +on: workflow_dispatch +permissions: + contents: read +--- + +# Workflow Without Engine + +This workflow does not specify an engine in frontmatter. + +Please analyze the repository. +` + err = os.WriteFile(localWorkflowPath, []byte(localWorkflowContent), 0644) + require.NoError(t, err, "should write local workflow file") + + // Add the workflow with --engine claude + cmd := exec.Command(setup.binaryPath, "add", localWorkflowPath, "--non-interactive", "--engine", "claude") + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + t.Logf("Command output:\n%s", outputStr) + + require.NoError(t, err, "add command should succeed: %s", outputStr) + + // Verify the workflow file was created + workflowsDir := filepath.Join(setup.tempDir, ".github", "workflows") + workflowFile := filepath.Join(workflowsDir, "no-engine-workflow.md") + _, err = os.Stat(workflowFile) + require.NoError(t, err, "workflow file should exist: %s", workflowFile) + + // Read and verify the engine field was added + content, err := os.ReadFile(workflowFile) + require.NoError(t, err, "should be able to read workflow file") + contentStr := string(content) + + // Should have engine: claude in frontmatter + assert.Contains(t, contentStr, "engine: claude", "workflow should have engine field added") +} + +// TestAddWorkflowEngineOverrideReplacesExisting tests that --engine flag replaces existing engine +func TestAddWorkflowEngineOverrideReplacesExisting(t *testing.T) { + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Create a local workflow file WITH an existing engine: copilot + sourceDir := filepath.Join(setup.tempDir, "source-workflows") + err := os.MkdirAll(sourceDir, 0755) + require.NoError(t, err, "should create source directory") + + localWorkflowPath := filepath.Join(sourceDir, "copilot-workflow.md") + localWorkflowContent := `--- +name: Copilot Workflow +on: workflow_dispatch +permissions: + contents: read +engine: copilot +--- + +# Copilot Workflow + +This workflow originally specifies copilot engine. + +Please analyze the repository. +` + err = os.WriteFile(localWorkflowPath, []byte(localWorkflowContent), 0644) + require.NoError(t, err, "should write local workflow file") + + // Add the workflow with --engine claude (should replace copilot) + cmd := exec.Command(setup.binaryPath, "add", localWorkflowPath, "--non-interactive", "--engine", "claude") + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + t.Logf("Command output:\n%s", outputStr) + + require.NoError(t, err, "add command should succeed: %s", outputStr) + + // Verify the workflow file was created + workflowsDir := filepath.Join(setup.tempDir, ".github", "workflows") + workflowFile := filepath.Join(workflowsDir, "copilot-workflow.md") + _, err = os.Stat(workflowFile) + require.NoError(t, err, "workflow file should exist: %s", workflowFile) + + // Read and verify the engine field was updated + content, err := os.ReadFile(workflowFile) + require.NoError(t, err, "should be able to read workflow file") + contentStr := string(content) + + // Should have engine: claude, NOT engine: copilot + assert.Contains(t, contentStr, "engine: claude", "workflow should have engine field updated to claude") + assert.NotContains(t, contentStr, "engine: copilot", "original copilot engine should be replaced") +} + +// TestAddWorkflowWithoutEngineOverridePreservesOriginal tests that without --engine, original engine is preserved +func TestAddWorkflowWithoutEngineOverridePreservesOriginal(t *testing.T) { + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + // Create a local workflow file WITH an engine specified + sourceDir := filepath.Join(setup.tempDir, "source-workflows") + err := os.MkdirAll(sourceDir, 0755) + require.NoError(t, err, "should create source directory") + + localWorkflowPath := filepath.Join(sourceDir, "claude-workflow.md") + localWorkflowContent := `--- +name: Claude Workflow +on: workflow_dispatch +permissions: + contents: read +engine: claude +--- + +# Claude Workflow + +This workflow specifies claude engine. + +Please analyze the repository. +` + err = os.WriteFile(localWorkflowPath, []byte(localWorkflowContent), 0644) + require.NoError(t, err, "should write local workflow file") + + // Add the workflow WITHOUT --engine flag + cmd := exec.Command(setup.binaryPath, "add", localWorkflowPath, "--non-interactive") + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + t.Logf("Command output:\n%s", outputStr) + + require.NoError(t, err, "add command should succeed: %s", outputStr) + + // Verify the workflow file was created + workflowsDir := filepath.Join(setup.tempDir, ".github", "workflows") + workflowFile := filepath.Join(workflowsDir, "claude-workflow.md") + _, err = os.Stat(workflowFile) + require.NoError(t, err, "workflow file should exist: %s", workflowFile) + + // Read and verify the original engine is preserved + content, err := os.ReadFile(workflowFile) + require.NoError(t, err, "should be able to read workflow file") + contentStr := string(content) + + // Should still have engine: claude (original) + assert.Contains(t, contentStr, "engine: claude", "original engine should be preserved") +} diff --git a/pkg/cli/add_interactive_engine.go b/pkg/cli/add_interactive_engine.go index 08eda08d84..e07495606b 100644 --- a/pkg/cli/add_interactive_engine.go +++ b/pkg/cli/add_interactive_engine.go @@ -19,40 +19,43 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error { return err } - // Determine default engine based on workflow preference, existing secrets, then environment + // Determine default engine based on existing secrets, workflow preference, then environment + // Priority order: flag override > existing secrets > workflow frontmatter > environment > default defaultEngine := string(constants.CopilotEngine) - existingSecretNote := "" + workflowSpecifiedEngine := "" + + // Check if workflow specifies a preferred engine in frontmatter + if c.resolvedWorkflows != nil && len(c.resolvedWorkflows.Workflows) > 0 { + for _, wf := range c.resolvedWorkflows.Workflows { + if wf.Engine != "" { + workflowSpecifiedEngine = wf.Engine + addInteractiveLog.Printf("Workflow specifies engine in frontmatter: %s", wf.Engine) + break + } + } + } // If engine is explicitly overridden via flag, use that if c.EngineOverride != "" { defaultEngine = c.EngineOverride } else { - // Priority 0: Check if workflow specifies a preferred engine in frontmatter - if c.resolvedWorkflows != nil && len(c.resolvedWorkflows.Workflows) > 0 { - for _, wf := range c.resolvedWorkflows.Workflows { - if wf.Engine != "" { - defaultEngine = wf.Engine - addInteractiveLog.Printf("Using engine from workflow frontmatter: %s", wf.Engine) - break - } - } - } - } - - // Only check secrets/environment if we haven't already set a preference - workflowHasPreference := c.resolvedWorkflows != nil && len(c.resolvedWorkflows.Workflows) > 0 && c.resolvedWorkflows.Workflows[0].Engine != "" - if c.EngineOverride == "" && !workflowHasPreference { // Priority 1: Check existing repository secrets using EngineOptions + // This takes precedence over workflow preference since users should use what's already available for _, opt := range constants.EngineOptions { if c.existingSecrets[opt.SecretName] { defaultEngine = opt.Value - existingSecretNote = fmt.Sprintf(" (existing %s secret will be used)", opt.SecretName) + addInteractiveLog.Printf("Found existing secret %s, recommending engine: %s", opt.SecretName, opt.Value) break } } - // Priority 2: Check environment variables if no existing secret found - if existingSecretNote == "" { + // Priority 2: If no existing secret found, use workflow frontmatter preference + if defaultEngine == string(constants.CopilotEngine) && workflowSpecifiedEngine != "" { + defaultEngine = workflowSpecifiedEngine + } + + // Priority 3: Check environment variables if no existing secret or workflow preference found + if defaultEngine == string(constants.CopilotEngine) && workflowSpecifiedEngine == "" { for _, opt := range constants.EngineOptions { envVar := opt.SecretName if opt.EnvVarName != "" { @@ -60,6 +63,7 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error { } if os.Getenv(envVar) != "" { defaultEngine = opt.Value + addInteractiveLog.Printf("Found env var %s, recommending engine: %s", envVar, opt.Value) break } } @@ -72,12 +76,23 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error { return c.collectAPIKey(c.EngineOverride) } - // Build engine options with notes about existing secrets + // Inform user if workflow specifies an engine + if workflowSpecifiedEngine != "" { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Workflow specifies engine: %s", workflowSpecifiedEngine))) + } + + // Build engine options with notes about existing secrets and workflow specification var engineOptions []huh.Option[string] for _, opt := range constants.EngineOptions { label := fmt.Sprintf("%s - %s", opt.Label, opt.Description) + // Add markers for secret availability and workflow specification if c.existingSecrets[opt.SecretName] { label += " [secret exists]" + } else { + label += " [no secret]" + } + if opt.Value == workflowSpecifiedEngine { + label += " [specified in workflow]" } engineOptions = append(engineOptions, huh.NewOption(label, opt.Value)) } diff --git a/pkg/cli/add_interactive_git.go b/pkg/cli/add_interactive_git.go index 304c7ddd3c..151219465a 100644 --- a/pkg/cli/add_interactive_git.go +++ b/pkg/cli/add_interactive_git.go @@ -22,7 +22,6 @@ func (c *AddInteractiveConfig) applyChanges(ctx context.Context, workflowFiles, // Pass Quiet=true to suppress detailed output (already shown earlier in interactive mode) // This returns the result including PR number and HasWorkflowDispatch opts := AddOptions{ - Number: 1, Verbose: c.Verbose, Quiet: true, EngineOverride: c.EngineOverride, diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index 2ca94e1cfb..03aada29b9 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -168,7 +168,6 @@ func (c *AddInteractiveConfig) determineFilesToAdd() (workflowFiles []string, in addInteractiveLog.Print("Determining files to add") // Parse the workflow specs to get the files that will be added - // This reuses logic from addWorkflowsNormal to determine what files get created for _, spec := range c.WorkflowSpecs { parsed, parseErr := parseWorkflowSpec(spec) if parseErr != nil { diff --git a/pkg/cli/add_interactive_workflow_test.go b/pkg/cli/add_interactive_workflow_test.go index b67aeb2e7f..0d2821bbf7 100644 --- a/pkg/cli/add_interactive_workflow_test.go +++ b/pkg/cli/add_interactive_workflow_test.go @@ -44,12 +44,17 @@ func TestGetWorkflowStatuses(t *testing.T) { statuses, err := getWorkflowStatuses(tt.pattern, tt.repoOverride, tt.verbose) // Either succeeds or fails gracefully, but shouldn't panic - if err == nil { - assert.NotNil(t, statuses, "Statuses should not be nil on success") - } else { + // Note: statuses may be nil even when err is nil (no workflows found) + if err != nil { // Error is acceptable in test environment without gh CLI setup assert.Error(t, err, "Expected error without gh CLI") } + // If no error and statuses exist, verify they have expected structure + if err == nil && statuses != nil { + for _, status := range statuses { + assert.NotEmpty(t, status.Workflow, "Workflow name should not be empty") + } + } }) } } diff --git a/pkg/cli/add_repo_only_test.go b/pkg/cli/add_repo_only_test.go index 2c9419a2f1..b3e0672c9e 100644 --- a/pkg/cli/add_repo_only_test.go +++ b/pkg/cli/add_repo_only_test.go @@ -3,7 +3,6 @@ package cli import ( - "os" "testing" ) @@ -59,26 +58,3 @@ func TestIsRepoOnlySpec(t *testing.T) { }) } } - -func TestListWorkflowsInPackage(t *testing.T) { - // Since we can't easily mock getPackagesDir, we'll just verify the function - // handles missing packages correctly - workflows, err := listWorkflowsInPackage("absolutely-nonexistent-repo-xyz123/test-repo-abc456", false) - if err == nil { - t.Errorf("Expected error for nonexistent package, got nil. Workflows: %v", workflows) - } -} - -func TestHandleRepoOnlySpecIntegration(t *testing.T) { - // This is more of an integration test that would require GitHub authentication - // We'll skip it in normal test runs - if os.Getenv("GITHUB_TOKEN") == "" && os.Getenv("GH_TOKEN") == "" { - t.Skip("Skipping integration test: no GitHub token available") - } - - // Test with verbose output to see the workflow listing - // Note: This will actually try to install the package - // err := handleRepoOnlySpec("githubnext/agentics", true) - // For now, we'll just verify the function exists and can be called - // without testing the actual GitHub interaction -} diff --git a/pkg/cli/add_wildcard_test.go b/pkg/cli/add_wildcard_test.go index 2cb6573706..abad1c3fb5 100644 --- a/pkg/cli/add_wildcard_test.go +++ b/pkg/cli/add_wildcard_test.go @@ -10,6 +10,8 @@ import ( "testing" "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestParseWorkflowSpecWithWildcard tests parsing workflow specs with wildcards @@ -142,104 +144,14 @@ on: push } } - // Test discovery - discovered, err := discoverWorkflowsInPackage("test-owner/test-repo", "", false) - if err != nil { - t.Fatalf("discoverWorkflowsInPackage() error = %v", err) - } - - if len(discovered) != len(workflows) { - t.Errorf("discoverWorkflowsInPackage() found %d workflows, expected %d", len(discovered), len(workflows)) - } - - // Verify discovered workflow paths - discoveredPaths := make(map[string]bool) - for _, spec := range discovered { - discoveredPaths[spec.WorkflowPath] = true - } - - for _, expectedPath := range workflows { - if !discoveredPaths[expectedPath] { - t.Errorf("Expected workflow %s not found in discovered workflows", expectedPath) - } - } - - // Verify all specs have correct repo info - for _, spec := range discovered { - if spec.RepoSlug != "test-owner/test-repo" { - t.Errorf("Workflow spec has incorrect RepoSlug: %s, expected test-owner/test-repo", spec.RepoSlug) - } - if spec.IsWildcard { - t.Errorf("Discovered workflow spec should not be marked as wildcard") - } - } } -// TestDiscoverWorkflowsInPackage_NotFound tests behavior when package is not found -func TestDiscoverWorkflowsInPackage_NotFound(t *testing.T) { - // Create a temporary packages directory +// TestExpandLocalWildcardWorkflows tests expanding local wildcard workflow specifications +func TestExpandLocalWildcardWorkflows(t *testing.T) { + // Create a temporary directory with workflow files tempDir := testutil.TempDir(t, "test-*") - // Override packages directory for testing - t.Setenv("HOME", tempDir) - - // Try to discover workflows in a non-existent package - _, err := discoverWorkflowsInPackage("nonexistent/repo", "", false) - if err == nil { - t.Error("discoverWorkflowsInPackage() expected error for non-existent package, got nil") - } - - if !strings.Contains(err.Error(), "package not found") { - t.Errorf("discoverWorkflowsInPackage() error should mention 'package not found', got: %v", err) - } -} - -// TestDiscoverWorkflowsInPackage_EmptyPackage tests behavior with empty package -func TestDiscoverWorkflowsInPackage_EmptyPackage(t *testing.T) { - // Create a temporary packages directory - tempDir := testutil.TempDir(t, "test-*") - - // Override packages directory for testing - t.Setenv("HOME", tempDir) - - // Create an empty package directory (use .aw/packages, not .gh-aw/packages) - packagePath := filepath.Join(tempDir, ".aw", "packages", "empty-owner", "empty-repo") - if err := os.MkdirAll(packagePath, 0755); err != nil { - t.Fatalf("Failed to create test directory: %v", err) - } - - // Test discovery - discovered, err := discoverWorkflowsInPackage("empty-owner/empty-repo", "", false) - if err != nil { - t.Fatalf("discoverWorkflowsInPackage() error = %v", err) - } - - if len(discovered) != 0 { - t.Errorf("discoverWorkflowsInPackage() found %d workflows in empty package, expected 0", len(discovered)) - } -} - -// TestExpandWildcardWorkflows tests expanding wildcard workflow specifications -func TestExpandWildcardWorkflows(t *testing.T) { - // Create a temporary packages directory structure - tempDir := testutil.TempDir(t, "test-*") - - // Override packages directory for testing - t.Setenv("HOME", tempDir) - - // Create a mock package with workflows - packagePath := filepath.Join(tempDir, ".aw", "packages", "test-org", "test-repo") - workflowsDir := filepath.Join(packagePath, "workflows") - if err := os.MkdirAll(workflowsDir, 0755); err != nil { - t.Fatalf("Failed to create test directories: %v", err) - } - // Create mock workflow files with valid frontmatter - workflows := []string{ - "workflows/workflow1.md", - "workflows/workflow2.md", - } - validWorkflowContent := `--- on: push --- @@ -247,13 +159,29 @@ on: push # Test Workflow ` - for _, wf := range workflows { - filePath := filepath.Join(packagePath, wf) + workflowFiles := []string{"workflow1.md", "workflow2.md", "workflow3.md"} + for _, wf := range workflowFiles { + filePath := filepath.Join(tempDir, wf) if err := os.WriteFile(filePath, []byte(validWorkflowContent), 0644); err != nil { t.Fatalf("Failed to create test workflow %s: %v", wf, err) } } + // Also create a non-workflow file that should be ignored + if err := os.WriteFile(filepath.Join(tempDir, "README.txt"), []byte("not a workflow"), 0644); err != nil { + t.Fatalf("Failed to create test file: %v", err) + } + + // Change to temp dir to test relative paths + origDir, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get current directory: %v", err) + } + defer func() { _ = os.Chdir(origDir) }() + if err := os.Chdir(tempDir); err != nil { + t.Fatalf("Failed to change to temp directory: %v", err) + } + tests := []struct { name string specs []*WorkflowSpec @@ -262,56 +190,25 @@ on: push errorContains string }{ { - name: "expand_single_wildcard", + name: "expand_local_wildcard", specs: []*WorkflowSpec{ { - RepoSpec: RepoSpec{ - RepoSlug: "test-org/test-repo", - Version: "", - }, - WorkflowPath: "*", + RepoSpec: RepoSpec{}, + WorkflowPath: "./*.md", WorkflowName: "*", IsWildcard: true, }, }, - expectedCount: 2, - expectError: false, - }, - { - name: "mixed_wildcard_and_specific", - specs: []*WorkflowSpec{ - { - RepoSpec: RepoSpec{ - RepoSlug: "test-org/test-repo", - Version: "", - }, - WorkflowPath: "*", - WorkflowName: "*", - IsWildcard: true, - }, - { - RepoSpec: RepoSpec{ - RepoSlug: "other-org/other-repo", - Version: "", - }, - WorkflowPath: "workflows/specific.md", - WorkflowName: "specific", - IsWildcard: false, - }, - }, - expectedCount: 3, // 2 from wildcard + 1 specific + expectedCount: 3, expectError: false, }, { name: "no_wildcard_specs", specs: []*WorkflowSpec{ { - RepoSpec: RepoSpec{ - RepoSlug: "other-org/other-repo", - Version: "", - }, - WorkflowPath: "workflows/specific.md", - WorkflowName: "specific", + RepoSpec: RepoSpec{}, + WorkflowPath: "./workflow1.md", + WorkflowName: "workflow1", IsWildcard: false, }, }, @@ -329,87 +226,66 @@ on: push for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := expandWildcardWorkflows(tt.specs, false) + result, err := expandLocalWildcardWorkflows(tt.specs, false) if tt.expectError { if err == nil { - t.Errorf("expandWildcardWorkflows() expected error, got nil") + t.Errorf("expandLocalWildcardWorkflows() expected error, got nil") return } if tt.errorContains != "" && !strings.Contains(err.Error(), tt.errorContains) { - t.Errorf("expandWildcardWorkflows() error should contain '%s', got: %v", tt.errorContains, err) + t.Errorf("expandLocalWildcardWorkflows() error should contain '%s', got: %v", tt.errorContains, err) } return } if err != nil { - t.Errorf("expandWildcardWorkflows() unexpected error: %v", err) + t.Errorf("expandLocalWildcardWorkflows() unexpected error: %v", err) return } if len(result) != tt.expectedCount { - t.Errorf("expandWildcardWorkflows() returned %d workflows, expected %d", len(result), tt.expectedCount) + t.Errorf("expandLocalWildcardWorkflows() returned %d workflows, expected %d", len(result), tt.expectedCount) } // Verify no wildcard specs remain in result for _, spec := range result { if spec.IsWildcard { - t.Errorf("expandWildcardWorkflows() result contains wildcard spec: %v", spec) + t.Errorf("expandLocalWildcardWorkflows() result contains wildcard spec: %v", spec) } } }) } } -// TestExpandWildcardWorkflows_ErrorHandling tests error cases for wildcard expansion -func TestExpandWildcardWorkflows_ErrorHandling(t *testing.T) { - // Create a temporary packages directory +// TestExpandLocalWildcardWorkflows_NoMatches tests behavior when no files match the wildcard +func TestExpandLocalWildcardWorkflows_NoMatches(t *testing.T) { + // Create an empty temporary directory tempDir := testutil.TempDir(t, "test-*") - // Override packages directory for testing - t.Setenv("HOME", tempDir) + // Change to temp dir to test relative paths + origDir, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get current directory: %v", err) + } + defer func() { _ = os.Chdir(origDir) }() + if err := os.Chdir(tempDir); err != nil { + t.Fatalf("Failed to change to temp directory: %v", err) + } - tests := []struct { - name string - specs []*WorkflowSpec - expectError bool - errorContains string - }{ + specs := []*WorkflowSpec{ { - name: "nonexistent_package", - specs: []*WorkflowSpec{ - { - RepoSpec: RepoSpec{ - RepoSlug: "nonexistent/repo", - Version: "", - }, - WorkflowPath: "*", - WorkflowName: "*", - IsWildcard: true, - }, - }, - expectError: true, - errorContains: "failed to discover workflows", + RepoSpec: RepoSpec{}, + WorkflowPath: "./*.md", + WorkflowName: "*", + IsWildcard: true, }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := expandWildcardWorkflows(tt.specs, false) - - if tt.expectError { - if err == nil { - t.Errorf("expandWildcardWorkflows() expected error, got nil") - return - } - if tt.errorContains != "" && !strings.Contains(err.Error(), tt.errorContains) { - t.Errorf("expandWildcardWorkflows() error should contain '%s', got: %v", tt.errorContains, err) - } - } else if err != nil { - t.Errorf("expandWildcardWorkflows() unexpected error: %v", err) - } - }) - } + _, err = expandLocalWildcardWorkflows(specs, false) + // Should error because no workflows found after expansion + require.Error(t, err, "Should error when no workflows match") + assert.Contains(t, err.Error(), "no workflows to add after expansion") } // TestAddWorkflowWithTracking_WildcardDuplicateHandling tests that when adding workflows from wildcard, @@ -454,17 +330,6 @@ on: push t.Fatalf("Failed to create existing workflow: %v", err) } - // Create a WorkflowSpec for the same workflow - spec := &WorkflowSpec{ - RepoSpec: RepoSpec{ - RepoSlug: "test-org/test-repo", - Version: "", - }, - WorkflowPath: "workflows/test-workflow.md", - WorkflowName: "test-workflow", - IsWildcard: false, - } - // Create a mock package structure with the workflow packagePath := filepath.Join(tempDir, ".aw", "packages", "test-org", "test-repo", "workflows") if err := os.MkdirAll(packagePath, 0755); err != nil { @@ -475,34 +340,4 @@ on: push t.Fatalf("Failed to create mock workflow: %v", err) } - // Test 1: Non-wildcard duplicate should return error - t.Run("non_wildcard_duplicate_returns_error", func(t *testing.T) { - opts := AddOptions{Number: 1} - err := addWorkflowWithTracking(spec, nil, opts) - if err == nil { - t.Error("Expected error for non-wildcard duplicate, got nil") - } - if err != nil && !strings.Contains(err.Error(), "already exists") { - t.Errorf("Expected 'already exists' error, got: %v", err) - } - }) - - // Test 2: Wildcard duplicate should return nil (skip with warning) - t.Run("wildcard_duplicate_returns_nil", func(t *testing.T) { - opts := AddOptions{Number: 1, FromWildcard: true} - err := addWorkflowWithTracking(spec, nil, opts) - if err != nil { - t.Errorf("Expected nil for wildcard duplicate (should skip), got error: %v", err) - } - }) - - // Test 3: Wildcard duplicate with force flag should succeed - t.Run("wildcard_duplicate_with_force_succeeds", func(t *testing.T) { - opts := AddOptions{Number: 1, Force: true, FromWildcard: true} - err := addWorkflowWithTracking(spec, nil, opts) - // This should succeed or return nil - if err != nil && strings.Contains(err.Error(), "already exists") { - t.Errorf("Expected success with force flag, got 'already exists' error: %v", err) - } - }) } diff --git a/pkg/cli/add_workflow_compilation.go b/pkg/cli/add_workflow_compilation.go index 64ee4d02b7..7d39ece0be 100644 --- a/pkg/cli/add_workflow_compilation.go +++ b/pkg/cli/add_workflow_compilation.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "path/filepath" - "strings" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" @@ -14,22 +13,6 @@ import ( var addWorkflowCompilationLog = logger.New("cli:add_workflow_compilation") -// updateWorkflowTitle updates the H1 title in workflow content by appending a number. -// This is used when creating multiple numbered copies of a workflow. -func updateWorkflowTitle(content string, number int) string { - // Find and update the first H1 header - lines := strings.Split(content, "\n") - for i, line := range lines { - if strings.HasPrefix(strings.TrimSpace(line), "# ") { - // Extract the title part and add number - title := strings.TrimSpace(line[2:]) - lines[i] = fmt.Sprintf("# %s %d", title, number) - break - } - } - return strings.Join(lines, "\n") -} - // compileWorkflow compiles a workflow file without refreshing stop time. // This is a convenience wrapper around compileWorkflowWithRefresh. func compileWorkflow(filePath string, verbose bool, quiet bool, engineOverride string) error { @@ -144,3 +127,10 @@ func addSourceToWorkflow(content, source string) (string, error) { // Use shared frontmatter logic that preserves formatting return addFieldToFrontmatter(content, "source", source) } + +// addEngineToWorkflow adds or updates the engine field in the workflow's frontmatter. +// This function preserves the existing frontmatter formatting while setting the engine field. +func addEngineToWorkflow(content, engine string) (string, error) { + // Use shared frontmatter logic that preserves formatting + return addFieldToFrontmatter(content, "engine", engine) +} diff --git a/pkg/cli/add_workflow_not_found_test.go b/pkg/cli/add_workflow_not_found_test.go deleted file mode 100644 index ba95bfa0bd..0000000000 --- a/pkg/cli/add_workflow_not_found_test.go +++ /dev/null @@ -1,203 +0,0 @@ -//go:build !integration - -package cli - -import ( - "bytes" - "os" - "path/filepath" - "strings" - "testing" - - "github.com/github/gh-aw/pkg/testutil" -) - -// TestDisplayAvailableWorkflows tests that displayAvailableWorkflows shows the list of available workflows -func TestDisplayAvailableWorkflows(t *testing.T) { - // Create a temporary packages directory structure - tempDir := testutil.TempDir(t, "test-*") - - // Override packages directory for testing - t.Setenv("HOME", tempDir) - - // Create a mock package structure - packagePath := filepath.Join(tempDir, ".aw", "packages", "test-owner", "test-repo") - workflowsDir := filepath.Join(packagePath, "workflows") - if err := os.MkdirAll(workflowsDir, 0755); err != nil { - t.Fatalf("Failed to create test directories: %v", err) - } - - // Create some mock workflow files with valid frontmatter - validWorkflowContent := `--- -on: push ---- - -# Test Workflow -` - - workflows := []string{ - "ci-doctor.md", - "daily-plan.md", - "weekly-summary.md", - } - - for _, wf := range workflows { - wfPath := filepath.Join(workflowsDir, wf) - if err := os.WriteFile(wfPath, []byte(validWorkflowContent), 0644); err != nil { - t.Fatalf("Failed to create workflow file %s: %v", wf, err) - } - } - - // Capture stderr output - oldStderr := os.Stderr - r, w, _ := os.Pipe() - os.Stderr = w - - // Call displayAvailableWorkflows - err := displayAvailableWorkflows("test-owner/test-repo", "", false) - - // Restore stderr and capture output - w.Close() - os.Stderr = oldStderr - var buf bytes.Buffer - buf.ReadFrom(r) - output := buf.String() - - if err != nil { - t.Errorf("displayAvailableWorkflows() unexpected error: %v", err) - } - - // Check that the output contains the expected workflow names - expectedWorkflows := []string{"ci-doctor", "daily-plan", "weekly-summary"} - for _, wf := range expectedWorkflows { - if !strings.Contains(output, wf) { - t.Errorf("displayAvailableWorkflows() output should contain workflow '%s', got:\n%s", wf, output) - } - } - - // Check that the output contains helpful information - if !strings.Contains(output, "Available workflows") { - t.Errorf("displayAvailableWorkflows() output should contain 'Available workflows', got:\n%s", output) - } - - if !strings.Contains(output, "Example:") { - t.Errorf("displayAvailableWorkflows() output should contain 'Example:', got:\n%s", output) - } -} - -// TestDisplayAvailableWorkflowsWithVersion tests displayAvailableWorkflows with a version -func TestDisplayAvailableWorkflowsWithVersion(t *testing.T) { - // Create a temporary packages directory structure - tempDir := testutil.TempDir(t, "test-*") - - // Override packages directory for testing - t.Setenv("HOME", tempDir) - - // Create a mock package structure - packagePath := filepath.Join(tempDir, ".aw", "packages", "test-owner", "test-repo") - workflowsDir := filepath.Join(packagePath, "workflows") - if err := os.MkdirAll(workflowsDir, 0755); err != nil { - t.Fatalf("Failed to create test directories: %v", err) - } - - // Create a mock workflow file - validWorkflowContent := `--- -on: push ---- - -# Test Workflow -` - wfPath := filepath.Join(workflowsDir, "test-workflow.md") - if err := os.WriteFile(wfPath, []byte(validWorkflowContent), 0644); err != nil { - t.Fatalf("Failed to create workflow file: %v", err) - } - - // Capture stderr output - oldStderr := os.Stderr - r, w, _ := os.Pipe() - os.Stderr = w - - // Call displayAvailableWorkflows with a version - err := displayAvailableWorkflows("test-owner/test-repo", "v1.0.0", false) - - // Restore stderr and capture output - w.Close() - os.Stderr = oldStderr - var buf bytes.Buffer - buf.ReadFrom(r) - output := buf.String() - - if err != nil { - t.Errorf("displayAvailableWorkflows() unexpected error: %v", err) - } - - // Check that the version is included in the example command - if !strings.Contains(output, "@v1.0.0") { - t.Errorf("displayAvailableWorkflows() output should include version '@v1.0.0', got:\n%s", output) - } -} - -// TestDisplayAvailableWorkflowsNoWorkflows tests when no workflows are found -func TestDisplayAvailableWorkflowsNoWorkflows(t *testing.T) { - // Create a temporary packages directory structure - tempDir := testutil.TempDir(t, "test-*") - - // Override packages directory for testing - t.Setenv("HOME", tempDir) - - // Create an empty package structure - packagePath := filepath.Join(tempDir, ".aw", "packages", "test-owner", "test-repo") - if err := os.MkdirAll(packagePath, 0755); err != nil { - t.Fatalf("Failed to create test directories: %v", err) - } - - // Capture stderr output - oldStderr := os.Stderr - r, w, _ := os.Pipe() - os.Stderr = w - - // Call displayAvailableWorkflows - err := displayAvailableWorkflows("test-owner/test-repo", "", false) - - // Restore stderr and capture output - w.Close() - os.Stderr = oldStderr - var buf bytes.Buffer - buf.ReadFrom(r) - output := buf.String() - - if err != nil { - t.Errorf("displayAvailableWorkflows() unexpected error: %v", err) - } - - // Check that the output contains a warning about no workflows - if !strings.Contains(output, "No workflows found") { - t.Errorf("displayAvailableWorkflows() output should contain 'No workflows found', got:\n%s", output) - } -} - -// TestDisplayAvailableWorkflowsPackageNotFound tests when package is not found -func TestDisplayAvailableWorkflowsPackageNotFound(t *testing.T) { - // Create a temporary packages directory - tempDir := testutil.TempDir(t, "test-*") - - // Override packages directory for testing - t.Setenv("HOME", tempDir) - - // Create packages directory but don't create the specific package - packagesDir := filepath.Join(tempDir, ".aw", "packages") - if err := os.MkdirAll(packagesDir, 0755); err != nil { - t.Fatalf("Failed to create packages directory: %v", err) - } - - // Call displayAvailableWorkflows with non-existent package - err := displayAvailableWorkflows("nonexistent/repo", "", false) - - if err == nil { - t.Error("displayAvailableWorkflows() expected error for non-existent package, got nil") - } - - if !strings.Contains(err.Error(), "package not found") { - t.Errorf("displayAvailableWorkflows() error should contain 'package not found', got: %v", err) - } -} diff --git a/pkg/cli/add_workflow_pr.go b/pkg/cli/add_workflow_pr.go index 81a45d07b9..7b37414a51 100644 --- a/pkg/cli/add_workflow_pr.go +++ b/pkg/cli/add_workflow_pr.go @@ -4,6 +4,7 @@ import ( "fmt" "math/rand" "os" + "regexp" "strings" "github.com/github/gh-aw/pkg/console" @@ -12,9 +13,39 @@ import ( var addWorkflowPRLog = logger.New("cli:add_workflow_pr") -// addWorkflowsWithPR handles workflow addition with PR creation and returns the PR number and URL. -func addWorkflowsWithPR(workflows []*WorkflowSpec, opts AddOptions) (int, string, error) { - addWorkflowPRLog.Printf("Adding %d workflow(s) with PR creation", len(workflows)) +// sanitizeBranchName sanitizes a string for use in a git branch name. +// Git branch names cannot contain: +// - spaces, ~, ^, :, \, ?, *, [, @{ +// - consecutive dots (..) +// - leading/trailing dots or slashes +// - control characters +func sanitizeBranchName(name string) string { + // Use base name only (no directory path) + name = normalizeWorkflowID(name) + + // Replace problematic characters with hyphens + // This regex matches any character that's not alphanumeric, hyphen, or underscore + invalidChars := regexp.MustCompile(`[^a-zA-Z0-9_-]+`) + name = invalidChars.ReplaceAllString(name, "-") + + // Remove consecutive hyphens + consecutiveHyphens := regexp.MustCompile(`-{2,}`) + name = consecutiveHyphens.ReplaceAllString(name, "-") + + // Trim leading/trailing hyphens + name = strings.Trim(name, "-") + + // Ensure non-empty (fallback to "workflow") + if name == "" { + name = "workflow" + } + + return name +} + +// addWorkflowsWithPR handles workflow addition with PR creation using pre-resolved workflows. +func addWorkflowsWithPR(workflows []*ResolvedWorkflow, opts AddOptions) (int, string, error) { + addWorkflowPRLog.Printf("Adding %d workflow(s) with PR creation (resolved)", len(workflows)) // Get current branch for restoration later currentBranch, err := getCurrentBranch() @@ -26,8 +57,10 @@ func addWorkflowsWithPR(workflows []*WorkflowSpec, opts AddOptions) (int, string addWorkflowPRLog.Printf("Current branch: %s", currentBranch) // Create temporary branch with random 4-digit number + // Use sanitized workflow name to avoid invalid git ref characters randomNum := rand.Intn(9000) + 1000 // Generate number between 1000-9999 - branchName := fmt.Sprintf("add-workflow-%s-%04d", strings.ReplaceAll(workflows[0].WorkflowPath, "/", "-"), randomNum) + sanitizedName := sanitizeBranchName(workflows[0].Spec.WorkflowPath) + branchName := fmt.Sprintf("add-workflow-%s-%04d", sanitizedName, randomNum) addWorkflowPRLog.Printf("Creating temporary branch: %s", branchName) @@ -48,12 +81,11 @@ func addWorkflowsWithPR(workflows []*WorkflowSpec, opts AddOptions) (int, string } }() - // Add workflows using the normal function logic + // Add workflows using the resolved workflow path addWorkflowPRLog.Print("Adding workflows to repository") - // Disable security scanner for PR creation to use workflow settings prOpts := opts prOpts.DisableSecurityScanner = false - if err := addWorkflowsNormal(workflows, prOpts); err != nil { + if err := addWorkflowsWithTracking(workflows, tracker, prOpts); err != nil { addWorkflowPRLog.Printf("Failed to add workflows: %v", err) // Rollback on error if rollbackErr := tracker.RollbackAllFiles(opts.Verbose); rollbackErr != nil && opts.Verbose { @@ -79,15 +111,14 @@ func addWorkflowsWithPR(workflows []*WorkflowSpec, opts AddOptions) (int, string // Commit changes var commitMessage, prTitle, prBody, joinedNames string if len(workflows) == 1 { - joinedNames = workflows[0].WorkflowName + joinedNames = workflows[0].Spec.WorkflowName commitMessage = fmt.Sprintf("Add agentic workflow %s", joinedNames) prTitle = fmt.Sprintf("Add agentic workflow %s", joinedNames) prBody = fmt.Sprintf("Add agentic workflow %s", joinedNames) } else { - // Get workflow.Workflo workflowNames := make([]string, len(workflows)) for i, wf := range workflows { - workflowNames[i] = wf.WorkflowName + workflowNames[i] = wf.Spec.WorkflowName } joinedNames = strings.Join(workflowNames, ", ") commitMessage = fmt.Sprintf("Add agentic workflows: %s", joinedNames) @@ -125,8 +156,6 @@ func addWorkflowsWithPR(workflows []*WorkflowSpec, opts AddOptions) (int, string addWorkflowPRLog.Printf("Successfully created PR #%d: %s", prNumber, prURL) - // Success - no rollback needed - // Switch back to original branch if err := switchBranch(currentBranch, opts.Verbose); err != nil { return prNumber, prURL, fmt.Errorf("failed to switch back to branch %s: %w", currentBranch, err) diff --git a/pkg/cli/add_workflow_pr_test.go b/pkg/cli/add_workflow_pr_test.go new file mode 100644 index 0000000000..991c8ffe17 --- /dev/null +++ b/pkg/cli/add_workflow_pr_test.go @@ -0,0 +1,200 @@ +//go:build !integration + +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSanitizeBranchName(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "simple workflow name", + input: "my-workflow", + expected: "my-workflow", + }, + { + name: "workflow with .md extension", + input: "my-workflow.md", + expected: "my-workflow", + }, + { + name: "full path", + input: ".github/workflows/my-workflow.md", + expected: "my-workflow", + }, + { + name: "path with spaces", + input: "my workflow.md", + expected: "my-workflow", + }, + { + name: "path with special chars", + input: "my:workflow?.md", + expected: "my-workflow", + }, + { + name: "path with dots", + input: "my..workflow.md", + expected: "my-workflow", + }, + { + name: "path with backslashes", + input: "path\\to\\workflow.md", + expected: "path-to-workflow", // On Linux, backslashes are not path separators + }, + { + name: "path with tilde", + input: "~my~workflow.md", + expected: "my-workflow", + }, + { + name: "path with caret", + input: "my^workflow.md", + expected: "my-workflow", + }, + { + name: "path with asterisk", + input: "my*workflow.md", + expected: "my-workflow", + }, + { + name: "path with brackets", + input: "my[workflow].md", + expected: "my-workflow", + }, + { + name: "path with at-brace", + input: "my@{workflow}.md", + expected: "my-workflow", + }, + { + name: "consecutive special chars", + input: "my---workflow.md", + expected: "my-workflow", + }, + { + name: "leading special chars", + input: "---my-workflow.md", + expected: "my-workflow", + }, + { + name: "trailing special chars", + input: "my-workflow---.md", + expected: "my-workflow", + }, + { + name: "empty after sanitization", + input: "....md", + expected: "workflow", + }, + { + name: "underscores preserved", + input: "my_workflow.md", + expected: "my_workflow", + }, + { + name: "numbers preserved", + input: "workflow123.md", + expected: "workflow123", + }, + { + name: "mixed case preserved", + input: "MyWorkflow.md", + expected: "MyWorkflow", + }, + { + name: "unicode characters replaced", + input: "workflow-日本語.md", + expected: "workflow", + }, + { + name: "emoji replaced", + input: "workflow-🚀-test.md", + expected: "workflow-test", + }, + { + name: "only special characters", + input: "!@#$%^&*()+=", + expected: "workflow", + }, + { + name: "only dots", + input: "...", + expected: "workflow", + }, + { + name: "only hyphens", + input: "---", + expected: "workflow", + }, + { + name: "very long string truncation behavior", + input: "this-is-a-very-long-workflow-name-that-exceeds-typical-branch-name-lengths.md", + expected: "this-is-a-very-long-workflow-name-that-exceeds-typical-branch-name-lengths", + }, + { + name: "spaces only", + input: " ", + expected: "workflow", + }, + { + name: "control characters", + input: "work\tflow\nname", + expected: "work-flow-name", + }, + { + name: "null bytes", + input: "work\x00flow", + expected: "work-flow", + }, + { + name: "mixed unicode and ascii", + input: "test-αβγ-workflow.md", + expected: "test-workflow", + }, + { + name: "accented characters", + input: "café-workflow.md", + expected: "caf-workflow", + }, + { + name: "cyrillic characters", + input: "workflow-работа.md", + expected: "workflow", + }, + { + name: "chinese characters only", + input: "工作流程.md", + expected: "workflow", + }, + { + name: "path separators extracts basename", + input: "a/b\\c/d.md", + expected: "d", // normalizeWorkflowID extracts base name + }, + { + name: "question mark and asterisk", + input: "test?file*.md", + expected: "test-file", + }, + { + name: "colon for windows paths", + input: "C:\\Users\\test.md", + expected: "C-Users-test", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := sanitizeBranchName(tt.input) + assert.Equal(t, tt.expected, result, "sanitizeBranchName(%q) should return %q", tt.input, tt.expected) + }) + } +} diff --git a/pkg/cli/add_workflow_repository.go b/pkg/cli/add_workflow_repository.go deleted file mode 100644 index 96eae8b58c..0000000000 --- a/pkg/cli/add_workflow_repository.go +++ /dev/null @@ -1,160 +0,0 @@ -package cli - -import ( - "fmt" - "os" - - "github.com/github/gh-aw/pkg/console" - "github.com/github/gh-aw/pkg/constants" - "github.com/github/gh-aw/pkg/logger" -) - -var repositoryLog = logger.New("cli:add_workflow_repository") - -// handleRepoOnlySpec handles the case when user provides only owner/repo without workflow name. -// It installs the package and lists available workflows with interactive selection. -func handleRepoOnlySpec(repoSpec string, verbose bool) error { - repositoryLog.Printf("Handling repo-only specification: %s", repoSpec) - - // Parse the repository specification to extract repo slug and version - spec, err := parseRepoSpec(repoSpec) - if err != nil { - return fmt.Errorf("invalid repository specification '%s': %w", repoSpec, err) - } - - // Install the repository - repoWithVersion := spec.RepoSlug - if spec.Version != "" { - repoWithVersion = fmt.Sprintf("%s@%s", spec.RepoSlug, spec.Version) - } - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Installing repository %s...", repoWithVersion))) - } - - if err := InstallPackage(repoWithVersion, verbose); err != nil { - return fmt.Errorf("failed to install repository %s: %w", repoWithVersion, err) - } - - // List workflows in the installed package with metadata - workflows, err := listWorkflowsWithMetadata(spec.RepoSlug, verbose) - if err != nil { - return fmt.Errorf("failed to list workflows in %s: %w", spec.RepoSlug, err) - } - - // Display the list of available workflows - if len(workflows) == 0 { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("No workflows found in repository %s", spec.RepoSlug))) - return nil - } - - // Try interactive selection first - selected, err := showInteractiveWorkflowSelection(spec.RepoSlug, workflows, spec.Version, verbose) - if err == nil && selected != "" { - // User selected a workflow, proceed to add it - repositoryLog.Printf("User selected workflow: %s", selected) - return nil // Successfully displayed and allowed selection - } - - // If interactive selection failed or was cancelled, fall back to table display - repositoryLog.Printf("Interactive selection failed or cancelled, showing table: %v", err) - - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Available workflows in %s:", spec.RepoSlug))) - fmt.Fprintln(os.Stderr, "") - - // Render workflows as a table using console helpers - fmt.Fprint(os.Stderr, console.RenderStruct(workflows)) - - fmt.Fprintln(os.Stderr, "Example:") - fmt.Fprintln(os.Stderr, "") - - // Show example with first workflow - exampleSpec := fmt.Sprintf("%s/%s", spec.RepoSlug, workflows[0].ID) - if spec.Version != "" { - exampleSpec += "@" + spec.Version - } - - fmt.Fprintf(os.Stderr, " %s add %s\n", string(constants.CLIExtensionPrefix), exampleSpec) - fmt.Fprintln(os.Stderr, "") - - return nil -} - -// showInteractiveWorkflowSelection displays an interactive list of workflows -// and allows the user to select one. -func showInteractiveWorkflowSelection(repoSlug string, workflows []WorkflowInfo, version string, verbose bool) (string, error) { - repositoryLog.Printf("Showing interactive workflow selection: repo=%s, workflows=%d", repoSlug, len(workflows)) - - // Convert WorkflowInfo to ListItems - items := make([]console.ListItem, len(workflows)) - for i, wf := range workflows { - items[i] = console.NewListItem(wf.Name, wf.Description, wf.ID) - } - - // Show interactive list - title := fmt.Sprintf("Select a workflow from %s:", repoSlug) - selectedID, err := console.ShowInteractiveList(title, items) - if err != nil { - return "", err - } - - // Build the workflow spec - workflowSpec := buildWorkflowSpecRef(repoSlug, selectedID, "", version) - - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("To add this workflow, run:")) - fmt.Fprintf(os.Stderr, " %s add %s\n", string(constants.CLIExtensionPrefix), workflowSpec) - fmt.Fprintln(os.Stderr, "") - - return selectedID, nil -} - -// displayAvailableWorkflows lists available workflows from an installed package -// with interactive selection when in TTY mode. -func displayAvailableWorkflows(repoSlug, version string, verbose bool) error { - repositoryLog.Printf("Displaying available workflows for repository: %s", repoSlug) - - // List workflows in the installed package with metadata - workflows, err := listWorkflowsWithMetadata(repoSlug, verbose) - if err != nil { - return fmt.Errorf("failed to list workflows in %s: %w", repoSlug, err) - } - - // Display the list of available workflows - if len(workflows) == 0 { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("No workflows found in repository %s", repoSlug))) - return nil - } - - // Try interactive selection first - _, err = showInteractiveWorkflowSelection(repoSlug, workflows, version, verbose) - if err == nil { - // Successfully displayed and allowed selection - return nil - } - - // If interactive selection failed or was cancelled, fall back to table display - repositoryLog.Printf("Interactive selection failed or cancelled, showing table: %v", err) - - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Available workflows in %s:", repoSlug))) - fmt.Fprintln(os.Stderr, "") - - // Render workflows as a table using console helpers - fmt.Fprint(os.Stderr, console.RenderStruct(workflows)) - - fmt.Fprintln(os.Stderr, "Example:") - fmt.Fprintln(os.Stderr, "") - - // Show example with first workflow - exampleSpec := fmt.Sprintf("%s/%s", repoSlug, workflows[0].ID) - if version != "" { - exampleSpec += "@" + version - } - - fmt.Fprintf(os.Stderr, " %s add %s\n", string(constants.CLIExtensionPrefix), exampleSpec) - fmt.Fprintln(os.Stderr, "") - - return nil -} diff --git a/pkg/cli/add_workflow_resolution.go b/pkg/cli/add_workflow_resolution.go index b428f4ae38..9c03ca9fda 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -3,6 +3,7 @@ package cli import ( "fmt" "os" + "path/filepath" "strings" "github.com/github/gh-aw/pkg/console" @@ -16,10 +17,10 @@ var resolutionLog = logger.New("cli:add_workflow_resolution") type ResolvedWorkflow struct { // Spec is the parsed workflow specification Spec *WorkflowSpec - // Content is the raw workflow content + // Content is the raw workflow content (convenience accessor, same as SourceInfo.Content) Content []byte - // SourceInfo contains source metadata (package path, commit SHA) - SourceInfo *WorkflowSourceInfo + // SourceInfo contains fetched workflow data including content, commit SHA, and source path + SourceInfo *FetchedWorkflow // Description is the workflow description extracted from frontmatter Description string // Engine is the preferred engine extracted from frontmatter (empty if not specified) @@ -32,15 +33,15 @@ type ResolvedWorkflow struct { type ResolvedWorkflows struct { // Workflows is the list of resolved workflows Workflows []*ResolvedWorkflow - // HasWildcard indicates if any of the original specs contained wildcards + // HasWildcard indicates if any of the original specs contained wildcards (local only) HasWildcard bool // HasWorkflowDispatch is true if any of the workflows has a workflow_dispatch trigger HasWorkflowDispatch bool } -// ResolveWorkflows resolves workflow specifications by parsing specs, installing repositories, -// expanding wildcards, and fetching workflow content (including descriptions). -// This is useful for showing workflow information before actually adding them. +// ResolveWorkflows resolves workflow specifications by parsing specs and fetching workflow content. +// For remote workflows, content is fetched directly from GitHub without cloning. +// Wildcards are only supported for local workflows (not remote repositories). func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, error) { resolutionLog.Printf("Resolving workflows: count=%d", len(workflows)) @@ -54,9 +55,8 @@ func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, err } } - // Parse workflow specifications and group by repository - repoVersions := make(map[string]string) // repo -> version - parsedSpecs := []*WorkflowSpec{} // List of parsed workflow specs + // Parse workflow specifications + parsedSpecs := []*WorkflowSpec{} for _, workflow := range workflows { spec, err := parseWorkflowSpec(workflow) @@ -64,13 +64,11 @@ func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, err return nil, fmt.Errorf("invalid workflow specification '%s': %w", workflow, err) } - // Handle repository installation and workflow name extraction - if existing, exists := repoVersions[spec.RepoSlug]; exists && existing != spec.Version { - return nil, fmt.Errorf("conflicting versions for repository %s: %s vs %s", spec.RepoSlug, existing, spec.Version) + // 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) } - repoVersions[spec.RepoSlug] = spec.Version - // Create qualified name for processing parsedSpecs = append(parsedSpecs, spec) } @@ -80,8 +78,8 @@ func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, err if repoErr == nil { // We successfully determined the current repository, check all workflow specs for _, spec := range parsedSpecs { - // Skip local workflow specs (starting with "./") - if strings.HasPrefix(spec.WorkflowPath, "./") { + // Skip local workflow specs + if isLocalWorkflowPath(spec.WorkflowPath) { continue } @@ -92,27 +90,7 @@ func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, err } // If we can't determine the current repository, proceed without the check - // Install required repositories - for repo, version := range repoVersions { - repoWithVersion := repo - if version != "" { - repoWithVersion = fmt.Sprintf("%s@%s", repo, version) - } - - resolutionLog.Printf("Installing repository: %s", repoWithVersion) - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Installing repository %s before adding workflows...", repoWithVersion))) - } - - // Install as global package (not local) to match the behavior expected - if err := InstallPackage(repoWithVersion, verbose); err != nil { - resolutionLog.Printf("Failed to install repository %s: %v", repoWithVersion, err) - return nil, fmt.Errorf("failed to install repository %s: %w", repoWithVersion, err) - } - } - - // Check if any workflow specs contain wildcards before expansion + // Check if any workflow specs contain wildcards (local only) hasWildcard := false for _, spec := range parsedSpecs { if spec.IsWildcard { @@ -121,11 +99,13 @@ func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, err } } - // Expand wildcards after installation - var err error - parsedSpecs, err = expandWildcardWorkflows(parsedSpecs, verbose) - if err != nil { - return nil, err + // Expand wildcards for local workflows only + if hasWildcard { + var err error + parsedSpecs, err = expandLocalWildcardWorkflows(parsedSpecs, verbose) + if err != nil { + return nil, err + } } // Fetch workflow content and metadata for each workflow @@ -133,28 +113,28 @@ func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, err hasWorkflowDispatch := false for _, spec := range parsedSpecs { - // Fetch workflow content - content, sourceInfo, err := findWorkflowInPackageForRepo(spec, verbose) + // Fetch workflow content - FetchWorkflowFromSource handles both local and remote + fetched, err := FetchWorkflowFromSource(spec, verbose) if err != nil { - return nil, fmt.Errorf("workflow '%s' not found: %w", spec.WorkflowPath, err) + return nil, fmt.Errorf("workflow '%s' not found: %w", spec.String(), err) } // Extract description from content - description := ExtractWorkflowDescription(string(content)) + description := ExtractWorkflowDescription(string(fetched.Content)) // Extract engine from content (if specified in frontmatter) - engine := ExtractWorkflowEngine(string(content)) + engine := ExtractWorkflowEngine(string(fetched.Content)) - // Check for workflow_dispatch trigger - workflowHasDispatch := checkWorkflowHasDispatch(spec, verbose) + // Check for workflow_dispatch trigger in content + workflowHasDispatch := checkWorkflowHasDispatchFromContent(string(fetched.Content)) if workflowHasDispatch { hasWorkflowDispatch = true } resolvedWorkflows = append(resolvedWorkflows, &ResolvedWorkflow{ Spec: spec, - Content: content, - SourceInfo: sourceInfo, + Content: fetched.Content, + SourceInfo: fetched, Description: description, Engine: engine, HasWorkflowDispatch: workflowHasDispatch, @@ -168,29 +148,28 @@ func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, err }, nil } -// expandWildcardWorkflows expands wildcard workflow specifications into individual workflow specs. -// For each wildcard spec, it discovers all workflows in the installed package and replaces -// the wildcard with the discovered workflows. Non-wildcard specs are passed through unchanged. -func expandWildcardWorkflows(specs []*WorkflowSpec, verbose bool) ([]*WorkflowSpec, error) { +// expandLocalWildcardWorkflows expands wildcard workflow specifications for local workflows only. +func expandLocalWildcardWorkflows(specs []*WorkflowSpec, verbose bool) ([]*WorkflowSpec, error) { expandedWorkflows := []*WorkflowSpec{} for _, spec := range specs { - if spec.IsWildcard { - resolutionLog.Printf("Expanding wildcard for repository: %s", spec.RepoSlug) + if spec.IsWildcard && isLocalWorkflowPath(spec.WorkflowPath) { + resolutionLog.Printf("Expanding local wildcard: %s", spec.WorkflowPath) if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Discovering workflows in %s...", spec.RepoSlug))) + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Discovering local workflows matching %s...", spec.WorkflowPath))) } - discovered, err := discoverWorkflowsInPackage(spec.RepoSlug, spec.Version, verbose) + // Expand local wildcard (e.g., ./*.md or ./workflows/*.md) + discovered, err := expandLocalWildcard(spec) if err != nil { - return nil, fmt.Errorf("failed to discover workflows in %s: %w", spec.RepoSlug, err) + return nil, fmt.Errorf("failed to expand wildcard %s: %w", spec.WorkflowPath, err) } if len(discovered) == 0 { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("No workflows found in %s", spec.RepoSlug))) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("No workflows found matching %s", spec.WorkflowPath))) } else { if verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Found %d workflow(s) in %s", len(discovered), spec.RepoSlug))) + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Found %d workflow(s)", len(discovered)))) } expandedWorkflows = append(expandedWorkflows, discovered...) } @@ -206,51 +185,69 @@ func expandWildcardWorkflows(specs []*WorkflowSpec, verbose bool) ([]*WorkflowSp return expandedWorkflows, nil } -// checkWorkflowHasDispatch checks if a single workflow has a workflow_dispatch trigger -func checkWorkflowHasDispatch(spec *WorkflowSpec, verbose bool) bool { - resolutionLog.Printf("Checking if workflow %s has workflow_dispatch trigger", spec.WorkflowName) - - // Find and read the workflow content - sourceContent, _, err := findWorkflowInPackageForRepo(spec, verbose) +// checkWorkflowHasDispatchFromContent checks if workflow content has a workflow_dispatch trigger +func checkWorkflowHasDispatchFromContent(content string) bool { + result, err := parser.ExtractFrontmatterFromContent(content) if err != nil { - resolutionLog.Printf("Could not fetch workflow content: %v", err) return false } - // Parse frontmatter to check on: triggers - result, err := parser.ExtractFrontmatterFromContent(string(sourceContent)) - if err != nil { - resolutionLog.Printf("Could not parse workflow frontmatter: %v", err) - return false - } - - // Check if 'on' section exists and contains workflow_dispatch onSection, exists := result.Frontmatter["on"] if !exists { - resolutionLog.Print("No 'on' section found in workflow") return false } - // Handle different on: formats switch on := onSection.(type) { case map[string]any: _, hasDispatch := on["workflow_dispatch"] - resolutionLog.Printf("workflow_dispatch in on map: %v", hasDispatch) return hasDispatch case string: - hasDispatch := strings.Contains(strings.ToLower(on), "workflow_dispatch") - resolutionLog.Printf("workflow_dispatch in on string: %v", hasDispatch) - return hasDispatch + return strings.Contains(strings.ToLower(on), "workflow_dispatch") case []any: for _, item := range on { if str, ok := item.(string); ok && strings.ToLower(str) == "workflow_dispatch" { - resolutionLog.Print("workflow_dispatch found in on array") return true } } return false default: - resolutionLog.Printf("Unknown on: section type: %T", onSection) return false } } + +// expandLocalWildcard expands a local wildcard path (e.g., ./*.md) into individual workflow specs +func expandLocalWildcard(spec *WorkflowSpec) ([]*WorkflowSpec, error) { + pattern := spec.WorkflowPath + + // Use filepath.Glob to expand the pattern + matches, err := filepath.Glob(pattern) + if err != nil { + return nil, fmt.Errorf("invalid wildcard pattern %s: %w", pattern, err) + } + + if len(matches) == 0 { + return nil, nil + } + + var result []*WorkflowSpec + for _, match := range matches { + // Only include .md files + if !strings.HasSuffix(match, ".md") { + continue + } + + // Create a new spec for each matched file + workflowName := normalizeWorkflowID(match) + result = append(result, &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: spec.RepoSlug, + Version: spec.Version, + }, + WorkflowPath: match, + WorkflowName: workflowName, + IsWildcard: false, + }) + } + + return result, nil +} diff --git a/pkg/cli/commands_test.go b/pkg/cli/commands_test.go index c28d3c0590..ab749429ea 100644 --- a/pkg/cli/commands_test.go +++ b/pkg/cli/commands_test.go @@ -499,76 +499,6 @@ Test workflow for command existence.` } } -// TestInstallPackage tests the InstallPackage function -func TestInstallPackage(t *testing.T) { - - // Create a temporary directory for testing - tempDir, err := os.MkdirTemp("", "gh-aw-test-*") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) - - // Mock the getPackagesDir function by temporarily changing directory - originalDir, _ := os.Getwd() - defer os.Chdir(originalDir) - - tests := []struct { - name string - repoSpec string - verbose bool - expectError bool - errorMsg string - }{ - { - name: "invalid repo spec", - repoSpec: "invalid", - verbose: false, - expectError: true, - errorMsg: "invalid repository specification", - }, - { - name: "empty repo spec", - repoSpec: "", - verbose: false, - expectError: true, - errorMsg: "invalid repository specification", - }, - { - name: "valid repo spec but download will fail", - repoSpec: "nonexistent/repo", - verbose: true, - expectError: true, - errorMsg: "failed to download workflows", - }, - { - name: "valid repo spec with version but download will fail", - repoSpec: "nonexistent/repo@v1.0.0", - verbose: false, - expectError: true, - errorMsg: "failed to download workflows", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := InstallPackage(tt.repoSpec, tt.verbose) - - if tt.expectError && err == nil { - t.Errorf("Expected error for test '%s', got nil", tt.name) - } else if !tt.expectError && err != nil { - t.Errorf("Unexpected error for test '%s': %v", tt.name, err) - } - - if tt.expectError && err != nil { - if !strings.Contains(err.Error(), tt.errorMsg) { - t.Errorf("Expected error containing '%s', got: %v", tt.errorMsg, err) - } - } - }) - } -} - func TestNewWorkflow(t *testing.T) { // Create a temporary directory for testing tempDir, err := os.MkdirTemp("", "test-new-workflow-*") diff --git a/pkg/cli/commands_utils_test.go b/pkg/cli/commands_utils_test.go index 1babcd29ec..4f13ac2869 100644 --- a/pkg/cli/commands_utils_test.go +++ b/pkg/cli/commands_utils_test.go @@ -8,7 +8,6 @@ import ( "strings" "testing" - "github.com/github/gh-aw/pkg/sliceutil" "github.com/github/gh-aw/pkg/testutil" ) @@ -114,87 +113,6 @@ func TestExtractWorkflowNameFromFile_NonExistentFile(t *testing.T) { } } -func TestUpdateWorkflowTitle(t *testing.T) { - tests := []struct { - name string - content string - number int - expected string - }{ - { - name: "content with H1 header", - content: `--- -title: Test ---- - -# Daily Test Coverage - -This is a workflow.`, - number: 2, - expected: `--- -title: Test ---- - -# Daily Test Coverage 2 - -This is a workflow.`, - }, - { - name: "content with H1 header with extra spaces", - content: ` # Weekly Research - -Content here.`, - number: 3, - expected: `# # Weekly Research 3 - -Content here.`, - }, - { - name: "content without H1 header", - content: `## H2 Header - -Content without H1.`, - number: 1, - expected: `## H2 Header - -Content without H1.`, - }, - { - name: "empty content", - content: "", - number: 1, - expected: "", - }, - { - name: "multiple H1 headers - only first is modified", - content: `# First Header - -Some content. - -# Second Header - -More content.`, - number: 5, - expected: `# First Header 5 - -Some content. - -# Second Header - -More content.`, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := updateWorkflowTitle(tt.content, tt.number) - if result != tt.expected { - t.Errorf("Expected:\n%s\n\nGot:\n%s", tt.expected, result) - } - }) - } -} - func TestIsGitRepo(t *testing.T) { // Test in current directory (should be a git repo based on project setup) result := isGitRepo() @@ -351,21 +269,6 @@ This is a test workflow with some content.` } } -func BenchmarkUpdateWorkflowTitle(b *testing.B) { - content := `--- -title: Test ---- - -# Daily Test Coverage - -This is a workflow with some content that needs title updating.` - - b.ResetTimer() - for i := 0; i < b.N; i++ { - _ = updateWorkflowTitle(content, i+1) - } -} - func BenchmarkFindIncludesInContent(b *testing.B) { content := `This is content with includes: @include shared/tools.md @@ -381,259 +284,6 @@ Final content.` } } -func TestCopyMarkdownFiles(t *testing.T) { - tests := []struct { - name string - sourceFiles map[string]string // path -> content - expectedTarget map[string]string // relative path -> content - verbose bool - expectError bool - }{ - { - name: "copy single markdown file", - sourceFiles: map[string]string{ - "workflow.md": `# Test Workflow -This is a test workflow.`, - }, - expectedTarget: map[string]string{ - "workflow.md": `# Test Workflow -This is a test workflow.`, - }, - verbose: false, - expectError: false, - }, - { - name: "copy multiple markdown files", - sourceFiles: map[string]string{ - "daily.md": `# Daily Workflow -Daily tasks`, - "weekly.md": `# Weekly Workflow -Weekly tasks`, - }, - expectedTarget: map[string]string{ - "daily.md": `# Daily Workflow -Daily tasks`, - "weekly.md": `# Weekly Workflow -Weekly tasks`, - }, - verbose: false, - expectError: false, - }, - { - name: "copy markdown files in subdirectories", - sourceFiles: map[string]string{ - "workflows/daily.md": `# Daily -Content`, - "workflows/weekly.md": `# Weekly -Content`, - "shared/utils.md": `# Utils -Shared content`, - }, - expectedTarget: map[string]string{ - "workflows/daily.md": `# Daily -Content`, - "workflows/weekly.md": `# Weekly -Content`, - "shared/utils.md": `# Utils -Shared content`, - }, - verbose: true, - expectError: false, - }, - { - name: "skip non-markdown files", - sourceFiles: map[string]string{ - "workflow.md": `# Test Workflow`, - "config.yaml": `name: test`, - "readme.txt": `This is a readme`, - "script.sh": `#!/bin/bash\necho "hello"`, - }, - expectedTarget: map[string]string{ - "workflow.md": `# Test Workflow`, - }, - verbose: false, - expectError: false, - }, - { - name: "handle empty source directory", - sourceFiles: map[string]string{ - "not-markdown.txt": `This won't be copied`, - }, - expectedTarget: map[string]string{}, - verbose: false, - expectError: false, - }, - { - name: "copy nested markdown files with complex structure", - sourceFiles: map[string]string{ - "level1/workflow1.md": `# Level 1 Workflow 1`, - "level1/level2/workflow2.md": `# Level 2 Workflow 2`, - "level1/level2/level3/workflow3.md": `# Level 3 Workflow 3`, - "other.txt": `Not copied`, - }, - expectedTarget: map[string]string{ - "level1/workflow1.md": `# Level 1 Workflow 1`, - "level1/level2/workflow2.md": `# Level 2 Workflow 2`, - "level1/level2/level3/workflow3.md": `# Level 3 Workflow 3`, - }, - verbose: false, - expectError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Create temporary source and target directories - sourceDir := testutil.TempDir(t, "test-*") - targetDir := testutil.TempDir(t, "test-*") - - // Create source files - for path, content := range tt.sourceFiles { - fullPath := filepath.Join(sourceDir, path) - // Create directory if needed - dir := filepath.Dir(fullPath) - if err := os.MkdirAll(dir, 0755); err != nil { - t.Fatalf("Failed to create source directory %s: %v", dir, err) - } - // Write file - if err := os.WriteFile(fullPath, []byte(content), 0644); err != nil { - t.Fatalf("Failed to create source file %s: %v", fullPath, err) - } - } - - // Test the function - err := copyMarkdownFiles(sourceDir, targetDir, tt.verbose) - - if tt.expectError { - if err == nil { - t.Errorf("Expected error but got none") - } - return - } else { - if err != nil { - t.Errorf("Unexpected error: %v", err) - return - } - } - - // Verify expected files were copied - for expectedPath, expectedContent := range tt.expectedTarget { - fullTargetPath := filepath.Join(targetDir, expectedPath) - - // Check if file exists - if _, err := os.Stat(fullTargetPath); os.IsNotExist(err) { - t.Errorf("Expected file %s was not copied", expectedPath) - continue - } - - // Check file content - content, err := os.ReadFile(fullTargetPath) - if err != nil { - t.Errorf("Failed to read copied file %s: %v", expectedPath, err) - continue - } - - if string(content) != expectedContent { - t.Errorf("File %s content mismatch:\nExpected: %q\nGot: %q", - expectedPath, expectedContent, string(content)) - } - } - - // Verify no unexpected files were copied (check that only .md files exist) - err = filepath.Walk(targetDir, func(path string, info os.FileInfo, walkErr error) error { - if walkErr != nil { - return walkErr - } - - if !info.IsDir() { - relPath, err := filepath.Rel(targetDir, path) - if err != nil { - return err - } - - // All files in target should be .md files - if !strings.HasSuffix(relPath, ".md") { - t.Errorf("Unexpected non-markdown file copied: %s", relPath) - } - } - return nil - }) - - if err != nil { - t.Errorf("Error walking target directory: %v", err) - } - }) - } -} - -func TestCopyMarkdownFiles_ErrorScenarios(t *testing.T) { - tests := []struct { - name string - setup func() (sourceDir, targetDir string, cleanup func()) - expectError bool - errorText string - }{ - { - name: "nonexistent source directory", - setup: func() (string, string, func()) { - targetDir := testutil.TempDir(t, "test-*") - return "/nonexistent/source", targetDir, func() {} - }, - expectError: true, - errorText: "no such file or directory", - }, - { - name: "permission denied on target directory", - setup: func() (string, string, func()) { - sourceDir := testutil.TempDir(t, "test-*") - targetDir := testutil.TempDir(t, "test-*") - - // Create a source file - sourceFile := filepath.Join(sourceDir, "test.md") - os.WriteFile(sourceFile, []byte("# Test"), 0644) - - // Make target directory read-only - os.Chmod(targetDir, 0444) - - cleanup := func() { - os.Chmod(targetDir, 0755) // Restore permissions for cleanup - } - - return sourceDir, targetDir, cleanup - }, - expectError: true, - errorText: "permission denied", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Skip permission tests when running as root (e.g., in Docker containers) - // Root can write to read-only directories, bypassing Unix permission checks - if tt.name == "permission denied on target directory" && os.Geteuid() == 0 { - t.Skip("Skipping permission test when running as root") - } - - sourceDir, targetDir, cleanup := tt.setup() - defer cleanup() - - err := copyMarkdownFiles(sourceDir, targetDir, false) - - if tt.expectError { - if err == nil { - t.Errorf("Expected error but got none") - } else if tt.errorText != "" && !sliceutil.ContainsIgnoreCase(err.Error(), tt.errorText) { - t.Errorf("Expected error containing %q, got: %v", tt.errorText, err) - } - } else { - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - } - }) - } -} - func TestIsRunnable(t *testing.T) { tests := []struct { name string diff --git a/pkg/cli/flags_test.go b/pkg/cli/flags_test.go index 366e4b5a76..776c770088 100644 --- a/pkg/cli/flags_test.go +++ b/pkg/cli/flags_test.go @@ -157,14 +157,6 @@ func TestShortFlagConsistency(t *testing.T) { shouldExist: true, description: "logs should have count short flag", }, - { - name: "add command does NOT have -c for --number", - shortFlag: "c", - longFlag: "number", - commandSetup: func() *cobra.Command { return NewAddCommand(validateEngineStub) }, - shouldExist: false, - description: "add should NOT have -c short flag for --number (conflict resolved)", - }, // -r flag (repo) { diff --git a/pkg/cli/local_workflow_integration_test.go b/pkg/cli/local_workflow_integration_test.go index 054a55682c..7f9a14da23 100644 --- a/pkg/cli/local_workflow_integration_test.go +++ b/pkg/cli/local_workflow_integration_test.go @@ -5,7 +5,6 @@ package cli import ( "os" "path/filepath" - "strings" "testing" "github.com/github/gh-aw/pkg/testutil" @@ -58,13 +57,9 @@ This is a test local workflow. } // Test parsing local workflow spec - // Note: This will fail if we're not in a git repository, which is expected + // Local workflows don't require a git repository or remote spec, err := parseWorkflowSpec("./workflows/test-local.md") if err != nil { - // If we're not in a git repository, skip the rest of the test - if strings.Contains(err.Error(), "failed to get current repository info") { - t.Skip("Skipping test because we're not in a git repository (this is expected behavior)") - } t.Fatalf("Failed to parse local workflow spec: %v", err) } @@ -78,6 +73,10 @@ This is a test local workflow. if spec.Version != "" { t.Errorf("Expected empty Version for local workflow, got %q", spec.Version) } + // Local workflows have no RepoSlug since they don't come from a remote source + if spec.RepoSlug != "" { + t.Errorf("Expected empty RepoSlug for local workflow, got %q", spec.RepoSlug) + } // Test String() method stringResult := spec.String() @@ -85,32 +84,9 @@ This is a test local workflow. t.Errorf("Expected String() './workflows/test-local.md', got %q", stringResult) } - // Test buildSourceString (should remove ./ prefix) + // Test buildSourceString - returns empty for local workflows (no remote source to track) sourceString := buildSourceString(spec) - expectedSourceString := spec.RepoSlug + "/workflows/test-local.md" - if sourceString != expectedSourceString { - t.Errorf("Expected buildSourceString() %q, got %q", expectedSourceString, sourceString) - } - - // Test findWorkflowInPackageForRepo - content, sourceInfo, err := findWorkflowInPackageForRepo(spec, false) - if err != nil { - t.Fatalf("Failed to find local workflow: %v", err) - } - - if string(content) != testContent { - t.Errorf("Content mismatch") - } - - if sourceInfo.PackagePath != "." { - t.Errorf("Expected PackagePath '.', got %q", sourceInfo.PackagePath) - } - - if sourceInfo.SourcePath != "./workflows/test-local.md" { - t.Errorf("Expected SourcePath './workflows/test-local.md', got %q", sourceInfo.SourcePath) - } - - if sourceInfo.CommitSHA != "" { - t.Errorf("Expected empty CommitSHA for local workflow, got %q", sourceInfo.CommitSHA) + if sourceString != "" { + t.Errorf("Expected buildSourceString() to return empty string for local workflow, got %q", sourceString) } } diff --git a/pkg/cli/local_workflow_trial_test.go b/pkg/cli/local_workflow_trial_test.go index 41bf7668fd..2785eb4063 100644 --- a/pkg/cli/local_workflow_trial_test.go +++ b/pkg/cli/local_workflow_trial_test.go @@ -5,7 +5,6 @@ package cli import ( "os" "path/filepath" - "strings" "testing" ) @@ -56,28 +55,33 @@ This is a test workflow. } // Verify the spec - if !strings.HasPrefix(spec.WorkflowPath, "./") { - t.Errorf("Expected WorkflowPath to start with './', got: %s", spec.WorkflowPath) + if !isLocalWorkflowPath(spec.WorkflowPath) { + t.Errorf("Expected WorkflowPath to be a local path, got: %s", spec.WorkflowPath) } if spec.WorkflowName != "test-workflow" { t.Errorf("Expected WorkflowName to be 'test-workflow', got: %s", spec.WorkflowName) } - // Test the local installation function - err = installLocalWorkflowInTrialMode(originalDir, tempDir, spec, &TrialOptions{DisableSecurityScanner: false}) + // Test the local workflow writing function (writeWorkflowToTrialDir) + // First read the workflow content + content, err := os.ReadFile(testFile) if err != nil { - t.Fatalf("Failed to install local workflow: %v", err) + t.Fatalf("Failed to read test workflow file: %v", err) + } + + result, err := writeWorkflowToTrialDir(tempDir, spec.WorkflowName, content, &TrialOptions{DisableSecurityScanner: true}) + if err != nil { + t.Fatalf("Failed to write local workflow to trial dir: %v", err) } // Verify the file was copied correctly - expectedDest := filepath.Join(tempDir, ".github/workflows", "test-workflow.md") - if _, err := os.Stat(expectedDest); os.IsNotExist(err) { - t.Errorf("Expected workflow file to be copied to %s, but it doesn't exist", expectedDest) + if _, err := os.Stat(result.DestPath); os.IsNotExist(err) { + t.Errorf("Expected workflow file to be written to %s, but it doesn't exist", result.DestPath) } // Verify the content matches - copiedContent, err := os.ReadFile(expectedDest) + copiedContent, err := os.ReadFile(result.DestPath) if err != nil { t.Fatalf("Failed to read copied workflow file: %v", err) } diff --git a/pkg/cli/mcp_server_add_test.go b/pkg/cli/mcp_server_add_test.go index b5c2a7f660..113cffba43 100644 --- a/pkg/cli/mcp_server_add_test.go +++ b/pkg/cli/mcp_server_add_test.go @@ -130,41 +130,25 @@ func TestMCPServer_AddToolInvocation(t *testing.T) { } defer session.Close() - // Test 1: Call with just repository (should list workflows) - t.Run("ListWorkflows", func(t *testing.T) { - callResult, err := session.CallTool(ctx, &mcp.CallToolParams{ + // Test 1: Call with just repository (should fail - repo-only specs no longer supported) + t.Run("RepoOnlySpecError", func(t *testing.T) { + _, err := session.CallTool(ctx, &mcp.CallToolParams{ Name: "add", Arguments: map[string]any{ "workflows": []any{"githubnext/agentics"}, }, }) - if err != nil { - t.Fatalf("Failed to call add tool: %v", err) - } - - // Verify we got some output - if len(callResult.Content) == 0 { - t.Fatal("add tool returned no content") - } - - // Extract text content - var outputText string - for _, content := range callResult.Content { - if textContent, ok := content.(*mcp.TextContent); ok { - outputText += textContent.Text - } - } - - if outputText == "" { - t.Fatal("add tool returned empty text content") + // Should return an error because repo-only specs are invalid + if err == nil { + t.Fatal("Expected error for repo-only spec, got success") } - t.Logf("add tool output (list workflows):\n%s", outputText) - - // Output should mention available workflows or indicate repository was processed - if !strings.Contains(outputText, "workflow") && !strings.Contains(outputText, "Workflow") { - t.Logf("Warning: Output doesn't mention 'workflow': %s", outputText) + // Error message should indicate the invalid format + errStr := err.Error() + t.Logf("add tool error (repo-only spec): %s", errStr) + if !strings.Contains(errStr, "failed to add workflows") { + t.Errorf("Expected error to mention 'failed to add workflows', got: %s", errStr) } }) diff --git a/pkg/cli/packages.go b/pkg/cli/packages.go index ef9b567951..a621732b75 100644 --- a/pkg/cli/packages.go +++ b/pkg/cli/packages.go @@ -20,257 +20,9 @@ var ( includePattern = regexp.MustCompile(`^@include(\?)?\s+(.+)$`) ) -// WorkflowInfo represents metadata about an available workflow -type WorkflowInfo struct { - ID string `console:"header:ID"` - Name string `console:"header:Name"` - Description string `console:"header:Description,omitempty"` - Path string `console:"-"` // Internal use only, not displayed -} - -// InstallPackage installs agentic workflows from a GitHub repository -func InstallPackage(repoSpec string, verbose bool) error { - packagesLog.Printf("Installing package: %s", repoSpec) - if verbose { - fmt.Fprintf(os.Stderr, "Installing package: %s\n", repoSpec) - } - - // Parse repository specification (org/repo[@version]) - spec, err := parseRepoSpec(repoSpec) - if err != nil { - packagesLog.Printf("Failed to parse repository specification: %v", err) - return fmt.Errorf("invalid repository specification: %w", err) - } - - packagesLog.Printf("Parsed repo spec: slug=%s, version=%s", spec.RepoSlug, spec.Version) - - if verbose { - fmt.Fprintf(os.Stderr, "Repository: %s\n", spec.RepoSlug) - if spec.Version != "" { - fmt.Fprintf(os.Stderr, "Version: %s\n", spec.Version) - } else { - fmt.Fprintf(os.Stderr, "Version: main (default)\n") - } - } - - // Get global packages directory - packagesDir, err := getPackagesDir() - if err != nil { - packagesLog.Printf("Failed to determine packages directory: %v", err) - return fmt.Errorf("failed to determine packages directory: %w", err) - } - - packagesLog.Printf("Using packages directory: %s", packagesDir) - if verbose { - fmt.Fprintf(os.Stderr, "Installing to global packages directory: %s\n", packagesDir) - } - - // Create packages directory - if err := os.MkdirAll(packagesDir, 0755); err != nil { - return fmt.Errorf("failed to create packages directory: %w", err) - } - - // Create target directory for this repository - targetDir := filepath.Join(packagesDir, spec.RepoSlug) - if err := os.MkdirAll(targetDir, 0755); err != nil { - return fmt.Errorf("failed to create package directory: %w", err) - } - - // Check if package already exists - if _, err := os.Stat(targetDir); err == nil { - entries, err := os.ReadDir(targetDir) - if err == nil && len(entries) > 0 { - packagesLog.Printf("Package %s already exists. Updating...\n", spec.RepoSlug) - // Remove existing content - if err := os.RemoveAll(targetDir); err != nil { - return fmt.Errorf("failed to remove existing package: %w", err) - } - if err := os.MkdirAll(targetDir, 0755); err != nil { - return fmt.Errorf("failed to recreate package directory: %w", err) - } - } - } - - // Download workflows from the repository - packagesLog.Printf("Downloading workflows to: %s", targetDir) - if err := downloadWorkflows(spec.RepoSlug, spec.Version, targetDir, verbose); err != nil { - packagesLog.Printf("Failed to download workflows: %v", err) - return fmt.Errorf("failed to download workflows: %w", err) - } - - packagesLog.Printf("Successfully installed package: %s", spec.RepoSlug) - return nil -} - -// downloadWorkflows downloads all .md files from the workflows directory of a GitHub repository -func downloadWorkflows(repo, version, targetDir string, verbose bool) error { - packagesLog.Printf("Downloading workflows from %s (version: %s) to %s", repo, version, targetDir) - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Downloading workflows from %s/workflows...", repo))) - } - - // Create a temporary directory for cloning - tempDir, err := os.MkdirTemp("", "gh-aw-clone-*") - if err != nil { - packagesLog.Printf("Failed to create temp directory: %v", err) - return fmt.Errorf("failed to create temp directory: %w", err) - } - defer os.RemoveAll(tempDir) - packagesLog.Printf("Created temporary directory: %s", tempDir) - - isSHA := IsCommitSHA(version) - - // Prepare fallback git clone arguments - // Support enterprise GitHub domains - githubHost := getGitHubHost() - - repoURL := fmt.Sprintf("%s/%s", githubHost, repo) - var gitArgs []string - if isSHA { - gitArgs = []string{"clone", repoURL, tempDir} - } else { - gitArgs = []string{"clone", "--depth", "1", repoURL, tempDir} - if version != "" && version != "main" { - gitArgs = append(gitArgs, "--branch", version) - } - } - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Cloning repository...")) - } - - // Start spinner for clone operation (only if not in verbose mode) - spinner := console.NewSpinner(fmt.Sprintf("Cloning %s...", repo)) - if !verbose { - spinner.Start() - } - - // Use helper to execute gh CLI with git fallback - _, stderr, err := ghExecOrFallback( - "git", - gitArgs, - []string{"GIT_TERMINAL_PROMPT=0"}, // Prevent credential prompts - ) - - // Stop spinner - if !verbose { - spinner.Stop() - } - - if err != nil { - return fmt.Errorf("failed to clone repository: %w (output: %s)", err, stderr) - } - - // If a specific SHA was requested, checkout that commit - if isSHA { - stdout, stderr, err := ghExecOrFallback( - "git", - []string{"-C", tempDir, "checkout", version}, - nil, - ) - if err != nil { - return fmt.Errorf("failed to checkout commit %s: %w (output: %s)", version, err, stderr+stdout) - } - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Checked out commit: %s", version))) - } - } - - // Get the current commit SHA from the cloned repository - stdout, stderr, err := ghExecOrFallback( - "git", - []string{"-C", tempDir, "rev-parse", "HEAD"}, - nil, - ) - if err != nil { - return fmt.Errorf("failed to get commit SHA: %w (output: %s)", err, stderr+stdout) - } - commitSHA := strings.TrimSpace(stdout) - - // Validate that we're at the expected commit if a specific SHA was requested - if isSHA && commitSHA != version { - return fmt.Errorf("cloned repository is at commit %s, but expected %s", commitSHA, version) - } - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Repository commit SHA: %s", commitSHA))) - } - - // Copy all .md files from temp directory to target - if err := copyMarkdownFiles(tempDir, targetDir, verbose); err != nil { - return err - } - - // Store the commit SHA in a metadata file for later retrieval - metadataPath := filepath.Join(targetDir, ".commit-sha") - if err := os.WriteFile(metadataPath, []byte(commitSHA), 0644); err != nil { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to write commit SHA metadata: %v", err))) - } - } - - return nil -} - -// copyMarkdownFiles recursively copies markdown files from source to target directory -func copyMarkdownFiles(sourceDir, targetDir string, verbose bool) error { - return filepath.Walk(sourceDir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - - // Skip if not a markdown file - if info.IsDir() || !strings.HasSuffix(info.Name(), ".md") { - return nil - } - - // Get relative path from source directory - relPath, err := filepath.Rel(sourceDir, path) - if err != nil { - return fmt.Errorf("failed to get relative path: %w", err) - } - - // Create target file path - targetFile := filepath.Join(targetDir, relPath) - - // Create target directory if needed - targetFileDir := filepath.Dir(targetFile) - if err := os.MkdirAll(targetFileDir, 0755); err != nil { - return fmt.Errorf("failed to create target directory %s: %w", targetFileDir, err) - } - - // Copy file - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Copying: %s -> %s", relPath, targetFile))) - } - - content, err := os.ReadFile(path) - if err != nil { - return fmt.Errorf("failed to read source file %s: %w", path, err) - } - - if err := os.WriteFile(targetFile, content, 0644); err != nil { - return fmt.Errorf("failed to write target file %s: %w", targetFile, err) - } - - return nil - }) -} - -// Package represents an installed package -type Package struct { - Name string - Path string - Workflows []string - CommitSHA string -} - -// WorkflowSourceInfo contains information about where a workflow was found -type WorkflowSourceInfo struct { - PackagePath string - SourcePath string - CommitSHA string // The actual commit SHA used when the package was installed -} +// WorkflowSourceInfo is an alias for FetchedWorkflow for backward compatibility. +// Deprecated: Use FetchedWorkflow directly instead. +type WorkflowSourceInfo = FetchedWorkflow // isValidWorkflowFile checks if a markdown file is a valid workflow by attempting to parse its frontmatter. // It validates that the file has proper YAML frontmatter delimited by "---" and contains the required "on" field. @@ -307,264 +59,8 @@ func isValidWorkflowFile(filePath string) bool { return true } -// listWorkflowsInPackage lists all available workflows in an installed package -func listWorkflowsInPackage(repoSlug string, verbose bool) ([]string, error) { - workflows, err := listWorkflowsWithMetadata(repoSlug, verbose) - if err != nil { - return nil, err - } - - // Convert WorkflowInfo to string paths for backwards compatibility - paths := make([]string, len(workflows)) - for i, wf := range workflows { - paths[i] = wf.Path - } - return paths, nil -} - -// listWorkflowsWithMetadata lists all available workflows in an installed package with metadata -func listWorkflowsWithMetadata(repoSlug string, verbose bool) ([]WorkflowInfo, error) { - packagesLog.Printf("Listing workflows in package: %s", repoSlug) - - packagesDir, err := getPackagesDir() - if err != nil { - return nil, fmt.Errorf("failed to get packages directory: %w", err) - } - - packagePath := filepath.Join(packagesDir, repoSlug) - - // Check if package exists - if _, err := os.Stat(packagePath); os.IsNotExist(err) { - return nil, fmt.Errorf("package not found: %s", repoSlug) - } - - var workflows []WorkflowInfo - - // Walk through the package directory to find all .md files - err = filepath.Walk(packagePath, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - - // Skip directories and non-markdown files - if info.IsDir() || !strings.HasSuffix(info.Name(), ".md") { - return nil - } - - // Skip metadata files - if info.Name() == ".commit-sha" { - return nil - } - - // Check if this is a valid workflow file - if !isValidWorkflowFile(path) { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Skipping non-workflow file: %s", path))) - } - return nil - } - - // Get relative path from package directory - relPath, err := filepath.Rel(packagePath, path) - if err != nil { - return err - } - - // Extract workflow ID (filename without extension) - workflowID := normalizeWorkflowID(path) - - // For workflows in workflows/ directory, use simplified ID - if strings.HasPrefix(relPath, "workflows/") { - workflowID = normalizeWorkflowID(strings.TrimPrefix(relPath, "workflows/")) - } - - // Extract name and description from frontmatter - name, description := extractWorkflowMetadata(path) - - // Add to list with metadata - workflows = append(workflows, WorkflowInfo{ - ID: workflowID, - Name: name, - Description: description, - Path: relPath, - }) - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Found workflow: %s (ID: %s, Name: %s)", relPath, workflowID, name))) - } - - return nil - }) - - if err != nil { - return nil, fmt.Errorf("failed to scan package directory: %w", err) - } - - packagesLog.Printf("Found %d workflows in package %s", len(workflows), repoSlug) - return workflows, nil -} - -// extractWorkflowMetadata extracts name and description from a workflow file's frontmatter -func extractWorkflowMetadata(filePath string) (name string, description string) { - content, err := os.ReadFile(filePath) - if err != nil { - return "", "" - } - - result, err := parser.ExtractFrontmatterFromContent(string(content)) - if err != nil { - return "", "" - } - - // Try to get name from frontmatter - if nameVal, ok := result.Frontmatter["name"]; ok { - if nameStr, ok := nameVal.(string); ok { - name = nameStr - } - } - - // If no name in frontmatter, try to extract from first H1 heading - if name == "" { - lines := strings.Split(result.Markdown, "\n") - for _, line := range lines { - trimmed := strings.TrimSpace(line) - if strings.HasPrefix(trimmed, "# ") { - name = strings.TrimSpace(trimmed[2:]) - break - } - } - } - - // If still no name, use filename as fallback (handled by caller) - if name == "" { - name = normalizeWorkflowID(filePath) - } - - // Try to get description from frontmatter - if descVal, ok := result.Frontmatter["description"]; ok { - if descStr, ok := descVal.(string); ok { - description = descStr - } - } - - return name, description -} - -// findWorkflowInPackageForRepo searches for a workflow in installed packages -func findWorkflowInPackageForRepo(workflow *WorkflowSpec, verbose bool) ([]byte, *WorkflowSourceInfo, error) { - - packagesDir, err := getPackagesDir() - if err != nil { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to get packages directory: %v", err))) - } - return nil, nil, fmt.Errorf("failed to get packages directory: %w", err) - } - - if _, err := os.Stat(packagesDir); os.IsNotExist(err) { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("No packages directory found at %s", packagesDir))) - } - return nil, nil, fmt.Errorf("no packages directory found") - } - - // Handle local workflows (starting with "./") - if strings.HasPrefix(workflow.WorkflowPath, "./") { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Searching local filesystem for workflow: %s", workflow.WorkflowPath))) - } - - // For local workflows, use current directory as packagePath - packagePath := "." - workflowFile := workflow.WorkflowPath - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Looking for local workflow: %s", workflowFile))) - } - - content, err := os.ReadFile(workflowFile) - if err != nil { - return nil, nil, fmt.Errorf("local workflow '%s' not found: %w", workflow.WorkflowPath, err) - } - - sourceInfo := &WorkflowSourceInfo{ - PackagePath: packagePath, - SourcePath: workflowFile, - CommitSHA: "", // Local workflows don't have commit SHA - } - - return content, sourceInfo, nil - } - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Searching packages in %s for workflow: %s", packagesDir, workflow.WorkflowPath))) - } - - // Check if workflow name contains org/repo prefix - // Fully qualified name: org/repo/workflow_name - packagePath := filepath.Join(packagesDir, workflow.RepoSlug) - workflowFile := filepath.Join(packagePath, workflow.WorkflowPath) - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Looking for qualified workflow: %s", workflowFile))) - } - - content, err := os.ReadFile(workflowFile) - if err != nil { - // If the initial path failed and it starts with "workflows/", - // try with ".github/workflows/" prefix as a fallback - if strings.HasPrefix(workflow.WorkflowPath, "workflows/") { - // Extract just the filename part after "workflows/" - filenamePart := strings.TrimPrefix(workflow.WorkflowPath, "workflows/") - fallbackPath := filepath.Join(".github", "workflows", filenamePart) - fallbackWorkflowFile := filepath.Join(packagePath, fallbackPath) - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Initial path not found, trying fallback: %s", fallbackWorkflowFile))) - } - - var fallbackErr error - content, fallbackErr = os.ReadFile(fallbackWorkflowFile) - if fallbackErr == nil { - // Success with fallback path, update workflowFile to the fallback path - workflowFile = fallbackWorkflowFile - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Found workflow at fallback path: %s", fallbackWorkflowFile))) - } - } else { - // Both attempts failed, return the original error - return nil, nil, fmt.Errorf("workflow '%s' not found in repo '%s'", workflow.WorkflowPath, workflow.RepoSlug) - } - } else { - // Not a workflows/ path, return the original error - return nil, nil, fmt.Errorf("workflow '%s' not found in repo '%s'", workflow.WorkflowPath, workflow.RepoSlug) - } - } - - // Try to read the commit SHA from metadata file - var commitSHA string - metadataPath := filepath.Join(packagePath, ".commit-sha") - if shaBytes, err := os.ReadFile(metadataPath); err == nil { - commitSHA = strings.TrimSpace(string(shaBytes)) - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Found commit SHA from metadata: %s", commitSHA))) - } - } else if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Could not read commit SHA metadata: %v", err))) - } - - sourceInfo := &WorkflowSourceInfo{ - PackagePath: packagePath, - SourcePath: workflowFile, - CommitSHA: commitSHA, - } - - return content, sourceInfo, nil - -} - -// collectPackageIncludeDependencies collects dependencies for package-based workflows -func collectPackageIncludeDependencies(content, packagePath string, verbose bool) ([]IncludeDependency, error) { +// collectLocalIncludeDependencies collects dependencies for package-based workflows +func collectLocalIncludeDependencies(content, packagePath string, verbose bool) ([]IncludeDependency, error) { var dependencies []IncludeDependency seen := make(map[string]bool) @@ -572,12 +68,12 @@ func collectPackageIncludeDependencies(content, packagePath string, verbose bool fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Collecting package dependencies from: %s", packagePath))) } - err := collectPackageIncludesRecursive(content, packagePath, &dependencies, seen, verbose) + err := collectLocalIncludeDependenciesRecursive(content, packagePath, &dependencies, seen, verbose) return dependencies, err } -// collectPackageIncludesRecursive recursively processes @include directives in package content -func collectPackageIncludesRecursive(content, baseDir string, dependencies *[]IncludeDependency, seen map[string]bool, verbose bool) error { +// collectLocalIncludeDependenciesRecursive recursively processes @include directives in package content +func collectLocalIncludeDependenciesRecursive(content, baseDir string, dependencies *[]IncludeDependency, seen map[string]bool, verbose bool) error { scanner := bufio.NewScanner(strings.NewReader(content)) for scanner.Scan() { line := scanner.Text() @@ -635,7 +131,7 @@ func collectPackageIncludesRecursive(content, baseDir string, dependencies *[]In // Recursively process includes in the included file includedDir := filepath.Dir(fullSourcePath) - if err := collectPackageIncludesRecursive(markdownContent, includedDir, dependencies, seen, verbose); err != nil { + if err := collectLocalIncludeDependenciesRecursive(markdownContent, includedDir, dependencies, seen, verbose); err != nil { if verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Error processing includes in %s: %v", fullSourcePath, err))) } @@ -724,75 +220,6 @@ type IncludeDependency struct { IsOptional bool // Whether this is an optional include (@include?) } -// discoverWorkflowsInPackage discovers all workflow files in an installed package -// Returns a list of WorkflowSpec for each discovered workflow -func discoverWorkflowsInPackage(repoSlug, version string, verbose bool) ([]*WorkflowSpec, error) { - packagesLog.Printf("Discovering workflows in package: %s (version: %s)", repoSlug, version) - - packagesDir, err := getPackagesDir() - if err != nil { - return nil, fmt.Errorf("failed to get packages directory: %w", err) - } - - packagePath := filepath.Join(packagesDir, repoSlug) - if _, err := os.Stat(packagePath); os.IsNotExist(err) { - return nil, fmt.Errorf("package not found: %s (try installing it first)", repoSlug) - } - - var workflows []*WorkflowSpec - - // Walk through the package directory and find all .md files - err = filepath.Walk(packagePath, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - - // Skip if not a markdown file - if info.IsDir() || !strings.HasSuffix(info.Name(), ".md") { - return nil - } - - // Check if this is a valid workflow file - if !isValidWorkflowFile(path) { - if verbose { - fmt.Fprintf(os.Stderr, "Skipping non-workflow file: %s\n", path) - } - return nil - } - - // Get relative path from package root - relPath, err := filepath.Rel(packagePath, path) - if err != nil { - return fmt.Errorf("failed to get relative path: %w", err) - } - - // Create workflow spec - spec := &WorkflowSpec{ - RepoSpec: RepoSpec{ - RepoSlug: repoSlug, - Version: version, - }, - WorkflowPath: relPath, - WorkflowName: normalizeWorkflowID(relPath), - } - - workflows = append(workflows, spec) - - if verbose { - fmt.Fprintf(os.Stderr, "Discovered workflow: %s\n", spec.String()) - } - - return nil - }) - - if err != nil { - return nil, fmt.Errorf("failed to walk package directory: %w", err) - } - - packagesLog.Printf("Discovered %d workflows in package %s", len(workflows), repoSlug) - return workflows, nil -} - // ExtractWorkflowDescription extracts the description field from workflow content string func ExtractWorkflowDescription(content string) string { result, err := parser.ExtractFrontmatterFromContent(content) diff --git a/pkg/cli/packages_fallback_test.go b/pkg/cli/packages_fallback_test.go deleted file mode 100644 index 72f7227028..0000000000 --- a/pkg/cli/packages_fallback_test.go +++ /dev/null @@ -1,260 +0,0 @@ -//go:build !integration - -package cli - -import ( - "os" - "path/filepath" - "strings" - "testing" -) - -// TestFindWorkflowWithGitHubWorkflowsFallback tests that the workflow resolution -// falls back to .github/workflows/ when the initial workflows/ path doesn't exist -func TestFindWorkflowWithGitHubWorkflowsFallback(t *testing.T) { - // Get the packages directory - packagesDir, err := getPackagesDir() - if err != nil { - t.Fatal(err) - } - - // Create a unique test directory to avoid conflicts - testRepoPath := filepath.Join(packagesDir, "test-fallback-org", "test-fallback-repo") - - // Clean up before and after test - defer os.RemoveAll(filepath.Join(packagesDir, "test-fallback-org")) - os.RemoveAll(filepath.Join(packagesDir, "test-fallback-org")) // Clean up any existing test data - - // Create .github/workflows/ directory structure - githubWorkflowsDir := filepath.Join(testRepoPath, ".github", "workflows") - err = os.MkdirAll(githubWorkflowsDir, 0755) - if err != nil { - t.Fatal(err) - } - - // Create a test workflow file in .github/workflows/ - testWorkflowContent := `--- -description: "Test fallback workflow" -on: - push: - branches: [main] -permissions: - contents: read -engine: claude ---- - -# Test Fallback Workflow - -This workflow tests the fallback path resolution. -` - - workflowFilePath := filepath.Join(githubWorkflowsDir, "test-fallback-workflow.md") - err = os.WriteFile(workflowFilePath, []byte(testWorkflowContent), 0644) - if err != nil { - t.Fatal(err) - } - - // Create .commit-sha metadata file - commitSHA := "abc123def456" - commitSHAPath := filepath.Join(testRepoPath, ".commit-sha") - err = os.WriteFile(commitSHAPath, []byte(commitSHA), 0644) - if err != nil { - t.Fatal(err) - } - - // Create a workflow spec that would normally look for workflows/test-fallback-workflow.md - // (which doesn't exist), but should fall back to .github/workflows/test-fallback-workflow.md - spec := &WorkflowSpec{ - RepoSpec: RepoSpec{ - RepoSlug: "test-fallback-org/test-fallback-repo", - Version: "main", - }, - WorkflowPath: "workflows/test-fallback-workflow.md", - WorkflowName: "test-fallback-workflow", - } - - // Test findWorkflowInPackageForRepo - should find the file via fallback - content, sourceInfo, err := findWorkflowInPackageForRepo(spec, false) - if err != nil { - t.Fatalf("Failed to find workflow with fallback: %v", err) - } - - // Verify content - if string(content) != testWorkflowContent { - t.Errorf("Content mismatch.\nExpected:\n%s\nGot:\n%s", testWorkflowContent, string(content)) - } - - // Verify source info - expectedSourcePath := filepath.Join(testRepoPath, ".github", "workflows", "test-fallback-workflow.md") - if sourceInfo.SourcePath != expectedSourcePath { - t.Errorf("Expected SourcePath %q, got %q", expectedSourcePath, sourceInfo.SourcePath) - } - - if sourceInfo.PackagePath != testRepoPath { - t.Errorf("Expected PackagePath %q, got %q", testRepoPath, sourceInfo.PackagePath) - } - - if sourceInfo.CommitSHA != commitSHA { - t.Errorf("Expected CommitSHA %q, got %q", commitSHA, sourceInfo.CommitSHA) - } -} - -// TestFindWorkflowWithoutFallback tests that the original path is used when it exists -func TestFindWorkflowWithoutFallback(t *testing.T) { - // Get the packages directory - packagesDir, err := getPackagesDir() - if err != nil { - t.Fatal(err) - } - - // Create a unique test directory to avoid conflicts - testRepoPath := filepath.Join(packagesDir, "test-original-org", "test-original-repo") - - // Clean up before and after test - defer os.RemoveAll(filepath.Join(packagesDir, "test-original-org")) - os.RemoveAll(filepath.Join(packagesDir, "test-original-org")) // Clean up any existing test data - - // Create workflows/ directory structure - workflowsDir := filepath.Join(testRepoPath, "workflows") - err = os.MkdirAll(workflowsDir, 0755) - if err != nil { - t.Fatal(err) - } - - // Create a test workflow file in workflows/ - testWorkflowContent := `--- -description: "Test original path workflow" -on: - push: - branches: [main] -permissions: - contents: read -engine: claude ---- - -# Test Original Path Workflow - -This workflow tests that the original path is used when it exists. -` - - workflowFilePath := filepath.Join(workflowsDir, "test-original-workflow.md") - err = os.WriteFile(workflowFilePath, []byte(testWorkflowContent), 0644) - if err != nil { - t.Fatal(err) - } - - // Create a workflow spec - spec := &WorkflowSpec{ - RepoSpec: RepoSpec{ - RepoSlug: "test-original-org/test-original-repo", - Version: "main", - }, - WorkflowPath: "workflows/test-original-workflow.md", - WorkflowName: "test-original-workflow", - } - - // Test findWorkflowInPackageForRepo - should find the file at the original path - content, sourceInfo, err := findWorkflowInPackageForRepo(spec, false) - if err != nil { - t.Fatalf("Failed to find workflow at original path: %v", err) - } - - // Verify content - if string(content) != testWorkflowContent { - t.Errorf("Content mismatch.\nExpected:\n%s\nGot:\n%s", testWorkflowContent, string(content)) - } - - // Verify source info - should be the original workflows/ path - expectedSourcePath := filepath.Join(testRepoPath, "workflows", "test-original-workflow.md") - if sourceInfo.SourcePath != expectedSourcePath { - t.Errorf("Expected SourcePath %q, got %q", expectedSourcePath, sourceInfo.SourcePath) - } -} - -// TestFindWorkflowFallbackFailure tests that an error is returned when neither path exists -func TestFindWorkflowFallbackFailure(t *testing.T) { - // Get the packages directory - packagesDir, err := getPackagesDir() - if err != nil { - t.Fatal(err) - } - - // Create a unique test directory to avoid conflicts - testRepoPath := filepath.Join(packagesDir, "test-failure-org", "test-failure-repo") - - // Clean up before and after test - defer os.RemoveAll(filepath.Join(packagesDir, "test-failure-org")) - os.RemoveAll(filepath.Join(packagesDir, "test-failure-org")) // Clean up any existing test data - - // Create just the repo directory but no workflow files - err = os.MkdirAll(testRepoPath, 0755) - if err != nil { - t.Fatal(err) - } - - // Create a workflow spec for a non-existent file - spec := &WorkflowSpec{ - RepoSpec: RepoSpec{ - RepoSlug: "test-failure-org/test-failure-repo", - Version: "main", - }, - WorkflowPath: "workflows/nonexistent.md", - WorkflowName: "nonexistent", - } - - // Test findWorkflowInPackageForRepo - should fail since file doesn't exist - _, _, err = findWorkflowInPackageForRepo(spec, false) - if err == nil { - t.Fatal("Expected error when workflow file doesn't exist, got nil") - } - - // Verify error message - expectedErrSubstring := "not found in repo" - if !strings.Contains(err.Error(), expectedErrSubstring) { - t.Errorf("Expected error to contain %q, got %q", expectedErrSubstring, err.Error()) - } -} - -// TestFindWorkflowNonWorkflowsPath tests that non-workflows/ paths don't trigger fallback -func TestFindWorkflowNonWorkflowsPath(t *testing.T) { - // Get the packages directory - packagesDir, err := getPackagesDir() - if err != nil { - t.Fatal(err) - } - - // Create a unique test directory to avoid conflicts - testRepoPath := filepath.Join(packagesDir, "test-nonworkflows-org", "test-nonworkflows-repo") - - // Clean up before and after test - defer os.RemoveAll(filepath.Join(packagesDir, "test-nonworkflows-org")) - os.RemoveAll(filepath.Join(packagesDir, "test-nonworkflows-org")) // Clean up any existing test data - - // Create just the repo directory but no workflow files - err = os.MkdirAll(testRepoPath, 0755) - if err != nil { - t.Fatal(err) - } - - // Create a workflow spec with a custom path (not starting with "workflows/") - spec := &WorkflowSpec{ - RepoSpec: RepoSpec{ - RepoSlug: "test-nonworkflows-org/test-nonworkflows-repo", - Version: "main", - }, - WorkflowPath: "custom/path/workflow.md", - WorkflowName: "workflow", - } - - // Test findWorkflowInPackageForRepo - should fail without attempting fallback - _, _, err = findWorkflowInPackageForRepo(spec, false) - if err == nil { - t.Fatal("Expected error when workflow file doesn't exist and path is not workflows/, got nil") - } - - // Verify error message - expectedErrSubstring := "not found in repo" - if !strings.Contains(err.Error(), expectedErrSubstring) { - t.Errorf("Expected error to contain %q, got %q", expectedErrSubstring, err.Error()) - } -} diff --git a/pkg/cli/packages_test.go b/pkg/cli/packages_test.go index e5de15ae41..aab0743360 100644 --- a/pkg/cli/packages_test.go +++ b/pkg/cli/packages_test.go @@ -118,7 +118,7 @@ on: push // Collect includes var dependencies []IncludeDependency seen := make(map[string]bool) - err := collectPackageIncludesRecursive(tt.content, tmpDir, &dependencies, seen, false) + err := collectLocalIncludeDependenciesRecursive(tt.content, tmpDir, &dependencies, seen, false) // Check error expectation if tt.expectedError && err == nil { @@ -167,7 +167,7 @@ func TestCollectPackageIncludesRecursive_CircularReference(t *testing.T) { // Collect includes starting from a.md var dependencies []IncludeDependency seen := make(map[string]bool) - err := collectPackageIncludesRecursive(aContent, tmpDir, &dependencies, seen, false) + err := collectLocalIncludeDependenciesRecursive(aContent, tmpDir, &dependencies, seen, false) if err != nil { t.Errorf("Unexpected error: %v", err) diff --git a/pkg/cli/remote_workflow.go b/pkg/cli/remote_workflow.go new file mode 100644 index 0000000000..fbd00c9465 --- /dev/null +++ b/pkg/cli/remote_workflow.go @@ -0,0 +1,357 @@ +package cli + +import ( + "bufio" + "fmt" + "os" + "path/filepath" + "regexp" + "strings" + + "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/parser" +) + +var remoteWorkflowLog = logger.New("cli:remote_workflow") + +// FetchedWorkflow contains content and metadata from a directly fetched workflow file. +// This is the unified type that combines content with source information. +type FetchedWorkflow struct { + Content []byte // The raw content of the workflow file + CommitSHA string // The resolved commit SHA at the time of fetch (empty for local) + IsLocal bool // true if this is a local workflow (from filesystem) + SourcePath string // The original source path (local path or remote path) +} + +// FetchWorkflowFromSource fetches a workflow file directly from GitHub without cloning. +// This is the preferred way to add remote workflows as it only fetches the specific +// files needed rather than cloning the entire repository. +// +// For local workflows (local filesystem paths), it reads from the local filesystem. +// For remote workflows, it uses the GitHub API to fetch the file content. +func FetchWorkflowFromSource(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, error) { + remoteWorkflowLog.Printf("Fetching workflow from source: spec=%s", spec.String()) + + // Handle local workflows + if isLocalWorkflowPath(spec.WorkflowPath) { + return fetchLocalWorkflow(spec, verbose) + } + + // Handle remote workflows from GitHub + return fetchRemoteWorkflow(spec, verbose) +} + +// fetchLocalWorkflow reads a workflow file from the local filesystem +func fetchLocalWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, error) { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Reading local workflow: %s", spec.WorkflowPath))) + } + + content, err := os.ReadFile(spec.WorkflowPath) + if err != nil { + return nil, fmt.Errorf("local workflow '%s' not found: %w", spec.WorkflowPath, err) + } + + return &FetchedWorkflow{ + Content: content, + CommitSHA: "", // Local workflows don't have a commit SHA + IsLocal: true, + SourcePath: spec.WorkflowPath, + }, nil +} + +// fetchRemoteWorkflow fetches a workflow file directly from GitHub using the API +func fetchRemoteWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, error) { + remoteWorkflowLog.Printf("Fetching remote workflow: repo=%s, path=%s, version=%s", + spec.RepoSlug, spec.WorkflowPath, spec.Version) + + // Parse owner and repo from the slug + parts := strings.SplitN(spec.RepoSlug, "/", 2) + if len(parts) != 2 { + return nil, fmt.Errorf("invalid repository slug: %s", spec.RepoSlug) + } + owner := parts[0] + repo := parts[1] + + // Determine the ref to use + ref := spec.Version + if ref == "" { + ref = "main" // Default to main branch + remoteWorkflowLog.Print("No version specified, defaulting to 'main'") + } + + if verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Fetching %s/%s/%s@%s...", owner, repo, spec.WorkflowPath, ref))) + } + + // Resolve the ref to a commit SHA for source tracking + commitSHA, err := parser.ResolveRefToSHA(owner, repo, ref) + if err != nil { + remoteWorkflowLog.Printf("Failed to resolve ref to SHA: %v", err) + // Continue without SHA - we can still fetch the content + commitSHA = "" + } else { + remoteWorkflowLog.Printf("Resolved ref %s to SHA: %s", ref, commitSHA) + if verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Resolved to commit: %s", commitSHA[:7]))) + } + } + + // Download the workflow file from GitHub + content, err := parser.DownloadFileFromGitHub(owner, repo, spec.WorkflowPath, ref) + if err != nil { + // Try with a workflows/ prefix if the direct path fails + if !strings.HasPrefix(spec.WorkflowPath, "workflows/") && !strings.Contains(spec.WorkflowPath, "/") { + // Try workflows/filename.md + altPath := "workflows/" + spec.WorkflowPath + if !strings.HasSuffix(altPath, ".md") { + altPath += ".md" + } + remoteWorkflowLog.Printf("Direct path failed, trying: %s", altPath) + if altContent, altErr := parser.DownloadFileFromGitHub(owner, repo, altPath, ref); altErr == nil { + return &FetchedWorkflow{ + Content: altContent, + CommitSHA: commitSHA, + IsLocal: false, + SourcePath: altPath, + }, nil + } + + // Try .github/workflows/filename.md + altPath = ".github/workflows/" + spec.WorkflowPath + if !strings.HasSuffix(altPath, ".md") { + altPath += ".md" + } + remoteWorkflowLog.Printf("Trying: %s", altPath) + if altContent, altErr := parser.DownloadFileFromGitHub(owner, repo, altPath, ref); altErr == nil { + return &FetchedWorkflow{ + Content: altContent, + CommitSHA: commitSHA, + IsLocal: false, + SourcePath: altPath, + }, nil + } + } + return nil, fmt.Errorf("failed to download workflow from %s/%s/%s@%s: %w", owner, repo, spec.WorkflowPath, ref, err) + } + + if verbose { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Downloaded workflow (%d bytes)", len(content)))) + } + + return &FetchedWorkflow{ + Content: content, + CommitSHA: commitSHA, + IsLocal: false, + SourcePath: spec.WorkflowPath, + }, nil +} + +// FetchIncludeFromSource fetches an include file from GitHub directly using a workflowspec format path. +// The includePath should be in the format: owner/repo/path/to/file.md[@ref] +// If the includePath is a relative path, it's resolved relative to the baseSpec. +// Returns: (content, section, error) where section is the #fragment from the path (e.g., "#section-name"). +func FetchIncludeFromSource(includePath string, baseSpec *WorkflowSpec, verbose bool) ([]byte, string, error) { + baseSpecStr := "" + if baseSpec != nil { + baseSpecStr = baseSpec.String() + } + remoteWorkflowLog.Printf("Fetching include from source: path=%s, base=%s", includePath, baseSpecStr) + + // Extract section reference (e.g., "#section-name") from the path upfront + // This ensures consistent behavior regardless of which code path is taken + cleanPath := includePath + var section string + if idx := strings.Index(includePath, "#"); idx != -1 { + cleanPath = includePath[:idx] + section = includePath[idx:] + } + + // Check if this is a workflowspec format (owner/repo/path[@ref]) + if isWorkflowSpecFormat(cleanPath) { + // Split on @ to get path and ref + parts := strings.SplitN(cleanPath, "@", 2) + pathPart := parts[0] + var ref string + if len(parts) == 2 { + ref = parts[1] + } else { + ref = "main" + } + + // Parse path: owner/repo/path/to/file.md + slashParts := strings.Split(pathPart, "/") + if len(slashParts) < 3 { + return nil, section, fmt.Errorf("invalid workflowspec: must be owner/repo/path[@ref]") + } + + owner := slashParts[0] + repo := slashParts[1] + filePath := strings.Join(slashParts[2:], "/") + + // Download the file + content, err := parser.DownloadFileFromGitHub(owner, repo, filePath, ref) + if err != nil { + return nil, section, fmt.Errorf("failed to fetch include from %s: %w", includePath, err) + } + + return content, section, nil + } + + // For relative paths, resolve against the base spec + if baseSpec != nil && baseSpec.RepoSlug != "" { + parts := strings.SplitN(baseSpec.RepoSlug, "/", 2) + if len(parts) == 2 { + owner := parts[0] + repo := parts[1] + ref := baseSpec.Version + if ref == "" { + ref = "main" + } + + // Remove @ ref suffix if present in the clean path (for relative paths with explicit refs) + filePath := cleanPath + if idx := strings.Index(filePath, "@"); idx != -1 { + filePath = filePath[:idx] + } + + // If it's a relative path starting with shared/, it's relative to .github/ + var fullPath string + if strings.HasPrefix(filePath, "shared/") { + fullPath = ".github/" + filePath + } else { + // Otherwise, resolve relative to the workflow path directory + baseDir := getParentDir(baseSpec.WorkflowPath) + if baseDir != "" { + fullPath = baseDir + "/" + filePath + } else { + fullPath = filePath + } + } + + content, err := parser.DownloadFileFromGitHub(owner, repo, fullPath, ref) + if err != nil { + return nil, section, fmt.Errorf("failed to fetch include %s from %s/%s: %w", filePath, owner, repo, err) + } + + return content, section, nil + } + } + + return nil, section, fmt.Errorf("cannot resolve include path: %s (no base spec provided)", includePath) +} + +// fetchAndSaveRemoteIncludes parses the workflow content for @include directives and fetches them from the remote source +func fetchAndSaveRemoteIncludes(content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error { + remoteWorkflowLog.Printf("Fetching remote includes for workflow: %s", spec.String()) + + // Parse the workflow content to find @include directives + includePattern := regexp.MustCompile(`^@include(\?)?\s+(.+)$`) + + scanner := bufio.NewScanner(strings.NewReader(content)) + seen := make(map[string]bool) + + for scanner.Scan() { + line := scanner.Text() + matches := includePattern.FindStringSubmatch(line) + if matches == nil { + continue + } + + isOptional := matches[1] == "?" + includePath := strings.TrimSpace(matches[2]) + + // Remove section reference for file fetching + filePath := includePath + if idx := strings.Index(includePath, "#"); idx != -1 { + filePath = includePath[:idx] + } + + // Skip if already processed + if seen[filePath] { + continue + } + seen[filePath] = true + + // Fetch the include file + includeContent, _, err := FetchIncludeFromSource(includePath, spec, verbose) + if err != nil { + if isOptional { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Optional include not found: %s", includePath))) + } + continue + } + return fmt.Errorf("failed to fetch include %s: %w", includePath, err) + } + + // Determine target path for the include file + var targetPath string + if strings.HasPrefix(filePath, "shared/") { + // shared/ files go to .github/shared/ + targetPath = filepath.Join(filepath.Dir(targetDir), filePath) + } else if isWorkflowSpecFormat(filePath) { + // Workflowspec includes: extract just the filename and put in shared/ + parts := strings.Split(filePath, "/") + filename := parts[len(parts)-1] + targetPath = filepath.Join(filepath.Dir(targetDir), "shared", filename) + } else { + // Relative includes go alongside the workflow + targetPath = filepath.Join(targetDir, filePath) + } + + // Create target directory if needed + if err := os.MkdirAll(filepath.Dir(targetPath), 0755); err != nil { + return fmt.Errorf("failed to create directory for %s: %w", targetPath, err) + } + + // Check if file already exists + fileExists := false + if _, err := os.Stat(targetPath); err == nil { + fileExists = true + if !force { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Include file already exists, skipping: %s", targetPath))) + } + continue + } + } + + // Write the include file + if err := os.WriteFile(targetPath, includeContent, 0600); err != nil { + return fmt.Errorf("failed to write include file %s: %w", targetPath, err) + } + + if verbose { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Fetched include: %s", targetPath))) + } + + // Track the file + if tracker != nil { + if fileExists { + tracker.TrackModified(targetPath) + } else { + tracker.TrackCreated(targetPath) + } + } + + // Recursively fetch includes from the fetched file + if err := fetchAndSaveRemoteIncludes(string(includeContent), spec, targetDir, verbose, force, tracker); err != nil { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch nested includes from %s: %v", filePath, err))) + } + } + } + + return nil +} + +// getParentDir returns the directory part of a path +func getParentDir(path string) string { + idx := strings.LastIndex(path, "/") + if idx == -1 { + return "" + } + return path[:idx] +} diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go new file mode 100644 index 0000000000..10e09602a7 --- /dev/null +++ b/pkg/cli/remote_workflow_test.go @@ -0,0 +1,290 @@ +//go:build !integration + +package cli + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFetchLocalWorkflow(t *testing.T) { + tests := []struct { + name string + content string + expectError bool + }{ + { + name: "valid workflow file", + content: `--- +name: Test Workflow +on: workflow_dispatch +--- + +# Test Workflow + +This is a test. +`, + expectError: false, + }, + { + name: "empty file", + content: "", + expectError: false, + }, + { + name: "minimal content", + content: "# Hello", + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a temporary file + tempDir := t.TempDir() + tempFile := filepath.Join(tempDir, "test-workflow.md") + err := os.WriteFile(tempFile, []byte(tt.content), 0644) + require.NoError(t, err, "should create temp file") + + spec := &WorkflowSpec{ + WorkflowPath: tempFile, + WorkflowName: "test-workflow", + } + + result, err := fetchLocalWorkflow(spec, false) + + if tt.expectError { + assert.Error(t, err, "expected error") + } else { + require.NoError(t, err, "should not error") + assert.Equal(t, []byte(tt.content), result.Content, "content should match") + assert.True(t, result.IsLocal, "should be marked as local") + assert.Empty(t, result.CommitSHA, "local workflows should not have commit SHA") + assert.Equal(t, tempFile, result.SourcePath, "source path should match") + } + }) + } +} + +func TestFetchLocalWorkflow_NonExistentFile(t *testing.T) { + spec := &WorkflowSpec{ + WorkflowPath: "/nonexistent/path/to/workflow.md", + WorkflowName: "nonexistent-workflow", + } + + result, err := fetchLocalWorkflow(spec, false) + + require.Error(t, err, "should error for non-existent file") + assert.Nil(t, result, "result should be nil on error") + assert.Contains(t, err.Error(), "not found", "error should mention file not found") +} + +func TestFetchLocalWorkflow_DirectoryInsteadOfFile(t *testing.T) { + tempDir := t.TempDir() + + spec := &WorkflowSpec{ + WorkflowPath: tempDir, // Pass directory instead of file + WorkflowName: "directory-workflow", + } + + result, err := fetchLocalWorkflow(spec, false) + + require.Error(t, err, "should error when path is a directory") + assert.Nil(t, result, "result should be nil on error") +} + +func TestFetchWorkflowFromSource_LocalRouting(t *testing.T) { + // Create a temporary local workflow file + tempDir := t.TempDir() + tempFile := filepath.Join(tempDir, "local-workflow.md") + content := "# Local Workflow\n\nTest content." + err := os.WriteFile(tempFile, []byte(content), 0644) + require.NoError(t, err, "should create temp file") + + spec := &WorkflowSpec{ + WorkflowPath: tempFile, + WorkflowName: "local-workflow", + } + + result, err := FetchWorkflowFromSource(spec, false) + + require.NoError(t, err, "should not error for local workflow") + assert.True(t, result.IsLocal, "should route to local fetch") + assert.Equal(t, []byte(content), result.Content, "content should match") +} + +func TestFetchWorkflowFromSource_RemoteRoutingWithInvalidSlug(t *testing.T) { + // Test with a remote workflow spec that has an invalid slug + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "invalid-slug-no-slash", + Version: "main", + }, + WorkflowPath: "workflow.md", + WorkflowName: "workflow", + } + + result, err := FetchWorkflowFromSource(spec, false) + + require.Error(t, err, "should error for invalid repo slug") + assert.Nil(t, result, "result should be nil on error") + assert.Contains(t, err.Error(), "invalid repository slug", "error should mention invalid slug") +} + +func TestFetchIncludeFromSource_WorkflowSpecParsing(t *testing.T) { + tests := []struct { + name string + includePath string + baseSpec *WorkflowSpec + expectSection string + expectError bool + errorContains string + }{ + { + name: "two parts falls through to cannot resolve", + includePath: "owner/repo", + baseSpec: nil, + expectSection: "", + expectError: true, + errorContains: "cannot resolve include path", // Not a workflowspec format (only 2 parts) + }, + { + name: "section extraction from workflowspec", + includePath: "owner/repo/path/file.md#section-name", + baseSpec: nil, + expectSection: "#section-name", + expectError: true, // Will fail to download, but section should be extracted + errorContains: "", // Don't check error message, just that section is extracted + }, + { + name: "no section in workflowspec", + includePath: "owner/repo/path/file.md", + baseSpec: nil, + expectSection: "", + expectError: true, // Will fail to download + errorContains: "", + }, + { + name: "relative path without base spec", + includePath: "shared/file.md", + baseSpec: nil, + expectSection: "", + expectError: true, + errorContains: "cannot resolve include path", + }, + { + name: "relative path with section but no base spec", + includePath: "shared/file.md#my-section", + baseSpec: nil, + expectSection: "#my-section", + expectError: true, + errorContains: "cannot resolve include path", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, section, err := FetchIncludeFromSource(tt.includePath, tt.baseSpec, false) + + if tt.expectError { + assert.Error(t, err, "expected error") + if tt.errorContains != "" { + assert.Contains(t, err.Error(), tt.errorContains, "error should contain expected text") + } + } else { + assert.NoError(t, err, "should not error") + } + + // Section should always be extracted consistently + assert.Equal(t, tt.expectSection, section, "section should match expected") + }) + } +} + +func TestFetchIncludeFromSource_SectionExtraction(t *testing.T) { + // Test that section is consistently extracted regardless of path type + tests := []struct { + name string + includePath string + expectSection string + }{ + { + name: "hash section", + includePath: "owner/repo/file.md#section", + expectSection: "#section", + }, + { + name: "complex section with hyphens", + includePath: "owner/repo/file.md#my-complex-section-name", + expectSection: "#my-complex-section-name", + }, + { + name: "no section", + includePath: "owner/repo/file.md", + expectSection: "", + }, + { + name: "section at end of path with ref", + includePath: "owner/repo/file.md@v1.0.0#section", + expectSection: "#section", // Section is extracted from the end regardless of @ref position + }, + { + name: "section after everything", + includePath: "owner/repo/file.md#section-name", + expectSection: "#section-name", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // We expect errors since these are remote paths, but section should still be extracted + _, section, _ := FetchIncludeFromSource(tt.includePath, nil, false) + assert.Equal(t, tt.expectSection, section, "section should be correctly extracted") + }) + } +} + +func TestGetParentDir(t *testing.T) { + tests := []struct { + name string + path string + expected string + }{ + { + name: "simple path", + path: "dir/file.md", + expected: "dir", + }, + { + name: "deep path", + path: "a/b/c/file.md", + expected: "a/b/c", + }, + { + name: "no directory", + path: "file.md", + expected: "", + }, + { + name: "trailing slash", + path: "dir/", + expected: "dir", + }, + { + name: "empty string", + path: "", + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getParentDir(tt.path) + assert.Equal(t, tt.expected, result, "getParentDir(%q) should return %q", tt.path, tt.expected) + }) + } +} diff --git a/pkg/cli/spec.go b/pkg/cli/spec.go index 7338736db0..a5722519db 100644 --- a/pkg/cli/spec.go +++ b/pkg/cli/spec.go @@ -33,11 +33,38 @@ type WorkflowSpec struct { IsWildcard bool // true if this is a wildcard spec (e.g., "owner/repo/*") } +// isLocalWorkflowPath checks if a path refers to a local filesystem workflow. +// Local paths include: +// - Relative paths starting with "./", "../", ".\", or "..\", or equal to "." / ".." +// - Absolute paths as determined by filepath.IsAbs (OS-specific) +// - UNC-style paths starting with "\\" or "//" (Windows network paths) +func isLocalWorkflowPath(path string) bool { + // Explicit relative path checks (POSIX and Windows-style) + if path == "." || path == ".." { + return true + } + if strings.HasPrefix(path, "./") || strings.HasPrefix(path, "../") || + strings.HasPrefix(path, ".\\") || strings.HasPrefix(path, "..\\") { + return true + } + + // OS-specific absolute paths (e.g., "/foo", "C:\foo", "D:/foo", UNC on Windows) + if filepath.IsAbs(path) { + return true + } + + // UNC paths (e.g., "\\server\share\file.md" or "//server/share/file.md") + if strings.HasPrefix(path, `\\`) || strings.HasPrefix(path, "//") { + return true + } + return false +} + // String returns the canonical string representation of the workflow spec // in the format "owner/repo/path[@version]" or just the WorkflowPath for local specs func (w *WorkflowSpec) String() string { - // For local workflows (starting with "./"), return just the WorkflowPath - if strings.HasPrefix(w.WorkflowPath, "./") { + // For local workflows, return just the WorkflowPath + if isLocalWorkflowPath(w.WorkflowPath) { return w.WorkflowPath } @@ -57,7 +84,7 @@ func isRepoOnlySpec(spec string) bool { } // Local paths are not repo-only specs - if strings.HasPrefix(spec, "./") { + if isLocalWorkflowPath(spec) { return false } @@ -195,10 +222,26 @@ func parseWorkflowSpec(spec string) (*WorkflowSpec, error) { return parseGitHubURL(spec) } - // Check if this is a local path starting with "./" - if strings.HasPrefix(spec, "./") { + // Check if this is a local path + if isLocalWorkflowPath(spec) { specLog.Print("Detected local path format") - return parseLocalWorkflowSpec(spec) + + ws, err := parseLocalWorkflowSpec(spec) + if err != nil { + return nil, err + } + + // Detect local wildcard specs like "./*.md" and mark them so that + // downstream expansion (e.g., expandLocalWildcardWorkflows) can run. + if strings.ContainsAny(spec, "*?[") { + ws.IsWildcard = true + // Ensure a stable WorkflowName for wildcard specs. + if ws.WorkflowName == "" { + ws.WorkflowName = spec + } + } + + return ws, nil } // Handle version first (anything after @) @@ -293,18 +336,11 @@ func parseLocalWorkflowSpec(spec string) (*WorkflowSpec, error) { return nil, fmt.Errorf("local workflow specification must end with '.md' extension: %s", spec) } - // Get current repository info - repoInfo, err := GetCurrentRepoSlug() - if err != nil { - specLog.Printf("Failed to get current repo slug: %v", err) - return nil, fmt.Errorf("failed to get current repository info for local workflow: %w", err) - } - - specLog.Printf("Parsed local workflow: repo=%s, path=%s", repoInfo, spec) + specLog.Printf("Parsed local workflow: path=%s", spec) return &WorkflowSpec{ RepoSpec: RepoSpec{ - RepoSlug: repoInfo, + RepoSlug: "", // Local workflows have no remote repo Version: "", // Local workflows have no version }, WorkflowPath: spec, // Keep the "./" prefix in WorkflowPath diff --git a/pkg/cli/spec_test.go b/pkg/cli/spec_test.go index 7bb0cbe119..3166f529de 100644 --- a/pkg/cli/spec_test.go +++ b/pkg/cli/spec_test.go @@ -400,12 +400,13 @@ func TestParseLocalWorkflowSpec(t *testing.T) { t.Errorf("parseWorkflowSpec() workflowName = %q, want %q", spec.WorkflowName, tt.wantWorkflowName) } - // For local workflows, verify repo is current repo and version is empty + // For local workflows: version and repo should both be empty + // (local workflows don't come from a remote source) if spec.Version != "" { t.Errorf("parseWorkflowSpec() version = %q, want empty string for local workflow", spec.Version) } - if spec.RepoSlug == "" { - t.Errorf("parseWorkflowSpec() repo should not be empty for local workflow") + if spec.RepoSlug != "" { + t.Errorf("parseWorkflowSpec() repo = %q, want empty string for local workflow", spec.RepoSlug) } }) } diff --git a/pkg/cli/trial_repository.go b/pkg/cli/trial_repository.go index 5b4de5ae9a..3529ebbf13 100644 --- a/pkg/cli/trial_repository.go +++ b/pkg/cli/trial_repository.go @@ -207,44 +207,75 @@ func installWorkflowInTrialMode(ctx context.Context, tempDir string, parsedSpec return fmt.Errorf("failed to change to temp directory: %w", err) } - // Check if this is a local workflow - if strings.HasPrefix(parsedSpec.WorkflowPath, "./") { - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Installing local workflow '%s' from '%s' in trial mode", parsedSpec.WorkflowName, parsedSpec.WorkflowPath))) - } + // Fetch workflow content - handle local workflows specially since they need + // to be resolved from the original directory, not the tempDir + specToFetch := parsedSpec + var fetched *FetchedWorkflow + + if isLocalWorkflowPath(parsedSpec.WorkflowPath) { + // For local workflows, temporarily change to original dir for fetch + // Use a closure to ensure directory is restored even on error + fetched, err = func() (*FetchedWorkflow, error) { + if chErr := os.Chdir(originalDir); chErr != nil { + return nil, fmt.Errorf("failed to change to original directory for local fetch: %w", chErr) + } + // Always restore to tempDir when this closure exits + defer os.Chdir(tempDir) - // For local workflows, copy the file directly from the filesystem - if err := installLocalWorkflowInTrialMode(originalDir, tempDir, parsedSpec, opts); err != nil { - return fmt.Errorf("failed to install local workflow: %w", err) - } + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Installing local workflow '%s' from '%s' in trial mode", parsedSpec.WorkflowName, parsedSpec.WorkflowPath))) + } + return FetchWorkflowFromSource(specToFetch, opts.Verbose) + }() } else { + // Remote workflows can be fetched from any directory if opts.Verbose { fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Installing workflow '%s' from '%s' in trial mode", parsedSpec.WorkflowName, parsedSpec.RepoSlug))) } + fetched, err = FetchWorkflowFromSource(specToFetch, opts.Verbose) + } - // Install the source repository as a package - if err := InstallPackage(parsedSpec.RepoSlug, opts.Verbose); err != nil { - return fmt.Errorf("failed to install source repository: %w", err) + if err != nil { + return fmt.Errorf("failed to fetch workflow: %w", err) + } + + content := fetched.Content + + // Add source field to frontmatter for remote workflows + if !fetched.IsLocal && fetched.CommitSHA != "" { + sourceString := buildSourceStringWithCommitSHA(parsedSpec, fetched.CommitSHA) + if sourceString != "" { + updatedContent, err := addSourceToWorkflow(string(content), sourceString) + if err != nil { + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to add source field: %v", err))) + } + } else { + content = []byte(updatedContent) + } } + } - // Add the workflow from the installed package - addOpts := AddOptions{ - Number: 1, - Verbose: opts.Verbose, - EngineOverride: "", - Name: "", - Force: true, - AppendText: opts.AppendText, - CreatePR: false, - Push: false, - NoGitattributes: false, - WorkflowDir: "", - NoStopAfter: false, - StopAfter: "", - DisableSecurityScanner: opts.DisableSecurityScanner, + // Use common helper for security scan, directory creation, and writing + result, err := writeWorkflowToTrialDir(tempDir, parsedSpec.WorkflowName, content, opts) + if err != nil { + return err + } + + if opts.Verbose { + if fetched.IsLocal { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Copied local workflow to %s", result.DestPath))) + } else { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Fetched remote workflow to %s", result.DestPath))) } - if _, err := AddWorkflows([]string{parsedSpec.String()}, addOpts); err != nil { - return fmt.Errorf("failed to add workflow: %w", err) + } + + // Fetch and save include dependencies for remote workflows + if !fetched.IsLocal { + if err := fetchAndSaveRemoteIncludes(string(content), parsedSpec, result.WorkflowsDir, opts.Verbose, true, nil); err != nil { + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch include dependencies: %v", err))) + } } } @@ -289,68 +320,53 @@ func installWorkflowInTrialMode(ctx context.Context, tempDir string, parsedSpec return nil } -// installLocalWorkflowInTrialMode installs a local workflow file for trial mode -func installLocalWorkflowInTrialMode(originalDir, tempDir string, parsedSpec *WorkflowSpec, opts *TrialOptions) error { - // Construct the source path (relative to original directory) - sourcePath := filepath.Join(originalDir, parsedSpec.WorkflowPath) - - // Validate source path - sourcePath, err := fileutil.ValidateAbsolutePath(sourcePath) - if err != nil { - return fmt.Errorf("invalid source path: %w", err) - } +// trialWorkflowWriteResult contains the result of writing a workflow to the trial directory +type trialWorkflowWriteResult struct { + DestPath string + WorkflowsDir string +} - // Check if the source file exists - if _, err := os.Stat(sourcePath); os.IsNotExist(err) { - return fmt.Errorf("local workflow file does not exist: %s", sourcePath) +// writeWorkflowToTrialDir handles the common workflow writing logic for trial mode: +// - Security scanning +// - Creating workflows directory +// - Appending optional text +// - Writing to destination +// Returns the destination path and workflows directory for further processing. +func writeWorkflowToTrialDir(tempDir string, workflowName string, content []byte, opts *TrialOptions) (*trialWorkflowWriteResult, error) { + // Security scan: reject workflows containing malicious or dangerous content + if !opts.DisableSecurityScanner { + if findings := workflow.ScanMarkdownSecurity(string(content)); len(findings) > 0 { + fmt.Fprintln(os.Stderr, console.FormatErrorMessage("Security scan failed for workflow")) + fmt.Fprintln(os.Stderr, workflow.FormatSecurityFindings(findings)) + return nil, fmt.Errorf("workflow '%s' failed security scan: %d issue(s) detected", workflowName, len(findings)) + } + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Security scan passed")) + } + } else if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Security scanning disabled")) } // Create the workflows directory in the temp directory workflowsDir := filepath.Join(tempDir, constants.GetWorkflowDir()) - - // Validate workflows directory path - workflowsDir, err = fileutil.ValidateAbsolutePath(workflowsDir) + workflowsDir, err := fileutil.ValidateAbsolutePath(workflowsDir) if err != nil { - return fmt.Errorf("invalid workflows directory path: %w", err) + return nil, fmt.Errorf("invalid workflows directory path: %w", err) } - if err := os.MkdirAll(workflowsDir, 0755); err != nil { - return fmt.Errorf("failed to create workflows directory: %w", err) + return nil, fmt.Errorf("failed to create workflows directory: %w", err) } // Construct the destination path - destPath := filepath.Join(workflowsDir, parsedSpec.WorkflowName+".md") - - // Validate destination path + destPath := filepath.Join(workflowsDir, workflowName+".md") destPath, err = fileutil.ValidateAbsolutePath(destPath) if err != nil { - return fmt.Errorf("invalid destination path: %w", err) - } - - // Read the source file - content, err := os.ReadFile(sourcePath) - if err != nil { - return fmt.Errorf("failed to read local workflow file: %w", err) - } - - // Security scan: reject workflows containing malicious or dangerous content - if !opts.DisableSecurityScanner { - if findings := workflow.ScanMarkdownSecurity(string(content)); len(findings) > 0 { - fmt.Fprintln(os.Stderr, console.FormatErrorMessage("Security scan failed for local workflow")) - fmt.Fprintln(os.Stderr, workflow.FormatSecurityFindings(findings)) - return fmt.Errorf("local workflow '%s' failed security scan: %d issue(s) detected", parsedSpec.WorkflowName, len(findings)) - } - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Security scan passed")) - } - } else if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Security scanning disabled")) + return nil, fmt.Errorf("invalid destination path: %w", err) } // Append text if provided if opts.AppendText != "" { contentStr := string(content) - // Ensure we have a newline before appending if !strings.HasSuffix(contentStr, "\n") { contentStr += "\n" } @@ -360,14 +376,13 @@ func installLocalWorkflowInTrialMode(originalDir, tempDir string, parsedSpec *Wo // Write the content to the destination if err := os.WriteFile(destPath, content, 0644); err != nil { - return fmt.Errorf("failed to write workflow to destination: %w", err) + return nil, fmt.Errorf("failed to write workflow to destination: %w", err) } - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Copied local workflow from %s to %s", sourcePath, destPath))) - } - - return nil + return &trialWorkflowWriteResult{ + DestPath: destPath, + WorkflowsDir: workflowsDir, + }, nil } // modifyWorkflowForTrialMode modifies the workflow to work in trial mode diff --git a/pkg/parser/remote_fetch.go b/pkg/parser/remote_fetch.go index b2f8c4ce59..2c57603b97 100644 --- a/pkg/parser/remote_fetch.go +++ b/pkg/parser/remote_fetch.go @@ -594,6 +594,25 @@ func resolveRemoteSymlinks(owner, repo, filePath, ref string) (string, error) { return "", fmt.Errorf("no symlinks found in path: %s", filePath) } +// DownloadFileFromGitHub downloads a file from a GitHub repository using the GitHub API. +// This is the exported wrapper for downloadFileFromGitHub. +// Parameters: +// - owner: Repository owner (e.g., "github") +// - repo: Repository name (e.g., "gh-aw") +// - path: Path to the file within the repository (e.g., ".github/workflows/workflow.md") +// - ref: Git reference (branch, tag, or commit SHA) +// Returns the file content as bytes or an error if the file cannot be retrieved. +func DownloadFileFromGitHub(owner, repo, path, ref string) ([]byte, error) { + return downloadFileFromGitHub(owner, repo, path, ref) +} + +// ResolveRefToSHA resolves a git ref (branch, tag, or short SHA) to its full commit SHA. +// This is the exported wrapper for resolveRefToSHA. +// If the ref is already a 40-character hex SHA, it returns it as-is. +func ResolveRefToSHA(owner, repo, ref string) (string, error) { + return resolveRefToSHA(owner, repo, ref) +} + func downloadFileFromGitHub(owner, repo, path, ref string) ([]byte, error) { return downloadFileFromGitHubWithDepth(owner, repo, path, ref, 0) }