From 219d743b8071b787c37b280d6defb5ec8c8aefb4 Mon Sep 17 00:00:00 2001 From: Abhisek Datta Date: Sun, 24 May 2026 12:46:06 +0530 Subject: [PATCH] chore: Standardise Error Codes (#286) * fix: Misc error handling fixes * fix: Sandbox error translation --- cmd/cloud/login.go | 2 +- cmd/cloud/logout.go | 2 +- cmd/cloud/sync.go | 4 +- cmd/sandbox/doctor_test.go | 4 +- cmd/sandbox/explain.go | 4 +- cmd/sandbox/explain_test.go | 2 +- cmd/sandbox/helpers.go | 16 ++++++-- cmd/sandbox/helpers_test.go | 44 ++++++++++++++++++++ cmd/sandbox/profile_diff.go | 2 +- cmd/sandbox/profile_init_test.go | 2 +- cmd/sandbox/profile_lint_test.go | 2 +- cmd/sandbox/profile_list.go | 2 +- cmd/sandbox/profile_list_test.go | 2 +- cmd/sandbox/profile_show.go | 2 +- cmd/sandbox/violations_list.go | 4 +- cmd/sandbox/violations_list_test.go | 2 +- config/config.go | 6 +-- errcodes/codes.go | 15 +++++-- go.mod | 2 +- internal/runner/execute.go | 4 +- internal/ui/error_convert_test.go | 61 +++++++++++++++++++++++++++- packagemanager/errors.go | 15 ++----- sandbox/executor/apply.go | 4 +- sandbox/executor/diagnostics.go | 4 +- sandbox/executor/diagnostics_test.go | 2 +- 25 files changed, 162 insertions(+), 47 deletions(-) create mode 100644 cmd/sandbox/helpers_test.go diff --git a/cmd/cloud/login.go b/cmd/cloud/login.go index 9c3914c..ea4fb1b 100644 --- a/cmd/cloud/login.go +++ b/cmd/cloud/login.go @@ -3,9 +3,9 @@ package cloud import ( "github.com/safedep/dry/cloud" "github.com/safedep/dry/log" - "github.com/safedep/pmg/internal/ui" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + "github.com/safedep/pmg/internal/ui" "github.com/spf13/cobra" ) diff --git a/cmd/cloud/logout.go b/cmd/cloud/logout.go index 11ffe42..c24dab4 100644 --- a/cmd/cloud/logout.go +++ b/cmd/cloud/logout.go @@ -3,9 +3,9 @@ package cloud import ( "github.com/safedep/dry/cloud" "github.com/safedep/dry/log" - "github.com/safedep/pmg/internal/ui" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + "github.com/safedep/pmg/internal/ui" "github.com/spf13/cobra" ) diff --git a/cmd/cloud/sync.go b/cmd/cloud/sync.go index 171b5b1..66bcb90 100644 --- a/cmd/cloud/sync.go +++ b/cmd/cloud/sync.go @@ -5,12 +5,12 @@ import ( "time" "github.com/safedep/dry/log" + "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/config" + "github.com/safedep/pmg/errcodes" "github.com/safedep/pmg/internal/analytics" "github.com/safedep/pmg/internal/audit" "github.com/safedep/pmg/internal/ui" - "github.com/safedep/dry/usefulerror" - "github.com/safedep/pmg/errcodes" "github.com/spf13/cobra" ) diff --git a/cmd/sandbox/doctor_test.go b/cmd/sandbox/doctor_test.go index 3957186..5e90644 100644 --- a/cmd/sandbox/doctor_test.go +++ b/cmd/sandbox/doctor_test.go @@ -10,10 +10,10 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - pmgsandbox "github.com/safedep/pmg/sandbox" - "github.com/safedep/pmg/sandbox/platform" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + pmgsandbox "github.com/safedep/pmg/sandbox" + "github.com/safedep/pmg/sandbox/platform" ) // stubProbe is a minimal probe used by tests. diff --git a/cmd/sandbox/explain.go b/cmd/sandbox/explain.go index c761959..c0f7a94 100644 --- a/cmd/sandbox/explain.go +++ b/cmd/sandbox/explain.go @@ -8,11 +8,11 @@ import ( "strings" "time" + "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/config" + "github.com/safedep/pmg/errcodes" "github.com/safedep/pmg/internal/ui" pmgsandbox "github.com/safedep/pmg/sandbox" - "github.com/safedep/dry/usefulerror" - "github.com/safedep/pmg/errcodes" "github.com/spf13/cobra" ) diff --git a/cmd/sandbox/explain_test.go b/cmd/sandbox/explain_test.go index fea3849..49ac3ba 100644 --- a/cmd/sandbox/explain_test.go +++ b/cmd/sandbox/explain_test.go @@ -12,9 +12,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + pmgsandbox "github.com/safedep/pmg/sandbox" ) func sampleReport() *pmgsandbox.ViolationReport { diff --git a/cmd/sandbox/helpers.go b/cmd/sandbox/helpers.go index 5798c97..07144d4 100644 --- a/cmd/sandbox/helpers.go +++ b/cmd/sandbox/helpers.go @@ -9,10 +9,10 @@ import ( "regexp" "strings" - "github.com/safedep/pmg/internal/ui" - pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + "github.com/safedep/pmg/internal/ui" + pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/spf13/cobra" ) @@ -70,7 +70,7 @@ func wrapUseful(err error, code, help string) error { if err == nil { return nil } - if _, ok := usefulerror.AsUsefulError(err); ok { + if hasUsefulError(err) { return err } return usefulerror.NewUsefulError(). @@ -84,7 +84,7 @@ func profileLoadError(err error) error { if err == nil { return nil } - if _, ok := usefulerror.AsUsefulError(err); ok { + if hasUsefulError(err) { return err } switch { @@ -99,6 +99,14 @@ func profileLoadError(err error) error { "Failed to load the sandbox profile. Run with --verbose for the underlying cause.") } +func hasUsefulError(err error) bool { + // AsUsefulError also runs global converters for plain errors like + // fs.ErrPermission. Contextual wrappers must only skip errors that already + // carry UsefulError details, otherwise generic converters hide command help. + var usefulErr usefulerror.UsefulError + return errors.As(err, &usefulErr) +} + func registryInitError(err error) error { return wrapUseful(err, ioErrorCode(err, errcodes.Unknown), "Failed to initialise the sandbox profile registry. Run with --verbose for details.") diff --git a/cmd/sandbox/helpers_test.go b/cmd/sandbox/helpers_test.go new file mode 100644 index 0000000..a174a43 --- /dev/null +++ b/cmd/sandbox/helpers_test.go @@ -0,0 +1,44 @@ +package sandbox + +import ( + "errors" + "fmt" + "io/fs" + "testing" + + "github.com/safedep/dry/usefulerror" + "github.com/safedep/pmg/errcodes" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestWrapUsefulPreservesContextForConvertibleErrors(t *testing.T) { + help := "Could not create the user profile directory. Check filesystem permissions for /var/root/pmg-repro/sandbox/profiles." + err := wrapUseful( + fmt.Errorf("failed to create user profile directory /var/root/pmg-repro/sandbox/profiles: %w", fs.ErrPermission), + errcodes.PermissionDenied, + help, + ) + + usefulErr, ok := usefulerror.AsUsefulError(err) + require.True(t, ok) + assert.Equal(t, errcodes.PermissionDenied, usefulErr.Code()) + assert.Equal(t, help, usefulErr.Help()) + assert.Contains(t, usefulErr.HumanError(), "failed to create user profile directory") +} + +func TestWrapUsefulLeavesExistingUsefulErrorsUnchanged(t *testing.T) { + original := usefulerror.NewUsefulError(). + WithCode(errcodes.InvalidArgument). + WithHumanError("already classified"). + WithHelp("existing help"). + Wrap(errors.New("root")) + + err := wrapUseful(original, errcodes.PermissionDenied, "new help") + + assert.Same(t, original, err) + usefulErr, ok := usefulerror.AsUsefulError(err) + require.True(t, ok) + assert.Equal(t, errcodes.InvalidArgument, usefulErr.Code()) + assert.Equal(t, "existing help", usefulErr.Help()) +} diff --git a/cmd/sandbox/profile_diff.go b/cmd/sandbox/profile_diff.go index ee89e3b..986b952 100644 --- a/cmd/sandbox/profile_diff.go +++ b/cmd/sandbox/profile_diff.go @@ -7,9 +7,9 @@ import ( "strings" "github.com/pmezard/go-difflib/difflib" + "github.com/safedep/pmg/errcodes" pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/safedep/pmg/sandbox/platform" - "github.com/safedep/pmg/errcodes" "github.com/spf13/cobra" "gopkg.in/yaml.v3" ) diff --git a/cmd/sandbox/profile_init_test.go b/cmd/sandbox/profile_init_test.go index 82d2b8e..6b17177 100644 --- a/cmd/sandbox/profile_init_test.go +++ b/cmd/sandbox/profile_init_test.go @@ -7,9 +7,9 @@ import ( "strings" "testing" - pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/cmd/sandbox/profile_lint_test.go b/cmd/sandbox/profile_lint_test.go index 498a8f3..98a8282 100644 --- a/cmd/sandbox/profile_lint_test.go +++ b/cmd/sandbox/profile_lint_test.go @@ -10,9 +10,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + pmgsandbox "github.com/safedep/pmg/sandbox" ) func writeUserProfileLint(t *testing.T, dir, name, body string) string { diff --git a/cmd/sandbox/profile_list.go b/cmd/sandbox/profile_list.go index a7caca4..34789e9 100644 --- a/cmd/sandbox/profile_list.go +++ b/cmd/sandbox/profile_list.go @@ -5,9 +5,9 @@ import ( "io" "strings" + "github.com/safedep/pmg/errcodes" "github.com/safedep/pmg/internal/ui" pmgsandbox "github.com/safedep/pmg/sandbox" - "github.com/safedep/pmg/errcodes" "github.com/spf13/cobra" ) diff --git a/cmd/sandbox/profile_list_test.go b/cmd/sandbox/profile_list_test.go index 583f379..2b02829 100644 --- a/cmd/sandbox/profile_list_test.go +++ b/cmd/sandbox/profile_list_test.go @@ -13,9 +13,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + pmgsandbox "github.com/safedep/pmg/sandbox" ) func newTestRegistry(t *testing.T, userDir string) registryFactory { diff --git a/cmd/sandbox/profile_show.go b/cmd/sandbox/profile_show.go index 00be4f1..6f4334f 100644 --- a/cmd/sandbox/profile_show.go +++ b/cmd/sandbox/profile_show.go @@ -6,9 +6,9 @@ import ( "os" "github.com/safedep/dry/log" + "github.com/safedep/pmg/errcodes" pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/safedep/pmg/sandbox/platform" - "github.com/safedep/pmg/errcodes" "github.com/spf13/cobra" "gopkg.in/yaml.v3" ) diff --git a/cmd/sandbox/violations_list.go b/cmd/sandbox/violations_list.go index 78e09c6..1a432fb 100644 --- a/cmd/sandbox/violations_list.go +++ b/cmd/sandbox/violations_list.go @@ -6,10 +6,10 @@ import ( "io" "time" - "github.com/safedep/pmg/internal/ui" - pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + "github.com/safedep/pmg/internal/ui" + pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/spf13/cobra" ) diff --git a/cmd/sandbox/violations_list_test.go b/cmd/sandbox/violations_list_test.go index 19f8b74..836597a 100644 --- a/cmd/sandbox/violations_list_test.go +++ b/cmd/sandbox/violations_list_test.go @@ -6,9 +6,9 @@ import ( "strings" "testing" - pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + pmgsandbox "github.com/safedep/pmg/sandbox" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) diff --git a/config/config.go b/config/config.go index 551f540..d0017b6 100644 --- a/config/config.go +++ b/config/config.go @@ -12,8 +12,8 @@ import ( packagev1 "buf.build/gen/go/safedep/api/protocolbuffers/go/safedep/messages/package/v1" "github.com/safedep/dry/log" - "github.com/safedep/dry/utils" "github.com/safedep/dry/usefulerror" + "github.com/safedep/dry/utils" "github.com/safedep/pmg/errcodes" "github.com/spf13/viper" ) @@ -337,8 +337,8 @@ func DefaultConfig() RuntimeConfig { }, }, Proxy: ProxyConfig{ - Enabled: true, - InstallOnly: false, + Enabled: true, + InstallOnly: false, SkipCommands: map[string][]string{}, }, }, diff --git a/errcodes/codes.go b/errcodes/codes.go index dd2e35f..73e44c0 100644 --- a/errcodes/codes.go +++ b/errcodes/codes.go @@ -7,10 +7,19 @@ const ( Timeout = "Timeout" Canceled = "Canceled" UnexpectedEOF = "UnexpectedEOF" - Unknown = "Unknown" Lifecycle = "Lifecycle" Network = "Network" - SandboxViolation = "SandboxViolation" PackageManagerExecutionFailed = "PackageManagerExecutionFailed" - BubblewrapNotFound = "bubblewrap_not_found" + BubblewrapNotFound = "BubblewrapNotFound" + + // Package manager error codes. + DependencyResolutionFailed = "DependencyResolutionFailed" + PackageParseFailed = "PackageParseFailed" + PackageAuthorNotFound = "PackageAuthorNotFound" + GitHubRateLimitExceeded = "GitHubRateLimitExceeded" + + // Unknown mirrors the default code that dry/usefulerror returns for errors + // created without an explicit code, so unset and explicitly-unknown errors + // classify identically (e.g. the bug-report hint in ui.ErrorExit). + Unknown = "unknown" ) diff --git a/go.mod b/go.mod index d85fee3..38b95fa 100644 --- a/go.mod +++ b/go.mod @@ -94,4 +94,4 @@ require ( modernc.org/memory v1.11.0 // indirect modernc.org/sqlite v1.48.1 // indirect sigs.k8s.io/yaml v1.4.0 // indirect -) \ No newline at end of file +) diff --git a/internal/runner/execute.go b/internal/runner/execute.go index 5746880..b6c412e 100644 --- a/internal/runner/execute.go +++ b/internal/runner/execute.go @@ -10,12 +10,13 @@ import ( "sync" "github.com/safedep/dry/log" + "github.com/safedep/dry/usefulerror" + "github.com/safedep/pmg/errcodes" "github.com/safedep/pmg/internal/pty" "github.com/safedep/pmg/internal/shim" "github.com/safedep/pmg/packagemanager" "github.com/safedep/pmg/sandbox" "github.com/safedep/pmg/sandbox/executor" - "github.com/safedep/dry/usefulerror" ) type ExecutionMode int @@ -141,6 +142,7 @@ func runPTY( if !result.ShouldRun() { return usefulerror.NewUsefulError(). Wrap(fmt.Errorf("sandbox not supported for PTY sessions")). + WithCode(errcodes.InvalidArgument). WithHumanError("Sandbox executed command cannot be used with PTY session. Please use non-interactive TTY mode instead.") } diff --git a/internal/ui/error_convert_test.go b/internal/ui/error_convert_test.go index b5b4117..cf36e35 100644 --- a/internal/ui/error_convert_test.go +++ b/internal/ui/error_convert_test.go @@ -76,7 +76,7 @@ func Test_ErrorConverters(t *testing.T) { inputError: fmt.Errorf("more context: %w", fmt.Errorf("outer context: %w", errors.New("root cause error"))), - wantNil: true, + wantNil: true, }, { name: "Nil", @@ -114,6 +114,65 @@ func Test_ErrorConverters(t *testing.T) { } } +func Test_convertToUsefulError(t *testing.T) { + tests := []struct { + name string + inputError error + wantCode string + wantHumanError string + wantNil bool + }{ + { + name: "Nil", + inputError: nil, + wantNil: true, + }, + { + name: "AlreadyUseful", + inputError: usefulerror.NewUsefulError(). + WithCode("CUSTOM"). + WithHumanError("Already useful"), + wantCode: "CUSTOM", + wantHumanError: "Already useful", + }, + { + name: "Converted", + inputError: &fs.PathError{Op: "open", Path: "/nonexistent/file.txt", Err: os.ErrNotExist}, + wantCode: errcodes.NotFound, + wantHumanError: "File or directory not found: /nonexistent/file.txt", + }, + { + name: "UnknownFallsBackToRootCause", + inputError: errors.New("some unknown error"), + wantCode: errcodes.Unknown, + wantHumanError: "some unknown error", + }, + { + name: "UnknownWrappedExtractsRootCause", + inputError: fmt.Errorf("more context: %w", + fmt.Errorf("outer context: %w", + errors.New("root cause error"))), + wantCode: errcodes.Unknown, + wantHumanError: "root cause error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := convertToUsefulError(tt.inputError) + + if tt.wantNil { + assert.Nil(t, result) + return + } + + assert.NotNil(t, result) + assert.Equal(t, tt.wantCode, result.Code()) + assert.Equal(t, tt.wantHumanError, result.HumanError()) + }) + } +} + func TestExtractPathFromError(t *testing.T) { tests := []struct { name string diff --git a/packagemanager/errors.go b/packagemanager/errors.go index 8e187d6..2084a94 100644 --- a/packagemanager/errors.go +++ b/packagemanager/errors.go @@ -5,13 +5,6 @@ import ( "github.com/safedep/pmg/errcodes" ) -const ( - errDependencyResolutionFailed = "DependencyResolutionFailed" - errPackageParseFailed = "PackageParseFailed" - errPackageAuthorNotFound = "PackageAuthorNotFound" - errGitHubRateLimitExceeded = "GitHubRateLimitExceeded" -) - var ( ErrPackageNotFound = usefulerror.NewUsefulError(). WithCode(errcodes.NotFound). @@ -31,25 +24,25 @@ var ( WithMsg("failed to resolve package version") ErrFailedToResolveDependencies = usefulerror.NewUsefulError(). - WithCode(errDependencyResolutionFailed). + WithCode(errcodes.DependencyResolutionFailed). WithHumanError("Failed to resolve dependencies."). WithHelp("Check your network connection and try again."). WithMsg("failed to resolve dependencies") ErrFailedToParsePackage = usefulerror.NewUsefulError(). - WithCode(errPackageParseFailed). + WithCode(errcodes.PackageParseFailed). WithHumanError("The package data could not be processed."). WithHelp("The package may be corrupted or in an unsupported format."). WithMsg("failed to parse package") ErrAuthorNotFound = usefulerror.NewUsefulError(). - WithCode(errPackageAuthorNotFound). + WithCode(errcodes.PackageAuthorNotFound). WithHumanError("The package author information could not be found."). WithHelp("This may be due to incomplete package metadata or network issues."). WithMsg("author not found") ErrGitHubRateLimitExceeded = usefulerror.NewUsefulError(). - WithCode(errGitHubRateLimitExceeded). + WithCode(errcodes.GitHubRateLimitExceeded). WithHumanError("GitHub API rate limit has been exceeded."). WithHelp("Wait for the rate limit to reset or configure authentication to increase your rate limit."). WithMsg("github api rate limit exceeded") diff --git a/sandbox/executor/apply.go b/sandbox/executor/apply.go index d724fd5..4e1ce20 100644 --- a/sandbox/executor/apply.go +++ b/sandbox/executor/apply.go @@ -7,13 +7,13 @@ import ( "path/filepath" "github.com/safedep/dry/log" + "github.com/safedep/dry/usefulerror" "github.com/safedep/dry/utils" "github.com/safedep/pmg/config" + "github.com/safedep/pmg/errcodes" "github.com/safedep/pmg/internal/audit" "github.com/safedep/pmg/sandbox" "github.com/safedep/pmg/sandbox/platform" - "github.com/safedep/dry/usefulerror" - "github.com/safedep/pmg/errcodes" ) type applySandboxConfig struct { diff --git a/sandbox/executor/diagnostics.go b/sandbox/executor/diagnostics.go index 9b37dac..6d366b2 100644 --- a/sandbox/executor/diagnostics.go +++ b/sandbox/executor/diagnostics.go @@ -4,10 +4,10 @@ import ( "fmt" "github.com/safedep/dry/log" - "github.com/safedep/pmg/config" - "github.com/safedep/pmg/sandbox" "github.com/safedep/dry/usefulerror" + "github.com/safedep/pmg/config" "github.com/safedep/pmg/errcodes" + "github.com/safedep/pmg/sandbox" ) // WrapCommandExecutionError converts a package manager execution error into a diff --git a/sandbox/executor/diagnostics_test.go b/sandbox/executor/diagnostics_test.go index 77b84f3..b515edf 100644 --- a/sandbox/executor/diagnostics_test.go +++ b/sandbox/executor/diagnostics_test.go @@ -6,9 +6,9 @@ import ( "os/exec" "testing" - "github.com/safedep/pmg/sandbox" "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/errcodes" + "github.com/safedep/pmg/sandbox" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" )