[RFC] errors: introduce structured hints + apply audit-driven rewrites

cli-plugins/plugin/plugin.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"sync"
 
@@ -14,6 +15,7 @@ import (
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/connhelper"
 	"github.com/docker/cli/cli/debug"
+	"github.com/docker/cli/internal/hint"
 	"github.com/moby/moby/client"
 	"github.com/spf13/cobra"
 	"go.opentelemetry.io/otel"
@@ -104,14 +106,21 @@ func Run(makeCmd func(command.Cli) *cobra.Command, meta metadata.Metadata, ops .
 			if stErr.StatusCode == 0 { // FIXME(thaJeztah): this should never be used with a zero status-code. Check if we do this anywhere.
 				stErr.StatusCode = 1
 			}
-			_, _ = fmt.Fprintln(dockerCLI.Err(), stErr)
+			printError(dockerCLI.Err(), stErr)
 			os.Exit(stErr.StatusCode)
 		}
-		_, _ = fmt.Fprintln(dockerCLI.Err(), err)
+		printError(dockerCLI.Err(), err)
 		os.Exit(1)
 	}
 }
 
+func printError(out io.Writer, err error) {
+	_, _ = fmt.Fprintln(out, err)
+	if h := hint.Of(err); h != "" {
+		_, _ = fmt.Fprintln(out, "\n"+h)
+	}
+}
+
 func withPluginClientConn(name string) command.CLIOption {
 	return func(cli *command.DockerCli) error {
 		cmd := "docker"

cli/cobra.go

@@ -82,6 +82,7 @@ func FlagErrorFunc(cmd *cobra.Command, err error) error {
 	}
 
 	return StatusError{
+		Cause:      err,
 		Status:     fmt.Sprintf("%s\n\nUsage:  %s\n\nRun '%s --help' for more information", err, cmd.UseLine(), cmd.CommandPath()),
 		StatusCode: 125,
 	}

cli/command/config/inspect.go

@@ -67,7 +67,7 @@ func runInspect(ctx context.Context, dockerCLI command.Cli, opts inspectOptions)
 	}
 
 	if err := inspectFormatWrite(configCtx, opts.names, getRef); err != nil {
-		return cli.StatusError{StatusCode: 1, Status: err.Error()}
+		return cli.StatusError{Cause: err, StatusCode: 1, Status: err.Error()}
 	}
 	return nil
 }

cli/command/container/cp.go

@@ -16,6 +16,7 @@ import (
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/streams"
+	"github.com/docker/cli/internal/hint"
 	"github.com/docker/go-units"
 	"github.com/moby/go-archive"
 	"github.com/moby/moby/client"
@@ -242,7 +243,10 @@ func runCopy(ctx context.Context, dockerCli command.Cli, opts copyOptions) error
 	case acrossContainers:
 		return errors.New("copying between containers is not supported")
 	default:
-		return errors.New("must specify at least one container source")
+		return hint.Wrap(
+			errors.New("one argument must reference a container as 'CONTAINER:PATH'"),
+			"Use 'docker cp CONTAINER:SRC LOCAL' to copy from a container, or 'docker cp LOCAL CONTAINER:DEST' to copy into one.",
+		)
 	}
 }
 

cli/command/container/cp_test.go

@@ -38,7 +38,7 @@ func TestRunCopyWithInvalidArguments(t *testing.T) {
 				source:      "./source",
 				destination: "./dest",
 			},
