Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cli-plugins/hooks/hook_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ package hooks
type ResponseType int

const (
// NextSteps renders suggestions under a shared "What's next:" header.
NextSteps ResponseType = 0
// GenericMessage renders before NextSteps, without a header or indentation.
// CLI versions that do not recognize this type ignore the response.
GenericMessage ResponseType = 1
)

// Request is the type representing the information
Expand Down
45 changes: 35 additions & 10 deletions cli-plugins/hooks/printer.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,42 @@ const (
indent = " "
)

// PrintNextSteps renders list of [NextSteps] messages and writes them
// to out. It is a no-op if messages is empty.
func PrintNextSteps(out io.Writer, messages []string) {
if len(messages) == 0 {
return
// PrintMessages groups messages by type and writes them to out. Generic messages
// appear first, without a header or indentation, followed by next steps under a
// shared "What's next:" header. Order within each type is preserved.
// Messages with no lines or an unknown type produce no output.
func PrintMessages(out io.Writer, messages []Response, evaluate func(Response) []string) {
grouped := make(map[ResponseType][]string)
for _, message := range messages {
lines := evaluate(message)
grouped[message.Type] = append(grouped[message.Type], lines...)
}
for _, responseType := range []ResponseType{GenericMessage, NextSteps} {
lines := grouped[responseType]
if len(lines) == 0 {
continue
}

_, _ = io.WriteString(out, whatsNext)
for _, msg := range messages {
_, _ = io.WriteString(out, indent)
_, _ = io.WriteString(out, msg)
_, _ = io.WriteString(out, "\n")
header, prefix := "\n", ""
if responseType == NextSteps {
header, prefix = whatsNext, indent
}

_, _ = io.WriteString(out, header)
for _, line := range lines {
_, _ = io.WriteString(out, prefix)
_, _ = io.WriteString(out, line)
_, _ = io.WriteString(out, "\n")
}
}
}

// PrintNextSteps renders list of [NextSteps] messages and writes them
// to out. It is a no-op if messages is empty.
//
// Deprecated: use [PrintMessages] instead.
func PrintNextSteps(out io.Writer, messages []string) {
PrintMessages(out, []Response{{Type: NextSteps}}, func(Response) []string {
return messages
})
}
85 changes: 84 additions & 1 deletion cli-plugins/hooks/printer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,85 @@ import (
"gotest.tools/v3/assert"
)

func TestPrintHookMessages(t *testing.T) {
func TestPrintMessages(t *testing.T) {
const header = "\n\x1b[1mWhat's next:\x1b[0m\n"

for _, tc := range []struct {
doc string
messages []hooks.Response
expected string
}{
{doc: "no messages"},
{
doc: "no lines",
messages: []hooks.Response{
{Type: hooks.GenericMessage},
{Type: hooks.NextSteps},
},
},
{
doc: "generic message",
messages: []hooks.Response{
{Type: hooks.GenericMessage, Template: "Session active"},
},
expected: "\nSession active\n",
},
{
doc: "preserve message formatting",
messages: []hooks.Response{
{Type: hooks.GenericMessage, Template: "Session active\n\n Details"},
},
expected: "\nSession active\n\n Details\n",
},
{
doc: "next steps",
messages: []hooks.Response{
{Type: hooks.NextSteps, Template: "Try another command"},
},
expected: header + " Try another command\n",
},
{
doc: "group interleaved types and preserve order within each type",
messages: []hooks.Response{
{Type: hooks.NextSteps, Template: "First suggestion"},
{Type: hooks.GenericMessage, Template: "Session active"},
{Type: hooks.NextSteps, Template: "Second suggestion\nThird suggestion"},
{Type: hooks.GenericMessage, Template: "Another status"},
},
expected: "\nSession active\nAnother status\n" + header + " First suggestion\n Second suggestion\n Third suggestion\n",
},
{
doc: "unknown type",
messages: []hooks.Response{
{Type: hooks.ResponseType(99), Template: "Ignored"},
},
},
{
doc: "unknown type does not affect known messages",
messages: []hooks.Response{
{Type: hooks.ResponseType(99), Template: "Ignored"},
{Type: hooks.GenericMessage, Template: "Session active"},
{Type: hooks.NextSteps, Template: "Try another command"},
},
expected: "\nSession active\n" + header + " Try another command\n",
},
} {
t.Run(tc.doc, func(t *testing.T) {
var w strings.Builder
hooks.PrintMessages(&w, tc.messages, func(message hooks.Response) []string {
if message.Template == "" {
return nil
}
lines, err := hooks.ParseTemplate(message.Template, nil)
assert.NilError(t, err)
return lines
})
assert.Equal(t, w.String(), tc.expected)
})
}
}

func TestPrintNextSteps(t *testing.T) {
const header = "\n\x1b[1mWhat's next:\x1b[0m\n"

tests := []struct {
Expand All @@ -34,6 +112,11 @@ func TestPrintHookMessages(t *testing.T) {
" Foo\n" +
" bar\n",
},
{
doc: "preserve empty and indented lines",
messages: []string{"", " Details", ""},
expectedOutput: header + " \n Details\n \n",
},
}
for _, tc := range tests {
t.Run(tc.doc, func(t *testing.T) {
Expand Down
12 changes: 11 additions & 1 deletion cli-plugins/hooks/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"bytes"
"errors"
"fmt"
"slices"
"strings"
"text/template"

Expand Down Expand Up @@ -43,7 +44,16 @@ func ParseTemplate(hookTemplate string, cmd *cobra.Command) ([]string, error) {
if n := strings.Count(out, "\n"); n > maxMessages {
return nil, fmt.Errorf("hook template contains too many messages (%d): maximum is %d", n, maxMessages)
}
return strings.SplitN(out, "\n", maxMessages), nil
lines := strings.SplitN(out, "\n", maxMessages)

// Skip messages containing only whitespace, but preserve empty lines
// in messages that have content (e.g., for paragraph spacing).
if !slices.ContainsFunc(lines, func(line string) bool {
return strings.TrimSpace(line) != ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] Whitespace-only template result drops silently — debug log no longer emitted

ParseTemplate returns (nil, nil) when the rendered template contains only whitespace (lines 51–54). The evaluate callback in runHooks (in manager/hooks.go) logs only on err != nil; a (nil, nil) return silently produces no output from PrintMessages. The behavior is correct — ignoring whitespace-only output is intentional — but the diagnostic that was previously emitted ("Plugin responded with an empty hook message; ignoring" from the old appendNextSteps) is now gone.

A developer investigating why a plugin's hook produces no visible output will no longer find any log line pointing to the whitespace-only template result. Consider adding a debug log in the runHooks evaluate callback alongside the existing error log, e.g.:

if lines == nil {
    logrus.WithField("plugin", ...).Debug("Plugin hook template rendered to whitespace-only; ignoring")
}
Confidence Score
🟡 moderate 57/100

}) {
return nil, nil
}
return lines, nil
}

var ErrHookTemplateParse = errors.New("failed to parse hook template")
Expand Down
12 changes: 11 additions & 1 deletion cli-plugins/hooks/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,17 @@ func TestParseTemplate(t *testing.T) {
{
doc: "empty template",
template: "",
expectedOutput: []string{""},
expectedOutput: nil,
},
{
doc: "only empty lines",
template: "\n\n",
expectedOutput: nil,
},
{
doc: "whitespace-only lines",
template: " \n\t\n ",
expectedOutput: nil,
},
{
doc: "plain message",
Expand Down
70 changes: 23 additions & 47 deletions cli-plugins/manager/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,18 @@ func RunPluginHooks(ctx context.Context, dockerCLI config.Provider, rootCmd, sub
}

func runHooks(ctx context.Context, cfg *configfile.ConfigFile, rootCmd, subCommand *cobra.Command, invokedCommand string, flags map[string]string, cmdErrorMessage string) {
nextSteps := invokeAndCollectHooks(ctx, cfg, rootCmd, subCommand, invokedCommand, flags, cmdErrorMessage)
hooks.PrintNextSteps(subCommand.ErrOrStderr(), nextSteps)
messages := invokeAndCollectHooks(ctx, cfg, rootCmd, invokedCommand, flags, cmdErrorMessage)
hooks.PrintMessages(subCommand.ErrOrStderr(), messages, func(message hooks.Response) []string {
lines, err := hooks.ParseTemplate(message.Template, subCommand)
if err != nil {
logrus.WithError(err).Debug("Failed to evaluate hook template")
return nil
}
return lines
})
}

func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, rootCmd, subCmd *cobra.Command, subCmdStr string, flags map[string]string, cmdErrorMessage string) []string {
func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, rootCmd *cobra.Command, subCmdStr string, flags map[string]string, cmdErrorMessage string) []hooks.Response {
if ctx.Err() != nil {
return nil
}
Expand All @@ -64,17 +71,17 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root
}

pluginDirs := getPluginDirs(cfg)
nextSteps := make([]string, 0, len(pluginsCfg))
var collected []hooks.Response

tryInvokeHook := func(pluginName string, pluginCfg map[string]string) (messages []string, ok bool, err error) {
tryInvokeHook := func(pluginName string, pluginCfg map[string]string) (hooks.Response, error) {
match, matched := pluginMatch(pluginCfg, subCmdStr, cmdErrorMessage)
if !matched {
return nil, false, nil
return hooks.Response{}, nil
}

p, err := getPlugin(pluginName, pluginDirs, rootCmd)
if err != nil {
return nil, false, err
return hooks.Response{}, err
}

resp, err := p.RunHook(ctx, hooks.Request{
Expand All @@ -83,29 +90,23 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root
CommandError: cmdErrorMessage,
})
if err != nil {
return nil, false, err
return hooks.Response{}, err
}

var message hooks.Response
if err := json.Unmarshal(resp, &message); err != nil {
return nil, false, fmt.Errorf("failed to unmarshal hook response (%q): %w", string(resp), err)
}

// currently the only hook type
if message.Type != hooks.NextSteps {
return nil, false, errors.New("unexpected hook response type: " + strconv.Itoa(int(message.Type)))
return hooks.Response{}, fmt.Errorf("failed to unmarshal hook response (%q): %w", string(resp), err)
}

messages, err = hooks.ParseTemplate(message.Template, subCmd)
if err != nil {
return nil, false, err
if message.Type != hooks.NextSteps && message.Type != hooks.GenericMessage {
return hooks.Response{}, errors.New("unexpected hook response type: " + strconv.Itoa(int(message.Type)))
}

return messages, true, nil
return message, nil
}

for pluginName, pluginCfg := range pluginsCfg {
messages, ok, err := tryInvokeHook(pluginName, pluginCfg)
message, err := tryInvokeHook(pluginName, pluginCfg)
if err != nil {
// skip misbehaving plugins, but don't halt execution
logrus.WithFields(logrus.Fields{
Expand All @@ -114,38 +115,13 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root
}).Debug("Plugin hook invocation failed")
continue
}
if !ok {
if message.Template == "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] Silent skip of empty-template responses loses the previous debug log

When a plugin returns a valid JSON response with an empty Template field, the new code silently continues without emitting any diagnostic. The previous implementation had a debug-level log entry (via the !ok path from pluginMatch) that helped developers trace why a plugin's hook output was suppressed.

While the skipping behavior is correct, the missing log reduces observability: a developer investigating why a plugin's hook produces no output will no longer see any log line pointing to the empty Template field.

Consider adding a debug log alongside the guard, similar to the existing invocation-failure log above it:

Suggested change
if message.Template == "" {
if message.Template == "" {
logrus.WithField("plugin", pluginName).Debug("Plugin responded with an empty hook message; ignoring")
continue
}
Confidence Score
🟢 strong 95/100

continue
}

var appended bool
nextSteps, appended = appendNextSteps(nextSteps, messages)
if !appended {
logrus.WithFields(logrus.Fields{
"plugin": pluginName,
}).Debug("Plugin responded with an empty hook message; ignoring")
}
}
return nextSteps
}

// appendNextSteps appends the processed hook output to the nextSteps slice.
// If the processed hook output is empty, it is not appended.
// Empty lines are not stripped if there's at least one non-empty line.
func appendNextSteps(nextSteps []string, processed []string) ([]string, bool) {
empty := true
for _, l := range processed {
if strings.TrimSpace(l) != "" {
empty = false
break
}
}

if empty {
return nextSteps, false
collected = append(collected, message)
}

return append(nextSteps, processed...), true
return collected
}

// pluginMatch takes a plugin configuration and a string representing the
Expand Down
Loading
Loading