diff --git a/pkg/cli/add_workflow_repository.go b/pkg/cli/add_workflow_repository.go index a7a030f246e..96eae8b58c6 100644 --- a/pkg/cli/add_workflow_repository.go +++ b/pkg/cli/add_workflow_repository.go @@ -100,10 +100,7 @@ func showInteractiveWorkflowSelection(repoSlug string, workflows []WorkflowInfo, } // Build the workflow spec - workflowSpec := fmt.Sprintf("%s/%s", repoSlug, selectedID) - if version != "" { - workflowSpec += "@" + version - } + workflowSpec := buildWorkflowSpecRef(repoSlug, selectedID, "", version) fmt.Fprintln(os.Stderr, "") fmt.Fprintln(os.Stderr, console.FormatInfoMessage("To add this workflow, run:")) diff --git a/pkg/cli/imports.go b/pkg/cli/imports.go index 62904f29e89..9e41e16380c 100644 --- a/pkg/cli/imports.go +++ b/pkg/cli/imports.go @@ -16,6 +16,20 @@ import ( var importsLog = logger.New("cli:imports") +// buildWorkflowSpecRef builds a workflowspec reference string from components. +// Format: owner/repo/path@version (e.g., "github/gh-aw/shared/mcp/arxiv.md@abc123") +// If commitSHA is provided, it takes precedence over version. +// If neither is provided, returns the path without a version suffix. +func buildWorkflowSpecRef(repoSlug, path, commitSHA, version string) string { + workflowSpec := repoSlug + "/" + path + if commitSHA != "" { + workflowSpec += "@" + commitSHA + } else if version != "" { + workflowSpec += "@" + version + } + return workflowSpec +} + // resolveImportPath resolves a relative import path to its full repository path // based on the workflow file's location func resolveImportPath(importPath string, workflowPath string) string { @@ -96,13 +110,7 @@ func processImportsWithWorkflowSpec(content string, workflow *WorkflowSpec, comm importsLog.Printf("Resolved import path: %s -> %s (workflow: %s)", importPath, resolvedPath, workflow.WorkflowPath) // Build workflowspec for this import - // Format: owner/repo/path@sha - workflowSpec := workflow.RepoSlug + "/" + resolvedPath - if commitSHA != "" { - workflowSpec += "@" + commitSHA - } else if workflow.Version != "" { - workflowSpec += "@" + workflow.Version - } + workflowSpec := buildWorkflowSpecRef(workflow.RepoSlug, resolvedPath, commitSHA, workflow.Version) importsLog.Printf("Converted import: %s -> %s", importPath, workflowSpec) processedImports = append(processedImports, workflowSpec) @@ -204,13 +212,7 @@ func processIncludesWithWorkflowSpec(content string, workflow *WorkflowSpec, com visited[filePath] = true // Build workflowspec for this include - // Format: owner/repo/path@sha - workflowSpec := workflow.RepoSlug + "/" + filePath - if commitSHA != "" { - workflowSpec += "@" + commitSHA - } else if workflow.Version != "" { - workflowSpec += "@" + workflow.Version - } + workflowSpec := buildWorkflowSpecRef(workflow.RepoSlug, filePath, commitSHA, workflow.Version) // Add section if present if sectionName != "" { @@ -355,13 +357,7 @@ func processIncludesInContent(content string, workflow *WorkflowSpec, commitSHA resolvedPath := resolveImportPath(filePath, workflow.WorkflowPath) // Build workflowspec for this include - // Format: owner/repo/path@sha - workflowSpec := workflow.RepoSlug + "/" + resolvedPath - if commitSHA != "" { - workflowSpec += "@" + commitSHA - } else if workflow.Version != "" { - workflowSpec += "@" + workflow.Version - } + workflowSpec := buildWorkflowSpecRef(workflow.RepoSlug, resolvedPath, commitSHA, workflow.Version) // Add section if present if sectionName != "" { @@ -384,23 +380,10 @@ func processIncludesInContent(content string, workflow *WorkflowSpec, commitSHA } // isWorkflowSpecFormat checks if a path already looks like a workflowspec +// A workflowspec is identified by having an @ version indicator (e.g., owner/repo/path@sha) +// Simple paths like "shared/mcp/file.md" are NOT workflowspecs and should be processed func isWorkflowSpecFormat(path string) bool { - // Check if it contains @ (ref separator) or looks like owner/repo/path - if strings.Contains(path, "@") { - return true - } - - // Remove section reference if present - cleanPath := path - if idx := strings.Index(path, "#"); idx != -1 { - cleanPath = path[:idx] - } - - // Check if it has at least 3 parts and doesn't start with . or / - parts := strings.Split(cleanPath, "/") - if len(parts) >= 3 && !strings.HasPrefix(cleanPath, ".") && !strings.HasPrefix(cleanPath, "/") { - return true - } - - return false + // The only reliable indicator of a workflowspec is the @ version separator + // Paths like "shared/mcp/arxiv.md" should be treated as local paths, not workflowspecs + return strings.Contains(path, "@") } diff --git a/pkg/cli/imports_test.go b/pkg/cli/imports_test.go index c240da07afa..98bd9dca625 100644 --- a/pkg/cli/imports_test.go +++ b/pkg/cli/imports_test.go @@ -262,3 +262,124 @@ Do research. t.Errorf("Result should NOT contain malformed path '%s' (the original bug)\nGot:\n%s", malformedPath, result) } } + +func TestIsWorkflowSpecFormat(t *testing.T) { + tests := []struct { + name string + path string + expected bool + }{ + { + name: "workflowspec with SHA", + path: "owner/repo/path/file.md@abc123", + expected: true, + }, + { + name: "workflowspec with version tag", + path: "owner/repo/file.md@v1.0.0", + expected: true, + }, + { + name: "workflowspec without version - NOT a workflowspec", + path: "owner/repo/path/file.md", + expected: false, // Without @, it's not detected as a workflowspec + }, + { + name: "three-part relative path - NOT a workflowspec", + path: "shared/mcp/arxiv.md", + expected: false, // Local path, not a workflowspec + }, + { + name: "two-part relative path", + path: "shared/file.md", + expected: false, + }, + { + name: "relative path with ./", + path: "./shared/file.md", + expected: false, + }, + { + name: "absolute path", + path: "/shared/file.md", + expected: false, + }, + { + name: "workflowspec with section and version", + path: "owner/repo/path/file.md@sha#section", + expected: true, + }, + { + name: "simple filename", + path: "file.md", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isWorkflowSpecFormat(tt.path) + if result != tt.expected { + t.Errorf("isWorkflowSpecFormat(%q) = %v, want %v", tt.path, result, tt.expected) + } + }) + } +} + +func TestProcessImportsWithWorkflowSpec_ThreePartPath(t *testing.T) { + // Test that three-part paths like "shared/mcp/arxiv.md" are correctly converted + // to workflowspecs, not skipped as if they were already workflowspecs + content := `--- +engine: copilot +imports: + - shared/mcp/arxiv.md + - shared/mood.md + - shared/mcp/brave.md +--- + +# Test Workflow + +Test content. +` + + workflow := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/gh-aw", + Version: "main", + }, + WorkflowPath: ".github/workflows/test-workflow.md", + } + + commitSHA := "abc123def456" + + result, err := processImportsWithWorkflowSpec(content, workflow, commitSHA, false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + // All imports should be converted to workflowspecs with the commit SHA + expectedImports := []string{ + "github/gh-aw/.github/workflows/shared/mcp/arxiv.md@abc123def456", + "github/gh-aw/.github/workflows/shared/mood.md@abc123def456", + "github/gh-aw/.github/workflows/shared/mcp/brave.md@abc123def456", + } + + for _, expected := range expectedImports { + if !strings.Contains(result, expected) { + t.Errorf("Expected result to contain '%s'\nGot:\n%s", expected, result) + } + } + + // The original paths should NOT appear unchanged + unchangedPaths := []string{ + "- shared/mcp/arxiv.md", + "- shared/mood.md", + "- shared/mcp/brave.md", + } + + for _, unchanged := range unchangedPaths { + if strings.Contains(result, unchanged) { + t.Errorf("Did not expect result to contain unchanged path '%s'\nGot:\n%s", unchanged, result) + } + } +} diff --git a/pkg/cli/packages.go b/pkg/cli/packages.go index 1a32868ae9b..ef9b567951b 100644 --- a/pkg/cli/packages.go +++ b/pkg/cli/packages.go @@ -139,12 +139,24 @@ func downloadWorkflows(repo, version, targetDir string, verbose bool) error { 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) } diff --git a/pkg/cli/run_push.go b/pkg/cli/run_push.go index 671b69bc0ce..69da218e643 100644 --- a/pkg/cli/run_push.go +++ b/pkg/cli/run_push.go @@ -362,7 +362,7 @@ func resolveImportPathLocal(importPath, baseDir string) string { } // Skip workflowspec format imports (owner/repo/path@sha) - if strings.Contains(importPath, "@") || isWorkflowSpecFormatLocal(importPath) { + if isWorkflowSpecFormatLocal(importPath) { runPushLog.Printf("Skipping workflowspec format import: %s", importPath) return "" } @@ -390,30 +390,9 @@ func resolveImportPathLocal(importPath, baseDir string) string { // isWorkflowSpecFormatLocal is a local version of isWorkflowSpecFormat for push functionality // This is duplicated from imports.go to avoid circular dependencies func isWorkflowSpecFormatLocal(path string) bool { - runPushLog.Printf("Checking if workflowspec format: %s", path) - - // Check if it contains @ (ref separator) or looks like owner/repo/path - if strings.Contains(path, "@") { - runPushLog.Printf("Path contains @ - workflowspec format: %s", path) - return true - } - - // Remove section reference if present - cleanPath := path - if idx := strings.Index(path, "#"); idx != -1 { - cleanPath = path[:idx] - runPushLog.Printf("Removed section reference: %s -> %s", path, cleanPath) - } - - // Check if it has at least 3 parts and doesn't start with . or / - parts := strings.Split(cleanPath, "/") - if len(parts) >= 3 && !strings.HasPrefix(cleanPath, ".") && !strings.HasPrefix(cleanPath, "/") { - runPushLog.Printf("Path has %d parts and matches owner/repo/path format - workflowspec format: %s", len(parts), path) - return true - } - - runPushLog.Printf("Path is not workflowspec format: %s", path) - return false + // The only reliable indicator of a workflowspec is the @ version separator + // Paths like "shared/mcp/arxiv.md" should be treated as local paths, not workflowspecs + return strings.Contains(path, "@") } // pushWorkflowFiles commits and pushes the workflow files to the repository diff --git a/pkg/cli/run_push_test.go b/pkg/cli/run_push_test.go index 79d28d9db69..88d20487674 100644 --- a/pkg/cli/run_push_test.go +++ b/pkg/cli/run_push_test.go @@ -179,7 +179,7 @@ func TestIsWorkflowSpecFormatLocal(t *testing.T) { { name: "workflowspec without SHA", path: "owner/repo/path/file.md", - expected: true, + expected: false, }, { name: "relative path with ./", @@ -199,13 +199,18 @@ func TestIsWorkflowSpecFormatLocal(t *testing.T) { { name: "workflowspec with section", path: "owner/repo/path/file.md#section", - expected: true, + expected: false, }, { name: "simple filename", path: "file.md", expected: false, }, + { + name: "workflowspec with @ version", + path: "owner/repo/path/file.md@v1.0.0", + expected: true, + }, } for _, tt := range tests { @@ -248,10 +253,10 @@ func TestResolveImportPathLocal(t *testing.T) { expected: "", }, { - name: "workflowspec format without @", + name: "path without @ is treated as local", importPath: "owner/repo/path/file.md", baseDir: baseDir, - expected: "", + expected: filepath.Join(baseDir, "owner/repo/path/file.md"), }, } diff --git a/pkg/cli/trial_repository.go b/pkg/cli/trial_repository.go index f328efeff8f..5b4de5ae9a0 100644 --- a/pkg/cli/trial_repository.go +++ b/pkg/cli/trial_repository.go @@ -181,23 +181,14 @@ func cloneTrialHostRepository(repoSlug string, verbose bool) (string, error) { return "", fmt.Errorf("invalid temporary directory path: %w", err) } - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Cloning host repository to: %s", tempDir))) - } - // Clone the repository using the full slug repoURL := fmt.Sprintf("https://github.com/%s.git", repoSlug) - cmd := exec.Command("git", "clone", repoURL, tempDir) - output, err := cmd.CombinedOutput() + output, err := workflow.RunGitCombined(fmt.Sprintf("Cloning %s...", repoSlug), "clone", repoURL, tempDir) if err != nil { return "", fmt.Errorf("failed to clone host repository %s: %w (output: %s)", repoURL, err, string(output)) } - if verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Cloned host repository to: %s", tempDir))) - } - return tempDir, nil } @@ -521,8 +512,9 @@ func cloneRepoContentsIntoHost(cloneRepoSlug string, cloneRepoVersion string, ho // Clone the source repository cloneURL := fmt.Sprintf("https://github.com/%s.git", cloneRepoSlug) - cmd := exec.Command("git", "clone", cloneURL, tempCloneDir) - if output, err := cmd.CombinedOutput(); err != nil { + + output, err := workflow.RunGitCombined(fmt.Sprintf("Cloning %s...", cloneRepoSlug), "clone", cloneURL, tempCloneDir) + if err != nil { return fmt.Errorf("failed to clone source repository %s: %w (output: %s)", cloneURL, err, string(output)) } @@ -533,22 +525,22 @@ func cloneRepoContentsIntoHost(cloneRepoSlug string, cloneRepoVersion string, ho // If a version/tag/SHA is specified, checkout that ref if cloneRepoVersion != "" { - cmd = exec.Command("git", "checkout", cloneRepoVersion) - if output, err := cmd.CombinedOutput(); err != nil { + checkoutCmd := exec.Command("git", "checkout", cloneRepoVersion) + if output, err := checkoutCmd.CombinedOutput(); err != nil { return fmt.Errorf("failed to checkout ref '%s': %w (output: %s)", cloneRepoVersion, err, string(output)) } } // Add the host repository as a new remote hostURL := fmt.Sprintf("https://github.com/%s.git", hostRepoSlug) - cmd = exec.Command("git", "remote", "add", "host", hostURL) - if output, err := cmd.CombinedOutput(); err != nil { + remoteCmd := exec.Command("git", "remote", "add", "host", hostURL) + if output, err := remoteCmd.CombinedOutput(); err != nil { return fmt.Errorf("failed to add host remote: %w (output: %s)", err, string(output)) } // Force push the current branch to the host repository's main branch - cmd = exec.Command("git", "push", "--force", "host", "HEAD:main") - if output, err := cmd.CombinedOutput(); err != nil { + pushCmd := exec.Command("git", "push", "--force", "host", "HEAD:main") + if output, err := pushCmd.CombinedOutput(); err != nil { return fmt.Errorf("failed to force push to host repository: %w (output: %s)", err, string(output)) } diff --git a/pkg/workflow/git_helpers.go b/pkg/workflow/git_helpers.go index f7662ab20dd..7e68999b61b 100644 --- a/pkg/workflow/git_helpers.go +++ b/pkg/workflow/git_helpers.go @@ -20,12 +20,17 @@ // Tag Detection: // - GetCurrentGitTag() - Detect current Git tag from environment or repository // +// Command Execution with Spinner: +// - RunGit() - Execute git command with spinner, returning stdout +// - RunGitCombined() - Execute git command with spinner, returning combined stdout+stderr +// // # Usage Patterns // // These functions are primarily used during workflow compilation to: // - Detect release contexts (tags vs. regular commits) // - Extract version information for releases // - Support conditional workflow behavior based on Git state +// - Execute git commands with spinner feedback in interactive terminals package workflow @@ -34,7 +39,9 @@ import ( "os/exec" "strings" + "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/tty" ) var gitHelpersLog = logger.New("workflow:git_helpers") @@ -78,3 +85,41 @@ func GetCurrentGitTag() string { gitHelpersLog.Printf("Using tag from git describe: %s", tag) return tag } + +// runGitWithSpinner executes a git command with an optional spinner. +// If stderr is a terminal, a spinner is shown while the command runs. +func runGitWithSpinner(spinnerMessage string, combined bool, args ...string) ([]byte, error) { + cmd := exec.Command("git", args...) + gitHelpersLog.Printf("Running git command: git %s", strings.Join(args, " ")) + + if tty.IsStderrTerminal() { + spinner := console.NewSpinner(spinnerMessage) + spinner.Start() + var output []byte + var err error + if combined { + output, err = cmd.CombinedOutput() + } else { + output, err = cmd.Output() + } + spinner.Stop() + return output, err + } + + if combined { + return cmd.CombinedOutput() + } + return cmd.Output() +} + +// RunGit executes a git command with an optional spinner, returning stdout. +// If stderr is a terminal, a spinner with the given message is shown. +func RunGit(spinnerMessage string, args ...string) ([]byte, error) { + return runGitWithSpinner(spinnerMessage, false, args...) +} + +// RunGitCombined executes a git command with an optional spinner, returning combined stdout+stderr. +// If stderr is a terminal, a spinner with the given message is shown. +func RunGitCombined(spinnerMessage string, args ...string) ([]byte, error) { + return runGitWithSpinner(spinnerMessage, true, args...) +}