Merged
Conversation
Previously when we run lets with subcommand that does not exist, we showed help message and exit code was 0. Now we chaning this behavour - if lets was run with subcommand that does not exist we show error message and return exit code 2
Reviewer's GuideImplements structured unknown-command handling that returns exit code 2 and prints Cobra suggestions for mistyped commands (including Sequence diagram for unknown command handling with exit code 2sequenceDiagram
actor User
participant OS
participant main
participant cobraRootCmd as cobra_Command_root
participant validateCommandArgs
participant unknownCommandError
participant getExitCode
User->>OS: run lets wrongcmd
OS->>main: start process with args
main->>cobraRootCmd: ExecuteContext(ctx)
cobraRootCmd->>validateCommandArgs: validateCommandArgs(cmd, args)
validateCommandArgs-->>cobraRootCmd: *unknownCommandError
cobraRootCmd-->>main: error *unknownCommandError
main->>getExitCode: getExitCode(err, 1)
getExitCode->>unknownCommandError: ExitCode()
unknownCommandError-->>getExitCode: 2
getExitCode-->>main: 2
main->>OS: os.Exit(2)
OS-->>User: process exits with code 2 and suggestion message
Class diagram for unknownCommandError and exit code helperclassDiagram
class unknownCommandError {
string message
Error() string
ExitCode() int
}
class ExitCoder {
<<interface>>
ExitCode() int
}
class getExitCodeFn {
getExitCode(err error, defaultCode int) int
}
class CobraCommand {
Use string
Short string
DisableSuggestions bool
SuggestionsMinimumDistance int
CommandPath() string
SuggestionsFor(arg string) []string
}
class validateCommandArgsFn {
validateCommandArgs(cmd *CobraCommand, args []string) error
}
class buildUnknownCommandMessageFn {
buildUnknownCommandMessage(cmd *CobraCommand, arg string) string
}
ExitCoder <|.. unknownCommandError
validateCommandArgsFn ..> unknownCommandError : returns
validateCommandArgsFn ..> buildUnknownCommandMessageFn : calls
buildUnknownCommandMessageFn ..> CobraCommand : uses
getExitCodeFn ..> ExitCoder : type assertion
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
buildUnknownCommandMessagemutatescmd.SuggestionsMinimumDistancewhen it is<= 0, which can override a deliberate0value (meaning "no minimum distance") and introduce hidden side-effects on the command; consider using a local default without writing back to the struct or only changing it when it is exactly the cobra zero-value (e.g. by checking an additional flag).- The
unknownCommandErrornow implements the sameExitCode()interface pattern already used byexecutor.ExecuteError; consider defining a small sharedinterface{ ExitCode() int }type or helper in a common package to avoid repeating the anonymous interface and keep error-handling conventions centralized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- `buildUnknownCommandMessage` mutates `cmd.SuggestionsMinimumDistance` when it is `<= 0`, which can override a deliberate `0` value (meaning "no minimum distance") and introduce hidden side-effects on the command; consider using a local default without writing back to the struct or only changing it when it is exactly the cobra zero-value (e.g. by checking an additional flag).
- The `unknownCommandError` now implements the same `ExitCode()` interface pattern already used by `executor.ExecuteError`; consider defining a small shared `interface{ ExitCode() int }` type or helper in a common package to avoid repeating the anonymous interface and keep error-handling conventions centralized.
## Individual Comments
### Comment 1
<location path="cmd/root_test.go" line_range="84-113" />
<code_context>
}
})
+
+ t.Run("should return exit code 2 for unknown command", func(t *testing.T) {
+ rootCmd, _ := newTestRootCmdWithConfig([]string{"fo"})
+
+ err := rootCmd.Execute()
+ if err == nil {
+ t.Fatal("expected unknown command error")
+ }
+
+ var exitCoder interface{ ExitCode() int }
+ if !errors.As(err, &exitCoder) {
+ t.Fatal("expected error with exit code")
+ }
+
+ if exitCode := exitCoder.ExitCode(); exitCode != 2 {
+ t.Fatalf("expected exit code 2, got %d", exitCode)
+ }
+
+ if !strings.Contains(err.Error(), `unknown command "fo"`) {
+ t.Fatalf("expected unknown command error, got %q", err.Error())
+ }
+
+ if !strings.Contains(err.Error(), "Did you mean this?") {
+ t.Fatalf("expected suggestions in error, got %q", err.Error())
+ }
+
+ if !strings.Contains(err.Error(), "\tfoo\n") {
+ t.Fatalf("expected foo suggestion, got %q", err.Error())
+ }
+ })
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for unknown commands that do not produce any suggestions
This subtest only covers the case where an unknown command yields suggestions (e.g., `foo`). The new code has a separate branch for when `SuggestionsFor` returns no results, where the "Did you mean this?" section should be omitted. Please add another subtest using an argument that won’t match any command (e.g. `zzzz_unknown_command`) to assert that:
- the exit code remains 2, and
- the error message includes the `unknown command` text but omits `Did you mean this?`.
That will exercise the `len(suggestions) == 0` path in `buildUnknownCommandMessage` and guard against regressions.
```suggestion
t.Run("should return exit code 2 for unknown command", func(t *testing.T) {
rootCmd, _ := newTestRootCmdWithConfig([]string{"fo"})
err := rootCmd.Execute()
if err == nil {
t.Fatal("expected unknown command error")
}
var exitCoder interface{ ExitCode() int }
if !errors.As(err, &exitCoder) {
t.Fatal("expected error with exit code")
}
if exitCode := exitCoder.ExitCode(); exitCode != 2 {
t.Fatalf("expected exit code 2, got %d", exitCode)
}
if !strings.Contains(err.Error(), `unknown command "fo"`) {
t.Fatalf("expected unknown command error, got %q", err.Error())
}
if !strings.Contains(err.Error(), "Did you mean this?") {
t.Fatalf("expected suggestions in error, got %q", err.Error())
}
if !strings.Contains(err.Error(), "\tfoo\n") {
t.Fatalf("expected foo suggestion, got %q", err.Error())
}
})
t.Run("should return exit code 2 for unknown command without suggestions", func(t *testing.T) {
rootCmd, _ := newTestRootCmdWithConfig([]string{"zzzz_unknown_command"})
err := rootCmd.Execute()
if err == nil {
t.Fatal("expected unknown command error")
}
var exitCoder interface{ ExitCode() int }
if !errors.As(err, &exitCoder) {
t.Fatal("expected error with exit code")
}
if exitCode := exitCoder.ExitCode(); exitCode != 2 {
t.Fatalf("expected exit code 2, got %d", exitCode)
}
if !strings.Contains(err.Error(), `unknown command "zzzz_unknown_command"`) {
t.Fatalf("expected unknown command error, got %q", err.Error())
}
if strings.Contains(err.Error(), "Did you mean this?") {
t.Fatalf("did not expect suggestions in error, got %q", err.Error())
}
})
}
```
</issue_to_address>
### Comment 2
<location path="cmd/root_test.go" line_range="115-82" />
<code_context>
+func TestSelfCmd(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for self subcommands with suggestions explicitly disabled
Since `buildUnknownCommandMessage` has a branch for `cmd.DisableSuggestions == true`, please add a subtest under `TestSelfCmd` that constructs a `self` command with `DisableSuggestions = true`, runs an unknown subcommand, asserts exit code 2, and verifies the error message does not include the suggestion header (`Did you mean this?`). This will confirm `DisableSuggestions` works correctly for `self` subcommands.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+84
to
+113
| t.Run("should return exit code 2 for unknown command", func(t *testing.T) { | ||
| rootCmd, _ := newTestRootCmdWithConfig([]string{"fo"}) | ||
|
|
||
| err := rootCmd.Execute() | ||
| if err == nil { | ||
| t.Fatal("expected unknown command error") | ||
| } | ||
|
|
||
| var exitCoder interface{ ExitCode() int } | ||
| if !errors.As(err, &exitCoder) { | ||
| t.Fatal("expected error with exit code") | ||
| } | ||
|
|
||
| if exitCode := exitCoder.ExitCode(); exitCode != 2 { | ||
| t.Fatalf("expected exit code 2, got %d", exitCode) | ||
| } | ||
|
|
||
| if !strings.Contains(err.Error(), `unknown command "fo"`) { | ||
| t.Fatalf("expected unknown command error, got %q", err.Error()) | ||
| } | ||
|
|
||
| if !strings.Contains(err.Error(), "Did you mean this?") { | ||
| t.Fatalf("expected suggestions in error, got %q", err.Error()) | ||
| } | ||
|
|
||
| if !strings.Contains(err.Error(), "\tfoo\n") { | ||
| t.Fatalf("expected foo suggestion, got %q", err.Error()) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Add coverage for unknown commands that do not produce any suggestions
This subtest only covers the case where an unknown command yields suggestions (e.g., foo). The new code has a separate branch for when SuggestionsFor returns no results, where the "Did you mean this?" section should be omitted. Please add another subtest using an argument that won’t match any command (e.g. zzzz_unknown_command) to assert that:
- the exit code remains 2, and
- the error message includes the
unknown commandtext but omitsDid you mean this?.
That will exercise the len(suggestions) == 0 path in buildUnknownCommandMessage and guard against regressions.
Suggested change
| t.Run("should return exit code 2 for unknown command", func(t *testing.T) { | |
| rootCmd, _ := newTestRootCmdWithConfig([]string{"fo"}) | |
| err := rootCmd.Execute() | |
| if err == nil { | |
| t.Fatal("expected unknown command error") | |
| } | |
| var exitCoder interface{ ExitCode() int } | |
| if !errors.As(err, &exitCoder) { | |
| t.Fatal("expected error with exit code") | |
| } | |
| if exitCode := exitCoder.ExitCode(); exitCode != 2 { | |
| t.Fatalf("expected exit code 2, got %d", exitCode) | |
| } | |
| if !strings.Contains(err.Error(), `unknown command "fo"`) { | |
| t.Fatalf("expected unknown command error, got %q", err.Error()) | |
| } | |
| if !strings.Contains(err.Error(), "Did you mean this?") { | |
| t.Fatalf("expected suggestions in error, got %q", err.Error()) | |
| } | |
| if !strings.Contains(err.Error(), "\tfoo\n") { | |
| t.Fatalf("expected foo suggestion, got %q", err.Error()) | |
| } | |
| }) | |
| } | |
| t.Run("should return exit code 2 for unknown command", func(t *testing.T) { | |
| rootCmd, _ := newTestRootCmdWithConfig([]string{"fo"}) | |
| err := rootCmd.Execute() | |
| if err == nil { | |
| t.Fatal("expected unknown command error") | |
| } | |
| var exitCoder interface{ ExitCode() int } | |
| if !errors.As(err, &exitCoder) { | |
| t.Fatal("expected error with exit code") | |
| } | |
| if exitCode := exitCoder.ExitCode(); exitCode != 2 { | |
| t.Fatalf("expected exit code 2, got %d", exitCode) | |
| } | |
| if !strings.Contains(err.Error(), `unknown command "fo"`) { | |
| t.Fatalf("expected unknown command error, got %q", err.Error()) | |
| } | |
| if !strings.Contains(err.Error(), "Did you mean this?") { | |
| t.Fatalf("expected suggestions in error, got %q", err.Error()) | |
| } | |
| if !strings.Contains(err.Error(), "\tfoo\n") { | |
| t.Fatalf("expected foo suggestion, got %q", err.Error()) | |
| } | |
| }) | |
| t.Run("should return exit code 2 for unknown command without suggestions", func(t *testing.T) { | |
| rootCmd, _ := newTestRootCmdWithConfig([]string{"zzzz_unknown_command"}) | |
| err := rootCmd.Execute() | |
| if err == nil { | |
| t.Fatal("expected unknown command error") | |
| } | |
| var exitCoder interface{ ExitCode() int } | |
| if !errors.As(err, &exitCoder) { | |
| t.Fatal("expected error with exit code") | |
| } | |
| if exitCode := exitCoder.ExitCode(); exitCode != 2 { | |
| t.Fatalf("expected exit code 2, got %d", exitCode) | |
| } | |
| if !strings.Contains(err.Error(), `unknown command "zzzz_unknown_command"`) { | |
| t.Fatalf("expected unknown command error, got %q", err.Error()) | |
| } | |
| if strings.Contains(err.Error(), "Did you mean this?") { | |
| t.Fatalf("did not expect suggestions in error, got %q", err.Error()) | |
| } | |
| }) | |
| } |
| @@ -78,4 +80,72 @@ func TestRootCmdWithConfig(t *testing.T) { | |||
| ) | |||
| } | |||
| }) | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for self subcommands with suggestions explicitly disabled
Since buildUnknownCommandMessage has a branch for cmd.DisableSuggestions == true, please add a subtest under TestSelfCmd that constructs a self command with DisableSuggestions = true, runs an unknown subcommand, asserts exit code 2, and verifies the error message does not include the suggestion header (Did you mean this?). This will confirm DisableSuggestions works correctly for self subcommands.
kindermax
force-pushed
the
error-exit-code-on-command-not-found
branch
from
e1e8fe2 to
8ded158
Compare
kindermax
force-pushed
the
error-exit-code-on-command-not-found
branch
from
8ded158 to
6c06355
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Ensure unknown commands exit with a consistent non-zero status and provide suggestions, and document agent usage and project structure.
Bug Fixes:
Enhancements:
Documentation:
Tests: