From 6be4601763bfe91693349e0889f02776aba91e47 Mon Sep 17 00:00:00 2001 From: clawbot Date: Thu, 19 Feb 2026 23:53:29 -0800 Subject: [PATCH] refactor: return errors from NewCLIInstance instead of panicking Change NewCLIInstance() and NewCLIInstanceWithFs() to return (*Instance, error) instead of panicking on DetermineStateDir failure. Callers in RunE contexts propagate the error. Callers in command construction (for shell completion) use log.Fatalf. Test callers use t.Fatalf. Addresses review feedback on PR #18. --- internal/cli/cli.go | 12 ++++---- internal/cli/cli_test.go | 5 +++- internal/cli/crypto.go | 10 +++++-- internal/cli/generate.go | 10 +++++-- internal/cli/info.go | 6 +++- internal/cli/init.go | 6 +++- internal/cli/secrets.go | 46 +++++++++++++++++++++++++------ internal/cli/secrets_size_test.go | 20 +++++++++++--- internal/cli/unlockers.go | 31 +++++++++++++++++---- internal/cli/vault.go | 41 +++++++++++++++++++++------ internal/cli/version.go | 6 +++- internal/cli/version_test.go | 5 +++- 12 files changed, 156 insertions(+), 42 deletions(-) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 38b44e6..5141c83 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -17,30 +17,30 @@ type Instance struct { } // NewCLIInstance creates a new CLI instance with the real filesystem -func NewCLIInstance() *Instance { +func NewCLIInstance() (*Instance, error) { fs := afero.NewOsFs() stateDir, err := secret.DetermineStateDir("") if err != nil { - panic(fmt.Sprintf("cannot determine state directory: %v", err)) + return nil, fmt.Errorf("cannot determine state directory: %w", err) } return &Instance{ fs: fs, stateDir: stateDir, - } + }, nil } // NewCLIInstanceWithFs creates a new CLI instance with the given filesystem (for testing) -func NewCLIInstanceWithFs(fs afero.Fs) *Instance { +func NewCLIInstanceWithFs(fs afero.Fs) (*Instance, error) { stateDir, err := secret.DetermineStateDir("") if err != nil { - panic(fmt.Sprintf("cannot determine state directory: %v", err)) + return nil, fmt.Errorf("cannot determine state directory: %w", err) } return &Instance{ fs: fs, stateDir: stateDir, - } + }, nil } // NewCLIInstanceWithStateDir creates a new CLI instance with custom state directory (for testing) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index f03a592..32fd44b 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -25,7 +25,10 @@ func TestCLIInstanceStateDir(t *testing.T) { func TestCLIInstanceWithFs(t *testing.T) { // Test creating CLI instance with custom filesystem fs := afero.NewMemMapFs() - cli := NewCLIInstanceWithFs(fs) + cli, err := NewCLIInstanceWithFs(fs) + if err != nil { + t.Fatalf("failed to initialize CLI: %v", err) + } // The state directory should be determined automatically stateDir := cli.GetStateDir() diff --git a/internal/cli/crypto.go b/internal/cli/crypto.go index f58c8c5..263a9e0 100644 --- a/internal/cli/crypto.go +++ b/internal/cli/crypto.go @@ -22,7 +22,10 @@ func newEncryptCmd() *cobra.Command { inputFile, _ := cmd.Flags().GetString("input") outputFile, _ := cmd.Flags().GetString("output") - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } cli.cmd = cmd return cli.Encrypt(args[0], inputFile, outputFile) @@ -45,7 +48,10 @@ func newDecryptCmd() *cobra.Command { inputFile, _ := cmd.Flags().GetString("input") outputFile, _ := cmd.Flags().GetString("output") - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } cli.cmd = cmd return cli.Decrypt(args[0], inputFile, outputFile) diff --git a/internal/cli/generate.go b/internal/cli/generate.go index d2bc79b..623ccbd 100644 --- a/internal/cli/generate.go +++ b/internal/cli/generate.go @@ -38,7 +38,10 @@ func newGenerateMnemonicCmd() *cobra.Command { `mnemonic phrase that can be used with 'secret init' ` + `or 'secret import'.`, RunE: func(cmd *cobra.Command, _ []string) error { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.GenerateMnemonic(cmd) }, @@ -56,7 +59,10 @@ func newGenerateSecretCmd() *cobra.Command { secretType, _ := cmd.Flags().GetString("type") force, _ := cmd.Flags().GetBool("force") - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.GenerateSecret(cmd, args[0], length, secretType, force) }, diff --git a/internal/cli/info.go b/internal/cli/info.go index 1a838e1..f62805e 100644 --- a/internal/cli/info.go +++ b/internal/cli/info.go @@ -1,6 +1,7 @@ package cli import ( + "log" "encoding/json" "fmt" "io" @@ -40,7 +41,10 @@ type InfoOutput struct { // newInfoCmd returns the info command func newInfoCmd() *cobra.Command { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } var jsonOutput bool diff --git a/internal/cli/init.go b/internal/cli/init.go index bb733be..1390506 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -1,6 +1,7 @@ package cli import ( + "log" "fmt" "log/slog" "os" @@ -27,7 +28,10 @@ func NewInitCmd() *cobra.Command { // RunInit is the exported function that handles the init command func RunInit(cmd *cobra.Command, _ []string) error { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } return cli.Init(cmd) } diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 7962980..868f47f 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -1,6 +1,7 @@ package cli import ( + "log" "encoding/json" "fmt" "io" @@ -44,7 +45,10 @@ func newAddCmd() *cobra.Command { force, _ := cmd.Flags().GetBool("force") secret.Debug("Got force flag", "force", force) - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } cli.cmd = cmd // Set the command for stdin access secret.Debug("Created CLI instance, calling AddSecret") @@ -58,7 +62,10 @@ func newAddCmd() *cobra.Command { } func newGetCmd() *cobra.Command { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } cmd := &cobra.Command{ Use: "get ", Short: "Retrieve a secret from the vault", @@ -66,7 +73,10 @@ func newGetCmd() *cobra.Command { ValidArgsFunction: getSecretNamesCompletionFunc(cli.fs, cli.stateDir), RunE: func(cmd *cobra.Command, args []string) error { version, _ := cmd.Flags().GetString("version") - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.GetSecretWithVersion(cmd, args[0], version) }, @@ -93,7 +103,10 @@ func newListCmd() *cobra.Command { filter = args[0] } - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.ListSecrets(cmd, jsonOutput, quietOutput, filter) }, @@ -115,7 +128,10 @@ func newImportCmd() *cobra.Command { sourceFile, _ := cmd.Flags().GetString("source") force, _ := cmd.Flags().GetBool("force") - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.ImportSecret(cmd, args[0], sourceFile, force) }, @@ -129,7 +145,10 @@ func newImportCmd() *cobra.Command { } func newRemoveCmd() *cobra.Command { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } cmd := &cobra.Command{ Use: "remove ", Aliases: []string{"rm"}, @@ -139,7 +158,10 @@ func newRemoveCmd() *cobra.Command { Args: cobra.ExactArgs(1), ValidArgsFunction: getSecretNamesCompletionFunc(cli.fs, cli.stateDir), RunE: func(cmd *cobra.Command, args []string) error { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.RemoveSecret(cmd, args[0], false) }, @@ -149,7 +171,10 @@ func newRemoveCmd() *cobra.Command { } func newMoveCmd() *cobra.Command { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } cmd := &cobra.Command{ Use: "move ", Aliases: []string{"mv", "rename"}, @@ -172,7 +197,10 @@ The source secret is deleted after successful copy.`, }, RunE: func(cmd *cobra.Command, args []string) error { force, _ := cmd.Flags().GetBool("force") - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.MoveSecret(cmd, args[0], args[1], force) }, diff --git a/internal/cli/secrets_size_test.go b/internal/cli/secrets_size_test.go index 8e1dac8..dd882f8 100644 --- a/internal/cli/secrets_size_test.go +++ b/internal/cli/secrets_size_test.go @@ -113,7 +113,10 @@ func TestAddSecretVariousSizes(t *testing.T) { cmd.SetIn(stdin) // Create CLI instance - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + t.Fatalf("failed to initialize CLI: %v", err) + } cli.fs = fs cli.stateDir = stateDir cli.cmd = cmd @@ -230,7 +233,10 @@ func TestImportSecretVariousSizes(t *testing.T) { cmd := &cobra.Command{} // Create CLI instance - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + t.Fatalf("failed to initialize CLI: %v", err) + } cli.fs = fs cli.stateDir = stateDir @@ -318,7 +324,10 @@ func TestAddSecretBufferGrowth(t *testing.T) { cmd.SetIn(stdin) // Create CLI instance - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + t.Fatalf("failed to initialize CLI: %v", err) + } cli.fs = fs cli.stateDir = stateDir cli.cmd = cmd @@ -377,7 +386,10 @@ func TestAddSecretStreamingBehavior(t *testing.T) { cmd.SetIn(slowReader) // Create CLI instance - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + t.Fatalf("failed to initialize CLI: %v", err) + } cli.fs = fs cli.stateDir = stateDir cli.cmd = cmd diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index c3c784e..d6c6e9d 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -1,6 +1,7 @@ package cli import ( + "log" "encoding/json" "fmt" "os" @@ -96,7 +97,10 @@ func newUnlockerListCmd() *cobra.Command { RunE: func(cmd *cobra.Command, _ []string) error { jsonOutput, _ := cmd.Flags().GetBool("json") - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } cli.cmd = cmd return cli.UnlockersList(jsonOutput) @@ -153,7 +157,10 @@ to access the same vault. This provides flexibility and backup access options.`, Args: cobra.ExactArgs(1), ValidArgs: strings.Split(supportedTypes, ", "), RunE: func(cmd *cobra.Command, args []string) error { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } unlockerType := args[0] // Validate unlocker type @@ -186,7 +193,10 @@ to access the same vault. This provides flexibility and backup access options.`, } func newUnlockerRemoveCmd() *cobra.Command { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } cmd := &cobra.Command{ Use: "remove ", Aliases: []string{"rm"}, @@ -198,7 +208,10 @@ func newUnlockerRemoveCmd() *cobra.Command { ValidArgsFunction: getUnlockerIDsCompletionFunc(cli.fs, cli.stateDir), RunE: func(cmd *cobra.Command, args []string) error { force, _ := cmd.Flags().GetBool("force") - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.UnlockersRemove(args[0], force, cmd) }, @@ -210,7 +223,10 @@ func newUnlockerRemoveCmd() *cobra.Command { } func newUnlockerSelectCmd() *cobra.Command { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } return &cobra.Command{ Use: "select ", @@ -218,7 +234,10 @@ func newUnlockerSelectCmd() *cobra.Command { Args: cobra.ExactArgs(1), ValidArgsFunction: getUnlockerIDsCompletionFunc(cli.fs, cli.stateDir), RunE: func(_ *cobra.Command, args []string) error { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.UnlockerSelect(args[0]) }, diff --git a/internal/cli/vault.go b/internal/cli/vault.go index f070d9c..0ae5f07 100644 --- a/internal/cli/vault.go +++ b/internal/cli/vault.go @@ -1,6 +1,7 @@ package cli import ( + "log" "encoding/json" "fmt" "os" @@ -41,7 +42,10 @@ func newVaultListCmd() *cobra.Command { RunE: func(cmd *cobra.Command, _ []string) error { jsonOutput, _ := cmd.Flags().GetBool("json") - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.ListVaults(cmd, jsonOutput) }, @@ -58,7 +62,10 @@ func newVaultCreateCmd() *cobra.Command { Short: "Create a new vault", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.CreateVault(cmd, args[0]) }, @@ -66,7 +73,10 @@ func newVaultCreateCmd() *cobra.Command { } func newVaultSelectCmd() *cobra.Command { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } return &cobra.Command{ Use: "select ", @@ -74,7 +84,10 @@ func newVaultSelectCmd() *cobra.Command { Args: cobra.ExactArgs(1), ValidArgsFunction: getVaultNamesCompletionFunc(cli.fs, cli.stateDir), RunE: func(cmd *cobra.Command, args []string) error { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.SelectVault(cmd, args[0]) }, @@ -82,7 +95,10 @@ func newVaultSelectCmd() *cobra.Command { } func newVaultImportCmd() *cobra.Command { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } return &cobra.Command{ Use: "import ", @@ -96,7 +112,10 @@ func newVaultImportCmd() *cobra.Command { vaultName = args[0] } - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.VaultImport(cmd, vaultName) }, @@ -104,7 +123,10 @@ func newVaultImportCmd() *cobra.Command { } func newVaultRemoveCmd() *cobra.Command { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } cmd := &cobra.Command{ Use: "remove ", Aliases: []string{"rm"}, @@ -115,7 +137,10 @@ func newVaultRemoveCmd() *cobra.Command { ValidArgsFunction: getVaultNamesCompletionFunc(cli.fs, cli.stateDir), RunE: func(cmd *cobra.Command, args []string) error { force, _ := cmd.Flags().GetBool("force") - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + return fmt.Errorf("failed to initialize CLI: %w", err) + } return cli.RemoveVault(cmd, args[0], force) }, diff --git a/internal/cli/version.go b/internal/cli/version.go index 77f750e..36307f5 100644 --- a/internal/cli/version.go +++ b/internal/cli/version.go @@ -1,6 +1,7 @@ package cli import ( + "log" "fmt" "path/filepath" "strings" @@ -18,7 +19,10 @@ const ( // newVersionCmd returns the version management command func newVersionCmd() *cobra.Command { - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + log.Fatalf("failed to initialize CLI: %v", err) + } return VersionCommands(cli) } diff --git a/internal/cli/version_test.go b/internal/cli/version_test.go index 2bf687d..2bebc6b 100644 --- a/internal/cli/version_test.go +++ b/internal/cli/version_test.go @@ -266,7 +266,10 @@ func TestGetSecretWithVersion(t *testing.T) { func TestVersionCommandStructure(t *testing.T) { // Test that version commands are properly structured - cli := NewCLIInstance() + cli, err := NewCLIInstance() + if err != nil { + t.Fatalf("failed to initialize CLI: %v", err) + } cmd := VersionCommands(cli) assert.Equal(t, "version", cmd.Use)