From 184528f9e2477a51495c07a34506d6d9568f24a4 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 20 Jul 2026 15:30:09 -0500 Subject: [PATCH 1/8] refactor(errors): centralize error classification, codes, and exit handling Make CLIError{code,message} the single structured error surface, built only in pkg/clierr. Libraries return sentinel/typed/wrapped errors; classification happens once at the boundary via clierr.Classify(err). - Rename pkg/errors -> pkg/clierr so it no longer shadows the stdlib errors package; importers use it unaliased alongside stdlib errors. - Canonical sentinels (ErrWorkspaceNotFound, ErrRateLimited) live in pkg/clierr like io.EOF; workspace, provider, and selfupdate wrap them, and Classify maps them to codes inline (no registry, no init indirection). - Preserve error chains: add Unwrap() to command.Error and pro.Error; wrap the gRPC clone site with %w. - ProviderExecError feeds provider-init stderr to the fingerprint table; classification stays init-scoped so other commands keep transparent child exit codes. - Exit codes: 0 success, 1 failure, 75 WorkspaceNotFound (consumed by the backhaul SSH retry). - Unify the MCP error surface onto the shared Code vocabulary; slim CLIError to code+message. - Root-level panic recovery; propagate the workspace-watcher panic instead of swallowing it; stop double-logging (log-and-return -> wrap-and-return). - Reduce comments to non-obvious rationale. --- cmd/internal/agentworkspace/build.go | 1 - cmd/internal/agentworkspace/setup_gpg.go | 24 +- cmd/mcp/errors.go | 28 +-- cmd/pro/start.go | 4 + cmd/provider/configure_shared.go | 13 +- cmd/root.go | 66 ++++-- cmd/root_test.go | 14 +- desktop/src/main/__tests__/cli.test.ts | 10 +- .../src/lib/components/ErrorCard.svelte | 20 +- desktop/src/shared/cli-error.ts | 26 --- pkg/agent/agent.go | 1 - pkg/agent/dockerless.go | 3 +- pkg/agent/workspace.go | 8 - .../clientimplementation/workspace_client.go | 3 +- pkg/clierr/errors.go | 86 +++++++ pkg/clierr/errors_test.go | 106 +++++++++ pkg/command/command.go | 4 + pkg/credentials/request.go | 4 +- pkg/daemon/platform/workspace_watcher.go | 19 +- pkg/errors/errors.go | 209 ------------------ pkg/errors/errors_test.go | 202 ----------------- pkg/errors/patterns.go | 92 -------- pkg/exitcode/codes.go | 16 +- pkg/log/logger.go | 13 +- pkg/provider/version_cache.go | 3 - pkg/provider/versions_github.go | 3 +- pkg/provider/versions_github_test.go | 4 +- pkg/selfupdate/errors.go | 23 ++ pkg/selfupdate/errors_test.go | 37 ++++ pkg/selfupdate/source.go | 4 +- pkg/telemetry/collect.go | 6 +- pkg/tunnel/browser.go | 10 +- pkg/tunnel/browser_test.go | 2 +- pkg/workspace/workspace.go | 6 +- 34 files changed, 373 insertions(+), 697 deletions(-) create mode 100644 pkg/clierr/errors.go create mode 100644 pkg/clierr/errors_test.go delete mode 100644 pkg/errors/errors.go delete mode 100644 pkg/errors/errors_test.go delete mode 100644 pkg/errors/patterns.go create mode 100644 pkg/selfupdate/errors.go create mode 100644 pkg/selfupdate/errors_test.go diff --git a/cmd/internal/agentworkspace/build.go b/cmd/internal/agentworkspace/build.go index d4522e134..3ad66ee7c 100644 --- a/cmd/internal/agentworkspace/build.go +++ b/cmd/internal/agentworkspace/build.go @@ -96,7 +96,6 @@ func (cmd *BuildCmd) Run(ctx context.Context) error { PushDuringBuild: workspaceInfo.CLIOptions.PushDuringBuild, }) if err != nil { - log.Errorf("Error building image: %v", err) return fmt.Errorf("build: %w", err) } diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index f034a3420..d8ba76706 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -86,8 +86,7 @@ func fetchAndDecodeKeys(ownerTrustB64 string) ([]byte, []byte, error) { log.Debugf("Fetching public key") rawPublicKeys, err := getPublicKeys() if err != nil { - log.Errorf("Fetch public key: %v", err) - return nil, nil, err + return nil, nil, fmt.Errorf("fetch public key: %w", err) } log.Debugf("Decoding public key") @@ -108,46 +107,39 @@ func fetchAndDecodeKeys(ownerTrustB64 string) ([]byte, []byte, error) { func configureGPGAgent(gpgConf *gpg.GPGConf) error { log.Debugf("Stopping container gpg-agent") if err := gpgConf.StopGpgAgent(); err != nil { - log.Errorf("stop container gpg-agent: %v", err) - return err + return fmt.Errorf("stop container gpg-agent: %w", err) } log.Debugf("Importing gpg public key in container") if err := gpgConf.ImportGpgKey(); err != nil { - log.Errorf("Import gpg public key in container: %v", err) - return err + return fmt.Errorf("import gpg public key in container: %w", err) } log.Debugf("Importing gpg owner trust in container") if err := gpgConf.ImportOwnerTrust(); err != nil { - log.Errorf("Import gpg owner trust in container: %v", err) - return err + return fmt.Errorf("import gpg owner trust in container: %w", err) } log.Debugf("Ensuring paths existence and permissions") if err := gpgConf.SetupRemoteSocketDirTree(); err != nil { - log.Errorf("Ensure paths existence and permissions: %v", err) - return err + return fmt.Errorf("ensure paths existence and permissions: %w", err) } // Now we again kill the agent and remove the socket to really be sure every // thing is clean log.Debugf("Ensure stopping container gpg-agent") if err := gpgConf.StopGpgAgent(); err != nil { - log.Errorf("Ensure stopping container gpg-agent: %v", err) - return err + return fmt.Errorf("ensure stopping container gpg-agent: %w", err) } log.Debugf("Setup local gnupg socket links") if err := gpgConf.SetupRemoteSocketLink(); err != nil { - log.Errorf("Setup local gnupg socket links: %v", err) - return err + return fmt.Errorf("setup local gnupg socket links: %w", err) } log.Debugf("Setup gpg.conf") if err := gpgConf.SetupGpgConf(); err != nil { - log.Errorf("Setup gpg.conf: %v", err) - return err + return fmt.Errorf("setup gpg.conf: %w", err) } return nil diff --git a/cmd/mcp/errors.go b/cmd/mcp/errors.go index a06ee802b..96b29b0f1 100644 --- a/cmd/mcp/errors.go +++ b/cmd/mcp/errors.go @@ -1,39 +1,21 @@ package mcp import ( - "errors" - - cliErrors "github.com/devsy-org/devsy/pkg/errors" - "github.com/devsy-org/devsy/pkg/workspace" + "github.com/devsy-org/devsy/pkg/clierr" ) -// ErrorPayload is the JSON shape attached to MCP tool errors so the agent gets -// the same structured information the Devsy CLI shows humans (code, hint, doc URL). type ErrorPayload struct { Code string `json:"code"` Message string `json:"message"` - Hint string `json:"hint,omitempty"` - DocURL string `json:"doc_url,omitempty"` } -// ClassifyError converts any error returned by an MCP handler into a structured -// payload using the same classifier the CLI uses. func ClassifyError(err error) ErrorPayload { - if err == nil { + classified := clierr.Classify(err) + if classified == nil { return ErrorPayload{} } - classified := cliErrors.Classify(err, cliErrors.ClassifyContext{}) - code := "internal_error" - if classified.Code != "" { - code = string(classified.Code) - } - if errors.Is(err, workspace.ErrWorkspaceNotFound) { - code = "workspace_not_found" - } return ErrorPayload{ - Code: code, - Message: err.Error(), - Hint: classified.Hint, - DocURL: classified.DocURL, + Code: string(classified.Code), + Message: classified.Message, } } diff --git a/cmd/pro/start.go b/cmd/pro/start.go index 69961e3c7..32a90b84f 100644 --- a/cmd/pro/start.go +++ b/cmd/pro/start.go @@ -2041,6 +2041,10 @@ func (e *Error) Error() string { return message + e.err.Error() } +func (e *Error) Unwrap() error { + return e.err +} + func getMachineUID() string { id, err := machineid.ID() if err != nil { diff --git a/cmd/provider/configure_shared.go b/cmd/provider/configure_shared.go index c7e4cfae0..e1fccc497 100644 --- a/cmd/provider/configure_shared.go +++ b/cmd/provider/configure_shared.go @@ -1,14 +1,12 @@ package provider import ( - "bytes" "context" "fmt" "io" "github.com/devsy-org/devsy/pkg/client/clientimplementation" "github.com/devsy-org/devsy/pkg/config" - cliErrors "github.com/devsy-org/devsy/pkg/errors" "github.com/devsy-org/devsy/pkg/log" options2 "github.com/devsy-org/devsy/pkg/options" provider2 "github.com/devsy-org/devsy/pkg/provider" @@ -166,10 +164,6 @@ func initProvider( provider *provider2.ProviderConfig, io2 initIO, ) error { - // Capture the sub-binary's stderr in parallel with forwarding it to the - // regular log sink so that errors.Classify has the real provider output - // to fingerprint, not just an opaque "exit status 1". - stderrBuf := &bytes.Buffer{} err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, Name: "init", @@ -178,13 +172,10 @@ func initProvider( Options: devsyConfig.ProviderOptions(provider.Name), Config: provider, Stdout: io2.stdout, - Stderr: io.MultiWriter(io2.stderr, stderrBuf), + Stderr: io2.stderr, }) if err != nil { - return cliErrors.Classify(fmt.Errorf("init: %w", err), cliErrors.ClassifyContext{ - Provider: provider.Name, - Stderr: stderrBuf.String(), - }) + return fmt.Errorf("init: %w", err) } if devsyConfig.Current().Providers == nil { devsyConfig.Current().Providers = map[string]*config.ProviderConfig{} diff --git a/cmd/root.go b/cmd/root.go index a017ace95..01bddb3be 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "os/exec" + "runtime/debug" "strings" "github.com/devsy-org/devsy/cmd/completion" @@ -22,8 +23,8 @@ import ( "github.com/devsy-org/devsy/cmd/self" "github.com/devsy-org/devsy/cmd/template" wsCmdPkg "github.com/devsy-org/devsy/cmd/workspace" + "github.com/devsy-org/devsy/pkg/clierr" "github.com/devsy-org/devsy/pkg/config" - cliErrors "github.com/devsy-org/devsy/pkg/errors" "github.com/devsy-org/devsy/pkg/exitcode" "github.com/devsy-org/devsy/pkg/flatpak" "github.com/devsy-org/devsy/pkg/log" @@ -101,18 +102,26 @@ func Execute() { os.Exit(run()) } -func run() int { +func run() (code int) { + machineMode := false + collector := telemetry.FromContext(gocontext.Background()) // noop until BootstrapCLI + defer func() { + if r := recover(); r != nil { + log.Errorf("panic: %v\n%s", r, debug.Stack()) + panicErr := clierr.NewPanic(r) + collector.RecordCLI(panicErr) + code = exitCodeForError(panicErr, machineMode) + } + collector.Flush() + }() + rootCmd, globalFlags := BuildRoot() - target := rootCmd - if found, _, findErr := rootCmd.Find(os.Args[1:]); findErr == nil && found != nil { - target = found - } - collector := telemetry.BootstrapCLI(target) + target := resolveTarget(rootCmd) + collector = telemetry.BootstrapCLI(target) rootCmd.SetContext(telemetry.WithCollector(gocontext.Background(), collector)) - defer func() { collector.Flush() }() isInternal := topLevelCommand(target) == internalCommand - machineMode := configureOutput(rootCmd, globalFlags, isInternal) + machineMode = configureOutput(rootCmd, globalFlags, isInternal) if !isInternal { if shouldExit, err := flatpak.ReexecOnHost(); err != nil { @@ -138,6 +147,13 @@ func run() int { return 0 } +func resolveTarget(rootCmd *cobra.Command) *cobra.Command { + if found, _, err := rootCmd.Find(os.Args[1:]); err == nil && found != nil { + return found + } + return rootCmd +} + func configureOutput( rootCmd *cobra.Command, globalFlags *flags.GlobalFlags, @@ -145,7 +161,7 @@ func configureOutput( ) bool { logOutput := logOutputFromArgs(os.Args[1:]) machineMode := isMachineConsumer(logOutput, isInternal) - rootCmd.SilenceErrors = machineMode + rootCmd.SilenceErrors = true rootCmd.SilenceUsage = machineMode format := logOutput @@ -176,18 +192,23 @@ func topLevelCommand(cmd *cobra.Command) string { func exitCodeForError(err error, machineMode bool) int { if err == nil { - return 0 + return exitcode.Success } - if code, ok := passthroughExitCode(err, machineMode); ok { - return code + cliErr := clierr.Classify(err) + + // Stay transparent for unclassified child-process exits (e.g. `devsy ssh -- cmd`). + if cliErr.Code == clierr.CodeUnknown { + if code, ok := passthroughExitCode(err, machineMode); ok { + return code + } } - renderCLIError(err, machineMode) + renderCLIError(cliErr, machineMode) if errors.Is(err, workspace.ErrWorkspaceNotFound) { - return exitcode.WorkspaceNotFound + return exitcode.Retryable } - return 1 + return exitcode.Failure } func passthroughExitCode(err error, machineMode bool) (int, bool) { @@ -206,18 +227,15 @@ func passthroughExitCode(err error, machineMode bool) (int, bool) { return 0, false } -func renderCLIError(err error, machineMode bool) { - cliErr := cliErrors.Classify(err, cliErrors.ClassifyContext{}) +func renderCLIError(cliErr *clierr.CLIError, machineMode bool) { + if cliErr == nil { + return + } if machineMode { log.JSONError(cliErr) return } - if cliErr.Hint != "" { - fmt.Fprintf(os.Stderr, "Hint: %s\n", cliErr.Hint) - } - if cliErr.DocURL != "" { - fmt.Fprintf(os.Stderr, "See: %s\n", cliErr.DocURL) - } + fmt.Fprintf(os.Stderr, "Error: %s\n", cliErr.Message) } // BuildRoot constructs the root command and returns it alongside the parsed diff --git a/cmd/root_test.go b/cmd/root_test.go index 1b46fd2c9..b07c7e120 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -1,14 +1,26 @@ package cmd import ( + "fmt" "os" "testing" "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/exitcode" + "github.com/devsy-org/devsy/pkg/workspace" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestExitCodeForError_WorkspaceNotFound(t *testing.T) { + err := fmt.Errorf("get workspace: %w", workspace.ErrWorkspaceNotFound) + assert.Equal(t, exitcode.Retryable, exitCodeForError(err, true)) +} + +func TestExitCodeForError_GenericFailure(t *testing.T) { + assert.Equal(t, exitcode.Failure, exitCodeForError(fmt.Errorf("boom"), true)) +} + func TestTopLevelCommand(t *testing.T) { rootCmd, _ := BuildRoot() @@ -129,7 +141,7 @@ func TestConfigureOutput_SilencesCobra(t *testing.T) { os.Args = append([]string{"devsy"}, tc.args...) machineMode := configureOutput(rootCmd, globalFlags, tc.isInternal) assert.Equal(t, tc.wantSilent, machineMode) - assert.Equal(t, tc.wantSilent, rootCmd.SilenceErrors) + assert.True(t, rootCmd.SilenceErrors) assert.Equal(t, tc.wantSilent, rootCmd.SilenceUsage) }) } diff --git a/desktop/src/main/__tests__/cli.test.ts b/desktop/src/main/__tests__/cli.test.ts index 24b89dfbc..4f611cc3e 100644 --- a/desktop/src/main/__tests__/cli.test.ts +++ b/desktop/src/main/__tests__/cli.test.ts @@ -79,14 +79,8 @@ describe("CliRunner", () => { typeof vi.fn > const cliErrorPayload = { - code: "AWS_PROFILE_MISSING", - message: "AWS credentials are not configured.", - hint: "Set AWS_PROFILE or create ~/.aws/credentials.", - docUrl: - "https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html", - provider: "aws", - cause: - "init: exit status 1: failed to get shared config profile, default", + code: "RATE_LIMITED", + message: "Rate limited by an upstream API. Wait and retry, or authenticate for a higher limit.", } const stderrLine = JSON.stringify({ level: "error", diff --git a/desktop/src/renderer/src/lib/components/ErrorCard.svelte b/desktop/src/renderer/src/lib/components/ErrorCard.svelte index 32d922a3e..52398b823 100644 --- a/desktop/src/renderer/src/lib/components/ErrorCard.svelte +++ b/desktop/src/renderer/src/lib/components/ErrorCard.svelte @@ -1,5 +1,5 @@