From 63f95c30d4b918dde2412f53bb94d2b6b31f04b3 Mon Sep 17 00:00:00 2001 From: Derek Misler Date: Wed, 11 Feb 2026 13:55:01 -0500 Subject: [PATCH 1/2] Fix: run plugin hooks on command failure, not just success Signed-off-by: Derek Misler --- cli-plugins/manager/hooks.go | 4 +- cli-plugins/manager/hooks_test.go | 79 +++++++++++++++++++++++++++++++ cmd/docker/docker.go | 10 ++-- cmd/docker/docker_test.go | 37 +++++++++++++++ 4 files changed, 125 insertions(+), 5 deletions(-) diff --git a/cli-plugins/manager/hooks.go b/cli-plugins/manager/hooks.go index 9d9c4629d106..b8b55d5e9ed8 100644 --- a/cli-plugins/manager/hooks.go +++ b/cli-plugins/manager/hooks.go @@ -42,11 +42,11 @@ func RunCLICommandHooks(ctx context.Context, dockerCLI config.Provider, rootCmd, // RunPluginHooks is the entrypoint for the hooks execution flow // after a plugin command was just executed by the CLI. -func RunPluginHooks(ctx context.Context, dockerCLI config.Provider, rootCmd, subCommand *cobra.Command, args []string) { +func RunPluginHooks(ctx context.Context, dockerCLI config.Provider, rootCmd, subCommand *cobra.Command, args []string, cmdErrorMessage string) { commandName := strings.Join(args, " ") flags := getNaiveFlags(args) - runHooks(ctx, dockerCLI.ConfigFile(), rootCmd, subCommand, commandName, flags, "") + runHooks(ctx, dockerCLI.ConfigFile(), rootCmd, subCommand, commandName, flags, cmdErrorMessage) } func runHooks(ctx context.Context, cfg *configfile.ConfigFile, rootCmd, subCommand *cobra.Command, invokedCommand string, flags map[string]string, cmdErrorMessage string) { diff --git a/cli-plugins/manager/hooks_test.go b/cli-plugins/manager/hooks_test.go index 3060a2e9f112..aa911ec697ce 100644 --- a/cli-plugins/manager/hooks_test.go +++ b/cli-plugins/manager/hooks_test.go @@ -1,12 +1,23 @@ package manager import ( + "context" "testing" + "github.com/docker/cli/cli/config/configfile" + "github.com/spf13/cobra" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) +type fakeConfigProvider struct { + cfg *configfile.ConfigFile +} + +func (f *fakeConfigProvider) ConfigFile() *configfile.ConfigFile { + return f.cfg +} + func TestGetNaiveFlags(t *testing.T) { testCases := []struct { args []string @@ -141,3 +152,71 @@ func TestAppendNextSteps(t *testing.T) { }) } } + +func TestRunPluginHooksPassesErrorMessage(t *testing.T) { + cfg := configfile.New("") + cfg.Plugins = map[string]map[string]string{ + "test-plugin": {"hooks": "build"}, + } + provider := &fakeConfigProvider{cfg: cfg} + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + + // Should not panic with empty error message (success case) + RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "") + + // Should not panic with non-empty error message (failure case) + RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "exit status 1") +} + +func TestInvokeAndCollectHooksForwardsErrorMessage(t *testing.T) { + cfg := configfile.New("") + cfg.Plugins = map[string]map[string]string{ + "nonexistent": {"hooks": "build"}, + } + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + + // Plugin binary doesn't exist — invokeAndCollectHooks skips it + // gracefully and returns empty. Verifies the error message path + // doesn't cause issues when forwarded through the call chain. + result := invokeAndCollectHooks( + context.Background(), cfg, root, sub, + "build", map[string]string{}, "exit status 1", + ) + assert.Check(t, is.Len(result, 0)) +} + +func TestInvokeAndCollectHooksNoPlugins(t *testing.T) { + cfg := configfile.New("") + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + + result := invokeAndCollectHooks( + context.Background(), cfg, root, sub, + "build", map[string]string{}, "some error", + ) + assert.Check(t, is.Len(result, 0)) +} + +func TestInvokeAndCollectHooksCancelledContext(t *testing.T) { + cfg := configfile.New("") + cfg.Plugins = map[string]map[string]string{ + "test-plugin": {"hooks": "build"}, + } + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel immediately + + result := invokeAndCollectHooks( + ctx, cfg, root, sub, + "build", map[string]string{}, "exit status 1", + ) + assert.Check(t, is.Nil(result)) +} diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index f09b6d74a745..56223a5815ed 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -480,10 +480,14 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { subCommand = ccmd if err != nil || pluginmanager.IsPluginCommand(ccmd) { err := tryPluginRun(ctx, dockerCli, cmd, args[0], envs) - if err == nil { - if ccmd != nil && dockerCli.Out().IsTerminal() && dockerCli.HooksEnabled() { - pluginmanager.RunPluginHooks(ctx, dockerCli, cmd, ccmd, args) + if ccmd != nil && dockerCli.Out().IsTerminal() && dockerCli.HooksEnabled() { + var errMessage string + if err != nil { + errMessage = err.Error() } + pluginmanager.RunPluginHooks(ctx, dockerCli, cmd, ccmd, args, errMessage) + } + if err == nil { return nil } if !errdefs.IsNotFound(err) { diff --git a/cmd/docker/docker_test.go b/cmd/docker/docker_test.go index d3cb536be1c5..171047e4be45 100644 --- a/cmd/docker/docker_test.go +++ b/cmd/docker/docker_test.go @@ -4,12 +4,14 @@ import ( "bytes" "context" "errors" + "fmt" "io" "os" "syscall" "testing" "time" + dockercli "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/commands" "github.com/docker/cli/cli/debug" @@ -126,6 +128,41 @@ func TestUserTerminatedError(t *testing.T) { assert.Equal(t, getExitCode(context.Cause(notifyCtx)), 143) } +func TestGetExitCode(t *testing.T) { + t.Run("nil error returns 0", func(t *testing.T) { + assert.Equal(t, getExitCode(nil), 0) + }) + + t.Run("generic error returns 1", func(t *testing.T) { + assert.Equal(t, getExitCode(errors.New("some failure")), 1) + }) + + t.Run("StatusError with code", func(t *testing.T) { + err := dockercli.StatusError{StatusCode: 42} + assert.Equal(t, getExitCode(err), 42) + }) + + t.Run("StatusError with zero code falls back to 1", func(t *testing.T) { + err := dockercli.StatusError{StatusCode: 0, Status: "something went wrong"} + assert.Equal(t, getExitCode(err), 1) + }) + + t.Run("wrapped StatusError", func(t *testing.T) { + err := fmt.Errorf("wrapper: %w", dockercli.StatusError{StatusCode: 99}) + assert.Equal(t, getExitCode(err), 99) + }) + + t.Run("SIGINT returns 130", func(t *testing.T) { + err := errCtxSignalTerminated{signal: syscall.SIGINT} + assert.Equal(t, getExitCode(err), 130) + }) + + t.Run("SIGTERM returns 143", func(t *testing.T) { + err := errCtxSignalTerminated{signal: syscall.SIGTERM} + assert.Equal(t, getExitCode(err), 143) + }) +} + func TestVisitAll(t *testing.T) { root := &cobra.Command{Use: "root"} sub1 := &cobra.Command{Use: "sub1"} From c04e639318d8cd07630863fb4383deab115ded57 Mon Sep 17 00:00:00 2001 From: Derek Misler Date: Thu, 12 Feb 2026 11:20:25 -0500 Subject: [PATCH 2/2] error-hooks approach Signed-off-by: Derek Misler --- cli-plugins/manager/hooks.go | 31 +++++- cli-plugins/manager/hooks_test.go | 179 +++++++++++++++++++++++++++--- cmd/docker/docker.go | 27 +++-- 3 files changed, 209 insertions(+), 28 deletions(-) diff --git a/cli-plugins/manager/hooks.go b/cli-plugins/manager/hooks.go index b8b55d5e9ed8..6a3212315f9a 100644 --- a/cli-plugins/manager/hooks.go +++ b/cli-plugins/manager/hooks.go @@ -70,7 +70,7 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root pluginDirs := getPluginDirs(cfg) nextSteps := make([]string, 0, len(pluginsCfg)) for pluginName, pluginCfg := range pluginsCfg { - match, ok := pluginMatch(pluginCfg, subCmdStr) + match, ok := pluginMatch(pluginCfg, subCmdStr, cmdErrorMessage) if !ok { continue } @@ -138,13 +138,34 @@ func appendNextSteps(nextSteps []string, processed []string) ([]string, bool) { // command being executed (such as 'image ls' – the root 'docker' is omitted) // and, if the configuration includes a hook for the invoked command, returns // the configured hook string. -func pluginMatch(pluginCfg map[string]string, subCmd string) (string, bool) { - configuredPluginHooks, ok := pluginCfg["hooks"] - if !ok || configuredPluginHooks == "" { +// +// Plugins can declare two types of hooks in their configuration: +// - "hooks": fires on every command invocation (success or failure) +// - "error-hooks": fires only when a command fails (cmdErrorMessage is non-empty) +func pluginMatch(pluginCfg map[string]string, subCmd string, cmdErrorMessage string) (string, bool) { + // Check "hooks" first — these always fire regardless of command outcome. + if match, ok := matchHookConfig(pluginCfg["hooks"], subCmd); ok { + return match, true + } + + // Check "error-hooks" — these only fire when there was an error. + if cmdErrorMessage != "" { + if match, ok := matchHookConfig(pluginCfg["error-hooks"], subCmd); ok { + return match, true + } + } + + return "", false +} + +// matchHookConfig checks if a comma-separated hook configuration string +// contains a prefix match for the given subcommand. +func matchHookConfig(configuredHooks string, subCmd string) (string, bool) { + if configuredHooks == "" { return "", false } - for hookCmd := range strings.SplitSeq(configuredPluginHooks, ",") { + for hookCmd := range strings.SplitSeq(configuredHooks, ",") { if hookMatch(hookCmd, subCmd) { return hookCmd, true } diff --git a/cli-plugins/manager/hooks_test.go b/cli-plugins/manager/hooks_test.go index aa911ec697ce..e39a1383118f 100644 --- a/cli-plugins/manager/hooks_test.go +++ b/cli-plugins/manager/hooks_test.go @@ -51,12 +51,15 @@ func TestGetNaiveFlags(t *testing.T) { func TestPluginMatch(t *testing.T) { testCases := []struct { - commandString string - pluginConfig map[string]string - expectedMatch string - expectedOk bool + doc string + commandString string + pluginConfig map[string]string + cmdErrorMessage string + expectedMatch string + expectedOk bool }{ { + doc: "hooks prefix match", commandString: "image ls", pluginConfig: map[string]string{ "hooks": "image", @@ -65,6 +68,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: true, }, { + doc: "hooks no match", commandString: "context ls", pluginConfig: map[string]string{ "hooks": "build", @@ -73,6 +77,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: false, }, { + doc: "hooks exact match", commandString: "context ls", pluginConfig: map[string]string{ "hooks": "context ls", @@ -81,6 +86,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: true, }, { + doc: "hooks first match wins", commandString: "image ls", pluginConfig: map[string]string{ "hooks": "image ls,image", @@ -89,6 +95,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: true, }, { + doc: "hooks empty string", commandString: "image ls", pluginConfig: map[string]string{ "hooks": "", @@ -97,6 +104,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: false, }, { + doc: "hooks partial token no match", commandString: "image inspect", pluginConfig: map[string]string{ "hooks": "image i", @@ -105,6 +113,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: false, }, { + doc: "hooks prefix token match", commandString: "image inspect", pluginConfig: map[string]string{ "hooks": "image", @@ -112,12 +121,140 @@ func TestPluginMatch(t *testing.T) { expectedMatch: "image", expectedOk: true, }, + { + doc: "error-hooks match on error", + commandString: "build", + pluginConfig: map[string]string{ + "error-hooks": "build", + }, + cmdErrorMessage: "exit status 1", + expectedMatch: "build", + expectedOk: true, + }, + { + doc: "error-hooks no match on success", + commandString: "build", + pluginConfig: map[string]string{ + "error-hooks": "build", + }, + cmdErrorMessage: "", + expectedMatch: "", + expectedOk: false, + }, + { + doc: "error-hooks prefix match on error", + commandString: "compose up", + pluginConfig: map[string]string{ + "error-hooks": "compose", + }, + cmdErrorMessage: "exit status 1", + expectedMatch: "compose", + expectedOk: true, + }, + { + doc: "error-hooks no match for wrong command", + commandString: "pull", + pluginConfig: map[string]string{ + "error-hooks": "build", + }, + cmdErrorMessage: "exit status 1", + expectedMatch: "", + expectedOk: false, + }, + { + doc: "hooks takes precedence over error-hooks", + commandString: "build", + pluginConfig: map[string]string{ + "hooks": "build", + "error-hooks": "build", + }, + cmdErrorMessage: "exit status 1", + expectedMatch: "build", + expectedOk: true, + }, + { + doc: "hooks fires on success even with error-hooks configured", + commandString: "build", + pluginConfig: map[string]string{ + "hooks": "build", + "error-hooks": "build", + }, + cmdErrorMessage: "", + expectedMatch: "build", + expectedOk: true, + }, + { + doc: "error-hooks with multiple commands", + commandString: "compose up", + pluginConfig: map[string]string{ + "error-hooks": "build,compose up,pull", + }, + cmdErrorMessage: "exit status 1", + expectedMatch: "compose up", + expectedOk: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.doc, func(t *testing.T) { + match, ok := pluginMatch(tc.pluginConfig, tc.commandString, tc.cmdErrorMessage) + assert.Equal(t, ok, tc.expectedOk) + assert.Equal(t, match, tc.expectedMatch) + }) + } +} + +func TestMatchHookConfig(t *testing.T) { + testCases := []struct { + doc string + configuredHooks string + subCmd string + expectedMatch string + expectedOk bool + }{ + { + doc: "empty config", + configuredHooks: "", + subCmd: "build", + expectedMatch: "", + expectedOk: false, + }, + { + doc: "exact match", + configuredHooks: "build", + subCmd: "build", + expectedMatch: "build", + expectedOk: true, + }, + { + doc: "prefix match", + configuredHooks: "image", + subCmd: "image ls", + expectedMatch: "image", + expectedOk: true, + }, + { + doc: "comma-separated match", + configuredHooks: "pull,build,push", + subCmd: "build", + expectedMatch: "build", + expectedOk: true, + }, + { + doc: "no match", + configuredHooks: "pull,push", + subCmd: "build", + expectedMatch: "", + expectedOk: false, + }, } for _, tc := range testCases { - match, ok := pluginMatch(tc.pluginConfig, tc.commandString) - assert.Equal(t, ok, tc.expectedOk) - assert.Equal(t, match, tc.expectedMatch) + t.Run(tc.doc, func(t *testing.T) { + match, ok := matchHookConfig(tc.configuredHooks, tc.subCmd) + assert.Equal(t, ok, tc.expectedOk) + assert.Equal(t, match, tc.expectedMatch) + }) } } @@ -170,21 +307,37 @@ func TestRunPluginHooksPassesErrorMessage(t *testing.T) { RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "exit status 1") } -func TestInvokeAndCollectHooksForwardsErrorMessage(t *testing.T) { +func TestRunPluginHooksErrorHooks(t *testing.T) { cfg := configfile.New("") cfg.Plugins = map[string]map[string]string{ - "nonexistent": {"hooks": "build"}, + "test-plugin": {"error-hooks": "build"}, } + provider := &fakeConfigProvider{cfg: cfg} root := &cobra.Command{Use: "docker"} sub := &cobra.Command{Use: "build"} root.AddCommand(sub) - // Plugin binary doesn't exist — invokeAndCollectHooks skips it - // gracefully and returns empty. Verifies the error message path - // doesn't cause issues when forwarded through the call chain. + // Should not panic — error-hooks with error message + RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "exit status 1") + + // Should not panic — error-hooks with no error (should be skipped) + RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "") +} + +func TestInvokeAndCollectHooksErrorHooksSkippedOnSuccess(t *testing.T) { + cfg := configfile.New("") + cfg.Plugins = map[string]map[string]string{ + "nonexistent": {"error-hooks": "build"}, + } + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + + // On success, error-hooks should not match, so the plugin + // binary is never looked up and no results are returned. result := invokeAndCollectHooks( context.Background(), cfg, root, sub, - "build", map[string]string{}, "exit status 1", + "build", map[string]string{}, "", ) assert.Check(t, is.Len(result, 0)) } diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 56223a5815ed..4936f2b28e48 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -119,6 +119,20 @@ func getExitCode(err error) int { return 1 } +// cmdErrorMessage extracts an error message suitable for passing to plugin +// hooks. If the error's message is empty (e.g. a StatusError with only an +// exit code), it falls back to a generic message so that hooks can still +// detect that the command failed. +func cmdErrorMessage(err error) string { + if err == nil { + return "" + } + if msg := err.Error(); msg != "" { + return msg + } + return fmt.Sprintf("exited with code %d", getExitCode(err)) +} + func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand { var ( opts *cliflags.ClientOptions @@ -480,11 +494,8 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { subCommand = ccmd if err != nil || pluginmanager.IsPluginCommand(ccmd) { err := tryPluginRun(ctx, dockerCli, cmd, args[0], envs) - if ccmd != nil && dockerCli.Out().IsTerminal() && dockerCli.HooksEnabled() { - var errMessage string - if err != nil { - errMessage = err.Error() - } + if ccmd != nil && dockerCli.Out().IsTerminal() && dockerCli.HooksEnabled() && !errdefs.IsNotFound(err) { + errMessage := cmdErrorMessage(err) pluginmanager.RunPluginHooks(ctx, dockerCli, cmd, ccmd, args, errMessage) } if err == nil { @@ -511,11 +522,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { // If the command is being executed in an interactive terminal // and hook are enabled, run the plugin hooks. if subCommand != nil && dockerCli.Out().IsTerminal() && dockerCli.HooksEnabled() { - var errMessage string - if err != nil { - errMessage = err.Error() - } - pluginmanager.RunCLICommandHooks(ctx, dockerCli, cmd, subCommand, errMessage) + pluginmanager.RunCLICommandHooks(ctx, dockerCli, cmd, subCommand, cmdErrorMessage(err)) } return err