-			expectedErr: "must specify at least one container source",
+			expectedErr: "one argument must reference a container as 'CONTAINER:PATH'",
 		},
 	}
 	for _, testcase := range testcases {

cli/command/container/create.go

@@ -20,6 +20,7 @@ import (
 	"github.com/docker/cli/cli/config/configfile"
 	"github.com/docker/cli/cli/config/types"
 	"github.com/docker/cli/cli/streams"
+	"github.com/docker/cli/internal/hint"
 	"github.com/docker/cli/internal/jsonstream"
 	"github.com/docker/cli/opts"
 	"github.com/moby/moby/api/types/mount"
@@ -97,10 +98,7 @@ func newCreateCommand(dockerCLI command.Cli) *cobra.Command {
 
 func runCreate(ctx context.Context, dockerCLI command.Cli, flags *pflag.FlagSet, options *createOptions, copts *containerOptions) error {
 	if err := validatePullOpt(options.pull); err != nil {
-		return cli.StatusError{
-			Status:     withHelp(err, "create").Error(),
-			StatusCode: 125,
-		}
+		return statusErrorWithHelp(err, "create", 125)
 	}
 	proxyConfig := dockerCLI.ConfigFile().ParseProxyConfig(dockerCLI.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetSlice()))
 	newEnv := make([]string, 0, len(proxyConfig))
@@ -119,10 +117,7 @@ func runCreate(ctx context.Context, dockerCLI command.Cli, flags *pflag.FlagSet,
 
 	containerCfg, err := parse(flags, copts, serverInfo.OSType)
 	if err != nil {
-		return cli.StatusError{
-			Status:     withHelp(err, "create").Error(),
-			StatusCode: 125,
-		}
+		return statusErrorWithHelp(err, "create", 125)
 	}
 	id, err := createContainer(ctx, dockerCLI, containerCfg, options)
 	if err != nil {
@@ -188,7 +183,10 @@ func (cid *cidFile) Write(id string) error {
 		return nil
 	}
 	if _, err := cid.file.WriteString(id); err != nil {
-		return fmt.Errorf("failed to write the container ID (%s) to file: %w", id, err)
+		return hint.Wrap(
+			fmt.Errorf("container %s was created, but writing its ID to %s failed: %w", id, cid.path, err),
+			fmt.Sprintf("The container exists on the daemon. Run 'docker rm %s' to remove it, or note the ID for later use.", id),
+		)
 	}
 	cid.written = true
 	return nil

cli/command/container/create_test.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/config/configfile"
+	"github.com/docker/cli/internal/hint"
 	"github.com/docker/cli/internal/test"
 	"github.com/google/go-cmp/cmp"
 	"github.com/moby/moby/api/types/container"
@@ -200,6 +201,25 @@ func TestCreateContainerImagePullPolicyInvalid(t *testing.T) {
 	}
 }
 
+func TestCreateContainerPreservesHintedParseError(t *testing.T) {
+	flags, copts := setupRunFlags()
+	assert.NilError(t, flags.Parse([]string{"--pid=container:", "image"}))
+
+	dockerCli := test.NewFakeCli(&fakeClient{})
+	err := runCreate(
+		context.TODO(),
+		dockerCli,
+		flags,
+		&createOptions{},
+		copts,
+	)
+
+	statusErr := cli.StatusError{}
+	assert.Check(t, errors.As(err, &statusErr))
+	assert.Check(t, statusErr.Cause != nil)
+	assert.Equal(t, hint.Of(err), "Valid forms are 'host' or 'container:<name|id>'.")
+}
+
 func TestCreateContainerValidateFlags(t *testing.T) {
 	for _, tc := range []struct {
 		name        string

cli/command/container/exec.go

@@ -10,6 +10,7 @@ import (
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/command/completion"
 	"github.com/docker/cli/cli/config/configfile"
+	"github.com/docker/cli/internal/hint"
 	"github.com/docker/cli/opts"
 	"github.com/moby/moby/api/types/container"
 	"github.com/moby/moby/client"
@@ -115,7 +116,10 @@ func RunExec(ctx context.Context, dockerCLI command.Cli, containerIDorName strin
 
 	execID := response.ID
 	if execID == "" {
-		return errors.New("exec ID empty")
+		return hint.Wrap(
+			errors.New("the Docker daemon returned an empty response when creating the exec session"),
+			"This is unexpected — please report it at https://github.com/moby/moby/issues with the daemon version ('docker version') and the command you ran.",
+		)
 	}
 
 	if options.Detach {

cli/command/container/exec_test.go

@@ -195,7 +195,7 @@ func TestRunExec(t *testing.T) {
 		{
 			doc:           "missing exec ID",
 			options:       NewExecOptions(),
-			expectedError: "exec ID empty",
+			expectedError: "the Docker daemon returned an empty response when creating the exec session",
 			client:        &fakeClient{},
 		},
 	}

cli/command/container/export.go

@@ -9,6 +9,7 @@ import (
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/command/completion"
+	"github.com/docker/cli/internal/hint"
 	"github.com/moby/moby/client"
 	"github.com/moby/sys/atomicwriter"
 	"github.com/spf13/cobra"
@@ -49,13 +50,16 @@ func runExport(ctx context.Context, dockerCLI command.Cli, opts exportOptions) e
 	var output io.Writer
 	if opts.output == "" {
 		if dockerCLI.Out().IsTerminal() {
-			return errors.New("cowardly refusing to save to a terminal. Use the -o flag or redirect")
+			return hint.Wrap(
+				errors.New("refusing to write a binary tar archive to the terminal"),
+				"Use '-o FILE' to write to a file, or redirect stdout, e.g. 'docker container export CONTAINER > out.tar'.",
+			)
 		}
 		output = dockerCLI.Out()
 	} else {
 		writer, err := atomicwriter.New(opts.output, 0o600)
 		if err != nil {
-			return fmt.Errorf("failed to export container: %w", err)
+			return fmt.Errorf("cannot open output file %q: %w", opts.output, err)
 		}
 		defer writer.Close()
 		output = writer

cli/command/container/export_test.go

@@ -44,6 +44,6 @@ func TestContainerExportOutputToIrregularFile(t *testing.T) {
 	cmd.SetErr(io.Discard)
 	cmd.SetArgs([]string{"-o", "/dev/random", "container"})
 
-	const expected = `failed to export container: cannot write to a character device file`
+	const expected = `cannot open output file "/dev/random": cannot write to a character device file`
 	assert.Error(t, cmd.Execute(), expected)
 }

cli/command/container/opts.go

@@ -18,6 +18,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/docker/cli/internal/hint"
 	"github.com/docker/cli/internal/lazyregexp"
 	"github.com/docker/cli/internal/volumespec"
 	"github.com/docker/cli/opts"
@@ -517,22 +518,34 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con
 
 	pidMode := container.PidMode(copts.pidMode)
 	if !pidMode.Valid() {
-		return nil, errors.New("--pid: invalid PID mode")
+		return nil, hint.Wrap(
+			fmt.Errorf("invalid --pid mode %q", copts.pidMode),
+			"Valid forms are 'host' or 'container:<name|id>'.",
+		)
 	}
 
 	utsMode := container.UTSMode(copts.utsMode)
 	if !utsMode.Valid() {
-		return nil, errors.New("--uts: invalid UTS mode")
+		return nil, hint.Wrap(
+			fmt.Errorf("invalid --uts mode %q", copts.utsMode),
+			"The only valid form is 'host'.",
+		)
 	}
 
 	usernsMode := container.UsernsMode(copts.usernsMode)
 	if !usernsMode.Valid() {
-		return nil, errors.New("--userns: invalid USER mode")
+		return nil, hint.Wrap(
+			fmt.Errorf("invalid --userns mode %q", copts.usernsMode),
+			"The only valid form is 'host'.",
+		)
 	}
 
 	cgroupnsMode := container.CgroupnsMode(copts.cgroupnsMode)
 	if !cgroupnsMode.Valid() {
-		return nil, errors.New("--cgroupns: invalid CGROUP mode")
+		return nil, hint.Wrap(
+			fmt.Errorf("invalid --cgroupns mode %q", copts.cgroupnsMode),
+			"Valid forms are 'private' or 'host'.",
+		)
 	}
 
 	restartPolicy, err := opts.ParseRestartPolicy(copts.restartPolicy)
@@ -920,7 +933,10 @@ func convertToStandardNotation(ports []string) ([]string, error) {
 func parseLoggingOpts(loggingDriver string, loggingOpts []string) (map[string]string, error) {
 	loggingOptsMap := opts.ConvertKVStringsToMap(loggingOpts)
 	if loggingDriver == "none" && len(loggingOpts) > 0 {
-		return map[string]string{}, fmt.Errorf("invalid logging opts for driver %s", loggingDriver)
+		return map[string]string{}, hint.Wrap(
+			errors.New("log driver \"none\" accepts no --log-opt entries"),
+			"Remove the --log-opt flag(s) or choose a different --log-driver.",
+		)
 	}
 	return loggingOptsMap, nil
 }
@@ -984,7 +1000,7 @@ func parseStorageOpts(storageOpts []string) (map[string]string, error) {
 	for _, option := range storageOpts {
 		k, v, ok := strings.Cut(option, "=")
 		if !ok {
-			return nil, errors.New("invalid storage option")
+			return nil, fmt.Errorf("invalid --storage-opt value %q: expected key=value", option)
 		}
 		m[k] = v
 	}
@@ -1095,12 +1111,12 @@ func validateLinuxPath(val string, validator func(string) bool) (string, error)
 	var mode string
 
 	if strings.Count(val, ":") > 2 {
-		return val, fmt.Errorf("bad format for path: %s", val)
+		return val, fmt.Errorf("invalid --device %q: too many ':' separators, expected [host-path:]container-path[:mode]", val)
 	}
 
 	split := strings.SplitN(val, ":", 3)
 	if split[0] == "" {
-		return val, fmt.Errorf("bad format for path: %s", val)
+		return val, fmt.Errorf("invalid --device %q: host path before ':' is empty, expected [host-path:]container-path[:mode]", val)
 	}
 	switch len(split) {
 	case 1:

cli/command/container/opts_test.go

@@ -767,7 +767,7 @@ func TestParseModes(t *testing.T) {
 	args := []string{"--pid=container:", "img", "cmd"}
 	assert.NilError(t, flags.Parse(args))
 	_, err := parse(flags, copts, runtime.GOOS)
-	assert.ErrorContains(t, err, "--pid: invalid PID mode")
+	assert.ErrorContains(t, err, "invalid --pid mode")
 
 	// pid ok
 	_, hostconfig, _, err := parseRun([]string{"--pid=host", "img", "cmd"})
@@ -778,7 +778,7 @@ func TestParseModes(t *testing.T) {
 
 	// uts ko
 	_, _, _, err = parseRun([]string{"--uts=container:", "img", "cmd"}) //nolint:dogsled
-	assert.ErrorContains(t, err, "--uts: invalid UTS mode")
+	assert.ErrorContains(t, err, "invalid --uts mode")
 
 	// uts ok
 	_, hostconfig, _, err = parseRun([]string{"--uts=host", "img", "cmd"})
@@ -923,8 +923,8 @@ func TestParseHealth(t *testing.T) {
 
 func TestParseLoggingOpts(t *testing.T) {
 	// logging opts ko
-	if _, _, _, err := parseRun([]string{"--log-driver=none", "--log-opt=anything", "img", "cmd"}); err == nil || err.Error() != "invalid logging opts for driver none" {
-		t.Fatalf("Expected an error with message 'invalid logging opts for driver none', got %v", err)
+	if _, _, _, err := parseRun([]string{"--log-driver=none", "--log-opt=anything", "img", "cmd"}); err == nil || !strings.HasPrefix(err.Error(), "log driver \"none\" accepts no --log-opt entries") {
+		t.Fatalf("Expected an error stating that 'none' accepts no --log-opt entries, got %v", err)
 	}
 	// logging opts ok
 	_, hostconfig, _, err := parseRun([]string{"--log-driver=syslog", "--log-opt=something", "img", "cmd"})
@@ -1034,22 +1034,24 @@ func TestValidateDevice(t *testing.T) {
 		"/hostPath:/containerPath:rw",
 		"/hostPath:/containerPath:mrw",
 	}
+	const emptyHostMsg = `: host path before ':' is empty, expected [host-path:]container-path[:mode]`
+	const tooManyMsg = `: too many ':' separators, expected [host-path:]container-path[:mode]`
 	invalid := map[string]string{
-		"":        "bad format for path: ",
+		"":        `invalid --device ""` + emptyHostMsg,
 		"./":      "./ is not an absolute path",
 		"../":     "../ is not an absolute path",
 		"/:../":   "../ is not an absolute path",
 		"/:path":  "path is not an absolute path",
-		":":       "bad format for path: :",
+		":":       `invalid --device ":"` + emptyHostMsg,
 		"/tmp:":   " is not an absolute path",
-		":test":   "bad format for path: :test",
-		":/test":  "bad format for path: :/test",
+		":test":   `invalid --device ":test"` + emptyHostMsg,
+		":/test":  `invalid --device ":/test"` + emptyHostMsg,
 		"tmp:":    " is not an absolute path",
-		":test:":  "bad format for path: :test:",
-		"::":      "bad format for path: ::",
-		":::":     "bad format for path: :::",
-		"/tmp:::": "bad format for path: /tmp:::",
-		":/tmp::": "bad format for path: :/tmp::",
+		":test:":  `invalid --device ":test:"` + emptyHostMsg,
+		"::":      `invalid --device "::"` + emptyHostMsg,
+		":::":     `invalid --device ":::"` + tooManyMsg,
+		"/tmp:::": `invalid --device "/tmp:::"` + tooManyMsg,
+		":/tmp::": `invalid --device ":/tmp::"` + tooManyMsg,
 		"path:ro": "ro is not an absolute path",
 		"path:rr": "rr is not an absolute path",
 		"a:/b:ro": "bad mode specified: ro",