diff --git a/pkg/actionpins/actionpins_internal_test.go b/pkg/actionpins/actionpins_internal_test.go index a483b1235d5..21cf09295a9 100644 --- a/pkg/actionpins/actionpins_internal_test.go +++ b/pkg/actionpins/actionpins_internal_test.go @@ -172,7 +172,8 @@ func TestFormatPinnedActionReference_PanicsWhenSHAIsEmpty(t *testing.T) { } func TestPinContextEmitOnce_InitializesAndDeduplicates(t *testing.T) { - t.Parallel() + // Not parallel: CaptureStderr swaps the global os.Stderr and is not safe + // to run concurrently with other tests that write to stderr. ctx := &PinContext{} @@ -953,7 +954,8 @@ func TestApplyContainerPinMapping(t *testing.T) { } func TestApplyContainerPinMapping_DeduplicatesInvalidWarnings(t *testing.T) { - t.Parallel() + // Not parallel: CaptureStderr swaps the global os.Stderr and is not safe + // to run concurrently with other tests that write to stderr. ctx := &PinContext{ ContainerMappings: map[string]string{ diff --git a/pkg/cli/add_package_manifest.go b/pkg/cli/add_package_manifest.go index 719a136b669..a31e3ec6199 100644 --- a/pkg/cli/add_package_manifest.go +++ b/pkg/cli/add_package_manifest.go @@ -11,6 +11,7 @@ import ( "github.com/goccy/go-yaml" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/errorutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/semverutil" @@ -1005,20 +1006,12 @@ func listRepositoryPackageDirSubdirsForHost(ctx context.Context, owner, repo, re } func normalizeRepositoryPackageRemoteError(err error) error { - if err == nil || !isRepositoryPackageRemoteNotFound(err) { + if err == nil || !errorutil.IsNotFoundError(err) { return err } return packageRemoteNotFoundError{cause: err} } -func isRepositoryPackageRemoteNotFound(err error) bool { - if err == nil { - return false - } - errText := strings.ToLower(err.Error()) - return strings.Contains(errText, "404") || strings.Contains(errText, "not found") -} - func resolveRepositoryPackageDefaultBranch(ctx context.Context, repoSlug, host string) (string, error) { args := []string{"api", "/repos/" + repoSlug, "--jq", ".default_branch"} var output []byte diff --git a/pkg/cli/branch_file_reader.go b/pkg/cli/branch_file_reader.go index 7bbf8018002..b26698d5793 100644 --- a/pkg/cli/branch_file_reader.go +++ b/pkg/cli/branch_file_reader.go @@ -8,6 +8,7 @@ import ( "os" "strings" + "github.com/github/gh-aw/pkg/errorutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) @@ -33,7 +34,7 @@ func readRemoteRepoBranchFileContext(ctx context.Context, repoOverride, branchNa cmd := workflow.ExecGHContext(ctx, args...) out, err := cmd.CombinedOutput() if err != nil { - if isRemoteFileNotFoundOutput(string(out)) { + if errorutil.IsNotFoundOutput(string(out)) { branchFileReaderLog.Printf("Remote file not found: path=%s, branch=%s", filePath, branchName) return nil, os.ErrNotExist } @@ -55,8 +56,3 @@ func readRemoteRepoBranchFileContext(ctx context.Context, repoOverride, branchNa func isRemoteFileNotFound(err error) bool { return errors.Is(err, os.ErrNotExist) } - -func isRemoteFileNotFoundOutput(output string) bool { - s := strings.ToLower(output) - return strings.Contains(s, "404") || strings.Contains(s, "not found") -} diff --git a/pkg/cli/checks_command.go b/pkg/cli/checks_command.go index 07056a1eebc..ab729e9fa8c 100644 --- a/pkg/cli/checks_command.go +++ b/pkg/cli/checks_command.go @@ -10,6 +10,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/errorutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/sliceutil" "github.com/github/gh-aw/pkg/workflow" @@ -240,7 +241,7 @@ func classifyGHAPIError(exitCode int, stderr string, prNumber string, repo strin lower := strings.ToLower(stderr) switch { - case strings.Contains(lower, "404") || strings.Contains(lower, "not found"): + case errorutil.IsNotFoundOutput(stderr): repoHint := "the current repository" if repo != "" { repoHint = repo diff --git a/pkg/cli/evals_branch.go b/pkg/cli/evals_branch.go index 13f4a45466b..bee2bef3404 100644 --- a/pkg/cli/evals_branch.go +++ b/pkg/cli/evals_branch.go @@ -11,6 +11,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/errorutil" "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/stringutil" "github.com/github/gh-aw/pkg/workflow" @@ -141,7 +142,7 @@ func resolveRunStateBranchRef(ctx context.Context, repoOverride, branchName stri cmd := workflow.ExecGHContext(ctx, args...) out, err := cmd.CombinedOutput() if err != nil { - if isRemoteFileNotFoundOutput(string(out)) { + if errorutil.IsNotFoundOutput(string(out)) { return "", os.ErrNotExist } return "", fmt.Errorf("failed to list commits for state branch %s: %w", branchName, err) diff --git a/pkg/cli/experiments_command.go b/pkg/cli/experiments_command.go index c03299d92c6..10f45d90724 100644 --- a/pkg/cli/experiments_command.go +++ b/pkg/cli/experiments_command.go @@ -17,6 +17,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/errorutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/setutil" @@ -715,7 +716,7 @@ func fetchRemoteExperimentDetails(repoOverride, branchName, workflowID string) ( var exitErr *exec.ExitError if errors.As(err, &exitErr) { stderr := strings.TrimSpace(string(exitErr.Stderr)) - if strings.Contains(stderr, "404") || strings.Contains(stderr, "not found") { + if errorutil.IsNotFoundOutput(stderr) { return nil, fmt.Errorf("experiment %q not found in %s", workflowID, repoOverride) } return nil, fmt.Errorf("failed to fetch experiment branch (exit %d): %s", exitErr.ExitCode(), stderr) diff --git a/pkg/cli/experiments_command_test.go b/pkg/cli/experiments_command_test.go index d5c9976ba5a..e145bdbb0ca 100644 --- a/pkg/cli/experiments_command_test.go +++ b/pkg/cli/experiments_command_test.go @@ -4,12 +4,25 @@ package cli import ( "encoding/json" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestFetchRemoteExperimentDetailsClassifiesTitleCaseNotFound(t *testing.T) { + fakeBinDir := t.TempDir() + fakeGH := filepath.Join(fakeBinDir, "gh") + require.NoError(t, os.WriteFile(fakeGH, []byte("#!/bin/sh\necho 'HTTP 404: Not Found' >&2\nexit 1\n"), 0o755)) + t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + _, err := fetchRemoteExperimentDetails("octo/repo", "experiments/missing", "missing") + + require.EqualError(t, err, `experiment "missing" not found in octo/repo`) +} + func TestExtractExperimentName(t *testing.T) { tests := []struct { name string diff --git a/pkg/cli/logs_download.go b/pkg/cli/logs_download.go index 4e07ecbc1f7..2df2b61379c 100644 --- a/pkg/cli/logs_download.go +++ b/pkg/cli/logs_download.go @@ -101,7 +101,7 @@ func downloadWorkflowRunLogs(ctx context.Context, runID int64, outputDir string, // Check both the Go error (via errorutil.IsNotFoundError) and the raw CLI output, // as the gh CLI may write "not found" to stdout without reflecting it in the error. // Also treat HTTP 410 Gone as non-critical (logs may be expired). - if errorutil.IsNotFoundError(err) || errorutil.IsNotFoundError(errors.New(string(output))) || errorutil.IsGoneError(err) { + if errorutil.IsNotFoundError(err) || errorutil.IsNotFoundOutput(string(output)) || errorutil.IsGoneError(err) { if verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("No logs found for run %d (may be expired or unavailable)", runID))) } diff --git a/pkg/cli/setup_repository.go b/pkg/cli/setup_repository.go index 14f19d4c9c5..01bf149785b 100644 --- a/pkg/cli/setup_repository.go +++ b/pkg/cli/setup_repository.go @@ -11,6 +11,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/ctxutil" + "github.com/github/gh-aw/pkg/errorutil" "github.com/github/gh-aw/pkg/gitutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" @@ -104,8 +105,9 @@ func checkSetupRepositoryExists(ctx context.Context, repo string) (bool, error) return strings.TrimSpace(string(output)) != "", nil } - message := strings.ToLower(string(output)) - if strings.Contains(message, "could not resolve to a repository") || strings.Contains(message, "http 404") || strings.Contains(message, "not found") { + outputStr := string(output) + message := strings.ToLower(outputStr) + if strings.Contains(message, "could not resolve to a repository") || errorutil.IsNotFoundOutput(outputStr) { return false, nil } return false, fmt.Errorf("failed to check repository %s: %w", repo, err) diff --git a/pkg/cli/workflow_run_metadata.go b/pkg/cli/workflow_run_metadata.go index 530164040b6..86f8e20a06c 100644 --- a/pkg/cli/workflow_run_metadata.go +++ b/pkg/cli/workflow_run_metadata.go @@ -3,7 +3,6 @@ package cli import ( "context" "encoding/json" - "errors" "fmt" "os" "path/filepath" @@ -53,7 +52,7 @@ func buildWorkflowRunMetadataArgs(runID int64, owner, repo, hostname string) []s func classifyWorkflowRunMetadataError(runID int64, err error, output []byte) error { outputStr := string(output) if errorutil.IsNotFoundError(err) || - errorutil.IsNotFoundError(errors.New(outputStr)) || + errorutil.IsNotFoundOutput(outputStr) || strings.Contains(outputStr, "Could not resolve") { return fmt.Errorf("workflow run %d not found. Please verify the run ID is correct and that you have access to the repository", runID) } diff --git a/pkg/errorutil/errors.go b/pkg/errorutil/errors.go index afc34f5d95c..dd98130138b 100644 --- a/pkg/errorutil/errors.go +++ b/pkg/errorutil/errors.go @@ -23,6 +23,17 @@ func IsNotFoundError(err error) bool { return matched } +// IsNotFoundOutput reports whether output represents an HTTP 404 / "not found" response. +// The check is case-insensitive and matches both the numeric literal "404" and +// the phrase "not found". +func IsNotFoundOutput(output string) bool { + matched := containsSubstring(output, "404", "not found") + if matched { + errorutilLog.Printf("Classified output as not-found (404): %s", output) + } + return matched +} + // IsForbiddenError reports whether err represents an HTTP 403 / "forbidden" response. // It returns false when err is nil. // The check is case-insensitive and only matches HTTP-style 403 patterns such as @@ -56,7 +67,11 @@ func containsErrorSubstring(err error, substrings ...string) bool { if err == nil { return false } - msg := strings.ToLower(err.Error()) + return containsSubstring(err.Error(), substrings...) +} + +func containsSubstring(value string, substrings ...string) bool { + msg := strings.ToLower(value) for _, substring := range substrings { if strings.Contains(msg, substring) { return true diff --git a/pkg/errorutil/errors_test.go b/pkg/errorutil/errors_test.go index 03ccab5465f..193028e9841 100644 --- a/pkg/errorutil/errors_test.go +++ b/pkg/errorutil/errors_test.go @@ -45,6 +45,26 @@ func TestIsNotFoundError(t *testing.T) { } } +func TestIsNotFoundOutput(t *testing.T) { + tests := []struct { + name string + output string + want bool + }{ + {name: "empty output", output: "", want: false}, + {name: "404 numeric literal", output: "HTTP 404: Not Found", want: true}, + {name: "title case not found", output: "GraphQL: Not Found", want: true}, + {name: "uppercase not found", output: "RESOURCE NOT FOUND", want: true}, + {name: "generic output", output: "something went wrong", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, errorutil.IsNotFoundOutput(tt.output)) + }) + } +} + func TestIsForbiddenError(t *testing.T) { tests := []struct { name string