From 800dd9fb81e36b31eaf5189188442cfc23b38520 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 24 Sep 2026 09:42:19 +0200 Subject: [PATCH 1/5] refactor(pass)!: return errors from run options BREAKING CHANGE: RunCommand now returns (*cobra.Command, error), and RunOption returns error. Invalid socket paths and timeouts fail during construction. --- plugins/pass/commands/run.go | 35 ++++++++++--- plugins/pass/commands/run_test.go | 86 ++++++++++++++++++++++++++++--- 2 files changed, 107 insertions(+), 14 deletions(-) diff --git a/plugins/pass/commands/run.go b/plugins/pass/commands/run.go index e951e5f2..45345ded 100644 --- a/plugins/pass/commands/run.go +++ b/plugins/pass/commands/run.go @@ -62,33 +62,52 @@ type runOpts struct { socketPath string } -type RunOption func(*runOpts) +// RunOption configures a run command and reports invalid option values. +type RunOption func(*runOpts) error // WithTimeout sets the client request timeout; 0 disables it. +// Negative durations return an error. func WithTimeout(timeout time.Duration) RunOption { - return func(o *runOpts) { + return func(o *runOpts) error { + if timeout < 0 { + return errors.New("request timeout duration cannot be negative") + } o.timeout = &timeout + return nil } } // WithResponseTimeout sets the client response header timeout; 0 disables it. +// Negative durations return an error. func WithResponseTimeout(responseTimeout time.Duration) RunOption { - return func(o *runOpts) { + return func(o *runOpts) error { + if responseTimeout < 0 { + return errors.New("response timeout duration cannot be negative") + } o.responseTimeout = &responseTimeout + return nil } } -// WithSocketPath overrides the engine socket path; empty uses [api.DesktopSocketPath]. +// WithSocketPath overrides the engine socket path. An empty path returns an error. func WithSocketPath(socketPath string) RunOption { - return func(o *runOpts) { + return func(o *runOpts) error { + if socketPath == "" { + return errors.New("no path provided") + } o.socketPath = socketPath + return nil } } -func RunCommand(options ...RunOption) *cobra.Command { +// RunCommand creates a command with the supplied options, returning an error if +// an option is invalid. The default socket path is [api.DesktopSocketPath]. +func RunCommand(options ...RunOption) (*cobra.Command, error) { opts := runOpts{} for _, o := range options { - o(&opts) + if err := o(&opts); err != nil { + return nil, err + } } cmd := &cobra.Command{ Use: "run -- CMD [ARGS...]", @@ -173,7 +192,7 @@ func RunCommand(options ...RunOption) *cobra.Command { } cmd.Flags().StringArrayVar(&opts.envFiles, "env-file", nil, "Read environment variables from a dotenv-formatted file. Repeatable; later files override earlier files and the process environment.") - return cmd + return cmd, nil } func mergeEnv(processEnv, files []string) ([]string, error) { diff --git a/plugins/pass/commands/run_test.go b/plugins/pass/commands/run_test.go index 584695d2..de758860 100644 --- a/plugins/pass/commands/run_test.go +++ b/plugins/pass/commands/run_test.go @@ -110,7 +110,11 @@ func runAsWrapper() { if socket := os.Getenv(helperSocketEnv); socket != "" { ropts = []RunOption{WithSocketPath(socket)} } - cmd := RunCommand(ropts...) + cmd, err := RunCommand(ropts...) + if err != nil { + _, _ = fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } cmd.SetArgs([]string{exe}) cmd.SetContext(context.Background()) cmd.SilenceUsage = true @@ -291,6 +295,73 @@ func TestMergeEnv(t *testing.T) { }) } +func TestRunCommandOptions(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + options []RunOption + wantErr string + }{ + {name: "defaults"}, + { + name: "explicit options", + options: []RunOption{ + WithSocketPath("/tmp/secrets-engine.sock"), + WithTimeout(time.Second), + WithResponseTimeout(time.Second), + }, + }, + { + name: "zero disables timeouts", + options: []RunOption{WithTimeout(0), WithResponseTimeout(0)}, + }, + { + name: "empty socket path", + options: []RunOption{WithSocketPath("")}, + wantErr: "no path provided", + }, + { + name: "negative request timeout", + options: []RunOption{WithTimeout(-time.Second)}, + wantErr: "request timeout duration cannot be negative", + }, + { + name: "negative response timeout", + options: []RunOption{WithResponseTimeout(-time.Second)}, + wantErr: "response timeout duration cannot be negative", + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + cmd, err := RunCommand(tt.options...) + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + assert.Nil(t, cmd) + return + } + require.NoError(t, err) + assert.NotNil(t, cmd) + }) + } + + t.Run("stops at the first option error", func(t *testing.T) { + t.Parallel() + optionErr := errors.New("invalid option") + laterOptionCalled := false + cmd, err := RunCommand( + func(*runOpts) error { return optionErr }, + func(*runOpts) error { + laterOptionCalled = true + return nil + }, + ) + require.ErrorIs(t, err, optionErr) + assert.Nil(t, cmd) + assert.False(t, laterOptionCalled) + }) +} + // TestRunCommand covers cobra-level behavior against a mock engine or none. // TestParseEnv and TestResolveEnv cover the details. func TestRunCommand(t *testing.T) { @@ -298,12 +369,13 @@ func TestRunCommand(t *testing.T) { require.NoError(t, err) t.Run("no command given returns arg error", func(t *testing.T) { - cmd := RunCommand() + cmd, err := RunCommand() + require.NoError(t, err) cmd.SetArgs([]string{}) cmd.SetContext(t.Context()) cmd.SetOut(testWriter{t}) cmd.SetErr(testWriter{t}) - err := cmd.Execute() + err = cmd.Execute() require.Error(t, err) assert.Contains(t, err.Error(), "requires at least 1 arg") }) @@ -354,7 +426,8 @@ func TestRunCommand(t *testing.T) { envFile := writeEnvFile(t, "SE_TOKEN=se://gh-token\n"+ helperActiveEnv+"=1\n"+ helperCheckEnv+"=SE_TOKEN=ghp_abc123\n") - cmd := RunCommand(WithTimeout(time.Second), WithSocketPath(engine.serve(t))) + cmd, err := RunCommand(WithTimeout(time.Second), WithSocketPath(engine.serve(t))) + require.NoError(t, err) cmd.SetArgs([]string{"--env-file", envFile, exe}) cmd.SetContext(t.Context()) cmd.SetOut(testWriter{t}) @@ -368,12 +441,13 @@ func TestRunCommand(t *testing.T) { secrets.MustParseID("gh-token"): "ghp_abc123", }} envFile := writeEnvFile(t, "SE_TOKEN=se://gh-token\n"+helperActiveEnv+"=1\n") - cmd := RunCommand(WithTimeout(time.Second), WithSocketPath(engine.serve(t))) + cmd, err := RunCommand(WithTimeout(time.Second), WithSocketPath(engine.serve(t))) + require.NoError(t, err) cmd.SetArgs([]string{"--env-file", envFile, exe}) cmd.SetContext(t.Context()) cmd.SetOut(testWriter{t}) cmd.SetErr(testWriter{t}) - err := cmd.Execute() + err = cmd.Execute() require.ErrorIs(t, err, client.ErrAccessDenied) assert.ErrorContains(t, err, "authorizing: access denied") assert.Equal(t, []string{"authorize gh-token"}, engine.recorded()) From 6eae1075347d807efcd709e2a698e87327703134 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 24 Sep 2026 10:05:17 +0200 Subject: [PATCH 2/5] refactor(pass): separate run command construction from option validation --- plugins/pass/commands/run.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/plugins/pass/commands/run.go b/plugins/pass/commands/run.go index 45345ded..1b76c496 100644 --- a/plugins/pass/commands/run.go +++ b/plugins/pass/commands/run.go @@ -109,6 +109,10 @@ func RunCommand(options ...RunOption) (*cobra.Command, error) { return nil, err } } + return newRunCommand(opts), nil +} + +func newRunCommand(opts runOpts) *cobra.Command { cmd := &cobra.Command{ Use: "run -- CMD [ARGS...]", Short: "Run a command with `se://` environment references resolved.", @@ -192,7 +196,7 @@ func RunCommand(options ...RunOption) (*cobra.Command, error) { } cmd.Flags().StringArrayVar(&opts.envFiles, "env-file", nil, "Read environment variables from a dotenv-formatted file. Repeatable; later files override earlier files and the process environment.") - return cmd, nil + return cmd } func mergeEnv(processEnv, files []string) ([]string, error) { From e6aac2b34863f951223b0c3a06dc13e15c31045b Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 24 Sep 2026 10:17:00 +0200 Subject: [PATCH 3/5] docs(pass): simplify RunCommand godoc --- plugins/pass/commands/run.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/plugins/pass/commands/run.go b/plugins/pass/commands/run.go index 1b76c496..87021e5d 100644 --- a/plugins/pass/commands/run.go +++ b/plugins/pass/commands/run.go @@ -100,8 +100,7 @@ func WithSocketPath(socketPath string) RunOption { } } -// RunCommand creates a command with the supplied options, returning an error if -// an option is invalid. The default socket path is [api.DesktopSocketPath]. +// RunCommand uses [api.DesktopSocketPath] by default. func RunCommand(options ...RunOption) (*cobra.Command, error) { opts := runOpts{} for _, o := range options { From 05142b6254514c661f734937fad6a2abd059b432 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 24 Sep 2026 10:22:40 +0200 Subject: [PATCH 4/5] docs(pass): remove redundant RunOption comment --- plugins/pass/commands/run.go | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/pass/commands/run.go b/plugins/pass/commands/run.go index 87021e5d..fc860759 100644 --- a/plugins/pass/commands/run.go +++ b/plugins/pass/commands/run.go @@ -62,7 +62,6 @@ type runOpts struct { socketPath string } -// RunOption configures a run command and reports invalid option values. type RunOption func(*runOpts) error // WithTimeout sets the client request timeout; 0 disables it. From 0dc0c34576c2c5a38c0dc9efb1b3d5382e4070a7 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 24 Sep 2026 10:43:18 +0200 Subject: [PATCH 5/5] docs(pass): simplify option comments after review --- plugins/pass/commands/run.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/pass/commands/run.go b/plugins/pass/commands/run.go index fc860759..85a45860 100644 --- a/plugins/pass/commands/run.go +++ b/plugins/pass/commands/run.go @@ -65,7 +65,6 @@ type runOpts struct { type RunOption func(*runOpts) error // WithTimeout sets the client request timeout; 0 disables it. -// Negative durations return an error. func WithTimeout(timeout time.Duration) RunOption { return func(o *runOpts) error { if timeout < 0 { @@ -77,7 +76,6 @@ func WithTimeout(timeout time.Duration) RunOption { } // WithResponseTimeout sets the client response header timeout; 0 disables it. -// Negative durations return an error. func WithResponseTimeout(responseTimeout time.Duration) RunOption { return func(o *runOpts) error { if responseTimeout < 0 { @@ -88,7 +86,7 @@ func WithResponseTimeout(responseTimeout time.Duration) RunOption { } } -// WithSocketPath overrides the engine socket path. An empty path returns an error. +// WithSocketPath overrides the default [api.DesktopSocketPath]. func WithSocketPath(socketPath string) RunOption { return func(o *runOpts) error { if socketPath == "" {