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

[derekmisler]

This PR is part of a trilogy:

1. docker/cli <-- you are here
2. docker/ai
3. docker/pinata

---

Closes https://github.com/docker/gordon/issues/125

- What I did

Enabled CLI plugin hooks to fire on command failure, not just success. Previously, when a plugin-delegated command like docker build (buildx) or docker compose up failed, the hooks system was skipped entirely. This meant plugins like ai, scout, and debug could never show "What's next:" hints for failed builds or compose errors, which is exactly when our users would benefit from a docker ai "fix my problem" command.

- How I did it

Two changes:

1. cli-plugins/manager/hooks.go: Added cmdErrorMessage string parameter to RunPluginHooks, forwarding it to runHooks instead of hardcoding "". This makes it symmetric with RunCLICommandHooks, which already accepts error messages.
2. cmd/docker/docker.go: Moved the plugin hooks invocation outside the if err == nil block so it fires regardless of plugin exit status. The error message is extracted the same way the native command path already does it.

- How to verify it

1. Register a plugin with hooks for a plugin-delegated command (e.g., buildx build):

# In ~/.docker/config.json, ensure:
# "features": { "hooks": "true" }
# "plugins": { "<plugin>": { "hooks": "buildx build" } }

2. Before this change — run a failing build, observe no "What's next:" hints:

echo "FROM nonexistent:image" > /tmp/Dockerfile
docker build /tmp
# No hints shown

3. After this change — same failing build now shows plugin hints:

docker build /tmp
# "What's next:" hints from registered plugins appear

4. Verify successful commands still show hints (no regression):

docker build -t test -f /dev/null .  # or any successful build
# Hooks fire with empty error message, same as before

5. Verify native commands are unaffected:

docker run nonexistent:image
# Hooks fire on failure (worked before, still works)

[codecov-commenter]

Codecov Report

❌ Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/docker/docker.go	0.00%	6 Missing ⚠️

📢 Thoughts on this report? Let us know!

[thaJeztah]

cc @vvoland @laurazard looks related to what was implemented in;

• hooks: pass command execution error to plugins #5033

[derekmisler]

cc @vvoland @laurazard looks related to what was implemented in;

• hooks: pass command execution error to plugins #5033

Yeah, that PR was helpful when working on this one. I saw we were hard-coding an empty "" in place of the error here: https://github.com/docker/cli/pull/5033/changes#diff-7e76b3d50db2ee696d1cdbaa3f1e429b179da962bb8f308960ef85bc0ba835a2R43

[derekmisler]

I'm not sure about that e2e test, @thaJeztah, it passes sometimes and fails others, and I can't figure out why.

[thaJeztah]

Yeah, that test is rather flaky; not an issue with this PR; I restarted CI.

Yeah, that PR was helpful when working on this one. I saw we were hard-coding an empty "" in place of the error here

Yup; I mostly pinged @vvoland and @laurazard to check if they recalled there was a specific reason; there was this line in the PR description;

This is only available for CLI command executions.

