diff --git a/cmd/gh-aw/main.go b/cmd/gh-aw/main.go index cbcfcdc8f7..5f465808b7 100644 --- a/cmd/gh-aw/main.go +++ b/cmd/gh-aw/main.go @@ -140,7 +140,8 @@ var removeCmd = &cobra.Command{ if len(args) > 0 { pattern = args[0] } - if err := cli.RemoveWorkflows(pattern); err != nil { + keepOrphans, _ := cmd.Flags().GetBool("keep-orphans") + if err := cli.RemoveWorkflows(pattern, keepOrphans); err != nil { fmt.Fprintln(os.Stderr, console.FormatErrorMessage(err.Error())) os.Exit(1) } @@ -322,6 +323,9 @@ func init() { compileCmd.Flags().Bool("auto-compile", false, "Generate auto-compile workflow file for automatic compilation") compileCmd.Flags().BoolP("watch", "w", false, "Watch for changes to workflow files and recompile automatically") + // Add flags to remove command + removeCmd.Flags().Bool("keep-orphans", false, "Skip removal of orphaned include files that are no longer referenced by any workflow") + // Add all commands to root rootCmd.AddCommand(listCmd) rootCmd.AddCommand(addCmd) diff --git a/docs/commands.md b/docs/commands.md index f5cfbaaf1f..9e51e5042e 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -44,6 +44,9 @@ gh aw compile --watch --auto-compile --verbose # Remove a workflow gh aw remove WorkflowName +# Remove a workflow without removing orphaned include files +gh aw remove WorkflowName --keep-orphans + # Run a workflow immediately gh aw run WorkflowName diff --git a/pkg/cli/commands.go b/pkg/cli/commands.go index 06b3e3ef2e..68b69ac594 100644 --- a/pkg/cli/commands.go +++ b/pkg/cli/commands.go @@ -849,7 +849,7 @@ func handleFileDeleted(mdFile string, verbose bool) { } // RemoveWorkflows removes workflows matching a pattern -func RemoveWorkflows(pattern string) error { +func RemoveWorkflows(pattern string, keepOrphans bool) error { workflowsDir := getWorkflowsDir() if _, err := os.Stat(workflowsDir); os.IsNotExist(err) { @@ -905,6 +905,17 @@ func RemoveWorkflows(pattern string) error { return nil } + // Preview orphaned includes that would be removed (if orphan removal is enabled) + var orphanedIncludes []string + if !keepOrphans { + var err error + orphanedIncludes, err = previewOrphanedIncludes(filesToRemove, false) + if err != nil { + fmt.Printf("Warning: Failed to preview orphaned includes: %v\n", err) + orphanedIncludes = []string{} // Continue with empty list + } + } + // Show what will be removed fmt.Printf("The following workflows will be removed:\n") for _, file := range filesToRemove { @@ -922,6 +933,14 @@ func RemoveWorkflows(pattern string) error { } } + // Show orphaned includes that will also be removed + if len(orphanedIncludes) > 0 { + fmt.Printf("\nThe following orphaned include files will also be removed (suppress with --keep-orphans):\n") + for _, include := range orphanedIncludes { + fmt.Printf(" %s (orphaned include)\n", include) + } + } + // Ask for confirmation fmt.Print("\nAre you sure you want to remove these workflows? [y/N]: ") reader := bufio.NewReader(os.Stdin) @@ -954,8 +973,8 @@ func RemoveWorkflows(pattern string) error { } } - // Clean up orphaned include files - if len(removedFiles) > 0 { + // Clean up orphaned include files (if orphan removal is enabled) + if len(removedFiles) > 0 && !keepOrphans { if err := cleanupOrphanedIncludes(false); err != nil { fmt.Printf("Warning: Failed to clean up orphaned includes: %v\n", err) } @@ -2438,6 +2457,8 @@ func cleanupOrphanedIncludes(verbose bool) error { } // Find all include files in .github/workflows + // Only consider files in subdirectories (like shared/) as potential include files + // Root-level .md files are workflow files, not include files workflowsDir := ".github/workflows" var allIncludes []string @@ -2447,12 +2468,16 @@ func cleanupOrphanedIncludes(verbose bool) error { } if !info.IsDir() && strings.HasSuffix(info.Name(), ".md") { - // This is an include file relPath, err := filepath.Rel(workflowsDir, path) if err != nil { return err } - allIncludes = append(allIncludes, relPath) + + // Only consider files in subdirectories as potential include files + // Root-level .md files are workflow files, not include files + if strings.Contains(relPath, string(filepath.Separator)) { + allIncludes = append(allIncludes, relPath) + } } return nil @@ -2479,6 +2504,104 @@ func cleanupOrphanedIncludes(verbose bool) error { return nil } +// previewOrphanedIncludes returns a list of include files that would become orphaned if the specified files were removed +func previewOrphanedIncludes(filesToRemove []string, verbose bool) ([]string, error) { + // Get all current markdown files + allMdFiles, err := getMarkdownWorkflowFiles() + if err != nil { + return nil, err + } + + // Create a map of files to remove for quick lookup + removeMap := make(map[string]bool) + for _, file := range filesToRemove { + removeMap[file] = true + } + + // Get the files that would remain after removal + var remainingFiles []string + for _, file := range allMdFiles { + if !removeMap[file] { + remainingFiles = append(remainingFiles, file) + } + } + + // If no files remain, all include files would be orphaned + if len(remainingFiles) == 0 { + return getAllIncludeFiles() + } + + // Collect all include dependencies from remaining workflows + usedIncludes := make(map[string]bool) + + for _, mdFile := range remainingFiles { + content, err := os.ReadFile(mdFile) + if err != nil { + if verbose { + fmt.Printf("Warning: Could not read %s for include analysis: %v\n", mdFile, err) + } + continue + } + + // Find includes used by this workflow + includes, err := findIncludesInContent(string(content), filepath.Dir(mdFile), verbose) + if err != nil { + if verbose { + fmt.Printf("Warning: Could not analyze includes in %s: %v\n", mdFile, err) + } + continue + } + + for _, include := range includes { + usedIncludes[include] = true + } + } + + // Find all include files and check which ones would be orphaned + allIncludes, err := getAllIncludeFiles() + if err != nil { + return nil, err + } + + var orphanedIncludes []string + for _, include := range allIncludes { + if !usedIncludes[include] { + orphanedIncludes = append(orphanedIncludes, include) + } + } + + return orphanedIncludes, nil +} + +// getAllIncludeFiles returns all include files in .github/workflows subdirectories +func getAllIncludeFiles() ([]string, error) { + workflowsDir := ".github/workflows" + var allIncludes []string + + err := filepath.Walk(workflowsDir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + + if !info.IsDir() && strings.HasSuffix(info.Name(), ".md") { + relPath, err := filepath.Rel(workflowsDir, path) + if err != nil { + return err + } + + // Only consider files in subdirectories as potential include files + // Root-level .md files are workflow files, not include files + if strings.Contains(relPath, string(filepath.Separator)) { + allIncludes = append(allIncludes, relPath) + } + } + + return nil + }) + + return allIncludes, err +} + // cleanupAllIncludes removes all include files when no workflows remain func cleanupAllIncludes(verbose bool) error { workflowsDir := ".github/workflows" @@ -2489,13 +2612,18 @@ func cleanupAllIncludes(verbose bool) error { } if !info.IsDir() && strings.HasSuffix(info.Name(), ".md") { - if err := os.Remove(path); err != nil { - if verbose { - fmt.Printf("Warning: Failed to remove include %s: %v\n", path, err) + relPath, _ := filepath.Rel(workflowsDir, path) + + // Only remove files in subdirectories (like shared/) as these are include files + // Root-level .md files are workflow files, not include files + if strings.Contains(relPath, string(filepath.Separator)) { + if err := os.Remove(path); err != nil { + if verbose { + fmt.Printf("Warning: Failed to remove include %s: %v\n", relPath, err) + } + } else { + fmt.Printf("Removed include: %s\n", relPath) } - } else { - relPath, _ := filepath.Rel(workflowsDir, path) - fmt.Printf("Removed include: %s\n", relPath) } } diff --git a/pkg/cli/commands_test.go b/pkg/cli/commands_test.go index 37fba8e1fa..262f84c355 100644 --- a/pkg/cli/commands_test.go +++ b/pkg/cli/commands_test.go @@ -1,7 +1,9 @@ package cli import ( + "fmt" "os" + "path/filepath" "strings" "testing" ) @@ -112,7 +114,7 @@ func TestCompileWorkflows(t *testing.T) { } func TestRemoveWorkflows(t *testing.T) { - err := RemoveWorkflows("test-pattern") + err := RemoveWorkflows("test-pattern", false) // Should not error since it's a stub implementation if err != nil { @@ -180,7 +182,7 @@ func TestAllCommandsExist(t *testing.T) { {func() error { return ListWorkflows(false) }, false, "ListWorkflows"}, {func() error { return AddWorkflow("", 1, false, "", "", false) }, false, "AddWorkflow (empty name)"}, // Shows help when empty, doesn't error {func() error { return CompileWorkflows("", false, "", false, false, false) }, false, "CompileWorkflows"}, // Should succeed when .github/workflows directory exists - {func() error { return RemoveWorkflows("test") }, false, "RemoveWorkflows"}, // Should handle missing directory gracefully + {func() error { return RemoveWorkflows("test", false) }, false, "RemoveWorkflows"}, // Should handle missing directory gracefully {func() error { return StatusWorkflows("test", false) }, false, "StatusWorkflows"}, // Should handle missing directory gracefully {func() error { return EnableWorkflows("test") }, false, "EnableWorkflows"}, // Should handle missing directory gracefully {func() error { return DisableWorkflows("test") }, false, "DisableWorkflows"}, // Should handle missing directory gracefully @@ -889,3 +891,341 @@ func TestCollectIncludesRecursiveCircularReference(t *testing.T) { t.Errorf("collectIncludesRecursive collected too many dependencies (%d), possible infinite loop", len(dependencies)) } } + +// TestCleanupOrphanedIncludes tests that root workflow files are not removed as "orphaned" includes +func TestCleanupOrphanedIncludes(t *testing.T) { + // Create a temporary directory structure + tmpDir, err := os.MkdirTemp("", "test-cleanup-orphaned") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + // Create .github/workflows directory + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + if err := os.MkdirAll(workflowsDir, 0755); err != nil { + t.Fatal(err) + } + + // Create shared subdirectory + sharedDir := filepath.Join(workflowsDir, "shared") + if err := os.MkdirAll(sharedDir, 0755); err != nil { + t.Fatal(err) + } + + // Create root workflow files (these should NOT be considered orphaned) + rootWorkflows := []string{"daily-plan.md", "weekly-research.md", "action-workflow-assessor.md"} + for _, name := range rootWorkflows { + content := fmt.Sprintf(`--- +on: + workflow_dispatch: +--- + +# %s + +This is a root workflow. +`, strings.TrimSuffix(name, ".md")) + if err := os.WriteFile(filepath.Join(workflowsDir, name), []byte(content), 0644); err != nil { + t.Fatal(err) + } + } + + // Create include files in shared/ directory (these should be considered orphaned if not used) + includeFiles := []string{"shared/common.md", "shared/tools.md"} + for _, name := range includeFiles { + content := `--- +tools: + github: + allowed: [] +--- + +This is an include file. +` + if err := os.WriteFile(filepath.Join(workflowsDir, name), []byte(content), 0644); err != nil { + t.Fatal(err) + } + } + + // Create one root workflow that actually uses an include + workflowWithInclude := `--- +on: + workflow_dispatch: +--- + +# Workflow with Include + +@include shared/common.md + +This workflow uses an include. +` + if err := os.WriteFile(filepath.Join(workflowsDir, "workflow-with-include.md"), []byte(workflowWithInclude), 0644); err != nil { + t.Fatal(err) + } + + // Change to the temporary directory to simulate the git root + oldDir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer func() { + if err := os.Chdir(oldDir); err != nil { + t.Logf("Warning: Failed to restore working directory: %v", err) + } + }() + + if err := os.Chdir(tmpDir); err != nil { + t.Fatal(err) + } + + // Run cleanup + err = cleanupOrphanedIncludes(true) + if err != nil { + t.Fatalf("cleanupOrphanedIncludes failed: %v", err) + } + + // Verify that root workflow files still exist + for _, name := range rootWorkflows { + filePath := filepath.Join(workflowsDir, name) + if _, err := os.Stat(filePath); os.IsNotExist(err) { + t.Errorf("Root workflow file %s was incorrectly removed as orphaned", name) + } + } + + // Verify that workflow-with-include.md still exists + if _, err := os.Stat(filepath.Join(workflowsDir, "workflow-with-include.md")); os.IsNotExist(err) { + t.Error("Workflow with include was incorrectly removed") + } + + // Verify that shared/common.md still exists (it's used by workflow-with-include.md) + if _, err := os.Stat(filepath.Join(workflowsDir, "shared", "common.md")); os.IsNotExist(err) { + t.Error("Used include file shared/common.md was incorrectly removed") + } + + // Verify that shared/tools.md was removed (it's truly orphaned) + if _, err := os.Stat(filepath.Join(workflowsDir, "shared", "tools.md")); !os.IsNotExist(err) { + t.Error("Orphaned include file shared/tools.md was not removed") + } +} + +// TestPreviewOrphanedIncludes tests the preview functionality for orphaned includes +func TestPreviewOrphanedIncludes(t *testing.T) { + // Create a temporary directory structure + tmpDir, err := os.MkdirTemp("", "test-preview-orphaned") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + // Create .github/workflows directory + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + if err := os.MkdirAll(workflowsDir, 0755); err != nil { + t.Fatal(err) + } + + // Create shared subdirectory + sharedDir := filepath.Join(workflowsDir, "shared") + if err := os.MkdirAll(sharedDir, 0755); err != nil { + t.Fatal(err) + } + + // Create workflow files + workflow1 := `--- +on: + workflow_dispatch: +--- + +# Workflow 1 + +@include shared/common.md + +This workflow uses common include. +` + if err := os.WriteFile(filepath.Join(workflowsDir, "workflow1.md"), []byte(workflow1), 0644); err != nil { + t.Fatal(err) + } + + workflow2 := `--- +on: + workflow_dispatch: +--- + +# Workflow 2 + +@include shared/tools.md + +This workflow uses tools include. +` + if err := os.WriteFile(filepath.Join(workflowsDir, "workflow2.md"), []byte(workflow2), 0644); err != nil { + t.Fatal(err) + } + + workflow3 := `--- +on: + workflow_dispatch: +--- + +# Workflow 3 + +@include shared/common.md + +This workflow also uses common include. +` + if err := os.WriteFile(filepath.Join(workflowsDir, "workflow3.md"), []byte(workflow3), 0644); err != nil { + t.Fatal(err) + } + + // Create include files + includeFiles := map[string]string{ + "shared/common.md": "Common include content", + "shared/tools.md": "Tools include content", + "shared/unused.md": "Unused include content", + } + for name, content := range includeFiles { + if err := os.WriteFile(filepath.Join(workflowsDir, name), []byte(content), 0644); err != nil { + t.Fatal(err) + } + } + + // Change to the temporary directory + oldDir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer func() { + if err := os.Chdir(oldDir); err != nil { + t.Logf("Warning: Failed to restore working directory: %v", err) + } + }() + + if err := os.Chdir(tmpDir); err != nil { + t.Fatal(err) + } + + // Test Case 1: Remove workflow2 - should orphan shared/tools.md but not shared/common.md + // Use relative paths like the real RemoveWorkflows function does + filesToRemove := []string{".github/workflows/workflow2.md"} + orphaned, err := previewOrphanedIncludes(filesToRemove, false) + if err != nil { + t.Fatalf("previewOrphanedIncludes failed: %v", err) + } + + expectedOrphaned := []string{"shared/tools.md", "shared/unused.md"} + if len(orphaned) != len(expectedOrphaned) { + t.Errorf("Expected %d orphaned includes, got %d: %v", len(expectedOrphaned), len(orphaned), orphaned) + } + + for _, expected := range expectedOrphaned { + found := false + for _, actual := range orphaned { + if actual == expected { + found = true + break + } + } + if !found { + t.Errorf("Expected %s to be orphaned, but it wasn't found in: %v", expected, orphaned) + } + } + + // shared/common.md should NOT be orphaned as it's used by workflow1 and workflow3 + for _, include := range orphaned { + if include == "shared/common.md" { + t.Error("shared/common.md should not be orphaned as it's used by remaining workflows") + } + } + + // Test Case 2: Remove all workflows - should orphan all includes + allFiles := []string{ + ".github/workflows/workflow1.md", + ".github/workflows/workflow2.md", + ".github/workflows/workflow3.md", + } + orphaned, err = previewOrphanedIncludes(allFiles, false) + if err != nil { + t.Fatalf("previewOrphanedIncludes failed: %v", err) + } + + expectedAllOrphaned := []string{"shared/common.md", "shared/tools.md", "shared/unused.md"} + if len(orphaned) != len(expectedAllOrphaned) { + t.Errorf("Expected %d orphaned includes when removing all workflows, got %d: %v", len(expectedAllOrphaned), len(orphaned), orphaned) + } +} + +// TestRemoveWorkflowsWithNoOrphansFlag tests that the --keep-orphans flag works correctly +func TestRemoveWorkflowsWithNoOrphansFlag(t *testing.T) { + // Create a temporary directory structure + tmpDir, err := os.MkdirTemp("", "test-keep-orphans-flag") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + // Create .github/workflows directory + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + if err := os.MkdirAll(workflowsDir, 0755); err != nil { + t.Fatal(err) + } + + // Create shared subdirectory + sharedDir := filepath.Join(workflowsDir, "shared") + if err := os.MkdirAll(sharedDir, 0755); err != nil { + t.Fatal(err) + } + + // Create a workflow that uses an include + workflowContent := `--- +on: + workflow_dispatch: +--- + +# Test Workflow + +@include shared/common.md + +This workflow uses an include. +` + if err := os.WriteFile(filepath.Join(workflowsDir, "test-workflow.md"), []byte(workflowContent), 0644); err != nil { + t.Fatal(err) + } + + // Create the include file + includeContent := `This is a shared include file.` + if err := os.WriteFile(filepath.Join(sharedDir, "common.md"), []byte(includeContent), 0644); err != nil { + t.Fatal(err) + } + + // Change to the temporary directory + oldDir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer func() { + if err := os.Chdir(oldDir); err != nil { + t.Logf("Warning: Failed to restore working directory: %v", err) + } + }() + + if err := os.Chdir(tmpDir); err != nil { + t.Fatal(err) + } + + // Test 1: Verify include file exists before removal + if _, err := os.Stat(filepath.Join(sharedDir, "common.md")); os.IsNotExist(err) { + t.Fatal("Include file should exist before removal") + } + + // Test 2: Check preview shows orphaned includes when flag is not used + filesToRemove := []string{".github/workflows/test-workflow.md"} + orphaned, err := previewOrphanedIncludes(filesToRemove, false) + if err != nil { + t.Fatalf("previewOrphanedIncludes failed: %v", err) + } + + if len(orphaned) != 1 || orphaned[0] != "shared/common.md" { + t.Errorf("Expected shared/common.md to be orphaned, got: %v", orphaned) + } + + // Note: We can't easily test the actual RemoveWorkflows function with user input + // since it requires interactive confirmation. The logic is tested through + // previewOrphanedIncludes and the flag handling is straightforward. +}