From 2da03cf6d39533a90e8b83e57dc230b5e79f20c1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Feb 2026 17:11:36 +0000 Subject: [PATCH 1/2] Initial plan From 9d431564a177636cebff4780617cf23d440852a9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Feb 2026 17:25:43 +0000 Subject: [PATCH 2/2] refactor: remove deprecated wrappers, dead code and unused symbols - Remove formatCompilationSummary, formatActionlintOutput, formatStatsTable thin wrappers from compile_output_formatter.go; update 3 call-sites in compile_orchestration.go to call underlying functions directly - Extract runBatchLockFileTool shared helper in compile_batch_operations.go; refactor runBatchActionlint and runBatchZizmor to use it - Remove deprecated GenerateOutputSchema[T]() from mcp_schema.go; update 3 test files to use GenerateSchema directly - Remove WorkflowSourceInfo type alias from packages.go (no callers) - Remove ShouldSkipRuntimeSetup from runtime_deduplication.go (always returns false) and its pointless test - Migrate 3 SplitRepoSlug call-sites to repoutil.SplitRepoSlug directly; remove cli.SplitRepoSlug wrapper and unused repoutil import from repo.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/compile_batch_operations.go | 40 +++++++------------ pkg/cli/compile_orchestration.go | 6 +-- pkg/cli/compile_output_formatter.go | 22 ----------- pkg/cli/mcp_schema.go | 6 --- pkg/cli/mcp_schema_test.go | 56 +++++++++++++-------------- pkg/cli/mcp_server_defaults_test.go | 6 +-- pkg/cli/mcp_tool_schemas_test.go | 6 +-- pkg/cli/packages.go | 4 -- pkg/cli/pr_command.go | 5 ++- pkg/cli/repo.go | 8 ---- pkg/cli/secret_set_command.go | 3 +- pkg/workflow/runtime_deduplication.go | 7 ---- pkg/workflow/runtime_setup_test.go | 40 ------------------- 13 files changed, 56 insertions(+), 153 deletions(-) diff --git a/pkg/cli/compile_batch_operations.go b/pkg/cli/compile_batch_operations.go index b4841503cc..3977625f31 100644 --- a/pkg/cli/compile_batch_operations.go +++ b/pkg/cli/compile_batch_operations.go @@ -37,48 +37,36 @@ import ( var compileBatchOperationsLog = logger.New("cli:compile_batch_operations") -// runBatchActionlint runs actionlint on all lock files in batch -func runBatchActionlint(lockFiles []string, verbose bool, strict bool) error { +// runBatchLockFileTool runs a batch tool on lock files with uniform error handling +func runBatchLockFileTool(toolName string, lockFiles []string, verbose bool, strict bool, runner func([]string, bool, bool) error) error { if len(lockFiles) == 0 { - compileBatchOperationsLog.Print("No lock files to lint with actionlint") + compileBatchOperationsLog.Printf("No lock files to process with %s", toolName) return nil } - compileBatchOperationsLog.Printf("Running batch actionlint on %d lock files", len(lockFiles)) + compileBatchOperationsLog.Printf("Running batch %s on %d lock files", toolName, len(lockFiles)) - if err := RunActionlintOnFiles(lockFiles, verbose, strict); err != nil { + if err := runner(lockFiles, verbose, strict); err != nil { if strict { - return fmt.Errorf("actionlint linter failed: %w", err) + return fmt.Errorf("%s failed: %w", toolName, err) } - // In non-strict mode, actionlint errors are warnings + // In non-strict mode, errors are warnings if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("actionlint warnings: %v", err))) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("%s warnings: %v", toolName, err))) } } return nil } +// runBatchActionlint runs actionlint on all lock files in batch +func runBatchActionlint(lockFiles []string, verbose bool, strict bool) error { + return runBatchLockFileTool("actionlint", lockFiles, verbose, strict, RunActionlintOnFiles) +} + // runBatchZizmor runs zizmor security scanner on all lock files in batch func runBatchZizmor(lockFiles []string, verbose bool, strict bool) error { - if len(lockFiles) == 0 { - compileBatchOperationsLog.Print("No lock files to scan with zizmor") - return nil - } - - compileBatchOperationsLog.Printf("Running batch zizmor on %d lock files", len(lockFiles)) - - if err := RunZizmorOnFiles(lockFiles, verbose, strict); err != nil { - if strict { - return fmt.Errorf("zizmor security scan failed: %w", err) - } - // In non-strict mode, zizmor errors are warnings - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("zizmor warnings: %v", err))) - } - } - - return nil + return runBatchLockFileTool("zizmor", lockFiles, verbose, strict, RunZizmorOnFiles) } // runBatchPoutine runs poutine security scanner once for the entire directory diff --git a/pkg/cli/compile_orchestration.go b/pkg/cli/compile_orchestration.go index 549537bdcf..6b03496d0d 100644 --- a/pkg/cli/compile_orchestration.go +++ b/pkg/cli/compile_orchestration.go @@ -489,7 +489,7 @@ func outputResults( if len(config.MarkdownFiles) > 0 { statsList = collectWorkflowStatisticsWrapper(config.MarkdownFiles) } - formatStatsTable(statsList) + displayStatsTable(statsList) } // Output JSON if requested @@ -501,12 +501,12 @@ func outputResults( fmt.Println(jsonStr) } else if !config.Stats { // Print summary for text output (skip if stats mode) - formatCompilationSummary(stats) + printCompilationSummary(stats) } // Display actionlint summary if enabled if config.Actionlint && !config.NoEmit && !config.JSONOutput { - formatActionlintOutput() + displayActionlintSummary() } return nil diff --git a/pkg/cli/compile_output_formatter.go b/pkg/cli/compile_output_formatter.go index 9086a53d65..911cfc8d98 100644 --- a/pkg/cli/compile_output_formatter.go +++ b/pkg/cli/compile_output_formatter.go @@ -14,11 +14,7 @@ // # Key Functions // // Summary Output: -// - formatCompilationSummary() - Format compilation statistics // - formatValidationOutput() - Format validation results as JSON -// -// These functions abstract output formatting, allowing the main compile -// orchestrator to focus on coordination while these handle presentation. package cli @@ -31,12 +27,6 @@ import ( var compileOutputFormatterLog = logger.New("cli:compile_output_formatter") -// formatCompilationSummary formats compilation statistics for display -// This is a wrapper around printCompilationSummary for consistency -func formatCompilationSummary(stats *CompilationStats) { - printCompilationSummary(stats) -} - // formatValidationOutput formats validation results as JSON func formatValidationOutput(results []ValidationResult) (string, error) { compileOutputFormatterLog.Printf("Formatting validation output for %d workflow(s)", len(results)) @@ -52,15 +42,3 @@ func formatValidationOutput(results []ValidationResult) (string, error) { return string(jsonBytes), nil } - -// formatActionlintOutput displays the actionlint summary -// This is a wrapper around displayActionlintSummary for consistency -func formatActionlintOutput() { - displayActionlintSummary() -} - -// formatStatsTable displays the workflow statistics table -// This is a wrapper around displayStatsTable for consistency -func formatStatsTable(statsList []*WorkflowStats) { - displayStatsTable(statsList) -} diff --git a/pkg/cli/mcp_schema.go b/pkg/cli/mcp_schema.go index 5763cd505c..5def4b2de9 100644 --- a/pkg/cli/mcp_schema.go +++ b/pkg/cli/mcp_schema.go @@ -35,12 +35,6 @@ func GenerateSchema[T any]() (*jsonschema.Schema, error) { return jsonschema.For[T](nil) } -// GenerateOutputSchema is deprecated. Use GenerateSchema instead. -// This function is kept for backward compatibility but will be removed in a future version. -func GenerateOutputSchema[T any]() (*jsonschema.Schema, error) { - return GenerateSchema[T]() -} - // AddSchemaDefault adds a default value to a property in a JSON schema. // This is useful for elicitation defaults (SEP-1024) that improve UX by // suggesting sensible starting values to MCP clients. diff --git a/pkg/cli/mcp_schema_test.go b/pkg/cli/mcp_schema_test.go index 3aaba23460..8abb8427c4 100644 --- a/pkg/cli/mcp_schema_test.go +++ b/pkg/cli/mcp_schema_test.go @@ -11,16 +11,16 @@ import ( "github.com/google/jsonschema-go/jsonschema" ) -func TestGenerateOutputSchema(t *testing.T) { +func TestGenerateSchema(t *testing.T) { t.Run("generates schema for simple struct", func(t *testing.T) { type SimpleOutput struct { Name string `json:"name" jsonschema:"Name of the item"` Count int `json:"count" jsonschema:"Number of items"` } - schema, err := GenerateOutputSchema[SimpleOutput]() + schema, err := GenerateSchema[SimpleOutput]() if err != nil { - t.Fatalf("GenerateOutputSchema failed: %v", err) + t.Fatalf("GenerateSchema failed: %v", err) } if schema == nil { @@ -54,9 +54,9 @@ func TestGenerateOutputSchema(t *testing.T) { Optional *string `json:"optional,omitempty" jsonschema:"Optional field"` } - schema, err := GenerateOutputSchema[OutputWithOptional]() + schema, err := GenerateSchema[OutputWithOptional]() if err != nil { - t.Fatalf("GenerateOutputSchema failed: %v", err) + t.Fatalf("GenerateSchema failed: %v", err) } if schema == nil { @@ -87,9 +87,9 @@ func TestGenerateOutputSchema(t *testing.T) { Nested NestedData `json:"nested" jsonschema:"Nested data"` } - schema, err := GenerateOutputSchema[OutputWithNested]() + schema, err := GenerateSchema[OutputWithNested]() if err != nil { - t.Fatalf("GenerateOutputSchema failed: %v", err) + t.Fatalf("GenerateSchema failed: %v", err) } if schema == nil { @@ -122,9 +122,9 @@ func TestGenerateOutputSchema(t *testing.T) { Items []string `json:"items" jsonschema:"List of items"` } - schema, err := GenerateOutputSchema[OutputWithSlice]() + schema, err := GenerateSchema[OutputWithSlice]() if err != nil { - t.Fatalf("GenerateOutputSchema failed: %v", err) + t.Fatalf("GenerateSchema failed: %v", err) } if schema == nil { @@ -157,9 +157,9 @@ func TestGenerateOutputSchema(t *testing.T) { }) t.Run("generates schema for WorkflowStatus", func(t *testing.T) { - schema, err := GenerateOutputSchema[WorkflowStatus]() + schema, err := GenerateSchema[WorkflowStatus]() if err != nil { - t.Fatalf("GenerateOutputSchema failed for WorkflowStatus: %v", err) + t.Fatalf("GenerateSchema failed for WorkflowStatus: %v", err) } if schema == nil { @@ -176,9 +176,9 @@ func TestGenerateOutputSchema(t *testing.T) { }) t.Run("generates schema for LogsData", func(t *testing.T) { - schema, err := GenerateOutputSchema[LogsData]() + schema, err := GenerateSchema[LogsData]() if err != nil { - t.Fatalf("GenerateOutputSchema failed for LogsData: %v", err) + t.Fatalf("GenerateSchema failed for LogsData: %v", err) } if schema == nil { @@ -195,9 +195,9 @@ func TestGenerateOutputSchema(t *testing.T) { }) t.Run("generates schema for AuditData", func(t *testing.T) { - schema, err := GenerateOutputSchema[AuditData]() + schema, err := GenerateSchema[AuditData]() if err != nil { - t.Fatalf("GenerateOutputSchema failed for AuditData: %v", err) + t.Fatalf("GenerateSchema failed for AuditData: %v", err) } if schema == nil { @@ -221,9 +221,9 @@ func TestAddSchemaDefault(t *testing.T) { Count int `json:"count" jsonschema:"Count field"` } - schema, err := GenerateOutputSchema[TestStruct]() + schema, err := GenerateSchema[TestStruct]() if err != nil { - t.Fatalf("GenerateOutputSchema failed: %v", err) + t.Fatalf("GenerateSchema failed: %v", err) } // Add defaults @@ -263,9 +263,9 @@ func TestAddSchemaDefault(t *testing.T) { Name string `json:"name" jsonschema:"Name field"` } - schema, err := GenerateOutputSchema[TestStruct]() + schema, err := GenerateSchema[TestStruct]() if err != nil { - t.Fatalf("GenerateOutputSchema failed: %v", err) + t.Fatalf("GenerateSchema failed: %v", err) } // Try to add default to non-existent property - should not error @@ -282,7 +282,7 @@ func TestAddSchemaDefault(t *testing.T) { }) } -func TestGenerateOutputSchemaWithDefaults(t *testing.T) { +func TestGenerateSchemaWithDefaults(t *testing.T) { t.Run("manually adds default values to schema", func(t *testing.T) { type OutputWithDefaults struct { Name string `json:"name" jsonschema:"Name of the item"` @@ -290,9 +290,9 @@ func TestGenerateOutputSchemaWithDefaults(t *testing.T) { Enabled bool `json:"enabled" jsonschema:"Whether enabled"` } - schema, err := GenerateOutputSchema[OutputWithDefaults]() + schema, err := GenerateSchema[OutputWithDefaults]() if err != nil { - t.Fatalf("GenerateOutputSchema failed: %v", err) + t.Fatalf("GenerateSchema failed: %v", err) } if schema == nil { @@ -363,9 +363,9 @@ func TestGenerateOutputSchemaWithDefaults(t *testing.T) { func TestGeneratedSchemasValidateRealOutput(t *testing.T) { t.Run("validates LogsData schema against real data", func(t *testing.T) { // Generate schema for LogsData - schema, err := GenerateOutputSchema[LogsData]() + schema, err := GenerateSchema[LogsData]() if err != nil { - t.Fatalf("GenerateOutputSchema failed: %v", err) + t.Fatalf("GenerateSchema failed: %v", err) } // Resolve the schema to prepare it for validation @@ -420,9 +420,9 @@ func TestGeneratedSchemasValidateRealOutput(t *testing.T) { t.Run("validates AuditData schema against real data", func(t *testing.T) { // Generate schema for AuditData - schema, err := GenerateOutputSchema[AuditData]() + schema, err := GenerateSchema[AuditData]() if err != nil { - t.Fatalf("GenerateOutputSchema failed: %v", err) + t.Fatalf("GenerateSchema failed: %v", err) } // Resolve the schema to prepare it for validation @@ -476,9 +476,9 @@ func TestGeneratedSchemasValidateRealOutput(t *testing.T) { t.Run("validates WorkflowStatus schema against real data", func(t *testing.T) { // Generate schema for WorkflowStatus - schema, err := GenerateOutputSchema[WorkflowStatus]() + schema, err := GenerateSchema[WorkflowStatus]() if err != nil { - t.Fatalf("GenerateOutputSchema failed: %v", err) + t.Fatalf("GenerateSchema failed: %v", err) } // Resolve the schema to prepare it for validation diff --git a/pkg/cli/mcp_server_defaults_test.go b/pkg/cli/mcp_server_defaults_test.go index 2446838953..4f609c0c80 100644 --- a/pkg/cli/mcp_server_defaults_test.go +++ b/pkg/cli/mcp_server_defaults_test.go @@ -22,7 +22,7 @@ func TestMCPToolElicitationDefaults(t *testing.T) { Fix bool `json:"fix,omitempty" jsonschema:"Apply automatic codemod fixes to workflows before compiling"` } - schema, err := GenerateOutputSchema[compileArgs]() + schema, err := GenerateSchema[compileArgs]() if err != nil { t.Fatalf("Failed to generate schema: %v", err) } @@ -60,7 +60,7 @@ func TestMCPToolElicitationDefaults(t *testing.T) { MaxTokens int `json:"max_tokens,omitempty" jsonschema:"Maximum number of tokens in output before triggering guardrail"` } - schema, err := GenerateOutputSchema[logsArgs]() + schema, err := GenerateSchema[logsArgs]() if err != nil { t.Fatalf("Failed to generate schema: %v", err) } @@ -131,7 +131,7 @@ func TestMCPToolElicitationDefaults(t *testing.T) { Count int `json:"count" jsonschema:"Count field"` } - schema, err := GenerateOutputSchema[testArgs]() + schema, err := GenerateSchema[testArgs]() if err != nil { t.Fatalf("Failed to generate schema: %v", err) } diff --git a/pkg/cli/mcp_tool_schemas_test.go b/pkg/cli/mcp_tool_schemas_test.go index 0c351b642b..7136f7ef7e 100644 --- a/pkg/cli/mcp_tool_schemas_test.go +++ b/pkg/cli/mcp_tool_schemas_test.go @@ -14,7 +14,7 @@ func TestMCPToolOutputSchemas(t *testing.T) { t.Run("logs schema can be generated (for future use)", func(t *testing.T) { // The logs tool currently doesn't use output schemas, but we verify // the helper can generate them for when they're needed in the future - schema, err := GenerateOutputSchema[LogsData]() + schema, err := GenerateSchema[LogsData]() if err != nil { t.Fatalf("Failed to generate schema for LogsData: %v", err) } @@ -52,7 +52,7 @@ func TestMCPToolOutputSchemas(t *testing.T) { t.Run("audit schema can be generated (for future use)", func(t *testing.T) { // The audit tool currently doesn't use output schemas (output can be filtered with jq), // but we verify the helper can generate them for when they're needed in the future - schema, err := GenerateOutputSchema[AuditData]() + schema, err := GenerateSchema[AuditData]() if err != nil { t.Fatalf("Failed to generate schema for AuditData: %v", err) } @@ -90,7 +90,7 @@ func TestMCPToolOutputSchemas(t *testing.T) { t.Run("status tool array schema can be generated", func(t *testing.T) { // Even though status tool doesn't use the schema (MCP requires objects), // verify the helper can generate a schema for the array type - schema, err := GenerateOutputSchema[[]WorkflowStatus]() + schema, err := GenerateSchema[[]WorkflowStatus]() if err != nil { t.Fatalf("Failed to generate schema for []WorkflowStatus: %v", err) } diff --git a/pkg/cli/packages.go b/pkg/cli/packages.go index c825fea32b..5fd44638c4 100644 --- a/pkg/cli/packages.go +++ b/pkg/cli/packages.go @@ -20,10 +20,6 @@ var ( includePattern = regexp.MustCompile(`^@include(\?)?\s+(.+)$`) ) -// 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. // diff --git a/pkg/cli/pr_command.go b/pkg/cli/pr_command.go index c941ea2f0f..0d1695a67f 100644 --- a/pkg/cli/pr_command.go +++ b/pkg/cli/pr_command.go @@ -14,6 +14,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/repoutil" "github.com/github/gh-aw/pkg/workflow" "github.com/spf13/cobra" ) @@ -576,7 +577,7 @@ func transferPR(prURL, targetRepo string, verbose bool) error { if err != nil { return fmt.Errorf("failed to determine target repository: %w", err) } - targetOwner, targetRepoName, err = SplitRepoSlug(slug) + targetOwner, targetRepoName, err = repoutil.SplitRepoSlug(slug) if err != nil { return fmt.Errorf("failed to parse target repository: %w", err) } @@ -603,7 +604,7 @@ func transferPR(prURL, targetRepo string, verbose bool) error { if isGitRepo() { slug, err := GetCurrentRepoSlug() if err == nil { - currentOwner, currentRepoName, err := SplitRepoSlug(slug) + currentOwner, currentRepoName, err := repoutil.SplitRepoSlug(slug) if err == nil && currentOwner == targetOwner && currentRepoName == targetRepoName { // We're already in the target repo workingDir = "." diff --git a/pkg/cli/repo.go b/pkg/cli/repo.go index 82e81153a5..eb378bdd22 100644 --- a/pkg/cli/repo.go +++ b/pkg/cli/repo.go @@ -7,7 +7,6 @@ import ( "sync" "github.com/github/gh-aw/pkg/logger" - "github.com/github/gh-aw/pkg/repoutil" "github.com/github/gh-aw/pkg/workflow" ) @@ -118,10 +117,3 @@ func GetCurrentRepoSlug() (string, error) { repoLog.Printf("Using cached repository slug: %s", result) return result, nil } - -// SplitRepoSlug wraps repoutil.SplitRepoSlug for backward compatibility. -// It splits a repository slug (owner/repo) into owner and repo parts. -// New code should use repoutil.SplitRepoSlug directly. -func SplitRepoSlug(slug string) (owner, repo string, err error) { - return repoutil.SplitRepoSlug(slug) -} diff --git a/pkg/cli/secret_set_command.go b/pkg/cli/secret_set_command.go index 787e83241f..fa995b6621 100644 --- a/pkg/cli/secret_set_command.go +++ b/pkg/cli/secret_set_command.go @@ -13,6 +13,7 @@ import ( "github.com/cli/go-gh/v2/pkg/api" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/repoutil" "github.com/github/gh-aw/pkg/tty" "github.com/spf13/cobra" "golang.org/x/crypto/nacl/box" @@ -83,7 +84,7 @@ Examples: secretSetLog.Printf("Using current repository: %s", repoSlug) } - owner, repo, splitErr := SplitRepoSlug(repoSlug) + owner, repo, splitErr := repoutil.SplitRepoSlug(repoSlug) if splitErr != nil { return fmt.Errorf("invalid repository slug %q: %w", repoSlug, splitErr) } diff --git a/pkg/workflow/runtime_deduplication.go b/pkg/workflow/runtime_deduplication.go index 35d74ebd1f..8b34ea3ea0 100644 --- a/pkg/workflow/runtime_deduplication.go +++ b/pkg/workflow/runtime_deduplication.go @@ -232,10 +232,3 @@ func DeduplicateRuntimeSetupStepsFromCustomSteps(customSteps string, runtimeRequ return deduplicatedStr, filteredRequirements, nil } - -// ShouldSkipRuntimeSetup checks if we should skip automatic runtime setup -// Deprecated: Runtime detection now smartly filters out existing runtimes instead of skipping entirely -// This function now always returns false for backward compatibility -func ShouldSkipRuntimeSetup(workflowData *WorkflowData) bool { - return false -} diff --git a/pkg/workflow/runtime_setup_test.go b/pkg/workflow/runtime_setup_test.go index e8da50075a..1cbd7f9535 100644 --- a/pkg/workflow/runtime_setup_test.go +++ b/pkg/workflow/runtime_setup_test.go @@ -455,46 +455,6 @@ func TestGenerateRuntimeSetupSteps(t *testing.T) { } } -func TestShouldSkipRuntimeSetup(t *testing.T) { - tests := []struct { - name string - data *WorkflowData - expected bool - }{ - { - name: "never skip - runtime filtering handles existing setup actions", - data: &WorkflowData{ - CustomSteps: `steps: - - uses: actions/setup-node@395ad3262231945c25e8478fd5baf05154b1d79f - - run: npm install`, - }, - expected: false, // Changed: we no longer skip, we filter instead - }, - { - name: "never skip when no setup actions", - data: &WorkflowData{ - CustomSteps: `steps: - - run: npm install`, - }, - expected: false, - }, - { - name: "never skip when no custom steps", - data: &WorkflowData{}, - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := ShouldSkipRuntimeSetup(tt.data) - if result != tt.expected { - t.Errorf("Expected %v, got %v", tt.expected, result) - } - }) - } -} - // Helper functions func getRequirementIDs(requirements map[string]*RuntimeRequirement) []string {