Fix: run plugin hooks on command failure, not just success

Signed-off-by: Derek Misler <derek.misler@docker.com>

Author: derekmisler

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) {

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))
+}

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) {

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"}