From 5429789a67aaec0fd360a85d413fc73b612a5238 Mon Sep 17 00:00:00 2001 From: espadonne Date: Sat, 23 May 2026 20:30:34 -0400 Subject: [PATCH 1/5] =?UTF-8?q?audit-I5:=20ParentRunE=20=E2=80=94=20Levens?= =?UTF-8?q?htein/prefix=20suggestions=20on=20subcommand=20typos?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/cmdutil/parent.go | 103 ++++++++++++++++++++++++++++++- internal/cmdutil/parent_test.go | 106 ++++++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 2 deletions(-) create mode 100644 internal/cmdutil/parent_test.go diff --git a/internal/cmdutil/parent.go b/internal/cmdutil/parent.go index 70c8469..3b08f79 100644 --- a/internal/cmdutil/parent.go +++ b/internal/cmdutil/parent.go @@ -4,6 +4,7 @@ package cmdutil import ( "fmt" + "strings" "github.com/spf13/cobra" ) @@ -15,15 +16,113 @@ import ( // the call through the parent's help text and exits 0, which made // typos like `shithub org INVALID` falsely look successful (E-audit E16). // +// audit-I5: subcommand-level "did you mean?" suggestions. cobra's +// suggestion machinery only fires at the root command's auto-error +// path; once execution reaches a parent's RunE the suggestion logic +// is bypassed. We walk the parent's children and Levenshtein-match +// the bad arg ourselves so `shithub pr klose` says "Did you mean +// close?" the same way `shithub reepo` already does. +// // gh emits "unknown command \"INVALID\" for \"gh org\""; we mirror that // shape and let the user discover the available subcommands via the -// suggestion line cobra appends automatically. +// suggestion line. func ParentRunE() func(*cobra.Command, []string) error { return func(c *cobra.Command, args []string) error { if len(args) == 0 { return c.Help() } - return fmt.Errorf("unknown command %q for %q; run '%s --help' for usage", + base := fmt.Sprintf("unknown command %q for %q; run '%s --help' for usage", args[0], c.CommandPath(), c.CommandPath()) + if suggestion := suggestSubcommand(c, args[0]); suggestion != "" { + return fmt.Errorf("%s\n\nDid you mean this?\n\t%s", base, suggestion) + } + return fmt.Errorf("%s", base) + } +} + +// suggestSubcommand returns the best-matching subcommand name when +// `want` is within Levenshtein-distance 2 of any visible child, or +// shares a 3+ char prefix. Returns "" when nothing's close enough. +// +// We don't reuse cobra's internal SuggestionsFor because it's +// unexported on *Command outside the auto-error path. The +// implementation is small and matches cobra's `SuggestionsMinimumDistance=2` +// default behavior. +func suggestSubcommand(parent *cobra.Command, want string) string { + type scored struct { + name string + dist int + } + var best scored + best.dist = -1 + wantLower := strings.ToLower(want) + for _, sub := range parent.Commands() { + if sub.Hidden { + continue + } + name := sub.Name() + lower := strings.ToLower(name) + // Prefix match wins outright — `shithub repo cre` → `create`. + if len(want) >= 3 && strings.HasPrefix(lower, wantLower) { + return name + } + d := levenshtein(wantLower, lower) + if d <= 2 && (best.dist == -1 || d < best.dist) { + best = scored{name: name, dist: d} + } + // Also check aliases. + for _, a := range sub.Aliases { + al := strings.ToLower(a) + if len(want) >= 3 && strings.HasPrefix(al, wantLower) { + return name + } + d := levenshtein(wantLower, al) + if d <= 2 && (best.dist == -1 || d < best.dist) { + best = scored{name: name, dist: d} + } + } + } + return best.name +} + +// levenshtein returns the edit distance between a and b. Tight loop +// suitable for the ~10-15 children typical of a shithub parent. +func levenshtein(a, b string) int { + if a == b { + return 0 + } + if len(a) == 0 { + return len(b) + } + if len(b) == 0 { + return len(a) + } + prev := make([]int, len(b)+1) + curr := make([]int, len(b)+1) + for j := range prev { + prev[j] = j + } + for i := 1; i <= len(a); i++ { + curr[0] = i + for j := 1; j <= len(b); j++ { + cost := 1 + if a[i-1] == b[j-1] { + cost = 0 + } + curr[j] = min3(curr[j-1]+1, prev[j]+1, prev[j-1]+cost) + } + prev, curr = curr, prev + } + return prev[len(b)] +} + +func min3(a, b, c int) int { + m := a + if b < m { + m = b + } + if c < m { + m = c } + return m } diff --git a/internal/cmdutil/parent_test.go b/internal/cmdutil/parent_test.go new file mode 100644 index 0000000..9c5f04d --- /dev/null +++ b/internal/cmdutil/parent_test.go @@ -0,0 +1,106 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cmdutil_test + +import ( + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/tenseleyFlow/shithub-cli/internal/cmdutil" +) + +// TestParentRunE_NoArgsPrintsHelp pins the bare-invocation path. +func TestParentRunE_NoArgsPrintsHelp(t *testing.T) { + parent := &cobra.Command{Use: "shithub", RunE: cmdutil.ParentRunE()} + parent.SetOut(&strings.Builder{}) + parent.SetErr(&strings.Builder{}) + parent.AddCommand(&cobra.Command{Use: "create"}) + if err := parent.RunE(parent, nil); err != nil { + t.Fatalf("bare invoke should print help and return nil; got %v", err) + } +} + +// TestParentRunE_UnknownSubcommandErrors pins exit-code-1 path. +func TestParentRunE_UnknownSubcommandErrors(t *testing.T) { + parent := &cobra.Command{Use: "parent", RunE: cmdutil.ParentRunE()} + parent.AddCommand(&cobra.Command{Use: "create"}) + err := parent.RunE(parent, []string{"totally-unknown-name"}) + if err == nil { + t.Fatal("unknown subcommand should return non-nil error") + } + if !strings.Contains(err.Error(), `unknown command "totally-unknown-name"`) { + t.Errorf("error should quote the bad arg: %v", err) + } +} + +// TestParentRunE_LevenshteinSuggestion pins audit-I5: typos at the +// subcommand level get a "Did you mean?" line. Pre-fix only the root +// command's auto-error path had suggestions; once execution reached +// a parent's RunE, the suggestion machinery was bypassed. +func TestParentRunE_LevenshteinSuggestion(t *testing.T) { + parent := &cobra.Command{Use: "pr", RunE: cmdutil.ParentRunE()} + parent.AddCommand(&cobra.Command{Use: "close"}) + parent.AddCommand(&cobra.Command{Use: "create"}) + parent.AddCommand(&cobra.Command{Use: "review"}) + + err := parent.RunE(parent, []string{"klose"}) + if err == nil { + t.Fatal("expected error from typo") + } + if !strings.Contains(err.Error(), "Did you mean this?") { + t.Errorf("missing suggestion preface: %v", err) + } + if !strings.Contains(err.Error(), "close") { + t.Errorf("expected to suggest 'close': %v", err) + } +} + +// TestParentRunE_PrefixSuggestion: 3+ char prefix match wins over +// Levenshtein. `shithub repo cre` → `create`. +func TestParentRunE_PrefixSuggestion(t *testing.T) { + parent := &cobra.Command{Use: "repo", RunE: cmdutil.ParentRunE()} + parent.AddCommand(&cobra.Command{Use: "create"}) + parent.AddCommand(&cobra.Command{Use: "delete"}) + + err := parent.RunE(parent, []string{"cre"}) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "create") { + t.Errorf("prefix 'cre' should suggest 'create': %v", err) + } +} + +// TestParentRunE_NoCloseMatchSkipsSuggestion: a typo far from every +// child should NOT emit a misleading suggestion. +func TestParentRunE_NoCloseMatchSkipsSuggestion(t *testing.T) { + parent := &cobra.Command{Use: "pr", RunE: cmdutil.ParentRunE()} + parent.AddCommand(&cobra.Command{Use: "close"}) + parent.AddCommand(&cobra.Command{Use: "create"}) + + err := parent.RunE(parent, []string{"absolutely-nothing-like-our-verbs"}) + if err == nil { + t.Fatal("expected error") + } + if strings.Contains(err.Error(), "Did you mean this?") { + t.Errorf("should not suggest for far-off typo: %v", err) + } +} + +// TestParentRunE_HiddenChildrenIgnored: hidden subcommands don't +// pollute the suggestion candidate set. +func TestParentRunE_HiddenChildrenIgnored(t *testing.T) { + parent := &cobra.Command{Use: "x", RunE: cmdutil.ParentRunE()} + parent.AddCommand(&cobra.Command{Use: "visible"}) + parent.AddCommand(&cobra.Command{Use: "hidden-target", Hidden: true}) + + err := parent.RunE(parent, []string{"hiddn-targt"}) + if err == nil { + t.Fatal("expected error") + } + if strings.Contains(err.Error(), "hidden-target") { + t.Errorf("hidden subcommand should not be suggested: %v", err) + } +} From 4377c7f47a6b49ad94243acd3373bd78200abc61 Mon Sep 17 00:00:00 2001 From: espadonne Date: Sat, 23 May 2026 20:30:35 -0400 Subject: [PATCH 2/5] =?UTF-8?q?audit-I6:=20crosskind=20=E2=80=94=20Asymmet?= =?UTF-8?q?ric=20flag=20on=20ErrWrongNamespace=20+=20CheckAsymmetric=20hel?= =?UTF-8?q?per?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/cmd/shared/crosskind/crosskind.go | 44 ++++++++++++++ pkg/cmd/shared/crosskind/crosskind_test.go | 70 ++++++++++++++++++++++ 2 files changed, 114 insertions(+) diff --git a/pkg/cmd/shared/crosskind/crosskind.go b/pkg/cmd/shared/crosskind/crosskind.go index ed7ed72..298eef4 100644 --- a/pkg/cmd/shared/crosskind/crosskind.go +++ b/pkg/cmd/shared/crosskind/crosskind.go @@ -38,6 +38,14 @@ type ErrWrongNamespace struct { // SuggestedVerb is the command tail to suggest (e.g. "close", // "ready"). Defaults to the verb portion of CmdName when empty. SuggestedVerb string + // Asymmetric marks verbs that exist on only one side of the + // namespace (e.g. `pr ready`, `pr merge`, `pr review`). When + // true, the formatted message explains the constraint instead + // of pointing at an `issue ` that doesn't exist. + // audit-I6: pre-fix `pr ready 1` on an issue suggested + // `try shithub issue ready 1` — but `issue ready` doesn't + // exist, so the user chased a dead command. + Asymmetric bool } // errWrongNamespaceSentinel is the marker for errors.Is matching; @@ -56,10 +64,34 @@ func (e *ErrWrongNamespace) Error() string { } else { kindNoun = "pull request" } + // audit-I6: asymmetric verbs (e.g. `pr ready`, `pr merge`, + // `pr review`) don't exist on the other side of the namespace. + // Pre-fix we still emitted "try `shithub issue ready N`" — a + // dead command. Now we explain the asymmetry and steer the + // user at `view` (which exists everywhere) for inspection. + if e.Asymmetric { + return fmt.Sprintf("%s: #%d is %s %s; `%s` only applies to %ss (run `shithub %s view %d` to inspect)", + e.CmdName, e.Number, articleFor(kindNoun), kindNoun, + e.CmdName, asymmetricSubjectFor(e.CmdName), tree, e.Number) + } return fmt.Sprintf("%s: #%d is %s %s; try `shithub %s %s %d`", e.CmdName, e.Number, articleFor(kindNoun), kindNoun, tree, suggested, e.Number) } +// asymmetricSubjectFor returns the noun-form ("pull request" or +// "issue") the asymmetric verb operates on, derived from the +// command prefix in CmdName. Used by Error() to render messages +// like "`pr ready` only applies to pull requests". +func asymmetricSubjectFor(cmdName string) string { + switch { + case len(cmdName) >= 3 && cmdName[:3] == "pr ": + return "pull request" + case len(cmdName) >= 6 && cmdName[:6] == "issue ": + return "issue" + } + return "the calling kind" +} + // Is supports errors.Is matching against the sentinel — callers that // don't need the typed fields can write `errors.Is(err, ErrWrongNamespaceSentinel)`. func (e *ErrWrongNamespace) Is(target error) bool { @@ -94,6 +126,16 @@ func articleFor(noun string) string { // and runs only on the wrong-side path because the expected-side // lookup is what the mutation already needs to do. func Check(ctx context.Context, ic *issues.Client, pc *pulls.Client, owner, name string, number int, expectedKind, cmdName, suggestedVerb string) error { + return CheckAsymmetric(ctx, ic, pc, owner, name, number, expectedKind, cmdName, suggestedVerb, false) +} + +// CheckAsymmetric is Check with an explicit `asymmetric` flag for +// verbs that exist on only one side of the namespace (e.g. +// `pr ready`, `pr merge`, `pr review`, `pr checkout`, `pr diff`, +// `pr update-branch`). When asymmetric=true and the wrong-side +// case fires, the error message explains the constraint instead +// of pointing at an `issue ` that doesn't exist. audit-I6. +func CheckAsymmetric(ctx context.Context, ic *issues.Client, pc *pulls.Client, owner, name string, number int, expectedKind, cmdName, suggestedVerb string, asymmetric bool) error { if number <= 0 { return nil } @@ -109,6 +151,7 @@ func Check(ctx context.Context, ic *issues.Client, pc *pulls.Client, owner, name return &ErrWrongNamespace{ Number: number, CmdName: cmdName, Found: "pr", SuggestedVerb: suggestedVerb, + Asymmetric: asymmetric, } } case "pr": @@ -119,6 +162,7 @@ func Check(ctx context.Context, ic *issues.Client, pc *pulls.Client, owner, name return &ErrWrongNamespace{ Number: number, CmdName: cmdName, Found: "issue", SuggestedVerb: suggestedVerb, + Asymmetric: asymmetric, } } default: diff --git a/pkg/cmd/shared/crosskind/crosskind_test.go b/pkg/cmd/shared/crosskind/crosskind_test.go index 65cc934..ad5a988 100644 --- a/pkg/cmd/shared/crosskind/crosskind_test.go +++ b/pkg/cmd/shared/crosskind/crosskind_test.go @@ -125,3 +125,73 @@ func TestIsWrongNamespace(t *testing.T) { t.Error("nil should not be detected") } } + +// TestCheckAsymmetric_PRReadyOnIssue pins audit-I6: an asymmetric +// PR-only verb on an issue number must NOT suggest the matching +// `issue ` command because it doesn't exist. The error +// explains the asymmetry and steers at `issue view N` instead. +func TestCheckAsymmetric_PRReadyOnIssue(t *testing.T) { + tf := cmdutiltest.New(t) + tf.Server.RegisterJSON(http.MethodGet, "/api/v1/repos/o/r/issues/1", 200, issues.Issue{Number: 1}) + + client, _ := tf.Factory.HTTPClient("") + ic := issues.NewClient(client) + pc := pulls.NewClient(client) + + err := crosskind.CheckAsymmetric(context.Background(), ic, pc, "o", "r", 1, "pr", "pr ready", "ready", true) + var w *crosskind.ErrWrongNamespace + if !errors.As(err, &w) { + t.Fatalf("want ErrWrongNamespace, got %v", err) + } + if !w.Asymmetric { + t.Error("Asymmetric flag should be set") + } + msg := err.Error() + if strings.Contains(msg, "shithub issue ready 1") { + t.Errorf("must NOT point at non-existent `issue ready`: %q", msg) + } + if !strings.Contains(msg, "only applies to pull requests") { + t.Errorf("expected asymmetry explanation: %q", msg) + } + if !strings.Contains(msg, "shithub issue view 1") { + t.Errorf("expected view-redirect fallback: %q", msg) + } +} + +// TestCheckAsymmetric_PRMergeOnIssue is the same shape for `pr merge`. +func TestCheckAsymmetric_PRMergeOnIssue(t *testing.T) { + tf := cmdutiltest.New(t) + tf.Server.RegisterJSON(http.MethodGet, "/api/v1/repos/o/r/issues/1", 200, issues.Issue{Number: 1}) + + client, _ := tf.Factory.HTTPClient("") + ic := issues.NewClient(client) + pc := pulls.NewClient(client) + + err := crosskind.CheckAsymmetric(context.Background(), ic, pc, "o", "r", 1, "pr", "pr merge", "merge", true) + if err == nil { + t.Fatal("expected ErrWrongNamespace") + } + if strings.Contains(err.Error(), "shithub issue merge") { + t.Errorf("must NOT point at non-existent `issue merge`: %v", err) + } +} + +// TestCheck_StillSuggestsForSymmetricVerb confirms the symmetric path +// (`pr close`, `pr edit`) keeps the existing "try `shithub issue X N`" +// redirect — those verbs DO exist on the other side. +func TestCheck_StillSuggestsForSymmetricVerb(t *testing.T) { + tf := cmdutiltest.New(t) + tf.Server.RegisterJSON(http.MethodGet, "/api/v1/repos/o/r/issues/1", 200, issues.Issue{Number: 1}) + + client, _ := tf.Factory.HTTPClient("") + ic := issues.NewClient(client) + pc := pulls.NewClient(client) + + err := crosskind.Check(context.Background(), ic, pc, "o", "r", 1, "pr", "pr close", "close") + if err == nil { + t.Fatal("expected ErrWrongNamespace") + } + if !strings.Contains(err.Error(), "shithub issue close 1") { + t.Errorf("symmetric verb should still redirect: %v", err) + } +} From 6cc53c906fff426593854c289b1fffa6722cb466 Mon Sep 17 00:00:00 2001 From: espadonne Date: Sat, 23 May 2026 20:30:35 -0400 Subject: [PATCH 3/5] =?UTF-8?q?audit-I6:=20pr=20ready=20=E2=80=94=20flag?= =?UTF-8?q?=20asymmetric=20so=20error=20doesn't=20suggest=20non-existent?= =?UTF-8?q?=20'issue=20ready'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/cmd/pr/ready/ready.go | 6 ++++-- pkg/cmd/pr/ready/ready_test.go | 13 +++++++++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/pr/ready/ready.go b/pkg/cmd/pr/ready/ready.go index e3c6cf2..15d61eb 100644 --- a/pkg/cmd/pr/ready/ready.go +++ b/pkg/cmd/pr/ready/ready.go @@ -98,9 +98,11 @@ func Run(ctx context.Context, opts *options) error { // H2: cross-namespace verb routing — if the number is actually an // issue, surface a friendly redirect rather than "pull request not - // found". + // found". audit-I6: `pr ready` is PR-only, so flag the redirect as + // asymmetric so the message explains the constraint instead of + // pointing at `issue ready` (which doesn't exist). ic := issues.NewClient(client) - if err := crosskind.Check(ctx, ic, pc, ref.Repo.Owner, ref.Repo.Name, ref.Number, "pr", "pr ready", "ready"); err != nil { + if err := crosskind.CheckAsymmetric(ctx, ic, pc, ref.Repo.Owner, ref.Repo.Name, ref.Number, "pr", "pr ready", "ready", true); err != nil { return err } diff --git a/pkg/cmd/pr/ready/ready_test.go b/pkg/cmd/pr/ready/ready_test.go index 490c057..dafab8e 100644 --- a/pkg/cmd/pr/ready/ready_test.go +++ b/pkg/cmd/pr/ready/ready_test.go @@ -148,7 +148,16 @@ func TestReadyWrongNamespaceRedirects(t *testing.T) { if !strings.Contains(err.Error(), "is an issue") { t.Errorf("error should redirect: %v", err) } - if !strings.Contains(err.Error(), "shithub issue ready 5") { - t.Errorf("error should suggest issue ready: %v", err) + // audit-I6: `pr ready` is asymmetric — error must NOT point at + // `issue ready` (which doesn't exist) and SHOULD point at the + // `issue view` inspection path. + if strings.Contains(err.Error(), "shithub issue ready") { + t.Errorf("must NOT suggest non-existent `issue ready`: %v", err) + } + if !strings.Contains(err.Error(), "only applies to pull requests") { + t.Errorf("expected asymmetry explanation: %v", err) + } + if !strings.Contains(err.Error(), "shithub issue view 5") { + t.Errorf("expected view-redirect fallback: %v", err) } } From 40aae5a16c6e0a6bd7cbc6d3f022da266cabedd8 Mon Sep 17 00:00:00 2001 From: espadonne Date: Sat, 23 May 2026 20:30:35 -0400 Subject: [PATCH 4/5] =?UTF-8?q?audit-I6:=20pr=20merge=20=E2=80=94=20flag?= =?UTF-8?q?=20asymmetric=20so=20error=20doesn't=20suggest=20non-existent?= =?UTF-8?q?=20'issue=20merge'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/cmd/pr/merge/merge.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 549cb13..0305410 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -184,9 +184,11 @@ func Run(ctx context.Context, opts *options) error { if err != nil { // H2: cross-namespace verb routing — if the number is an issue, // surface a friendly redirect instead of "pull request not found". + // audit-I6: `pr merge` is PR-only — mark asymmetric so the + // redirect doesn't point at the non-existent `issue merge`. if api.IsNotFoundError(err) { ic := issues.NewClient(client) - if cerr := crosskind.Check(ctx, ic, pc, ref.Repo.Owner, ref.Repo.Name, ref.Number, "pr", "pr merge", "merge"); cerr != nil { + if cerr := crosskind.CheckAsymmetric(ctx, ic, pc, ref.Repo.Owner, ref.Repo.Name, ref.Number, "pr", "pr merge", "merge", true); cerr != nil { return cerr } } From f099d84f27dd1e5e347698aa579b1e83bb35e52f Mon Sep 17 00:00:00 2001 From: espadonne Date: Sat, 23 May 2026 20:30:35 -0400 Subject: [PATCH 5/5] =?UTF-8?q?audit-I6:=20pr=20review=20=E2=80=94=20flag?= =?UTF-8?q?=20asymmetric=20so=20error=20doesn't=20suggest=20non-existent?= =?UTF-8?q?=20'issue=20review'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/cmd/pr/review/review.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/review/review.go b/pkg/cmd/pr/review/review.go index fd170ee..b35e049 100644 --- a/pkg/cmd/pr/review/review.go +++ b/pkg/cmd/pr/review/review.go @@ -142,8 +142,10 @@ func Run(ctx context.Context, opts *options) error { // H2: cross-namespace verb routing. Submitting a review on an // issue number surfaced "pull request not found"; redirect instead. + // audit-I6: `pr review` is PR-only — flag asymmetric so we don't + // emit "try `shithub issue review N`" pointing at a dead command. ic := issues.NewClient(client) - if cerr := crosskind.Check(ctx, ic, pc, ref.Repo.Owner, ref.Repo.Name, ref.Number, "pr", "pr review", "review"); cerr != nil { + if cerr := crosskind.CheckAsymmetric(ctx, ic, pc, ref.Repo.Owner, ref.Repo.Name, ref.Number, "pr", "pr review", "review", true); cerr != nil { return cerr }