diff --git a/cmd/root.go b/cmd/root.go index 425bded7..c25ed996 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -377,12 +377,18 @@ func installUnknownSubcommandGuard(cmd *cobra.Command) { // they have moved to the typed surface. func unknownSubcommandRunE(cmd *cobra.Command, args []string) error { if len(args) == 0 { - // A bare group (e.g. `sheets`) legitimately prints help. But an unknown - // flag placed before any subcommand (`sheets --badflag`) is whitelisted - // away by installUnknownSubcommandGuard, which also leaves args empty — - // without this check it would silently fall through to help + exit 0. - // Recover the swallowed flag tokens and fail structured so agents (and - // the flagDidYouMean contract) still see a real error. + // A truly bare group (e.g. `sheets`) legitimately prints help. But any + // flag token with no subcommand is a user error: a pure group consumes + // no flags of its own, so the flag must belong to a (missing) subcommand. + // installUnknownSubcommandGuard whitelists those flags and leaves args + // empty, so without this they would silently fall through to help + + // exit 0 — letting an agent mistake a malformed call (`im --format json`, + // `sheets --badflag`) for success. Recover the swallowed tokens from the + // raw invocation and fail structured instead. + flags := flagTokensInArgs(rawInvocationArgs) + if len(flags) == 0 { + return cmd.Help() + } if unknown := unknownFlagTokens(cmd, rawInvocationArgs); len(unknown) > 0 { return &output.ExitError{ Code: output.ExitValidation, @@ -407,7 +413,22 @@ func unknownSubcommandRunE(cmd *cobra.Command, args []string) error { }, } } - return cmd.Help() + // Every flag is valid for some subcommand, but no subcommand was given + // (e.g. `im --format json`). Distinct from unknown_flag: the flags are + // real, the subcommand is what's missing. + return &output.ExitError{ + Code: output.ExitValidation, + Detail: &output.ErrDetail{ + Type: "missing_subcommand", + Message: fmt.Sprintf("missing subcommand for %q; flag %s belongs to a subcommand, not the group", cmd.CommandPath(), strings.Join(flags, ", ")), + Hint: fmt.Sprintf("run `%s --help` to list subcommands and their flags", cmd.CommandPath()), + Detail: map[string]any{ + "command_path": cmd.CommandPath(), + "flags": flags, + "suggestions": []string{}, + }, + }, + } } unknown := args[0] available, deprecated := availableSubcommandNames(cmd) @@ -444,13 +465,14 @@ func unknownSubcommandRunE(cmd *cobra.Command, args []string) error { } } -// unknownFlagTokens returns the -/-- tokens in rawArgs that cmd does not define. -// installUnknownSubcommandGuard whitelists unknown flags on pure groups so a -// mistyped subcommand still reaches the suggestion path; the side effect is that -// a lone unknown flag (no subcommand) is swallowed, leaving the group to fall -// through to help. This recovers those tokens so the caller can fail structured. -func unknownFlagTokens(cmd *cobra.Command, rawArgs []string) []string { - var unknown []string +// flagTokensInArgs returns the flag-like tokens (-x, --foo, --foo=bar) in +// rawArgs, stopping at the "--" positional terminator. Whether a flag is +// defined is not considered (see unknownFlagTokens for that). A pure group +// with any flag token but no subcommand is a user error — a pure group +// consumes no flags of its own, so the flag must belong to a subcommand — so +// the caller fails structured instead of falling through to help. +func flagTokensInArgs(rawArgs []string) []string { + var toks []string for _, a := range rawArgs { if a == "--" { break // everything after -- is positional @@ -458,6 +480,20 @@ func unknownFlagTokens(cmd *cobra.Command, rawArgs []string) []string { if len(a) < 2 || a[0] != '-' { continue } + toks = append(toks, a) + } + return toks +} + +// unknownFlagTokens returns the flag tokens in rawArgs that cmd does not define +// (on itself, inherited, or any direct subcommand). installUnknownSubcommandGuard +// whitelists unknown flags on pure groups so a mistyped subcommand still reaches +// the suggestion path; the side effect is that flags before a subcommand are +// swallowed. This recovers the genuinely-unknown ones so the caller can name +// them in a "did you mean" envelope. +func unknownFlagTokens(cmd *cobra.Command, rawArgs []string) []string { + var unknown []string + for _, a := range flagTokensInArgs(rawArgs) { name := strings.SplitN(strings.TrimLeft(a, "-"), "=", 2)[0] if name != "" && !flagDefinedInTree(cmd, name) { unknown = append(unknown, a) @@ -566,6 +602,11 @@ func flagDidYouMean(c *cobra.Command, ferr error) error { // error text ("unknown flag: --query" → "query"). Returns ok=false for anything // else (missing argument, invalid value, unknown shorthand) so the caller keeps // those structured but generic — hallucinated flags are essentially always long. +// +// CONTRACT: this matches cobra's English wording "unknown flag: --" (go.mod +// pins github.com/spf13/cobra). If cobra rewords this or gains i18n the match +// silently fails and unknown flags degrade to a generic flag_error — re-verify +// this prefix when bumping cobra. func unknownFlagName(err error) (string, bool) { const p = "unknown flag: --" msg := err.Error() diff --git a/cmd/unknown_subcommand_test.go b/cmd/unknown_subcommand_test.go index e765341e..7153b647 100644 --- a/cmd/unknown_subcommand_test.go +++ b/cmd/unknown_subcommand_test.go @@ -152,6 +152,48 @@ func TestUnknownSubcommandRunE_FlagBeforeSubcommandIsStructured(t *testing.T) { } } +func TestUnknownSubcommandRunE_ValidFlagWithoutSubcommandIsStructured(t *testing.T) { + _, drive, _ := newGroupTree() + // --query is defined on the +search subcommand, so it is a *valid* flag that + // was placed before the (omitted) subcommand. Unlike an unknown flag, this + // must still fail structured (missing_subcommand) rather than fall through to + // help + exit 0 — `drive --query x` is a malformed call, not a help request. + for _, c := range drive.Commands() { + if c.Name() == "+search" { + c.Flags().String("query", "", "") + } + } + installUnknownSubcommandGuard(drive.Root()) + + rawInvocationArgs = []string{"drive", "--query", "x"} + t.Cleanup(func() { rawInvocationArgs = nil }) + + err := drive.RunE(drive, nil) + if err == nil { + t.Fatal("expected a structured missing_subcommand error, got nil (help fallthrough)") + } + var exitErr *output.ExitError + if !errors.As(err, &exitErr) { + t.Fatalf("expected *output.ExitError, got %T", err) + } + if exitErr.Code != output.ExitValidation { + t.Errorf("exit code = %d, want %d", exitErr.Code, output.ExitValidation) + } + if exitErr.Detail == nil || exitErr.Detail.Type != "missing_subcommand" { + t.Fatalf("detail.Type = %v, want missing_subcommand", exitErr.Detail) + } + detail, ok := exitErr.Detail.Detail.(map[string]any) + if !ok { + t.Fatalf("detail is not a map: %#v", exitErr.Detail.Detail) + } + if flags, _ := detail["flags"].([]string); len(flags) != 1 || flags[0] != "--query" { + t.Errorf("detail.flags = %v, want [--query]", detail["flags"]) + } + if detail["command_path"] != "lark-cli drive" { + t.Errorf("detail.command_path = %v, want lark-cli drive", detail["command_path"]) + } +} + func TestUnknownSubcommandRunE_NoArgsShowsHelp(t *testing.T) { _, drive, _ := newGroupTree() installUnknownSubcommandGuard(drive.Root()) diff --git a/go.mod b/go.mod index 1ee4b73c..8c76bd7d 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/sergi/go-diff v1.4.0 github.com/skip2/go-qrcode v0.0.0-20200617195104-da1b6568686e github.com/smartystreets/goconvey v1.8.1 - github.com/spf13/cobra v1.10.2 + github.com/spf13/cobra v1.10.2 // flag-error-text contract: see cmd/root.go unknownFlagName github.com/spf13/pflag v1.0.9 github.com/stretchr/testify v1.11.1 github.com/tidwall/gjson v1.18.0 diff --git a/shortcuts/common/runner.go b/shortcuts/common/runner.go index 2906830e..aa507f48 100644 --- a/shortcuts/common/runner.go +++ b/shortcuts/common/runner.go @@ -1117,8 +1117,15 @@ func registerShortcutFlagsWithContext(ctx context.Context, cmd *cobra.Command, f cmd.Flags().Bool("yes", false, "confirm high-risk operation") } if s.PrintFlagSchema != nil { - cmd.Flags().Bool("print-schema", false, "print JSON Schema for a composite flag instead of executing") - cmd.Flags().String("flag-name", "", "flag whose schema to print (omit to list introspectable flags); used with --print-schema") + // Guard against a shortcut that already declares these reserved + // introspection flags: pflag panics on a duplicate registration. + // Mirrors the Lookup guard on --format above. + if cmd.Flags().Lookup("print-schema") == nil { + cmd.Flags().Bool("print-schema", false, "print JSON Schema for a composite flag instead of executing") + } + if cmd.Flags().Lookup("flag-name") == nil { + cmd.Flags().String("flag-name", "", "flag whose schema to print (omit to list introspectable flags); used with --print-schema") + } } cmd.Flags().StringP("jq", "q", "", "jq expression to filter JSON output") cmdutil.AddShortcutIdentityFlag(ctx, cmd, f, s.AuthTypes) diff --git a/shortcuts/common/runner_flag_completion_test.go b/shortcuts/common/runner_flag_completion_test.go index 3c96dbd1..b984397d 100644 --- a/shortcuts/common/runner_flag_completion_test.go +++ b/shortcuts/common/runner_flag_completion_test.go @@ -96,3 +96,43 @@ func TestShortcutMount_FlagCompletionsDisabled(t *testing.T) { t.Fatal("did not expect completion func for --format when disabled") } } + +// TestShortcutMount_ReservedIntrospectionFlagCollision verifies the reserved +// --print-schema / --flag-name flags are registered defensively: a shortcut +// that already declares same-named flags must not trigger pflag's duplicate- +// registration panic (the Lookup guard in registerShortcutFlagsWithContext). +func TestShortcutMount_ReservedIntrospectionFlagCollision(t *testing.T) { + f, _, _, _ := cmdutil.TestFactory(t, nil) + parent := &cobra.Command{Use: "root"} + shortcut := Shortcut{ + Service: "docs", + Command: "+introspect", + Description: "x", + // The shortcut's own flags collide with the names the runner auto- + // injects when PrintFlagSchema is set. Without the guard, pflag panics. + Flags: []Flag{ + {Name: "print-schema", Desc: "user-defined collision"}, + {Name: "flag-name", Desc: "user-defined collision"}, + }, + PrintFlagSchema: func(string) ([]byte, error) { return nil, nil }, + Execute: func(context.Context, *RuntimeContext) error { return nil }, + } + + defer func() { + if r := recover(); r != nil { + t.Fatalf("Mount panicked on a reserved-flag name collision (Lookup guard missing?): %v", r) + } + }() + shortcut.Mount(parent, f) + + cmd, _, err := parent.Find([]string{"+introspect"}) + if err != nil { + t.Fatalf("Find() error = %v", err) + } + if cmd.Flags().Lookup("print-schema") == nil { + t.Error("print-schema flag should still exist after the guarded registration") + } + if cmd.Flags().Lookup("flag-name") == nil { + t.Error("flag-name flag should still exist after the guarded registration") + } +}