And I do recall there was some chatter at the the about some risks with hook being triggered recursively (with a growing number of plugins, there's also a growing amount of overhead).

[thaJeztah]

Just so I don't forget; this function is exported (and may be in use elsewhere), so we probably can't change the signature.

[derekmisler]

This was the only implementation site I could find: https://github.com/docker/cli/pull/6794/changes#diff-3dcd6325b70a5f5db393a3ecc08060b69007fcef371fb4dabf3164da95646fb6R488

[thaJeztah]

Ah, yes, maybe we're fine; at least nothing in a public repository AFAICS; https://grep.app/search?q=.RunPluginHooks

We should probably consider to move some of this to internal/, and make the "public" (cli-plugins/xxxx) wrappers for the internal implementation so that we have a better separation between "these are only to be used by the CLI itself" and "these are "ok(ish)" for external consumers". We've been painted in a corner too many times because things were meant for the CLI itself, but happened to be exported, and now ... got used by other projects.

[thaJeztah]

Forgot to post this, but in case it's useful information; I was curious what the performance impact would be. For that I chose a "minimal" call to some plugins; I picked <plugin> version, which should be very lightweight, and (I expect) not be making API calls.

First, timing when running that command as "plugin" (docker <plugin-name> version); note that some of the overhead here is in the CLI, which may still needs optimization (it currently scans for CLI plugin candidates that can provide a subcommand, so effectively, all plugins may get invoked here for the "metadata" subcommand). I also set a remote context (-c remote1), pointing ao a ssh:// connection;

hyperfine --runs 5 --warmup 2 'docker -c remote1 --version'
Benchmark 1: docker -c remote1 --version
  Time (mean ± σ):      20.5 ms ±   2.0 ms    [User: 10.7 ms, System: 8.5 ms]
  Range (min … max):    17.5 ms …  23.2 ms    5 runs

hyperfine --runs 5 --warmup 2 'docker -c remote1 compose version'
Benchmark 1: docker -c remote1 compose version
  Time (mean ± σ):      53.1 ms ±   2.4 ms    [User: 19.4 ms, System: 13.2 ms]
  Range (min … max):    50.0 ms …  55.5 ms    5 runs

hyperfine --runs 5 --warmup 2 'docker -c remote1 ai version'
Benchmark 1: docker -c remote1 ai version
  Time (mean ± σ):      72.6 ms ±   5.1 ms    [User: 31.8 ms, System: 17.0 ms]
  Range (min … max):    65.2 ms …  78.4 ms    5 runs

hyperfine --runs 5 --warmup 2 'docker -c remote1 buildx version'
Benchmark 1: docker -c remote1 buildx version
  Time (mean ± σ):      76.7 ms ±   3.7 ms    [User: 37.4 ms, System: 19.8 ms]
  Range (min … max):    71.7 ms …  81.2 ms    5 runs

Also looking at running the same command, but invoking the plugins directly;

hyperfine --runs 5 --warmup 2 'docker --version'
Benchmark 1: docker --version
  Time (mean ± σ):      13.6 ms ±   1.9 ms    [User: 7.1 ms, System: 5.3 ms]
  Range (min … max):    11.8 ms …  16.7 ms    5 runs

hyperfine --runs 5 --warmup 2 '~/.docker/cli-plugins/docker-compose version'
Benchmark 1: ~/.docker/cli-plugins/docker-compose version
  Time (mean ± σ):      17.9 ms ±   3.1 ms    [User: 6.5 ms, System: 3.4 ms]
  Range (min … max):    15.7 ms …  23.3 ms    5 runs

hyperfine --runs 5 --warmup 2 '~/.docker/cli-plugins/docker-ai version'
Benchmark 1: ~/.docker/cli-plugins/docker-ai version
  Time (mean ± σ):      28.4 ms ±   1.6 ms    [User: 12.7 ms, System: 5.3 ms]
  Range (min … max):    25.9 ms …  29.8 ms    5 runs

hyperfine --runs 5 --warmup 2 '~/.docker/cli-plugins/docker-buildx version'
Benchmark 1: ~/.docker/cli-plugins/docker-buildx version
  Time (mean ± σ):      54.1 ms ±  13.0 ms    [User: 20.3 ms, System: 13.4 ms]
  Range (min … max):    38.6 ms …  74.6 ms    5 runs

Looks like the ai plugin is not the slowest, but still takes twice as much as the CLI itself (not sure why buildx is so slow though; that's surprising!)

[thaJeztah]

This extra overhead may be less problematic in the "unhappy" path (something failed), although it's possible there's scripts that run a command to detect errors; I THINK (but must double-check) we skip hooks if there's no TTY attached, but it's possible we invoke the hook, but don't print the output.

The overhead is of course also "relative" to the command executed; 50ms of extra time probably doesn't matter on a failing docker pull;

hyperfine --runs 5 --warmup 2 --ignore-failure 'docker pull no-sucn-image'
Benchmark 1: docker pull no-sucn-image
  Time (mean ± σ):     822.0 ms ±  62.9 ms    [User: 25.6 ms, System: 28.1 ms]
  Range (min … max):   765.2 ms … 930.0 ms    5 runs

  Warning: Ignoring non-zero exit code.
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

However the way hooks currently work is that they must be registered for each command they should be called on. This means that if we want the ai plugin to be called for assistance on any command, it would be invoked after every command; regardless if the command was successful or failed. That means that setting a hook on every command would add at least 50ms extra; that's based on running the version subcommand, so it'll probably be more.

One thing we could consider is to introduce a separate config for "error-hooks"; i.e., only invoke the hook for errors, but not for the "happy path"; currently hooks are defined like this;

        "plugins": {
                "debug": {
                        "hooks": "exec"
                },
                "scout": {
                        "hooks": "pull,image pull,buildx build"
                }
        },
        "features": {
                "hooks": "true"
        }

We could add a similar config (error-hooks, failure-hooks ?); note that adding hooks for all commands would be ... slightly tedious (every command has to be enumerated in the list); not sure what's best for that; we could have a "error-handler" or wildcard support, but that may become a sliding slope.

There's another thing we must verify (just thinking out loud here, haven't thought it through, and haven't looked at the logic);

• docker compose build nowadays also hands off builds to buildx, and (similar to the CLI itself) invokes buildx as a plugin (docker compose build -> invokes ~/.docker/cli-plugins/docker-compose build -> invokes ~/.docker/cli-plugins/docker-buildx bake). I wonder how hooks are called in that scenario (could it invoke hooks multiple times, recurse?)
• similar to the above; how do we currently handle failures when invoking a hook? (we must make sure we don't trigger a "error-hook" if a "hook" or "error-hook" failed 😂)

[vvoland]

+1 on error hooks for this one