🔧 Fix Import Path Detection in Workflow Specification

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:"))

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, "@")
 }

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)
+		}
+	}
+}

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)
 	}

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

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"),
 		},
 	}