diff --git a/pkg/cli/codemod_engine_env_secrets.go b/pkg/cli/codemod_engine_env_secrets.go index be164f683e7..e18463de0e9 100644 --- a/pkg/cli/codemod_engine_env_secrets.go +++ b/pkg/cli/codemod_engine_env_secrets.go @@ -263,6 +263,7 @@ func getTopLevelEnvSecretsGuidedErrorCodemod() Codemod { Name: "Detect secrets in top-level env section (manual fix required)", Description: "Detects secrets in the top-level env: block that will be leaked to the agent container in strict mode, and emits a guided error pointing users to engine-specific secret configuration.", IntroducedIn: "1.5.0", + Guided: true, Apply: func(content string, frontmatter map[string]any) (string, bool, error) { envValue, hasEnv := frontmatter["env"] if !hasEnv { diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index 7225d34c9e9..15914c14d06 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -16,9 +16,26 @@ type Codemod struct { Name string // Human-readable name Description string // Description of what the codemod does IntroducedIn string // Version where this codemod was introduced + Guided bool // If true, errors from Apply are guided/manual-fix errors (not auto-correctable) Apply func(content string, frontmatter map[string]any) (string, bool, error) } +// GuidedError is returned when a codemod with Guided: true emits an error. +// Unlike regular processing errors, a guided error signals that the file was +// read successfully but requires a human to manually address an issue that +// cannot be auto-corrected by any codemod. +type GuidedError struct { + Cause error +} + +func (e *GuidedError) Error() string { + return e.Cause.Error() +} + +func (e *GuidedError) Unwrap() error { + return e.Cause +} + // CodemodResult represents the result of applying a codemod type CodemodResult struct { Applied bool // Whether the codemod was applied diff --git a/pkg/cli/fix_command.go b/pkg/cli/fix_command.go index 7e83abb438c..e6108ae6fca 100644 --- a/pkg/cli/fix_command.go +++ b/pkg/cli/fix_command.go @@ -1,6 +1,7 @@ package cli import ( + "errors" "fmt" "os" "path/filepath" @@ -160,6 +161,8 @@ func runFixCommand(workflowIDs []string, write bool, verbose bool, workflowDir s // Process each file var totalFixed int var totalFiles int + var totalGuidedErrors int + var totalProcessingErrors int var workflowsNeedingFixes []workflowFixInfo for _, file := range files { @@ -168,6 +171,13 @@ func runFixCommand(workflowIDs []string, write bool, verbose bool, workflowDir s fixed, appliedFixes, err := processWorkflowFileWithInfo(file, codemods, write, verbose) if err != nil { fmt.Fprintf(os.Stderr, "%s\n", console.FormatErrorMessage(fmt.Sprintf("Error processing %s: %v", filepath.Base(file), err))) + var guidedErr *GuidedError + if errors.As(err, &guidedErr) { + totalGuidedErrors++ + totalFiles++ + } else { + totalProcessingErrors++ + } continue } @@ -239,7 +249,7 @@ func runFixCommand(workflowIDs []string, write bool, verbose bool, workflowDir s if write { if totalFixed > 0 { fmt.Fprintf(os.Stderr, "%s\n", console.FormatSuccessMessage(fmt.Sprintf("✓ Fixed %d of %d workflow files", totalFixed, totalFiles))) - } else { + } else if totalGuidedErrors == 0 && totalProcessingErrors == 0 { fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage("✓ No fixes needed")) } } else { @@ -257,11 +267,24 @@ func runFixCommand(workflowIDs []string, write bool, verbose bool, workflowDir s for _, wf := range workflowsNeedingFixes { fmt.Fprintf(os.Stderr, " gh aw fix %s --write\n", strings.TrimSuffix(wf.File, ".md")) } - } else { + } else if totalGuidedErrors == 0 && totalProcessingErrors == 0 { fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage("✓ No fixes needed")) } } + if totalGuidedErrors > 0 { + pluralSuffix := "file needs" + if totalGuidedErrors > 1 { + pluralSuffix = "files need" + } + fmt.Fprintf(os.Stderr, "%s\n", console.FormatErrorMessage(fmt.Sprintf("%d %s a manual fix", totalGuidedErrors, pluralSuffix))) + return &ExitCodeError{Code: 2} + } + + if totalProcessingErrors > 0 { + return &ExitCodeError{Code: 1} + } + return nil } @@ -302,7 +325,11 @@ func processWorkflowFileWithInfo(filePath string, codemods []Codemod, write bool newContent, applied, err := codemod.Apply(currentContent, currentResult.Frontmatter) if err != nil { fixLog.Printf("Codemod %s failed: %v", codemod.ID, err) - return false, nil, fmt.Errorf("codemod %s failed: %w", codemod.ID, err) + wrappedErr := fmt.Errorf("codemod %s failed: %w", codemod.ID, err) + if codemod.Guided { + return false, nil, &GuidedError{Cause: wrappedErr} + } + return false, nil, wrappedErr } if applied { diff --git a/pkg/cli/fix_command_test.go b/pkg/cli/fix_command_test.go index 934f136d0b1..b134118ab8f 100644 --- a/pkg/cli/fix_command_test.go +++ b/pkg/cli/fix_command_test.go @@ -1177,3 +1177,228 @@ tools: t.Errorf("Expected scaffolded Serena workflow to pass languages through to upstream import, got:\n%s", scaffolded) } } + +func TestRunFix_GuidedErrorReturnsExitCode2(t *testing.T) { + tmpDir := t.TempDir() + workflowFile := filepath.Join(tmpDir, "byok-workflow.md") + + // Workflow with a secret in top-level env (triggers the guided-error codemod) + content := `--- +on: workflow_dispatch +env: + AZURE_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }} +--- + +# BYOK Workflow +` + require.NoError(t, os.WriteFile(workflowFile, []byte(content), 0644)) + + err := RunFix(FixConfig{ + Write: false, + WorkflowDir: tmpDir, + }) + + require.Error(t, err, "RunFix should return an error when a guided error is triggered") + var exitErr *ExitCodeError + require.ErrorAs(t, err, &exitErr, "error should be an ExitCodeError") + assert.Equal(t, 2, exitErr.Code, "exit code should be 2 for guided errors") +} + +func TestRunFix_GuidedErrorDoesNotPrintNoFixesNeeded(t *testing.T) { + tmpDir := t.TempDir() + workflowFile := filepath.Join(tmpDir, "byok-workflow.md") + + // Workflow with a secret in top-level env (triggers the guided-error codemod) + content := `--- +on: workflow_dispatch +env: + AZURE_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }} +--- + +# BYOK Workflow +` + require.NoError(t, os.WriteFile(workflowFile, []byte(content), 0644)) + + // Capture stderr + origStderr := os.Stderr + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stderr = w + t.Cleanup(func() { os.Stderr = origStderr }) + + _ = RunFix(FixConfig{ + Write: false, + WorkflowDir: tmpDir, + }) + + require.NoError(t, w.Close()) + output, err := io.ReadAll(r) + require.NoError(t, err) + outputStr := string(output) + + assert.NotContains(t, outputStr, "No fixes needed", "should not print 'No fixes needed' when a guided error is present") + assert.Contains(t, outputStr, "1 file needs a manual fix", "should print singular form for a single guided error") +} + +func TestRunFix_GuidedErrorWithMultipleFiles(t *testing.T) { + tmpDir := t.TempDir() + + // File 1: has guided error + content1 := `--- +on: workflow_dispatch +env: + TOKEN: ${{ secrets.MY_TOKEN }} +--- + +# Workflow 1 +` + // File 2: has guided error + content2 := `--- +on: workflow_dispatch +env: + API_KEY: ${{ secrets.API_KEY }} +--- + +# Workflow 2 +` + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "workflow-one.md"), []byte(content1), 0644)) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "workflow-two.md"), []byte(content2), 0644)) + + // Capture stderr + origStderr := os.Stderr + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stderr = w + t.Cleanup(func() { os.Stderr = origStderr }) + + runErr := RunFix(FixConfig{ + Write: false, + WorkflowDir: tmpDir, + }) + + require.NoError(t, w.Close()) + output, err := io.ReadAll(r) + require.NoError(t, err) + outputStr := string(output) + + require.Error(t, runErr) + var exitErr *ExitCodeError + require.ErrorAs(t, runErr, &exitErr) + assert.Equal(t, 2, exitErr.Code) + + assert.Contains(t, outputStr, "2 files need a manual fix", "should report plural count") +} + +func TestProcessWorkflowFileWithInfo_GuidedErrorWrapped(t *testing.T) { + tmpDir := t.TempDir() + workflowFile := filepath.Join(tmpDir, "test.md") + + content := `--- +on: workflow_dispatch +env: + TOKEN: ${{ secrets.MY_TOKEN }} +--- + +# Test +` + require.NoError(t, os.WriteFile(workflowFile, []byte(content), 0644)) + + guidedCodemod := getCodemodByID("top-level-env-secrets-guided-error") + require.NotNil(t, guidedCodemod) + + _, _, err := processWorkflowFileWithInfo(workflowFile, []Codemod{*guidedCodemod}, false, false) + + require.Error(t, err) + var guidedErr *GuidedError + assert.ErrorAs(t, err, &guidedErr, "error should be wrapped as GuidedError") +} + +func TestRunFix_HardProcessingErrorReturnsExitCode1(t *testing.T) { + tmpDir := t.TempDir() + + // Create a directory where a file is expected — reading it will fail + subdir := filepath.Join(tmpDir, "not-a-file.md") + require.NoError(t, os.Mkdir(subdir, 0755)) + + err := RunFix(FixConfig{ + Write: false, + WorkflowDir: tmpDir, + }) + + require.Error(t, err, "RunFix should return an error when a hard processing error occurs") + var exitErr *ExitCodeError + require.ErrorAs(t, err, &exitErr, "error should be an ExitCodeError") + assert.Equal(t, 1, exitErr.Code, "exit code should be 1 for hard processing errors") +} + +func TestRunFix_HardProcessingErrorDoesNotPrintNoFixesNeeded(t *testing.T) { + tmpDir := t.TempDir() + + // Create a directory where a file is expected — reading it will fail + subdir := filepath.Join(tmpDir, "not-a-file.md") + require.NoError(t, os.Mkdir(subdir, 0755)) + + origStderr := os.Stderr + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stderr = w + t.Cleanup(func() { os.Stderr = origStderr }) + + _ = RunFix(FixConfig{ + Write: false, + WorkflowDir: tmpDir, + }) + + require.NoError(t, w.Close()) + output, readErr := io.ReadAll(r) + require.NoError(t, readErr) + + assert.NotContains(t, string(output), "No fixes needed", "should not print 'No fixes needed' when a hard processing error occurred") +} + +func TestRunFix_GuidedErrorCountsInTotalFiles(t *testing.T) { + tmpDir := t.TempDir() + + // File 1: clean (no fixes needed) + clean := `--- +on: workflow_dispatch +--- + +# Clean Workflow +` + // File 2: guided error + guided := `--- +on: workflow_dispatch +env: + TOKEN: ${{ secrets.MY_TOKEN }} +--- + +# BYOK Workflow +` + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "a-clean.md"), []byte(clean), 0644)) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "b-guided.md"), []byte(guided), 0644)) + + origStderr := os.Stderr + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stderr = w + t.Cleanup(func() { os.Stderr = origStderr }) + + runErr := RunFix(FixConfig{ + Write: false, + WorkflowDir: tmpDir, + }) + + require.NoError(t, w.Close()) + output, readErr := io.ReadAll(r) + require.NoError(t, readErr) + + // Should still report guided error count and exit 2 + require.Error(t, runErr) + var exitErr *ExitCodeError + require.ErrorAs(t, runErr, &exitErr) + assert.Equal(t, 2, exitErr.Code) + + // "No fixes needed" must be absent + assert.NotContains(t, string(output), "No fixes needed") +}