diff --git a/cmd/build.go b/cmd/build.go index ae766bfdd5..a4becdb435 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -236,9 +236,14 @@ func buildInternalWithConfig(ctx context.Context, inv cmdutil.InvocationContext, } rootCmd.SetContext(ctx) - rootCmd.SetIn(cfg.streams.In) - rootCmd.SetOut(cfg.streams.Out) - rootCmd.SetErr(cfg.streams.ErrOut) + // f.IOStreams is cfg.streams with any stream the caller left unset filled + // in, so cobra and the commands write to the same three destinations. + rootCmd.SetIn(f.IOStreams.In) + // Cobra renders framework output such as --version through this writer, + // before it reaches any command callback. Type a writer failure at that + // boundary so a broken stdout pipe is never reported as invalid input. + rootCmd.SetOut(internalErrorWriter{Writer: f.IOStreams.Out}) + rootCmd.SetErr(f.IOStreams.ErrOut) // Root-only usage template (curated Usage synopsis + skills footer); see // rootUsageTemplate. @@ -371,11 +376,23 @@ func buildInternalWithConfig(ctx context.Context, inv cmdutil.InvocationContext, if hookRegistry != nil { installHooks(rootCmd, hookRegistry) } + if hasConcealedCommands { installHelpCommand(rootCmd) } finalizeRootCommandGroups(rootCmd, runtime.surface) + // Type errors only after the command tree is final. Plugin wrappers are + // therefore inside the execution boundary (a wrapper failure is internal), + // while the concealment-specific help command is covered without exposing + // it to plugins. The stateless wrappers also make repeated Execute calls on + // a Build-produced tree independent. + // + // A callback installed past this line is outside the walk and owns its own + // classification. An untyped error from one is read as a bad command line, + // which is why the fatal guards below build a typed error themselves. + instrumentErrorBoundaries(rootCmd) + if hookRegistry != nil && !cfg.deferStartup { if err := emitStartup(ctx, hookRegistry); err != nil { installPluginLifecycleErrorGuard(rootCmd, err) @@ -390,5 +407,6 @@ func buildInternalWithConfig(ctx context.Context, inv cmdutil.InvocationContext, func finalizeFailedBuild(runtime *buildRuntime, root *cobra.Command) (*buildRuntime, *cobra.Command, *hook.Registry) { finalizeRootCommandGroups(root, runtime.surface) + instrumentErrorBoundaries(root) return runtime, root, nil } diff --git a/cmd/error_boundary.go b/cmd/error_boundary.go new file mode 100644 index 0000000000..411edcbe60 --- /dev/null +++ b/cmd/error_boundary.go @@ -0,0 +1,151 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package cmd + +import ( + "errors" + "io" + + "github.com/spf13/cobra" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/output" +) + +// instrumentErrorBoundaries walks the final command tree and types errors at +// the boundary that owns them: +// +// - Args: cobra's own positional validators (ExactArgs, MaximumNArgs, ...) +// return plain errors. Wrapping at the single place they are invoked +// converts every one of them — including validators added later — into a +// typed validation error, so no call site has to remember to do it. +// - PersistentPreRunE / PreRunE / RunE / PostRunE / PersistentPostRunE: +// these are application and plugin execution seams. A plain error escaping +// one is a missing classification in our code, so it becomes internal. +// +// The walk is deliberately stateless. Reusing one tree for several Execute +// calls or building several trees in one process cannot leak classification +// state between invocations. +// +// Cobra registers three subtrees of its own during Execute, after this walk has +// already run. Each is accounted for without it: +// +// - help: carries a Run, not a RunE, so it never hands an error back to +// Execute. A failure to render help or usage text ends the process on the +// spot instead, which is why LifecycleContext lists it among the failures +// that never reach Shutdown. +// - __complete: also a Run. Its MinimumNArgs validator is the only thing +// that can fail, and cobra surfaces that as a residual validation error +// for normalizeRootError to classify. +// - completion and its per-shell subcommands: the only lazily registered +// RunE bodies. Each returns what writing the generated script returns, to +// the writer cobra captured from the root when it registered the command — +// internalErrorWriter, which types its own failures. Their NoArgs +// validators fail as residual cobra validation, like __complete's. +func instrumentErrorBoundaries(root *cobra.Command) { + if root == nil { + return + } + instrumentCommandBoundaries(root) +} + +func instrumentCommandBoundaries(cmd *cobra.Command) { + if inner := cmd.Args; inner != nil { + cmd.Args = func(c *cobra.Command, args []string) error { + return typedArgsError(inner(c, args)) + } + } + + if inner := cmd.PersistentPreRunE; inner != nil { + cmd.PersistentPreRunE = func(c *cobra.Command, args []string) error { + return typedCommandError(inner(c, args)) + } + } + if inner := cmd.PreRunE; inner != nil { + cmd.PreRunE = func(c *cobra.Command, args []string) error { + return typedCommandError(inner(c, args)) + } + } + if inner := cmd.RunE; inner != nil { + cmd.RunE = func(c *cobra.Command, args []string) error { + return typedCommandError(inner(c, args)) + } + } + if inner := cmd.PostRunE; inner != nil { + cmd.PostRunE = func(c *cobra.Command, args []string) error { + return typedCommandError(inner(c, args)) + } + } + if inner := cmd.PersistentPostRunE; inner != nil { + cmd.PersistentPostRunE = func(c *cobra.Command, args []string) error { + return typedCommandError(inner(c, args)) + } + } + + for _, sub := range cmd.Commands() { + instrumentCommandBoundaries(sub) + } +} + +// typedArgsError converts a positional-argument rejection into a typed +// validation error. A validator that already returns a typed error (the +// shortcut framework's own) is left alone, so it keeps the richer param and +// hint it produced. +func typedArgsError(err error) error { + if err == nil { + return nil + } + if hasOwnedErrorSemantics(err) { + return err + } + return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err.Error()). + WithCause(err) +} + +// typedCommandError preserves errors that already own their classification or +// exit behavior. Any other error escaped application or plugin execution +// without going through errs, which is an internal contract violation. +func typedCommandError(err error) error { + if err == nil { + return nil + } + if hasOwnedErrorSemantics(err) { + return err + } + return errs.WrapInternal(err) +} + +// hasOwnedErrorSemantics reports whether an error already controls either its +// structured envelope or its exit-only result. errors.As intentionally +// recognizes signals behind a wrapping error so boundary instrumentation does +// not destroy their semantics. +func hasOwnedErrorSemantics(err error) bool { + if _, ok := errs.ProblemOf(err); ok { + return true + } + var bare *output.BareError + if errors.As(err, &bare) { + return true + } + var partial *output.PartialFailureError + if errors.As(err, &partial) { + return true + } + return false +} + +// internalErrorWriter types failures at Cobra's output boundary. In +// particular, Cobra renders --version before any RunE seam; without this +// writer a broken stdout pipe would look like an invalid command line. +type internalErrorWriter struct { + io.Writer +} + +func (w internalErrorWriter) Write(p []byte) (int, error) { + n, err := w.Writer.Write(p) + if n < len(p) && err == nil { + err = io.ErrShortWrite + } + return n, errs.WrapInternal(err) +} diff --git a/cmd/error_boundary_test.go b/cmd/error_boundary_test.go new file mode 100644 index 0000000000..d7852f358a --- /dev/null +++ b/cmd/error_boundary_test.go @@ -0,0 +1,819 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package cmd + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/extension/platform" + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/hook" + "github.com/larksuite/cli/internal/output" +) + +// writeAppConfigWithoutUser installs a config holding an app but no logged-in +// user, which is what drives `auth check` down its exit-code-only signal path. +func writeAppConfigWithoutUser(t *testing.T, cfgDir string) { + t.Helper() + body := `{"apps":[{"name":"probe","appId":"cli_probe","appSecret":"probe-secret","brand":"feishu","users":[]}],"currentApp":"probe"}` + if err := os.WriteFile(filepath.Join(cfgDir, "config.json"), []byte(body), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } +} + +// captureShutdownErr registers a plugin whose only job is to record the error +// handed to the Shutdown lifecycle event. +func captureShutdownErr(t *testing.T, observed *error, fired *int) { + t.Helper() + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + platform.Register(platform.NewPlugin("probe", "1.0"). + On(platform.Shutdown, "capture", + func(_ context.Context, lc *platform.LifecycleContext) error { + *fired++ + *observed = lc.Err + return nil + }). + MustBuild()) +} + +func quietNotices(t *testing.T) { + t.Helper() + t.Setenv("LARKSUITE_CLI_NO_UPDATE_NOTIFIER", "1") + t.Setenv("LARKSUITE_CLI_NO_SKILLS_NOTIFIER", "1") +} + +// TestExitCodeOnlySignalSurvivesFullDispatch pins the contract that an +// exit-code-only signal keeps its exit code and writes nothing to stderr when +// it travels the whole way through ExecuteWithOptions. Classifying it instead +// would put a second, contradictory error envelope on stderr next to the +// result already on stdout, and replace exit 1 with the internal-fault code. +func TestExitCodeOnlySignalSurvivesFullDispatch(t *testing.T) { + cfgDir := tmpHome(t) + quietNotices(t) + writeAppConfigWithoutUser(t, cfgDir) + + var observed error + fired := 0 + captureShutdownErr(t, &observed, &fired) + + code, stdout, stderr := executeWithCapturedOS(t, nil, + "auth", "check", "--scope", "im:message:send_as_bot") + + if stderr != "" { + t.Errorf("stderr must stay empty for an exit-code-only signal, got %q", stderr) + } + if !strings.Contains(stdout, `"ok": false`) { + t.Errorf("the result envelope belongs on stdout, got %q", stdout) + } + if code != 1 { + t.Errorf("exit code = %d, want 1 (the signal's own code)", code) + } + if fired != 1 { + t.Fatalf("Shutdown handler fired %d times, want 1", fired) + } + if _, ok := errs.ProblemOf(observed); ok { + t.Errorf("Shutdown observed a classified error %v; an exit-code-only "+ + "signal carries no Problem and must pass through unchanged", observed) + } + var bare *output.BareError + if !errors.As(observed, &bare) { + t.Errorf("Shutdown observed %T, want the original *output.BareError", observed) + } +} + +// TestShutdownHookCannotRewriteUserVisibleFailure pins that what the user got +// is settled before the Shutdown event fires. Typed errors carry exported +// fields, so a handler reaching through errs.ProblemOf really can write to the +// error it is given; ordering is what makes that harmless. +func TestShutdownHookCannotRewriteUserVisibleFailure(t *testing.T) { + tmpHome(t) + quietNotices(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + fired := 0 + platform.Register(platform.NewPlugin("tamper", "1.0"). + On(platform.Shutdown, "rewrite", + func(_ context.Context, lc *platform.LifecycleContext) error { + fired++ + if p, ok := errs.ProblemOf(lc.Err); ok { + p.Category = errs.CategoryNetwork + p.Subtype = "rewritten_by_plugin" + p.Message = "rewritten by plugin" + p.Hint = "rewritten by plugin" + } + return nil + }). + MustBuild()) + + code, _, stderr := executeWithCapturedOS(t, nil, "definitely-not-a-command") + + // Without this, a Shutdown handler that never runs would leave the + // envelope untouched and pass the assertions below for the wrong reason. + if fired != 1 { + t.Fatalf("Shutdown handler fired %d times, want 1", fired) + } + + var envelope struct { + Error struct { + Category errs.Category `json:"type"` + Subtype errs.Subtype `json:"subtype"` + Message string `json:"message"` + Hint string `json:"hint"` + } `json:"error"` + } + if err := json.Unmarshal([]byte(stderr), &envelope); err != nil { + t.Fatalf("stderr is not a JSON envelope: %v\n%s", err, stderr) + } + if envelope.Error.Category != errs.CategoryValidation || + envelope.Error.Subtype != errs.SubtypeInvalidArgument { + t.Errorf("plugin rewrote the envelope: %s/%s", + envelope.Error.Category, envelope.Error.Subtype) + } + if strings.Contains(envelope.Error.Message, "rewritten") || + strings.Contains(envelope.Error.Hint, "rewritten") { + t.Errorf("plugin rewrote user-visible text: message=%q hint=%q", + envelope.Error.Message, envelope.Error.Hint) + } + if code != output.ExitValidation { + t.Errorf("exit code = %d, want %d; a Shutdown handler must not change it", + code, output.ExitValidation) + } +} + +// TestCobraValidationFailuresAreUserErrors covers every shape cobra rejects a +// command line with before any command body runs. All of them are mistakes in +// what the user typed, so none may be reported as an internal fault — that +// would tell the user the tool broke and would count their typo against +// service health. +func TestCobraValidationFailuresAreUserErrors(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + {"unknown command", []string{"definitely-not-a-command"}}, + {"unknown subcommand", []string{"sheets", "+definitely-nope"}}, + {"missing required flag", []string{"auth", "check"}}, + {"wrong argument count", []string{"profile", "remove", "first", "second"}}, + {"positional arg on a shortcut", []string{"wiki", "+space-list", "stray"}}, + {"flag group one-required", []string{ + "sheets", "+csv-put", "--spreadsheet-token", "Xxxxxxxxxxx", + "--sheet-id", "abc", "--csv", "a,b"}}, + {"flag group mutually exclusive", []string{ + "sheets", "+csv-put", "--spreadsheet-token", "Xxxxxxxxxxx", + "--sheet-id", "abc", "--csv", "a,b", "--start-cell", "A1", "--range", "A1"}}, + } { + t.Run(tc.name, func(t *testing.T) { + tmpHome(t) + quietNotices(t) + + var observed error + fired := 0 + captureShutdownErr(t, &observed, &fired) + + code, _, stderr := executeWithCapturedOS(t, nil, tc.args...) + + var envelope struct { + Error struct { + Category errs.Category `json:"type"` + Subtype errs.Subtype `json:"subtype"` + Message string `json:"message"` + } `json:"error"` + } + if err := json.Unmarshal([]byte(stderr), &envelope); err != nil { + t.Fatalf("stderr is not a JSON envelope: %v\n%s", err, stderr) + } + if envelope.Error.Category != errs.CategoryValidation || + envelope.Error.Subtype != errs.SubtypeInvalidArgument { + t.Errorf("reported as %s/%s, want %s/%s", + envelope.Error.Category, envelope.Error.Subtype, + errs.CategoryValidation, errs.SubtypeInvalidArgument) + } + if code != output.ExitValidation { + t.Errorf("exit code = %d, want %d", code, output.ExitValidation) + } + + // The Shutdown handler must agree with what the user was told. + if fired != 1 { + t.Fatalf("Shutdown handler fired %d times, want 1", fired) + } + problem, ok := errs.ProblemOf(observed) + if !ok { + t.Fatalf("Shutdown observed unclassified %T (%v)", observed, observed) + } + if problem.Category != envelope.Error.Category || + problem.Subtype != envelope.Error.Subtype || + problem.Message != envelope.Error.Message { + t.Errorf("Shutdown observed %s/%s %q, envelope reports %s/%s %q", + problem.Category, problem.Subtype, problem.Message, + envelope.Error.Category, envelope.Error.Subtype, envelope.Error.Message) + } + if got := output.ExitCodeOf(observed); got != code { + t.Errorf("Shutdown observed exit code %d, process exited %d", got, code) + } + }) + } +} + +// TestEveryArgsValidatorProducesTypedErrors is the guard that keeps a newly +// added positional validator from silently reporting a user's mistake as an +// internal fault: instrumentErrorBoundaries must reach every command in the +// tree, so no Args validator can return an unclassified error. Empty, single, +// and oversized probes cover both lower- and upper-bound validators. +func TestEveryArgsValidatorProducesTypedErrors(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t), WithoutPlugins()) + + oversized := make([]string, 64) + for i := range oversized { + oversized[i] = fmt.Sprintf("stray%d", i+1) + } + probes := []struct { + name string + args []string + }{ + {name: "empty"}, + {name: "single", args: []string{"stray"}}, + {name: "oversized", args: oversized}, + } + hits := make([]int, len(probes)) + var unguarded []string + forEachCommand(root, func(c *cobra.Command) { + if c.Args == nil { + return + } + for i, probe := range probes { + err := c.Args(c, probe.args) + if err == nil { + continue + } + hits[i]++ + problem, ok := errs.ProblemOf(err) + if !ok { + unguarded = append(unguarded, + fmt.Sprintf("%s/%s (unclassified)", c.CommandPath(), probe.name)) + continue + } + // A typed error is not enough: rejecting what the user typed must be + // reported as a user error, never as an internal fault. + if problem.Category != errs.CategoryValidation || + problem.Subtype != errs.SubtypeInvalidArgument { + unguarded = append(unguarded, fmt.Sprintf("%s/%s (%s/%s)", + c.CommandPath(), probe.name, problem.Category, problem.Subtype)) + } + } + }) + + for i, hit := range hits { + if hit == 0 { + t.Errorf("no Args validator rejected the %s probe; the walk did not cover that shape", probes[i].name) + } + } + if len(unguarded) > 0 { + t.Errorf("these commands misreport a positional-argument mistake: %v", unguarded) + } +} + +// forEachCommand visits root and every command beneath it. +func forEachCommand(cmd *cobra.Command, visit func(*cobra.Command)) { + visit(cmd) + for _, sub := range cmd.Commands() { + forEachCommand(sub, visit) + } +} + +// unrenderableTypedError is a problem carrier the envelope writer cannot +// serialize: the exported func field makes json.Marshal fail. A plugin's Wrap +// chain returning a value like this is the realistic way to reach the +// dispatcher's last-resort branch. +type unrenderableTypedError struct { + *errs.Problem + Leak func() `json:"leak"` +} + +type silentShortWriter struct{} + +func (silentShortWriter) Write(p []byte) (int, error) { + if len(p) == 0 { + return 0, nil + } + return len(p) - 1, nil +} + +// TestUnrenderableTypedErrorStillReachesStderr pins why the last-resort branch +// rebuilds the error instead of reusing it: the value it receives has just +// failed to serialize, so handing the same value to the writer again would +// leave the user with a non-zero exit and a silent stderr. +// +// The probe deliberately carries a category other than internal. With an +// internal one, a fallback that discarded the producer's category would land +// on the same answer by coincidence and the assertions below would hold for +// the wrong reason. +func TestUnrenderableTypedErrorStillReachesStderr(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + + broken := &unrenderableTypedError{ + Problem: &errs.Problem{ + Category: errs.CategoryNetwork, + Subtype: errs.SubtypeNetworkTimeout, + Message: "upstream blew up", + }, + Leak: func() {}, + } + + // Precondition: this value really cannot render itself. + if output.WriteTypedErrorEnvelope(io.Discard, broken, "user") { + t.Fatal("precondition failed: the probe error serialized successfully") + } + + f, _, _, _ := cmdutil.TestFactory(t, nil) + errOut := &bytes.Buffer{} + f.IOStreams.ErrOut = errOut + + exit := handleRootError(f, broken, nil) + + if errOut.Len() == 0 { + t.Fatal("stderr is empty; a non-zero exit must always be explained") + } + var envelope struct { + Error struct { + Category errs.Category `json:"type"` + Message string `json:"message"` + } `json:"error"` + } + if err := json.Unmarshal(errOut.Bytes(), &envelope); err != nil { + t.Fatalf("stderr is not a JSON envelope: %v\n%s", err, errOut.String()) + } + if envelope.Error.Message != "upstream blew up" { + t.Errorf("message = %q, want the original text preserved", envelope.Error.Message) + } + if want := output.ExitCodeOf(broken); exit != want { + t.Errorf("exit = %d, want %d; the rebuilt error must keep the original category's exit code", + exit, want) + } +} + +// TestShortcutPositionalErrorPointsAtTheFlags pins the diagnostics the shortcut +// framework adds over the generic Args wrapper: the stray word the user typed, +// and where to look for the flags to use instead. +// +// It also pins what the envelope must not carry. `param` names the parameter +// the caller has to correct and agents read it literally; a shortcut declares +// no positional parameter, so echoing the stray word there would send an agent +// looking for a flag by that name. It would also put unbounded user input in a +// field the contract classes as stable per subtype. +func TestShortcutPositionalErrorPointsAtTheFlags(t *testing.T) { + tmpHome(t) + quietNotices(t) + + _, _, stderr := executeWithCapturedOS(t, nil, "wiki", "+space-list", "stray") + + var envelope struct { + Error struct { + Message string `json:"message"` + Hint string `json:"hint"` + Param string `json:"param"` + Params []errs.InvalidParam `json:"params"` + } `json:"error"` + } + if err := json.Unmarshal([]byte(stderr), &envelope); err != nil { + t.Fatalf("stderr is not a JSON envelope: %v\n%s", err, stderr) + } + if !strings.Contains(envelope.Error.Message, "stray") { + t.Errorf("message = %q, want the stray word shown", envelope.Error.Message) + } + if !strings.Contains(envelope.Error.Hint, "--help") { + t.Errorf("hint = %q, want it to point at --help", envelope.Error.Hint) + } + if envelope.Error.Param != "" { + t.Errorf("param = %q, want it absent: a shortcut has no positional parameter to name", + envelope.Error.Param) + } + if len(envelope.Error.Params) != 0 { + t.Errorf("params = %v, want none", envelope.Error.Params) + } +} + +// TestErrorClassificationFollowsBoundaryNotText keeps text matching from +// creeping back into classification. The exact same message is a user error +// when an Args validator returns it and an internal fault when application +// execution returns it. Every error-returning Cobra callback is covered. +func TestErrorClassificationFollowsBoundaryNotText(t *testing.T) { + const sharedText = `required flag(s) "csv" not set` + causes := map[string]error{} + newCause := func(name string) error { + causes[name] = errors.New(sharedText) + return causes[name] + } + + cmd := &cobra.Command{ + Use: "probe", + Args: func(*cobra.Command, []string) error { + return newCause("Args") + }, + PersistentPreRunE: func(*cobra.Command, []string) error { + return newCause("PersistentPreRunE") + }, + PreRunE: func(*cobra.Command, []string) error { + return newCause("PreRunE") + }, + RunE: func(*cobra.Command, []string) error { + return newCause("RunE") + }, + PostRunE: func(*cobra.Command, []string) error { + return newCause("PostRunE") + }, + PersistentPostRunE: func(*cobra.Command, []string) error { + return newCause("PersistentPostRunE") + }, + } + instrumentErrorBoundaries(cmd) + + assertClassified := func(name string, got error, category errs.Category) { + t.Helper() + problem, ok := errs.ProblemOf(got) + if !ok { + t.Fatalf("%s returned unclassified %T (%v)", name, got, got) + } + wantSubtype := errs.SubtypeUnknown + if category == errs.CategoryValidation { + wantSubtype = errs.SubtypeInvalidArgument + } + if problem.Category != category || problem.Subtype != wantSubtype { + t.Errorf("%s classified as %s/%s, want %s/%s", + name, problem.Category, problem.Subtype, category, wantSubtype) + } + if !errors.Is(got, causes[name]) { + t.Errorf("%s lost its original cause", name) + } + } + + assertClassified("Args", cmd.Args(cmd, nil), errs.CategoryValidation) + for name, invoke := range map[string]func() error{ + "PersistentPreRunE": func() error { return cmd.PersistentPreRunE(cmd, nil) }, + "PreRunE": func() error { return cmd.PreRunE(cmd, nil) }, + "RunE": func() error { return cmd.RunE(cmd, nil) }, + "PostRunE": func() error { return cmd.PostRunE(cmd, nil) }, + "PersistentPostRunE": func() error { return cmd.PersistentPostRunE(cmd, nil) }, + } { + assertClassified(name, invoke(), errs.CategoryInternal) + } +} + +func TestErrorBoundaryPassesTypedErrorsAndExitSignalsUnchanged(t *testing.T) { + instrumentErrorBoundaries(nil) // must not panic + + typed := errs.NewNetworkError(errs.SubtypeNetworkTimeout, "timed out") + bare := fmt.Errorf("wrapped bare: %w", output.ErrBare(7)) + partial := fmt.Errorf("wrapped partial: %w", output.PartialFailure(8)) + for name, err := range map[string]error{ + "typed": typed, "bare": bare, "partial": partial, + } { + if got := typedCommandError(err); got != err { + t.Errorf("command %s identity changed: got %T %v, want %T %v", name, got, got, err, err) + } + if got := typedArgsError(err); got != err { + t.Errorf("Args %s identity changed: got %T %v, want %T %v", name, got, got, err, err) + } + } +} + +// TestRepeatedExecutionDoesNotLeakClassification proves classification is +// attached to the callback result, not retained as mutable process/tree state. +func TestRepeatedExecutionDoesNotLeakClassification(t *testing.T) { + bodyCause := errors.New("body failed") + root := &cobra.Command{Use: "root", SilenceErrors: true, SilenceUsage: true} + body := &cobra.Command{ + Use: "body", + RunE: func(*cobra.Command, []string) error { return bodyCause }, + } + validation := &cobra.Command{ + Use: "validation", + RunE: func(*cobra.Command, []string) error { return nil }, + } + validation.Flags().String("required", "", "required probe") + if err := validation.MarkFlagRequired("required"); err != nil { + t.Fatal(err) + } + root.AddCommand(body, validation) + instrumentErrorBoundaries(root) + + root.SetArgs([]string{"body"}) + first := root.Execute() + if p, ok := errs.ProblemOf(first); !ok || p.Category != errs.CategoryInternal || !errors.Is(first, bodyCause) { + t.Fatalf("first execution = %T %v, want internal preserving body cause", first, first) + } + + root.SetArgs([]string{"validation"}) + second := normalizeRootError(root.Execute()) + if p, ok := errs.ProblemOf(second); !ok || p.Category != errs.CategoryValidation { + t.Fatalf("second execution = %T %v, want residual Cobra validation independent of the first run", second, second) + } +} + +// TestFinalBuiltHelpCommandIsInstrumented pins build ordering. Concealment +// installs a custom help RunE after plugin hooks; a raw failure inside that +// late command is application execution, not a malformed command line. +func TestFinalBuiltHelpCommandIsInstrumented(t *testing.T) { + tmpHome(t) + quietNotices(t) + registerRestriction(t, []string{"skills/read"}, nil) + + _, root, _ := buildInternal( + context.Background(), + buildInvocationForTest(t), + ConcealRestrictedCommands(), + ) + want := errors.New("usage renderer failed") + root.SetOut(io.Discard) + root.SetUsageFunc(func(*cobra.Command) error { return want }) + root.SetArgs([]string{"help", "definitely-missing"}) + + got := root.Execute() + problem, ok := errs.ProblemOf(got) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeUnknown { + t.Fatalf("late help failure = %T %v, want internal/unknown", got, got) + } + if !errors.Is(got, want) { + t.Error("late help failure lost the usage renderer cause") + } +} + +func TestLazyCompletionArgsFailureIsValidation(t *testing.T) { + tmpHome(t) + _, root, _ := buildInternal( + context.Background(), buildInvocationForTest(t), WithoutPlugins(), WithoutServiceCommands(), + ) + root.SetArgs([]string{"__complete"}) + + got := normalizeRootError(root.Execute()) + problem, ok := errs.ProblemOf(got) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("lazy completion Args failure = %T %v, want validation/invalid_argument", got, got) + } +} + +func TestVersionWriterFailureIsInternal(t *testing.T) { + tmpHome(t) + want := &failingWriter{limit: 0} + _, root, _ := buildInternal( + context.Background(), + buildInvocationForTest(t), + WithIO(strings.NewReader(""), want, io.Discard), + WithoutPlugins(), + WithoutServiceCommands(), + ) + root.SetArgs([]string{"--version"}) + + got := normalizeRootError(root.Execute()) + problem, ok := errs.ProblemOf(got) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeUnknown { + t.Fatalf("version writer failure = %T %v, want internal/unknown", got, got) + } + if !errors.Is(got, io.ErrShortWrite) { + t.Error("version writer failure lost io.ErrShortWrite") + } +} + +func TestInternalErrorWriterTypesSilentShortWrite(t *testing.T) { + n, got := (internalErrorWriter{Writer: silentShortWriter{}}).Write([]byte("version")) + if n != len("version")-1 { + t.Fatalf("bytes written = %d, want %d", n, len("version")-1) + } + problem, ok := errs.ProblemOf(got) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeUnknown { + t.Fatalf("silent short write = %T %v, want internal/unknown", got, got) + } + if !errors.Is(got, io.ErrShortWrite) { + t.Error("silent short write did not preserve io.ErrShortWrite") + } +} + +// TestWrapperFailureIsOurFaultNotTheUsers pins that a plugin wrapper failing +// before it delegates is attributed to us. The wrapper chain is part of +// executing the command, so its failure is never a mistake in what the user +// typed — reporting it as invalid input would send the user looking at their +// own command line for a fault that is not there. +func TestWrapperFailureIsOurFaultNotTheUsers(t *testing.T) { + tmpHome(t) + quietNotices(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + platform.Register(platform.NewPlugin("backend", "1.0"). + Wrap("gate", platform.All(), func(next platform.Handler) platform.Handler { + return func(context.Context, platform.Invocation) error { + // Fails without delegating, and without using AbortError. + return errors.New("plugin backend unavailable") + } + }).FailOpen().MustBuild()) + + code, _, stderr := executeWithCapturedOS(t, nil, "profile", "list") + + var envelope struct { + Error struct { + Category errs.Category `json:"type"` + Subtype errs.Subtype `json:"subtype"` + } `json:"error"` + } + if err := json.Unmarshal([]byte(stderr), &envelope); err != nil { + t.Fatalf("stderr is not a JSON envelope: %v\n%s", err, stderr) + } + if envelope.Error.Category != errs.CategoryInternal || + envelope.Error.Subtype != errs.SubtypeUnknown { + t.Errorf("reported as %s/%s, want %s/%s", + envelope.Error.Category, envelope.Error.Subtype, + errs.CategoryInternal, errs.SubtypeUnknown) + } + if code != output.ExitInternal { + t.Errorf("exit code = %d, want %d", code, output.ExitInternal) + } +} + +// TestShutdownHookCannotRewriteBareExit pins that the user-visible exit is +// settled before Shutdown. Clone isolation for both exit signal types is +// covered directly in internal/hook. +func TestShutdownHookCannotRewriteBareExit(t *testing.T) { + cfgDir := tmpHome(t) + quietNotices(t) + writeAppConfigWithoutUser(t, cfgDir) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + rewrote := false + platform.Register(platform.NewPlugin("tamper", "1.0"). + On(platform.Shutdown, "rewrite", + func(_ context.Context, lc *platform.LifecycleContext) error { + var bare *output.BareError + if errors.As(lc.Err, &bare) { + bare.Code = 0 + rewrote = true + } + return nil + }). + MustBuild()) + + code, _, _ := executeWithCapturedOS(t, nil, + "auth", "check", "--scope", "im:message:send_as_bot") + + if !rewrote { + t.Fatal("the handler never saw an exit-code-only signal; the test no longer covers its own premise") + } + if code != 1 { + t.Errorf("exit code = %d, want 1; a Shutdown handler must not be able to change it", code) + } +} + +// TestShutdownHandlersDoNotSeeEachOthersEdits pins that handlers are isolated +// from one another. They run in registration order against the same failure, so +// sharing one error value would let whichever runs first decide what every +// later audit handler records. +func TestShutdownHandlersDoNotSeeEachOthersEdits(t *testing.T) { + tmpHome(t) + quietNotices(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + var secondSaw errs.Category + secondRan := false + platform.Register(platform.NewPlugin("tamper", "1.0"). + On(platform.Shutdown, "rewrite", + func(_ context.Context, lc *platform.LifecycleContext) error { + if p, ok := errs.ProblemOf(lc.Err); ok { + p.Category = errs.CategoryNetwork + p.Subtype = "rewritten_by_first_handler" + } + return nil + }). + MustBuild()) + platform.Register(platform.NewPlugin("audit", "1.0"). + On(platform.Shutdown, "observe", + func(_ context.Context, lc *platform.LifecycleContext) error { + secondRan = true + if p, ok := errs.ProblemOf(lc.Err); ok { + secondSaw = p.Category + } + return nil + }). + MustBuild()) + + executeWithCapturedOS(t, nil, "definitely-not-a-command") + + if !secondRan { + t.Fatal("the second handler never ran; the test no longer covers its own premise") + } + if secondSaw != errs.CategoryValidation { + t.Errorf("second handler observed %q, want %q — the first handler's edit leaked", + secondSaw, errs.CategoryValidation) + } +} + +// TestFailingShutdownHookLeavesTheEnvelopeLastOnStderr pins the byte order of +// the two independent writers that share stderr. The hook layer warns about a +// handler that fails or is skipped; the dispatcher writes the JSON envelope. +// A reader recovers the envelope by scanning back from the end of stderr for a +// JSON object, so a warning printed after it would hide it — the envelope is +// pretty-printed across several lines and its own braces are indented, leaving +// nothing for that scan to land on. +// +// The handler is also given a mutable failure to prove the ordering costs +// nothing: what the user is told is decided before the event fires, so the +// handler still cannot rewrite it. +func TestFailingShutdownHookLeavesTheEnvelopeLastOnStderr(t *testing.T) { + tmpHome(t) + quietNotices(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + // The hook layer captured os.Stderr at package init, so point its warnings + // at the same buffer the factory writes the envelope to. In production both + // are the one stderr file descriptor. + var stderr bytes.Buffer + t.Cleanup(hook.SetStderrForTesting(&stderr)) + + handlerRan := false + platform.Register(platform.NewPlugin("audit", "1.0"). + On(platform.Shutdown, "explode", + func(_ context.Context, lc *platform.LifecycleContext) error { + handlerRan = true + if p, ok := errs.ProblemOf(lc.Err); ok { + p.Subtype = "rewritten_by_shutdown_handler" + } + return errors.New("audit sink unreachable") + }). + MustBuild()) + + oldArgs := os.Args + t.Cleanup(func() { os.Args = oldArgs }) + os.Args = []string{"lark-cli", "definitely-not-a-command"} + + code := ExecuteWithOptions(WithIO(strings.NewReader(""), io.Discard, &stderr)) + + if !handlerRan { + t.Fatal("the Shutdown handler never ran; the test no longer covers its own premise") + } + out := stderr.String() + if !strings.Contains(out, "warning: shutdown hook") { + t.Fatalf("no hook warning on stderr; the test no longer covers its own premise:\n%s", out) + } + + payload := trailingJSONObject(out) + if payload == "" { + t.Fatalf("no JSON object at the end of stderr; the envelope is unreachable:\n%s", out) + } + var envelope struct { + Error struct { + Category errs.Category `json:"type"` + Subtype errs.Subtype `json:"subtype"` + } `json:"error"` + } + if err := json.Unmarshal([]byte(payload), &envelope); err != nil { + t.Fatalf("trailing content is not the error envelope: %v\n%s", err, payload) + } + if envelope.Error.Category != errs.CategoryValidation || + envelope.Error.Subtype != errs.SubtypeInvalidArgument { + t.Errorf("envelope reports %s/%s, want %s/%s — a Shutdown handler rewrote it", + envelope.Error.Category, envelope.Error.Subtype, + errs.CategoryValidation, errs.SubtypeInvalidArgument) + } + if code != output.ExitValidation { + t.Errorf("exit code = %d, want %d", code, output.ExitValidation) + } +} + +// trailingJSONObject mirrors how a reader recovers the envelope from a stderr +// stream that may also carry framework warnings: take the last line that starts +// a JSON object and everything after it. Leading noise is tolerated; trailing +// noise is not, which is the property the test above exists to protect. +func trailingJSONObject(raw string) string { + raw = strings.TrimSpace(raw) + if json.Valid([]byte(raw)) { + return raw + } + start := strings.LastIndex(raw, "\n{") + if start < 0 { + return "" + } + candidate := raw[start+1:] + if !json.Valid([]byte(candidate)) { + return "" + } + return candidate +} diff --git a/cmd/root.go b/cmd/root.go index 31e7020a11..f90c378b82 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -4,9 +4,11 @@ package cmd import ( + "bytes" "context" "errors" "fmt" + "io" "io/fs" "os" "sort" @@ -109,18 +111,36 @@ func executeWithOptions(opts []BuildOption) int { runErr := rootCmd.Execute() + // Application and plugin callbacks were typed at their execution + // boundaries. Any plain error left here came from cobra rejecting command + // discovery or the command line itself (required/group flags, or a lazy + // completion Args validator). Normalize it before Shutdown so handlers + // observe exactly the Category / Subtype / exit code the user receives. + runErr = normalizeRootError(runErr) + + // Decide the envelope and the exit code before notifying plugins. Every + // error value reachable here has exported fields, and cloning cannot cover + // the ones an extension defines, so deciding early is what makes the + // lifecycle error a snapshot: a handler receives the real value and may do + // as it likes with it, while what the user gets is already bytes. + var report rootErrorReport + if runErr != nil { + report = renderRootError(f, runErr, runtime.recovery) + } + // Fire Shutdown lifecycle hooks regardless of run outcome. - // emitShutdown imposes a 2s total deadline and never propagates handler - // errors (Emit's documented Shutdown contract), so it cannot block exit - // or alter the user-visible exit code. + // emitShutdown never propagates handler errors (Emit's documented Shutdown + // contract). Its 2s budget is checked between handlers, so a single handler + // that ignores ctx can still delay exit. Under the default streams a skipped + // or failing handler warns on the same stderr the envelope goes to, which is + // why the envelope is written after this and not before: readers scan back + // from the end of stderr for the JSON object. if reg != nil && !isCompletionCommand(os.Args) { _ = hook.Emit(ctx, reg, platform.Shutdown, runErr) } - if runErr != nil { - return handleRootError(f, runErr, runtime.recovery) - } - return 0 + report.flush(f.IOStreams.ErrOut) + return report.exitCode } // isDeferredBootstrapProfileError identifies the one bootstrap parse failure @@ -253,8 +273,38 @@ func configureFlagCompletions(args []string) { cmdutil.SetFlagCompletionsEnabled(isCompletionCommand(args)) } -// handleRootError dispatches a command error to the appropriate handler -// and returns the process exit code. +// rootErrorReport is what a failed invocation has decided to report: the exact +// stderr bytes the user will see, and the exit code that goes with them. +// +// Deciding both before the Shutdown event fires is what stops a lifecycle +// handler from changing either — the values are already bytes, so no handler +// can reach them and no error type has to be cloneable for the guarantee to +// hold. Writing those bytes after the event is what keeps the envelope the +// trailing content of stderr, which is where readers look for it. +// +// That ordering matters because the hook layer warns about a failing or +// skipped handler on the process stderr, and under the default streams that is +// the same file descriptor the envelope goes to. A caller that redirects +// stderr with WithIO splits the two: the envelope follows the caller's writer +// while hook warnings stay on the process stderr, so the two never interleave +// and the ordering is moot rather than wrong. +type rootErrorReport struct { + envelope []byte + exitCode int +} + +// flush writes the decided envelope. The two exit-code-only signals decide on +// no envelope at all and write nothing. +func (r rootErrorReport) flush(w io.Writer) { + if len(r.envelope) == 0 { + return + } + _, _ = w.Write(r.envelope) +} + +// renderRootError decides what a command error reports, without writing it. +// It accepts any error; every error that owns the stderr envelope renders one, +// while the two exit-code-only signals deliberately render nothing. // // Dispatch order: // 1. Typed errors from errs/ (e.g. *errs.PermissionError, *errs.APIError, @@ -265,17 +315,15 @@ func configureFlagCompletions(args []string) { // constructed typed at their origin (internal/auth, internal/core), so the // dispatcher no longer promotes any legacy shape here. // 2. PartialFailure / BareError signals: the result envelope is already on -// stdout; honor the exit code and write nothing to stderr. -// 3. Residual cobra usage errors (missing required flag, unknown command, -// argument validation): typed as an invalid_argument envelope (exit 2), -// matching the explicit flag/subcommand guards. Flag parse errors are -// already typed upstream by the root FlagErrorFunc. -func handleRootError( +// stdout; honor the exit code and render nothing for stderr. +// 3. Anything else is defensive: production normalizes residual cobra +// validation before calling this function. Rebuild it as internal so an +// unexpected untyped value still produces a structured stderr envelope. +func renderRootError( f *cmdutil.Factory, err error, projector *recovery.Projector, -) int { - errOut := f.IOStreams.ErrOut +) rootErrorReport { renderedErr := err // When the typed error is a need_user_authorization signal, fold in the @@ -288,23 +336,22 @@ func handleRootError( renderedErr = presentRootError(f, err, projector) } - // Staged dispatch: capture the typed exit code BEFORE attempting the - // envelope write. WriteTypedErrorEnvelope is best-effort on the wire - // (partial-write still returns true) so the exit code we read here is - // preserved even if stderr is torn — torn stderr must not downgrade - // typed exits 3/4/6/10 to the plain "Error:" path with exit 1. - // WriteTypedErrorEnvelope still returns false when err carries no - // Problem; in that case we fall through to the signal / plain-text paths. - typedExit := output.ExitCodeOf(err) - if output.WriteTypedErrorEnvelope(errOut, renderedErr, string(f.ResolvedIdentity)) { - return typedExit + // Typed dispatch: the exit code comes from the producer's error, not from + // whether rendering succeeded. Rendering into memory keeps the two + // independent — a stderr that tears on flush cannot downgrade typed exits + // 3/4/6/10 to the plain "Error:" path with exit 1. WriteTypedErrorEnvelope + // returns false when err carries no Problem; in that case we fall through + // to the signal / rebuild paths. + var envelope bytes.Buffer + if output.WriteTypedErrorEnvelope(&envelope, renderedErr, string(f.ResolvedIdentity)) { + return rootErrorReport{envelope: envelope.Bytes(), exitCode: output.ExitCodeOf(err)} } // Partial-failure (batch / multi-status): the ok:false result envelope is // already on stdout; set the exit code and write nothing to stderr. var pfErr *output.PartialFailureError if errors.As(err, &pfErr) { - return pfErr.Code + return rootErrorReport{exitCode: pfErr.Code} } // Silent-exit signal (e.g. `auth check` predicate, or `update --json`): @@ -312,58 +359,78 @@ func handleRootError( // write nothing to stderr. var bareErr *output.BareError if errors.As(err, &bareErr) { - return bareErr.Code - } - - // Errors reaching here are untyped: every RunE returns a typed errs.* error - // and flag-parse errors are typed by the root FlagErrorFunc. The remainder - // is either a cobra usage mistake (missing required flag, unknown command, - // wrong arg count), which cobra surfaces as a plain error identified by its - // stable text — the same external contract unknownFlagName relies on — or an - // untyped error that leaked past the typed boundary. Classify the former as - // invalid_argument (exit 2, like the explicit guards); treat the latter as an - // internal fault (exit 5) rather than blaming the user's input. The message - // is preserved either way, and the typed envelope still carries any pending - // deprecation notice. - var fallback error - if isCobraUsageError(err) { - fallback = errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err.Error()) - } else { - fallback = errs.NewInternalError(errs.SubtypeUnknown, "%s", err.Error()).WithCause(err) - } - output.WriteTypedErrorEnvelope(errOut, fallback, string(f.ResolvedIdentity)) - return output.ExitCodeOf(fallback) -} - -// cobraUsageErrorMarkers are the stable error-text fragments cobra / pflag -// (pinned at v1.10.2) emit for usage mistakes — missing required flag, unknown -// command / flag, wrong argument count. Cobra surfaces these as plain errors, -// not a typed value we can match on, so the dispatcher recognizes them by text; -// this is the same external contract unknownFlagName already depends on. A -// residual error matching none of these has leaked the typed boundary and is -// treated as an internal fault, not a user error. -var cobraUsageErrorMarkers = []string{ - "unknown command ", - "unknown flag: ", - "unknown shorthand", - "required flag(s) ", - "flag needs an argument", - "bad flag syntax:", - "no such flag ", - "invalid argument ", - "arg(s), ", // accepts / requires N arg(s), received / only received M -} - -// isCobraUsageError reports whether err is a cobra / pflag usage mistake, -// identified by the stable error text of the pinned cobra version. -func isCobraUsageError(err error) bool { - msg := err.Error() - for _, m := range cobraUsageErrorMarkers { - if strings.Contains(msg, m) { - return true - } + return rootErrorReport{exitCode: bareErr.Code} + } + + // Reaching here means the render above failed, so err cannot serialize + // itself — passing the same value on would fail identically and leave + // stderr blank. Build a fresh typed error carrying its message instead, + // which is what keeps stderr from going silent on a typed exit. Reset first + // because a failed render may have left a partial object behind. + envelope.Reset() + fallback := rebuildTypedError(err) + output.WriteTypedErrorEnvelope(&envelope, fallback, string(f.ResolvedIdentity)) + return rootErrorReport{envelope: envelope.Bytes(), exitCode: output.ExitCodeOf(fallback)} +} + +// handleRootError renders a command error and writes it immediately, returning +// the process exit code. executeWithOptions splits the two steps instead, so +// that the Shutdown event runs between them. +func handleRootError( + f *cmdutil.Factory, + err error, + projector *recovery.Projector, +) int { + report := renderRootError(f, err, projector) + report.flush(f.IOStreams.ErrOut) + return report.exitCode +} + +// normalizeRootError gives a residual cobra error a typed validation envelope. +// Application and plugin error-returning callbacks are wrapped separately by +// instrumentErrorBoundaries, so the only untyped errors expected here are +// cobra's own command discovery, required/group flag, and lazy completion Args +// failures. Classification follows the boundary that produced the error, +// never its text. The message and original error are preserved. +// +// Already-typed errors and the two exit-code-only signal types +// (*output.PartialFailureError, *output.BareError) pass through unchanged. +// +// executeWithOptions calls this immediately after rootCmd.Execute() and +// before emitting the Shutdown lifecycle event, so a plugin's Shutdown +// handler observes the same classification handleRootError writes to +// stderr. +func normalizeRootError(err error) error { + if err == nil { + return nil } - return false + if hasOwnedErrorSemantics(err) { + return err + } + return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err.Error()). + WithCause(err) +} + +// rebuildTypedError always constructs a new typed error for err, which is what +// the envelope-write fallback needs: the value it holds has just proven it +// cannot serialize itself, so passing it on would fail identically. +// +// An error that already carries a classification keeps it. Failing to render is +// not failing to classify, and re-deciding the category here would hand the user +// a different one than the rest of the system agreed on — including the +// lifecycle handlers, which see the error as it was produced. Copying its +// Problem yields a value that can serialize; fields an extension added +// alongside are dropped, which is unavoidable for a value that could not be +// written in the first place. +// +// An untyped value here is defensive only: production normalizes before +// dispatch. Treat it as internal instead of making a second user-input guess. +func rebuildTypedError(err error) error { + if problem, ok := errs.ProblemOf(err); ok { + clone := *problem + return &clone + } + return errs.WrapInternal(err) } // installUnknownSubcommandGuard replaces cobra's silent help fallback on diff --git a/cmd/root_integration_test.go b/cmd/root_integration_test.go index 1a96094a28..24ce8dc9a5 100644 --- a/cmd/root_integration_test.go +++ b/cmd/root_integration_test.go @@ -50,6 +50,10 @@ func buildIntegrationRootCmd(t *testing.T, f *cmdutil.Factory) *cobra.Command { rootCmd.AddCommand(api.NewCmdApi(f, nil)) service.RegisterServiceCommands(rootCmd, f) shortcuts.RegisterShortcuts(rootCmd, f) + // Production types the boundaries once, at the end of the build, on the + // finished tree; do it here rather than per execution so a tree reused + // across several runs behaves the way a built one does. + instrumentErrorBoundaries(rootCmd) return rootCmd } @@ -59,7 +63,7 @@ func executeRootIntegration(t *testing.T, f *cmdutil.Factory, rootCmd *cobra.Com t.Helper() rootCmd.SetArgs(args) if err := rootCmd.Execute(); err != nil { - return handleRootError(f, err, nil) + return handleRootError(f, normalizeRootError(err), nil) } return 0 } @@ -129,6 +133,7 @@ func buildStrictModeIntegrationRootCmdWithCatalog(t *testing.T, f *cmdutil.Facto if mode := f.ResolveStrictMode(context.Background()); mode.IsActive() { pruneForStrictMode(rootCmd, mode) } + instrumentErrorBoundaries(rootCmd) return rootCmd } diff --git a/cmd/root_test.go b/cmd/root_test.go index 21836ea66d..d450f208ac 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -289,7 +289,7 @@ func TestHandleRootError_DeprecatedAliasMissingFlagStructured(t *testing.T) { }) // The bare error shape cobra's ValidateRequiredFlags produces: not a typed // errs.* error, so it reaches the deprecation fallback. - exit := handleRootError(f, fmt.Errorf(`required flag(s) %q not set`, "values"), nil) + exit := handleRootError(f, normalizeRootError(fmt.Errorf(`required flag(s) %q not set`, "values")), nil) out := errOut.String() if strings.HasPrefix(strings.TrimSpace(out), "Error:") { @@ -396,7 +396,7 @@ func TestHandleRootError_NoDeprecationTypesUsageError(t *testing.T) { errOut := &bytes.Buffer{} f.IOStreams.ErrOut = errOut - exit := handleRootError(f, fmt.Errorf(`required flag(s) %q not set`, "values"), nil) + exit := handleRootError(f, normalizeRootError(fmt.Errorf(`required flag(s) %q not set`, "values")), nil) out := errOut.String() if strings.HasPrefix(strings.TrimSpace(out), "Error:") { @@ -427,7 +427,7 @@ func TestHandleRootError_LeakedUntypedErrorBecomesInternal(t *testing.T) { errOut := &bytes.Buffer{} f.IOStreams.ErrOut = errOut - exit := handleRootError(f, fmt.Errorf("upstream helper exploded: %w", io.ErrUnexpectedEOF), nil) + exit := handleRootError(f, typedCommandError(fmt.Errorf("upstream helper exploded: %w", io.ErrUnexpectedEOF)), nil) errObj := decodeErrorEnvelope(t, errOut.Bytes()) if got := errObj["type"]; got != "internal" { @@ -712,3 +712,55 @@ func TestApplyNeedAuthorizationHint_AppendsExistingHint(t *testing.T) { t.Errorf("presenter mutated producer hint: %q", authErr.Hint) } } + +// TestNormalizeRootError pins the classification executeWithOptions applies +// (via normalizeRootError) immediately after rootCmd.Execute(), before the +// Shutdown lifecycle hook fires: a residual cobra usage error becomes a +// typed validation error, while already-typed errors and the two exit-code-only +// signals pass through unchanged. Application errors are typed earlier at +// their callback boundary. Without this, a plugin's +// Shutdown handler would observe an untyped cobra error that disagrees with +// the Category/Subtype the stderr envelope ultimately carries. +func TestNormalizeRootError(t *testing.T) { + t.Run("nil", func(t *testing.T) { + if got := normalizeRootError(nil); got != nil { + t.Errorf("normalizeRootError(nil) = %v, want nil", got) + } + }) + + t.Run("already typed passes through unchanged", func(t *testing.T) { + typed := errs.NewPermissionError(errs.SubtypePermissionDenied, "denied") + if got := normalizeRootError(typed); got != error(typed) { + t.Errorf("normalizeRootError(typed) = %v, want the same typed error unchanged", got) + } + }) + + t.Run("PartialFailureError passes through unchanged", func(t *testing.T) { + pfErr := &output.PartialFailureError{Code: 1} + if got := normalizeRootError(pfErr); got != error(pfErr) { + t.Errorf("normalizeRootError(PartialFailureError) = %v, want the same signal unchanged", got) + } + }) + + t.Run("BareError passes through unchanged", func(t *testing.T) { + bareErr := output.ErrBare(output.ExitAuth) + if got := normalizeRootError(bareErr); got != error(bareErr) { + t.Errorf("normalizeRootError(BareError) = %v, want the same signal unchanged", got) + } + }) + + t.Run("cobra usage error becomes typed validation", func(t *testing.T) { + original := fmt.Errorf(`required flag(s) %q not set`, "csv") + got := normalizeRootError(original) + var ve *errs.ValidationError + if !errors.As(got, &ve) { + t.Fatalf("normalizeRootError(cobra usage error) = %v, want *errs.ValidationError", got) + } + if ve.Subtype != errs.SubtypeInvalidArgument { + t.Errorf("Subtype = %q, want %q", ve.Subtype, errs.SubtypeInvalidArgument) + } + if !errors.Is(got, original) { + t.Error("normalizeRootError(cobra usage error) lost the original cause") + } + }) +} diff --git a/extension/platform/README.md b/extension/platform/README.md index 68856fcc7e..5d77abf6d0 100644 --- a/extension/platform/README.md +++ b/extension/platform/README.md @@ -155,6 +155,45 @@ sequenceDiagram A rule or strict-mode denial bypasses the `Wrap` chain entirely — observers still fire so audit plugins see the rejected dispatch. +`On(Shutdown)` receives the invocation's failure in `LifecycleContext.Err` — +from the command itself, or from the framework rejecting the command line +before any command ran. It reports the same category and subtype the CLI +wrote to stderr, so a plugin that records failures classifies them the way +the user was told: + +```go +import "github.com/larksuite/cli/errs" + +r.On(platform.Shutdown, "audit", func(_ context.Context, lc *platform.LifecycleContext) error { + if lc.Err == nil { + return nil // the command succeeded + } + problem, ok := errs.ProblemOf(lc.Err) + if !ok { + // An exit-code-only signal: the result is already on stdout and no + // error envelope was written. There is nothing to classify. + return nil + } + record(problem.Category, problem.Subtype) + return nil +}) +``` + +Always check that boolean. `Err` keeps the wrapping the command returned, so +`errors.Is` and `errors.As` still reach through it. Treat it as read-only: +writing to it never changes the envelope or the exit code the user gets, and +for a wrapped chain the value is shared with the rest of the handlers. + +A handler that fails, or that the 2s budget skips, is reported as a warning on +the **process** stderr. That is deliberate but worth knowing when embedding: +`cmd.WithIO` redirects the CLI's own output, not this warning, so a host that +captures stderr through `WithIO` will not see it. + +Not every failure reaches this event: bootstrap rejections, a plugin whose own +installation or `Startup` handler failed, shell-completion invocations, and a +failure to render help or usage text all end the process without emitting +`Shutdown`. Treat it as best-effort, not an exhaustive audit trail. + ## Safety contract (read this) - A plugin calling `Restrict()` MUST declare `FailClosed`. The Builder diff --git a/extension/platform/lifecycle.go b/extension/platform/lifecycle.go index 63a05487b0..980b9ec3e1 100644 --- a/extension/platform/lifecycle.go +++ b/extension/platform/lifecycle.go @@ -37,9 +37,31 @@ const ( Shutdown ) -// LifecycleContext is passed to LifecycleHandler. Err is the error from -// the preceding command (when Event == Shutdown after a failed RunE); -// otherwise nil. +// LifecycleContext is passed to LifecycleHandler. When Event == Shutdown, Err +// is the failure the invocation ended with — from the command itself, or from +// the framework rejecting the command line before any command ran; otherwise +// nil. +// +// Err is the error the command returned, with its wrapping intact, so +// errors.Is and errors.As reach whatever the producer put in the chain. It +// carries the same Category and Subtype the CLI wrote to its stderr envelope. +// +// What the user receives is settled before this event fires, so writing to Err +// cannot change it. Where the SDK can copy the error it hands each handler its +// own value, so one handler cannot change what the next one observes; where it +// cannot — a wrapped chain, or a type defined outside the SDK — the value is +// shared. Treat Err as read-only and that distinction stops mattering. +// +// Read it with errs.ProblemOf and check the boolean — two exit-code-only +// signals carry no Problem and write no envelope, because their result is +// already on stdout: a partial failure, and a bare predicate exit. +// +// Some failures end the process before this event can be emitted, so a handler +// must not be relied on as an exhaustive audit trail. Bootstrap rejections, a +// plugin whose own installation or Startup handler failed, and shell-completion +// invocations all exit without a Shutdown event. So does a failure to render +// help or usage text from cobra's own help command, which ends the process on +// the spot rather than returning. type LifecycleContext struct { Event LifecycleEvent Err error diff --git a/internal/cmdutil/factory.go b/internal/cmdutil/factory.go index d1966209ff..86a96c8c64 100644 --- a/internal/cmdutil/factory.go +++ b/internal/cmdutil/factory.go @@ -283,7 +283,17 @@ func (f *Factory) RequireBuiltinCredentialProvider(ctx context.Context, command } provName, err := f.Credential.ActiveExtensionProviderName(ctx) if err != nil { - return err + // A provider that already classified its failure keeps that + // classification: rewrapping would discard its category, its retry + // hint and the exit code that goes with them. + if _, ok := errs.ProblemOf(err); ok { + return err + } + // This runs in PersistentPreRunE, ahead of the command body, so an + // unclassified error escaping here would be read as a mistake in what + // the user typed. A provider lookup failure is not that. + return errs.NewInternalError(errs.SubtypeUnknown, + "cannot determine the active credential provider: %v", err).WithCause(err) } if provName == "" { return nil diff --git a/internal/cmdutil/factory_test.go b/internal/cmdutil/factory_test.go index dfb5dae120..3102a1a9ac 100644 --- a/internal/cmdutil/factory_test.go +++ b/internal/cmdutil/factory_test.go @@ -514,6 +514,33 @@ func TestRequireBuiltinCredentialProvider_NilCredential(t *testing.T) { } } +// A provider that already classified its own failure must keep that +// classification. Rewrapping it would replace, for example, a retryable +// network timeout with an internal fault and change the exit code with it. +func TestRequireBuiltinCredentialProvider_KeepsProviderClassification(t *testing.T) { + typed := errs.NewNetworkError(errs.SubtypeNetworkTimeout, "provider lookup timed out") + stub := &stubExtProvider{name: "env", err: typed} + cred := credential.NewCredentialProvider([]extcred.Provider{stub}, nil, nil, nil) + + f, _, _, _ := TestFactory(t, nil) + f.Credential = cred + + err := f.RequireBuiltinCredentialProvider(context.Background(), "auth") + + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error %T carries no Problem", err) + } + if problem.Category != errs.CategoryNetwork || problem.Subtype != errs.SubtypeNetworkTimeout { + t.Errorf("classified as %s/%s, want %s/%s (the provider's own classification)", + problem.Category, problem.Subtype, errs.CategoryNetwork, errs.SubtypeNetworkTimeout) + } + var netErr *errs.NetworkError + if !errors.As(err, &netErr) { + t.Errorf("error = %T, want the provider's *errs.NetworkError preserved", err) + } +} + func TestRequireBuiltinCredentialProvider_PropagatesProviderError(t *testing.T) { sentinel := errors.New("provider unavailable") stub := &stubExtProvider{name: "env", err: sentinel} @@ -527,3 +554,38 @@ func TestRequireBuiltinCredentialProvider_PropagatesProviderError(t *testing.T) t.Fatalf("error = %v, want sentinel", err) } } + +// A provider failure that carries no classification of its own is a fault in +// the tool, not in what the user typed. This runs in PersistentPreRunE, ahead +// of the command body, where an unclassified error is read as a bad command +// line — so the classification has to be applied here, and the exit code the +// user sees has to say internal fault rather than invalid input. +func TestRequireBuiltinCredentialProvider_UnclassifiedProviderErrorBecomesInternal(t *testing.T) { + sentinel := errors.New("provider unavailable") + stub := &stubExtProvider{name: "env", err: sentinel} + cred := credential.NewCredentialProvider([]extcred.Provider{stub}, nil, nil, nil) + + f, _, _, _ := TestFactory(t, nil) + f.Credential = cred + + err := f.RequireBuiltinCredentialProvider(context.Background(), "auth") + + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error %T carries no Problem; an unclassified failure would be blamed on the user", err) + } + if problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeUnknown { + t.Errorf("classified as %s/%s, want %s/%s", + problem.Category, problem.Subtype, errs.CategoryInternal, errs.SubtypeUnknown) + } + var internalErr *errs.InternalError + if !errors.As(err, &internalErr) { + t.Errorf("error = %T, want *errs.InternalError", err) + } + if got := output.ExitCodeOf(err); got != output.ExitInternal { + t.Errorf("exit code = %d, want %d", got, output.ExitInternal) + } + if !errors.Is(err, sentinel) { + t.Error("classification dropped the provider's own error") + } +} diff --git a/internal/hook/emit.go b/internal/hook/emit.go index c7cf6ed265..7e38cb6ea0 100644 --- a/internal/hook/emit.go +++ b/internal/hook/emit.go @@ -8,7 +8,10 @@ import ( "fmt" "time" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/extension/platform" + "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/recovery" ) // shutdownDeadline is the hard upper bound on how long Shutdown @@ -68,24 +71,84 @@ func Emit(ctx context.Context, reg *Registry, event platform.LifecycleEvent, las if len(handlers) == 0 { return nil } - lc := &platform.LifecycleContext{Event: event, Err: lastErr} - if event == platform.Shutdown { - return emitShutdown(ctx, handlers, lc) + return emitShutdown(ctx, handlers, event, lastErr) } for _, h := range handlers { - if err := callLifecycleSafe(ctx, h, lc); err != nil { + if err := callLifecycleSafe(ctx, h, newLifecycleContext(event, lastErr)); err != nil { return err } } return nil } +// newLifecycleContext builds one handler's own context. Typed error fields are +// exported, so handing every handler the same value would let the first one +// observing a failure change what the rest of them see. Each handler therefore +// gets its own context, and its own copy of the error wherever the value can be +// copied. +func newLifecycleContext(event platform.LifecycleEvent, lastErr error) *platform.LifecycleContext { + return &platform.LifecycleContext{Event: event, Err: copyLifecycleErr(lastErr)} +} + +// copyLifecycleErr returns an independent value when err is itself one of the +// error shapes this module owns. +// +// Everything else is shared as-is, and a chain that merely wraps an owned shape +// counts as everything else: the value inside is not the error the command +// returned, so substituting it would drop the wrapper's message and any +// sentinel errors.Is could reach through it. A type defined outside these +// shapes cannot be copied at all without reflecting over fields we do not know. +// LifecycleContext documents both limits and asks handlers to treat Err as +// read-only. +func copyLifecycleErr(err error) error { + if err == nil { + return nil + } + if !ownsErrorValue(err) { + return err + } + if clone, ok := recovery.CloneTyped(err); ok { + return clone + } + // A typed nil pointer held in a non-nil interface has nothing to copy; + // recovery.CloneTyped reports the same for the typed errors it owns. + switch signal := err.(type) { //nolint:errorlint // deliberate: see ownsErrorValue + case *output.BareError: + if signal == nil { + return err + } + clone := *signal + return &clone + case *output.PartialFailureError: + if signal == nil { + return err + } + clone := *signal + return &clone + } + return err +} + +// ownsErrorValue reports whether err is itself one of the owned shapes rather +// than a chain wrapping one. The copy helpers locate their target with +// errors.As and recovery.CloneTyped, both of which search the whole chain, so +// they are only safe to apply once err has been shown to be that target. +// +//nolint:errorlint // asserting on err is the point: what err is, not what it wraps. +func ownsErrorValue(err error) bool { + switch err.(type) { + case *output.BareError, *output.PartialFailureError, errs.TypedError: + return true + } + return false +} + // emitShutdown enforces the 2-second total deadline. Handlers receive // a derived context with the remaining budget; once the budget is // exhausted, the remaining handlers are skipped (with a stderr // warning) and Emit returns. -func emitShutdown(parent context.Context, handlers []LifecycleEntry, lc *platform.LifecycleContext) error { +func emitShutdown(parent context.Context, handlers []LifecycleEntry, event platform.LifecycleEvent, lastErr error) error { ctx, cancel := context.WithTimeout(parent, shutdownDeadline) defer cancel() deadline := time.Now().Add(shutdownDeadline) @@ -95,7 +158,7 @@ func emitShutdown(parent context.Context, handlers []LifecycleEntry, lc *platfor fmt.Fprintf(stderr(), "warning: shutdown deadline exceeded; skipping hook %q\n", h.Name) continue } - if err := callLifecycleSafe(ctx, h, lc); err != nil { + if err := callLifecycleSafe(ctx, h, newLifecycleContext(event, lastErr)); err != nil { // Shutdown errors are logged, not propagated -- exit is // non-recoverable anyway. fmt.Fprintf(stderr(), "warning: shutdown hook %q: %v\n", h.Name, err) diff --git a/internal/hook/emit_test.go b/internal/hook/emit_test.go index df6b0af618..c6369a03e4 100644 --- a/internal/hook/emit_test.go +++ b/internal/hook/emit_test.go @@ -6,11 +6,22 @@ package hook import ( "context" "errors" + "fmt" "testing" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/extension/platform" + "github.com/larksuite/cli/internal/output" ) +// extensionTypedError models a plugin-defined typed error. The SDK cannot +// safely clone fields it does not own, so LifecycleContext documents that this +// shape is shared and must be treated as read-only. +type extensionTypedError struct { + *errs.Problem + PrivateState []string +} + // A Startup handler returning a regular error must surface as a typed // *LifecycleError with Panic=false so the cmd-layer guard can pick // reason_code=lifecycle_failed. @@ -108,3 +119,113 @@ func TestEmit_ShutdownErrorsSwallowed(t *testing.T) { t.Errorf("Shutdown errors must NOT propagate, got: %v", err) } } + +func TestCopyLifecycleErr(t *testing.T) { + t.Run("nil", func(t *testing.T) { + if got := copyLifecycleErr(nil); got != nil { + t.Fatalf("copyLifecycleErr(nil) = %v, want nil", got) + } + }) + + t.Run("owned typed error", func(t *testing.T) { + cause := errors.New("invalid input") + original := errs.NewValidationError(errs.SubtypeInvalidArgument, "bad value"). + WithParam("--value"). + WithCause(cause) + got, ok := copyLifecycleErr(original).(*errs.ValidationError) + if !ok { + t.Fatalf("copy = %T, want *errs.ValidationError", copyLifecycleErr(original)) + } + if got == original { + t.Fatal("typed error was shared instead of cloned") + } + if got.Param != original.Param || !errors.Is(got, cause) { + t.Errorf("clone lost fields or cause: %+v", got) + } + got.Message = "changed" + if original.Message == got.Message { + t.Fatal("mutating the clone changed the producer's error") + } + }) + + t.Run("bare signal", func(t *testing.T) { + original := output.ErrBare(7) + got, ok := copyLifecycleErr(original).(*output.BareError) + if !ok || got == original || got.Code != original.Code { + t.Fatalf("copy = %#v, want distinct BareError with code %d", got, original.Code) + } + got.Code = 0 + if original.Code != 7 { + t.Fatal("mutating the BareError clone changed the producer's signal") + } + }) + + t.Run("partial failure signal", func(t *testing.T) { + original := output.PartialFailure(8) + got, ok := copyLifecycleErr(original).(*output.PartialFailureError) + if !ok || got == original || got.Code != original.Code { + t.Fatalf("copy = %#v, want distinct PartialFailureError with code %d", got, original.Code) + } + got.Code = 0 + if original.Code != 8 { + t.Fatal("mutating the PartialFailureError clone changed the producer's signal") + } + }) + + t.Run("extension typed error is read-only pass-through", func(t *testing.T) { + original := &extensionTypedError{ + Problem: &errs.Problem{ + Category: errs.CategoryNetwork, + Subtype: errs.SubtypeNetworkTimeout, + Message: "extension timeout", + }, + PrivateState: []string{"opaque"}, + } + if got := copyLifecycleErr(original); got != error(original) { + t.Fatalf("copy = %T %v, want exact extension error pass-through", got, got) + } + }) + + // A wrapper is part of what the command returned. Copying the typed error + // found inside it would hand the handler a shorter message and break the + // errors.Is the wrapper exists to support, so a wrapped chain is passed + // through whole. + t.Run("wrapped typed error keeps its wrapper", func(t *testing.T) { + sentinel := errors.New("plugin backend unavailable") + typed := errs.NewValidationError(errs.SubtypeInvalidArgument, "bad value") + original := fmt.Errorf("plugin %q: %w: %w", "backend", sentinel, typed) + + got := copyLifecycleErr(original) + if got != original { + t.Fatalf("copy = %T %q, want the wrapped chain shared as-is", got, got) + } + if !errors.Is(got, sentinel) { + t.Error("copy no longer reaches the wrapper's sentinel") + } + if problem, ok := errs.ProblemOf(got); !ok || + problem.Subtype != errs.SubtypeInvalidArgument { + t.Errorf("copy = %v, want the wrapped classification still readable", got) + } + }) + + t.Run("wrapped exit signal keeps its wrapper", func(t *testing.T) { + bare := output.ErrBare(7) + original := fmt.Errorf("plugin short-circuit: %w", bare) + + got := copyLifecycleErr(original) + if got != original { + t.Fatalf("copy = %T %q, want the wrapped chain shared as-is", got, got) + } + if output.ExitCodeOf(got) != 7 { + t.Errorf("exit code = %d, want 7 still readable through the wrapper", + output.ExitCodeOf(got)) + } + }) + + t.Run("typed nil pointer", func(t *testing.T) { + var bare *output.BareError + if got := copyLifecycleErr(error(bare)); got != error(bare) { + t.Fatalf("copy = %#v, want the nil-pointer value passed through untouched", got) + } + }) +} diff --git a/internal/recovery/clone_exhaustive_test.go b/internal/recovery/clone_exhaustive_test.go new file mode 100644 index 0000000000..03998dbdb9 --- /dev/null +++ b/internal/recovery/clone_exhaustive_test.go @@ -0,0 +1,156 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package recovery + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "slices" + "strings" + "testing" +) + +// TestCloneTypedHandlesEveryTypedError fails when errs declares a typed error +// CloneTyped does not clone. +// +// CloneTyped switches over concrete types behind errs.TypedError, an exported +// interface with no closed set of implementations, so the compiler cannot tell +// anyone that a new one was missed. The cost of missing one is silent: the +// value falls through to the default branch, callers are handed the producer's +// own error instead of a copy, and the isolation that keeps one Shutdown +// handler's edit from reaching the next is gone with nothing failing. +// +// Comparing the two sets from source is what turns that into a red build. +func TestCloneTypedHandlesEveryTypedError(t *testing.T) { + declared := typedErrorsDeclaredInErrs(t) + cloned := typedErrorsHandledByCloneTyped(t) + + if len(declared) == 0 { + t.Fatal("found no typed errors in errs; the scan no longer covers its own premise") + } + + for _, name := range declared { + if !slices.Contains(cloned, name) { + t.Errorf("errs.%s is a typed error but CloneTyped does not clone it: "+ + "add a case for it, or every caller silently shares the producer's value", name) + } + } + for _, name := range cloned { + if !slices.Contains(declared, name) { + t.Errorf("CloneTyped has a case for errs.%s, which errs no longer declares", name) + } + } +} + +// typedErrorsDeclaredInErrs returns every type in errs that carries a Problem, +// which is what makes a value satisfy errs.TypedError: Problem itself supplies +// ProblemDetail, and a struct embedding it promotes that method. +func typedErrorsDeclaredInErrs(t *testing.T) []string { + t.Helper() + pkg := parsePackage(t, filepath.Join("..", "..", "errs")) + + var names []string + for _, file := range pkg { + ast.Inspect(file, func(n ast.Node) bool { + spec, ok := n.(*ast.TypeSpec) + if !ok { + return true + } + structType, ok := spec.Type.(*ast.StructType) + if !ok { + return true + } + if spec.Name.Name == "Problem" || embedsProblem(structType) { + names = append(names, spec.Name.Name) + } + return true + }) + } + slices.Sort(names) + return slices.Compact(names) +} + +// embedsProblem reports whether the struct embeds Problem by value, the shape +// every typed error in errs uses to inherit ProblemDetail. +func embedsProblem(structType *ast.StructType) bool { + for _, field := range structType.Fields.List { + if len(field.Names) > 0 { + continue // named field, not an embed + } + if ident, ok := field.Type.(*ast.Ident); ok && ident.Name == "Problem" { + return true + } + } + return false +} + +// typedErrorsHandledByCloneTyped returns the errs type names CloneTyped's +// switch names in a case clause. +func typedErrorsHandledByCloneTyped(t *testing.T) []string { + t.Helper() + pkg := parsePackage(t, ".") + + var names []string + for _, file := range pkg { + ast.Inspect(file, func(n ast.Node) bool { + decl, ok := n.(*ast.FuncDecl) + if !ok || decl.Name.Name != "CloneTyped" { + return true + } + ast.Inspect(decl.Body, func(inner ast.Node) bool { + clause, ok := inner.(*ast.CaseClause) + if !ok { + return true + } + for _, expr := range clause.List { + if name, ok := errsTypeName(expr); ok { + names = append(names, name) + } + } + return true + }) + return false + }) + } + slices.Sort(names) + return slices.Compact(names) +} + +// errsTypeName extracts X from a `case *errs.X` clause expression. +func errsTypeName(expr ast.Expr) (string, bool) { + star, ok := expr.(*ast.StarExpr) + if !ok { + return "", false + } + sel, ok := star.X.(*ast.SelectorExpr) + if !ok { + return "", false + } + pkgIdent, ok := sel.X.(*ast.Ident) + if !ok || pkgIdent.Name != "errs" { + return "", false + } + return sel.Sel.Name, true +} + +func parsePackage(t *testing.T, dir string) []*ast.File { + t.Helper() + fset := token.NewFileSet() + pkgs, err := parser.ParseDir(fset, dir, func(fi fs.FileInfo) bool { + return !strings.HasSuffix(fi.Name(), "_test.go") + }, 0) + if err != nil { + t.Fatalf("parse %s: %v", dir, err) + } + var files []*ast.File + for _, pkg := range pkgs { + for _, file := range pkg.Files { + files = append(files, file) + } + } + return files +} diff --git a/shortcuts/common/runner.go b/shortcuts/common/runner.go index e0b1e1681a..651536867a 100644 --- a/shortcuts/common/runner.go +++ b/shortcuts/common/runner.go @@ -1278,15 +1278,20 @@ func handleShortcutDryRun(f *cmdutil.Factory, rctx *RuntimeContext, s *Shortcut) } // rejectPositionalArgs returns a cobra.PositionalArgs that rejects any -// positional arguments. It returns a plain cobra usage error; the root -// handler classifies it into the typed validation envelope (exit 2), the -// same path as other cobra usage failures. +// positional arguments. Shortcuts take every value through a flag, so a bare +// word on the command line is always a mistake; the error is typed here at the +// point of rejection, lists the stray words and points at the command's help. +// +// No Param is set: `param` names the parameter the caller must correct, and a +// shortcut declares no positional parameter for a stray word to belong to. func rejectPositionalArgs() cobra.PositionalArgs { return func(cmd *cobra.Command, args []string) error { if len(args) == 0 { return nil } - return fmt.Errorf("positional arguments are not supported (got %q); pass values via flags", args) + return errs.NewValidationError(errs.SubtypeInvalidArgument, + "positional arguments are not supported (got %q); pass values via flags", args). + WithHint(fmt.Sprintf("run `%s --help` to see the flags this command accepts", cmd.CommandPath())) } } diff --git a/shortcuts/common/runner_args_test.go b/shortcuts/common/runner_args_test.go index 3ddb0cebb2..9446dd76b3 100644 --- a/shortcuts/common/runner_args_test.go +++ b/shortcuts/common/runner_args_test.go @@ -5,10 +5,12 @@ package common import ( "context" + "errors" "reflect" "strings" "testing" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/cmdutil" "github.com/spf13/cobra" ) @@ -22,7 +24,7 @@ func TestRejectPositionalArgs_WithArgs(t *testing.T) { if err == nil { t.Fatal("expected error for positional arg, got nil") } - // rejectPositionalArgs returns a raw fmt.Errorf via cobra's PositionalArgs contract — not a typed envelope, message-substring assertion is intentional. + // The message keeps every stray word so the user can see all of them at once. if !strings.Contains(err.Error(), "positional arguments are not supported") { t.Errorf("expected positional args rejection message, got: %v", err) } @@ -40,13 +42,37 @@ func TestRejectPositionalArgs_MultipleArgs(t *testing.T) { if err == nil { t.Fatal("expected error for multiple positional args, got nil") } - // rejectPositionalArgs returns a raw fmt.Errorf via cobra's PositionalArgs contract — not a typed envelope, message-substring assertion is intentional. + // All stray words belong in the message: naming only the first would hide + // the rest from a user who passed several. if !strings.Contains(err.Error(), "positional arguments are not supported") { t.Errorf("unexpected error message: %v", err) } if !strings.Contains(err.Error(), "hello") || !strings.Contains(err.Error(), "world") { t.Errorf("expected all positional args in error, got: %v", err) } + var verr *errs.ValidationError + if !errors.As(err, &verr) { + t.Fatalf("error = %T, want *errs.ValidationError", err) + } + // param identifies the parameter the caller must correct, and agents read it + // literally. A shortcut declares no positional parameter, so echoing what the + // user typed there would name a parameter that does not exist. + if verr.Param != "" { + t.Errorf("param = %q, want it unset: a shortcut has no positional parameter to name", verr.Param) + } + if len(verr.Params) != 0 { + t.Errorf("params = %v, want none", verr.Params) + } + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error %T carries no Problem", err) + } + if problem.Category != errs.CategoryValidation || + problem.Subtype != errs.SubtypeInvalidArgument { + t.Errorf("classified as %s/%s, want %s/%s", + problem.Category, problem.Subtype, + errs.CategoryValidation, errs.SubtypeInvalidArgument) + } } func TestRejectPositionalArgs_NoArgs(t *testing.T) {