[PerryLink]

What type of PR is this?

• bug

What this PR does / why we need it:

HandleExitCoder looked for an ExitCoder — and for a MultiError — with a bare type assertion, so an exit error wrapped with fmt.Errorf("%w") was not recognised. Nothing called OsExiter, nothing was written to ErrWriter, and the process exited with the default code 1:

cmd.Action = func(ctx context.Context, cmd *cli.Command) error {
    return fmt.Errorf("deploy failed: %w", cli.Exit("galactic perimeter breach", 9))
}
// printed nothing, exited 1 (expected: the message, exit code 9)

exitError.Unwrap() (#1090, #2444) already lets errors.Is/errors.As see the error inside an ExitCoder. This makes the consumer honour the same relationship in the other direction, for an ExitCoder that was wrapped with %w.

The top-level error is still classified with a type assertion first, and errors.As is only the fallback:

• a MultiError that is not itself an ExitCoder is handled as a whole, so the documented "last exit code wins" also holds for multi-errors that implement Unwrap() []error (for example multierr.Combine) instead of errors.As finding their first ExitCoder;
• inside handleMultiError only the ExitCoder lookup unwraps, so an ExitCoder whose message is a MultiError keeps its own code, and a MultiError wrapped in fmt.Errorf is printed as the wrapper message.

The doc comment gets two lines about the wrapped case, so the two generated goldens are updated in follow-up commits — godoc-current.txt via make generate, then testdata/godoc-v3.x.txt via what make v3approve does. Those are the only extra files; make v3diff passes with them.

Which issue(s) this PR fixes:

Fixes #2478

Special notes for your reviewer:

• The regression tests replace the package-level OsExiter and ErrWriter, so they are not parallel — same constraint as the other thirteen tests in errors_test.go, noted in a comment above the helper.
• MultiError ordering is deliberate: a multi-error that also implements Unwrap() []error must be handled as a whole. The three new tests pin the three cases raised in review.
• The golden diffs are mechanical: both files were byte-identical to godoc-current.txt before this PR, and the only difference after regeneration is those two added comment lines.

Testing

go test -run 'TestHandleExitCoder' ./   # the three new cases fail on the previous head
go test ./...                           # ok
make lint                               # ok
make diffcheck                          # ok
make v3diff                             # ok

On the previous head the new cases failed with expected: 3, actual: 2, expected: 5, actual: 1 and "a\nb\n" does not contain "after: a"; with the change all pass, and the existing TestHandleExitCoder*, TestExit* and TestMultiError* tests still pass.

`HandleExitCoder` now finds an `ExitCoder` that is wrapped in another error, for example with `fmt.Errorf("deploy failed: %w", cli.Exit("...", 9))`, the same way `errors.As` does: it prints the error and exits with that code instead of leaving the error to the caller. A `MultiError` passed directly still exits with its last exit code.

This includes a wrapped `*exec.ExitError`, which has an `ExitCode` method: an Action returning `fmt.Errorf("git push: %w", err)` for a failed command now exits with the command's exit code, or with 255 on Linux if the command was killed by a signal. An unwrapped `*exec.ExitError` already behaved this way.

[Juneezee]

I will also review this PR in the morning (~9 hours from now)

[Juneezee]

Thanks. The ExitCoder part fixes #2478, but the PR also changes MultiError handling, which the description says is untouched.

1. "Last exit code wins" breaks for multi-errors that implement Unwrap() []error. The new top-level errors.As runs before the MultiError branch and walks into them, so it picks the first ExitCoder:

cli/errors.go

Lines 178 to 196 in 5ed7a70

	var exitErr ExitCoder
	if errors.As(err, &exitErr) {
	if msg := err.Error(); msg != "" {
	if _, ok := exitErr.(ErrorFormatter); ok {
	_, _ = fmt.Fprintf(ErrWriter, "%+v\n", err)
	} else {
	_, _ = fmt.Fprintln(ErrWriter, err)
	}
	}
	OsExiter(exitErr.ExitCode())
	return
	}
	var multiErr MultiError
	if errors.As(err, &multiErr) {
	code := handleMultiError(multiErr)
	OsExiter(code)
	return
	}

For example, with go.uber.org/multierr v1.11.0:

return multierr.Combine(cli.Exit("first", 2), cli.Exit("second", 3))

• main: prints first and second on separate lines, exits 3.
• This PR: prints first; second, exits 2.

main matches the documented behavior:

cli/errors.go

Lines 164 to 166 in 05d84c1

	// If the given error instead implements MultiError, each error will be checked
	// for the ExitCoder interface, and OsExiter will be called with the last exit
	// code found, or exit code 1 if no ExitCoder is found.

2. The two MultiError lookups are not needed for #2478, are untested, and change behavior. Changing either back to a type assertion keeps go test ./... passing:

cli/errors.go

Lines 191 to 204 in 5ed7a70

	var multiErr MultiError
	if errors.As(err, &multiErr) {
	code := handleMultiError(multiErr)
	OsExiter(code)
	return
	}
	}
	func handleMultiError(multiErr MultiError) int {
	code := 1
	for _, merr := range multiErr.Errors() {
	var nested MultiError
	if errors.As(merr, &nested) {
	code = handleMultiError(nested)

• newMultiError(errors.New("x"), Exit(newMultiError(errors.New("a"), errors.New("b")), 5)) exits 5 on main and 1 here, so the ExitCoder loses its code.
• fmt.Errorf("after: %w", newMultiError(errors.New("a"), errors.New("b"))) inside a MultiError prints a instead of after: a.

Suggestion for 1 and 2: keep both MultiError lookups as type assertions, and only fall back to errors.As for ExitCoder when the error is not a MultiError:

	exitErr, isExitCoder := err.(ExitCoder)
	if multiErr, ok := err.(MultiError); ok && !isExitCoder {
		code := handleMultiError(multiErr)
		OsExiter(code)
		return
	}

	if isExitCoder || errors.As(err, &exitErr) {
		// print and exit as before
	}

In handleMultiError, only the ExitCoder lookup changes to errors.As. With this, your new test and go test ./... pass, and all the cases above behave as on main. This test covers 1 (passes with the old errors.go, fails with this PR):

type unwrapMultiError []error

func (m unwrapMultiError) Error() string   { return errors.Join(m...).Error() }
func (m unwrapMultiError) Errors() []error { return m }
func (m unwrapMultiError) Unwrap() []error { return m }

func TestHandleExitCoder_MultiErrorWithUnwrap(t *testing.T) {
	code, called, _ := captureExit(unwrapMultiError{Exit("first", 2), Exit("second", 3)})
	assert.True(t, called)
	assert.Equal(t, 3, code)
}

3. Wrapped *exec.ExitError now exits too. It has ExitCode() int (from *os.ProcessState), so errors.As matches it. For example:

cmd := &cli.Command{
	Action: func(ctx context.Context, cmd *cli.Command) error {
		return fmt.Errorf("git push: %w", exec.Command("sh", "-c", "exit 3").Run())
	},
}
if err := cmd.Run(context.Background(), os.Args); err != nil {
	log.Fatal(err)
}

• main: Run returns the error, log.Fatal prints it, exit code 1.
• This PR: Run prints the error and calls os.Exit(3) itself, so log.Fatal (or any code after Run) never runs. If the child is killed by a signal, ExitCode() is -1 and the process exits with 255 on Linux.

An unwrapped *exec.ExitError already behaves this way on main, so this is consistent, but it changes what existing programs do. Please mention it in the release note.

4. The test comment is not accurate. Exit does not document anything about being wrapped:

cli/errors.go

Lines 122 to 128 in 05d84c1

	// Exit wraps a message and exit code into an error, which by default is
	// handled with a call to os.Exit during default error handling.
	//
	// This is the simplest way to trigger a non-zero exit code for a Command without
	// having to call os.Exit manually. During testing, this behavior can be avoided
	// by overriding the ExitErrHandler function on a Command or the package-global
	// OsExiter function.

and Unwrap (#1090, #2444) goes the other way: it lets errors.Is/errors.As see the error inside an ExitCoder, not find an ExitCoder inside another error:

cli/errors.go

Lines 155 to 159 in 05d84c1

	// Unwrap returns the error given to [Exit], so that [errors.Is] and
	// [errors.As] can inspect it through the ExitCoder wrapper.
	func (ee *exitError) Unwrap() error {
	return ee.err
	}

Could you reword or drop this part?

cli/errors_test.go

Lines 115 to 118 in 5ed7a70

	// An ExitCoder keeps its exit code and message when it is wrapped with
	// fmt.Errorf("%w"): [Exit] documents that the underlying error stays reachable
	// through the standard wrapping helpers, and callers add context to an Action
	// error all the time.

5. Nit: captureExit saves fakeOsExiter as oldExiter and restores without defer. The other tests in this file use defer func() { OsExiter = fakeOsExiter }().

cli/errors_test.go

Lines 106 to 110 in 5ed7a70

	oldWriter, oldExiter := ErrWriter, fakeOsExiter
	var buf bytes.Buffer
	ErrWriter = &buf
	HandleExitCoder(err)
	ErrWriter, OsExiter = oldWriter, oldExiter

[PerryLink]

Thanks for the detailed review — all five points are addressed in 17b4e59.

1 & 2. The top-level error is classified with type assertions again, and errors.As is only the fallback, exactly as you suggested:

exitErr, isExitCoder := err.(ExitCoder)
if multiErr, ok := err.(MultiError); ok && !isExitCoder {
	code := handleMultiError(multiErr)
	OsExiter(code)
	return
}

if isExitCoder || errors.As(err, &exitErr) {
	// print and exit as before
}

In handleMultiError the nested MultiError lookup is a type assertion again, and only the ExitCoder lookup uses errors.As. Both of your examples now behave as on main: multierr.Combine(...) exits 3 with one message per line, the nested Exit(newMultiError(...), 5) keeps 5, and the wrapped MultiError inside a MultiError prints after: a.

3. Mentioned in the release note, including the -1 → 255 case for a signal-killed child.

4. Reworded — the comment no longer claims anything about what Exit documents; it now says that callers add context with %w all the time and that exitError.Unwrap already lets errors.Is/errors.As see through the ExitCoder, so the other direction should work too.

5. captureExit restores OsExiter and ErrWriter with defer now.

The three cases are pinned as tests (TestHandleExitCoder_MultiErrorWithUnwrap, TestHandleExitCoder_NestedExitCoderKeepsCode, TestHandleExitCoder_MultiErrorKeepsWrapperMessage). On the previous head they fail with expected: 3, actual: 2, expected: 5, actual: 1 and "a\nb\n" does not contain "after: a"; with the change they pass, and go test ./..., make lint, make diffcheck and make v3diff are all green.

[PerryLink]

One check is failing on the new head, and it is not a test failure: check-binary-size on ubuntu-24.04 @ Go stable (Go 1.27.2). Every other platform job was cancelled by the fail-fast, and golangci-lint plus ubuntu-24.04 @ Go oldstable are green.

2.3MB is the current binary size
  🚨 2.2MB is the target max size

Measured locally with Go 1.27.2, building examples/example-cli exactly as scripts/build.go does:

main (05d84c1):          3,772,576 bytes
main + this change:      3,784,864 bytes   (+12,288)

So the change itself adds 12 KB to the example binary, which is what pushes the rounded figure from 2.2 to 2.3. Nothing in the diff warrants that on its face — it is three lookups moving from type assertions to errors.As — but I could not find a smaller shape that keeps the behaviour you asked for, and I would rather show the measurement than guess.

scripts/build.go itself says the way out is to raise desiredMaxBinarySize and desiredMinBinarySize by the same amount. That is a policy change and not something I want to slip into a bug-fix PR, so I have left it out. Tell me which you prefer:

1. I add a commit that raises both constants by 0.1 MB (2.2 → 2.3, 1.49 → 1.59), or
2. you would rather take the check as advisory here, or
3. the size increase is unacceptable and this belongs in a follow-up with a leaner implementation.

[Juneezee]

Thanks for the fixes. Now that main requires Go 1.26 (#2483), I pushed a few changes to your branch:

1. Rebased onto main, squashed the two chore: commits that regenerate the godoc goldens into the first commit, and dropped the merge commit.

2. Replaced errors.As with errors.AsType. errors.As links the reflection code of internal/reflectlite, which nothing else in the example binary uses. It added 12288 bytes and failed check-binary-size (2.3MB against the 2.2MB limit). errors.AsType finds the same errors without reflection, so the measured size grows by 4096 bytes instead:

cli/errors.go

Lines 178 to 187 in e2ccb61

	_, isExitCoder := err.(ExitCoder)
	// A MultiError is handled as a whole: unwrapping it would find its first
	// ExitCoder and lose the documented "last exit code wins".
	if multiErr, ok := err.(MultiError); ok && !isExitCoder {
	code := handleMultiError(multiErr)
	OsExiter(code)
	return
	}
	if exitErr, ok := errors.AsType[ExitCoder](err); ok {

cli/errors.go

Lines 203 to 209 in e2ccb61

	if multiErr2, ok := merr.(MultiError); ok {
	code = handleMultiError(multiErr2)
	} else if merr != nil {
	fmt.Fprintln(ErrWriter, merr)
	if exitErr, ok := errors.AsType[ExitCoder](merr); ok {
	code = exitErr.ExitCode()
	}

3. Reworded the first commit message. HandleExitCoder does nothing with an error it does not recognise, so the exit code 1 came from main (for example log.Fatal(err)). The "last exit code wins" behaviour of MultiError also changed in that commit; the second commit restores it.

4. Updated the release note. A wrapped MultiError does not keep "last exit code wins": errors.AsType finds its first ExitCoder, as errors.As would. For example, this exits with 2:

return fmt.Errorf("deploy: %w", multierr.Combine(cli.Exit("first", 2), cli.Exit("second", 3)))

Only a MultiError passed directly still exits with its last exit code.

[PerryLink]

Thanks for taking it the rest of the way — the rebase onto Go 1.26, folding the two golden commits into the first one, and errors.AsType for the final lookup are all cleaner than what I had. The MultiError ordering is the part I would have got wrong without your review.