From ef5da4b5ba3c658fc2914a4926ccb1b349f11521 Mon Sep 17 00:00:00 2001 From: "zhaoyukun.yk" Date: Tue, 11 Aug 2026 21:01:55 +0800 Subject: [PATCH] fix: classify invalid CLI input as validation errors Cobra surfaced several command-line validation failures as plain errors. Classifying them by message text could report correctable input as internal/unknown, misleading agents and returning the wrong exit code. Classify errors at the boundary that produces them. Args and residual Cobra validation become validation/invalid_argument, while raw execution hooks and plugin failures become internal/unknown. Preserve typed errors, causes, bare exits, and partial failures. Make final-tree instrumentation stateless, type pre-callback framework failures at their source, and keep rendering and Shutdown lifecycle observations consistent. A Shutdown handler receives the error the command returned with its wrapping intact, so errors.Is still reaches the producer's sentinels, and it cannot change what the user was told because that is decided before the event fires. The envelope is still written after the event, keeping it the trailing content of stderr where readers look for it even when a failing hook warns on the same stream. Guard the error copier against a typed error being added without it, so handlers cannot silently start sharing a producer's value again. Add regression coverage for repeated execution, late help, lazy completion, writer failures, shortcut diagnostics, credential-provider classification, lifecycle isolation, and stderr write order. --- cmd/build.go | 24 +- cmd/error_boundary.go | 151 ++++ cmd/error_boundary_test.go | 819 +++++++++++++++++++++ cmd/root.go | 225 ++++-- cmd/root_integration_test.go | 7 +- cmd/root_test.go | 58 +- extension/platform/README.md | 39 + extension/platform/lifecycle.go | 28 +- internal/cmdutil/factory.go | 12 +- internal/cmdutil/factory_test.go | 62 ++ internal/hook/emit.go | 75 +- internal/hook/emit_test.go | 121 +++ internal/recovery/clone_exhaustive_test.go | 156 ++++ shortcuts/common/runner.go | 13 +- shortcuts/common/runner_args_test.go | 30 +- 15 files changed, 1718 insertions(+), 102 deletions(-) create mode 100644 cmd/error_boundary.go create mode 100644 cmd/error_boundary_test.go create mode 100644 internal/recovery/clone_exhaustive_test.go 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) {