Merge pull request #6794 from derekmisler/cli-hints-for-docker-ai-after-buildcompose-failur

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

Author: thaJeztah

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

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
@@ -40,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",
@@ -54,6 +68,7 @@ func TestPluginMatch(t *testing.T) {
 			expectedOk:    true,
 		},
 		{
+			doc:           "hooks no match",
 			commandString: "context ls",
 			pluginConfig: map[string]string{
 				"hooks": "build",
@@ -62,6 +77,7 @@ func TestPluginMatch(t *testing.T) {
 			expectedOk:    false,
 		},
 		{
+			doc:           "hooks exact match",
 			commandString: "context ls",
 			pluginConfig: map[string]string{
 				"hooks": "context ls",
@@ -70,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",
@@ -78,6 +95,7 @@ func TestPluginMatch(t *testing.T) {
 			expectedOk:    true,
 		},
 		{
+			doc:           "hooks empty string",
 			commandString: "image ls",
 			pluginConfig: map[string]string{
 				"hooks": "",
@@ -86,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",
@@ -94,19 +113,148 @@ func TestPluginMatch(t *testing.T) {
 			expectedOk:    false,
 		},
 		{
+			doc:           "hooks prefix token match",
 			commandString: "image inspect",
 			pluginConfig: map[string]string{
 				"hooks": "image",
 			},
 			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)
+		})
 	}
 }
 
@@ -141,3 +289,87 @@ 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 TestRunPluginHooksErrorHooks(t *testing.T) {
+	cfg := configfile.New("")
+	cfg.Plugins = map[string]map[string]string{
+		"test-plugin": {"error-hooks": "build"},
+	}
+	provider := &fakeConfigProvider{cfg: cfg}
+	root := &cobra.Command{Use: "docker"}
+	sub := &cobra.Command{Use: "build"}
+	root.AddCommand(sub)
+
+	// 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{}, "",
+	)
+	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))
+}