diff --git a/cli/cmd/config.go b/cli/cmd/config.go index 08e4d477..ee5490d8 100644 --- a/cli/cmd/config.go +++ b/cli/cmd/config.go @@ -148,7 +148,10 @@ var ctxUseCmd = &cobra.Command{ Args: cobra.ExactArgs(1), Example: ` kates ctx use local kates ctx use prod`, - Run: func(cmd *cobra.Command, args []string) { + // RunE, as ctx set has: with Run a missing context exited 0, so in + // "kates ctx use lab && kates test create ..." the test went to whichever + // context was current. + RunE: func(cmd *cobra.Command, args []string) error { name := args[0] var ctx Context err := updateConfig(func(cfg *Config) error { @@ -162,13 +165,12 @@ var ctxUseCmd = &cobra.Command{ }) switch { case errors.Is(err, errContextNotFound): - output.Error(fmt.Sprintf("Context '%s' not found. Run: kates ctx show", name)) - return + return cmdErr(fmt.Sprintf("Context '%s' not found. Run: kates ctx show", name)) case err != nil: - output.Error("Failed to save: " + err.Error()) - return + return cmdErr("Failed to save: " + err.Error()) } output.Success(fmt.Sprintf("Switched to '%s' → %s", name, ctx.URL)) + return nil }, } @@ -176,7 +178,7 @@ var ctxDeleteCmd = &cobra.Command{ Use: "delete ", Short: "Remove a context", Args: cobra.ExactArgs(1), - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { name := args[0] var current string err := updateConfig(func(cfg *Config) error { @@ -196,36 +198,34 @@ var ctxDeleteCmd = &cobra.Command{ }) switch { case errors.Is(err, errContextNotFound): - output.Error(fmt.Sprintf("Context '%s' not found", name)) - return + return cmdErr(fmt.Sprintf("Context '%s' not found", name)) case err != nil: - output.Error("Failed to save: " + err.Error()) - return + return cmdErr("Failed to save: " + err.Error()) } output.Success(fmt.Sprintf("Context '%s' deleted", name)) if current != "" { output.Hint(fmt.Sprintf(" Active context: %s", current)) } + return nil }, } var ctxCurrentCmd = &cobra.Command{ Use: "current", Short: "Print the active context name and URL", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { cfg := loadConfig() if cfg.CurrentContext == "" && contextFlag == "" { - output.Warn("No active context. Run: kates ctx set --url ") - return + return cmdErr("No active context. Run: kates ctx set --url ") } // The name --context or KATES_CONTEXT gives, when it gives one: it // used to print the file's current name beside the other one's URL. name, ctx, err := resolveContext(cfg) if err != nil { - output.Error(err.Error()) - return + return cmdErr(err.Error()) } output.Success(fmt.Sprintf("%s → %s", name, ctx.URL)) + return nil }, } @@ -317,7 +317,7 @@ key; otherwise the context arrives without a key.`, kates ctx export > team-contexts.yaml kates ctx export --name staging kates ctx export --reveal > my-contexts.yaml`, - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { cfg := loadConfig() exportCfg := cfg if ctxExportFlag != "" { @@ -327,8 +327,7 @@ key; otherwise the context arrives without a key.`, Contexts: map[string]Context{ctxExportFlag: ctx}, } } else { - output.Error(fmt.Sprintf("Context '%s' not found", ctxExportFlag)) - return + return cmdErr(fmt.Sprintf("Context '%s' not found", ctxExportFlag)) } } // Keys used to go out in clear, so a file meant for a teammate, or a @@ -354,14 +353,14 @@ key; otherwise the context arrives without a key.`, } data, err := yaml.Marshal(exportCfg) if err != nil { - output.Error("Failed to marshal config: " + err.Error()) - return + return cmdErr("Failed to marshal config: " + err.Error()) } fmt.Print(string(data)) // On stderr, so the YAML on stdout stays exactly what import reads. if masked { fmt.Fprintln(output.Err, output.DimStyle.Render(" API keys and proxy passwords are masked; pass --reveal to include them")) } + return nil }, } @@ -372,7 +371,7 @@ var ctxImportCmd = &cobra.Command{ Short: "Import contexts from a YAML file (merges with existing)", Example: ` kates ctx import --file team-contexts.yaml cat contexts.yaml | kates ctx import`, - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { var data []byte var err error if ctxImportFile != "" { @@ -381,14 +380,12 @@ var ctxImportCmd = &cobra.Command{ data, err = io.ReadAll(os.Stdin) } if err != nil { - output.Error("Failed to read input: " + err.Error()) - return + return cmdErr("Failed to read input: " + err.Error()) } var incoming Config if err := yaml.Unmarshal(data, &incoming); err != nil { - output.Error("Invalid YAML: " + err.Error()) - return + return cmdErr("Invalid YAML: " + err.Error()) } imported := 0 @@ -402,8 +399,7 @@ var ctxImportCmd = &cobra.Command{ return nil }) if err != nil { - output.Error("Failed to save: " + err.Error()) - return + return cmdErr("Failed to save: " + err.Error()) } output.Success(fmt.Sprintf("Imported %d context(s)", imported)) names := make([]string, 0, len(incoming.Contexts)) @@ -417,6 +413,7 @@ var ctxImportCmd = &cobra.Command{ output.Warn(w) } } + return nil }, } diff --git a/cli/cmd/context_test.go b/cli/cmd/context_test.go index 3efd163d..e5edb445 100644 --- a/cli/cmd/context_test.go +++ b/cli/cmd/context_test.go @@ -136,6 +136,37 @@ func TestUnknownContextCanBeCreated(t *testing.T) { } } +// The ctx commands that fail used to exit 0, so in +// "kates ctx import --file f && kates ctx use lab && kates test create ..." +// a missing lab sent the test to whichever context was current. +func TestCtxFailuresExitOne(t *testing.T) { + dir := t.TempDir() + invalid := filepath.Join(dir, "invalid.yaml") + if err := os.WriteFile(invalid, []byte("contexts: ["), 0o600); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{ + {"ctx", "use", "nope"}, + {"ctx", "delete", "nope"}, + {"ctx", "export", "--name", "nope"}, + {"ctx", "import", "--file", filepath.Join(dir, "missing.yaml")}, + {"ctx", "import", "--file", invalid}, + {"--context", "nope", "ctx", "current"}, + } { + out, code, _ := runKatesCLI(t, nil, args...) + if code != 1 { + t.Errorf("%v: exit %d, want 1:\n%s", args, code, out) + } + if strings.Contains(out, "contexts:") { + t.Errorf("%v printed contexts:\n%s", args, out) + } + } + + if out, code, _ := runKatesCLI(t, nil, "ctx", "use", "lab"); code != 0 { + t.Errorf("ctx use lab: exit %d, want 0:\n%s", code, out) + } +} + // kates status named the file's current context, not the one --context chose. func TestStatusNamesTheContextUsed(t *testing.T) { out, code, _ := runKatesCLI(t, nil, "--context", "labb", "status") diff --git a/cli/cmd/ctx_export_test.go b/cli/cmd/ctx_export_test.go index 17a3b12f..beda9be7 100644 --- a/cli/cmd/ctx_export_test.go +++ b/cli/cmd/ctx_export_test.go @@ -17,9 +17,10 @@ const ( ) // runCtxCommandLine runs a ctx command line as runCommandLine does, for the -// ctx commands, which use Run rather than RunE. It resets the flags they bind -// first, as a new process would start. -func runCtxCommandLine(t *testing.T, args ...string) { +// ctx commands, and returns the command's error: ctx show uses Run rather +// than RunE and has none. It resets the flags they bind first, as a new +// process would start. +func runCtxCommandLine(t *testing.T, args ...string) error { t.Helper() ctxExportFlag, ctxExportReveal, ctxImportFile = "", false, "" t.Cleanup(func() { ctxExportFlag, ctxExportReveal, ctxImportFile = "", false, "" }) @@ -30,7 +31,11 @@ func runCtxCommandLine(t *testing.T, args ...string) { if err := cmd.ParseFlags(rest); err != nil { t.Fatalf("flags %q: %v", rest, err) } + if cmd.RunE != nil { + return cmd.RunE(cmd, cmd.Flags().Args()) + } cmd.Run(cmd, cmd.Flags().Args()) + return nil } func exportTestConfig() Config { @@ -68,7 +73,7 @@ func TestCtxExport_MasksKeys(t *testing.T) { t.Setenv("HOME", t.TempDir()) mustSaveConfig(t, exportTestConfig()) - stdout, stderr, err := commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "export"); return nil }) + stdout, stderr, err := commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "export") }) if err != nil { t.Fatal(err) } @@ -89,7 +94,7 @@ func TestCtxExport_MasksKeys(t *testing.T) { } // --name masks too. - stdout, _, _ = commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "export", "--name", "lab"); return nil }) + stdout, _, _ = commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "export", "--name", "lab") }) if strings.Contains(string(stdout), exportLongKey) || !strings.Contains(string(stdout), "api-key: Zk3v****") { t.Errorf("export --name lab:\n%s", stdout) } @@ -99,7 +104,7 @@ func TestCtxExport_Reveal(t *testing.T) { t.Setenv("HOME", t.TempDir()) mustSaveConfig(t, exportTestConfig()) - stdout, stderr, _ := commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "export", "--reveal"); return nil }) + stdout, stderr, _ := commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "export", "--reveal") }) var got Config if err := yaml.Unmarshal(stdout, &got); err != nil { t.Fatalf("export is not YAML: %v\n%s", err, stdout) @@ -123,8 +128,8 @@ func TestCtxExport_Reveal(t *testing.T) { func TestCtxImport_RoundTrip(t *testing.T) { t.Setenv("HOME", t.TempDir()) mustSaveConfig(t, exportTestConfig()) - revealed, _, _ := commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "export", "--reveal"); return nil }) - masked, _, _ := commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "export"); return nil }) + revealed, _, _ := commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "export", "--reveal") }) + masked, _, _ := commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "export") }) dir := t.TempDir() revealedFile, maskedFile := filepath.Join(dir, "revealed.yaml"), filepath.Join(dir, "masked.yaml") for file, data := range map[string][]byte{revealedFile: revealed, maskedFile: masked} { @@ -135,7 +140,7 @@ func TestCtxImport_RoundTrip(t *testing.T) { t.Run("revealed", func(t *testing.T) { t.Setenv("HOME", t.TempDir()) - _, _, _ = commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "import", "--file", revealedFile); return nil }) + _, _, _ = commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "import", "--file", revealedFile) }) got := loadConfig().Contexts for name, want := range exportTestConfig().Contexts { if got[name] != want { @@ -149,7 +154,7 @@ func TestCtxImport_RoundTrip(t *testing.T) { mustSaveConfig(t, Config{CurrentContext: "lab", Contexts: map[string]Context{ "lab": {URL: "http://localhost:8080", Output: "json", APIKey: exportLongKey, KeySource: "kates-api-key@x"}, }}) - stdout, _, _ := commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "import", "--file", maskedFile); return nil }) + stdout, _, _ := commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "import", "--file", maskedFile) }) got := loadConfig().Contexts if lab := got["lab"]; lab.APIKey != exportLongKey || lab.KeySource != "kates-api-key@x" || lab.Output != "table" { t.Errorf("lab = %+v, want the file's settings with the key and key-source it had", lab) @@ -212,7 +217,7 @@ func TestCtxImport_MaskedKeyStaysWithItsServer(t *testing.T) { if err := os.WriteFile(file, []byte(body), 0o600); err != nil { t.Fatal(err) } - stdout, _, _ := commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "import", "--file", file); return nil }) + stdout, _, _ := commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "import", "--file", file) }) lab := loadConfig().Contexts["lab"] if tt.wantKey && (lab.APIKey != exportLongKey || lab.KeySource != "kates-api-key@x") { t.Errorf("lab = %+v, want the key and key-source it had", lab) @@ -245,7 +250,7 @@ func TestCtxExport_MasksProxyAndKeySource(t *testing.T) { "nopass": {URL: "http://localhost:8080", ProxyURL: "http://proxy.corp:3128"}, }}) - masked, _, _ := commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "export"); return nil }) + masked, _, _ := commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "export") }) for _, secret := range []string{exportLongKey, "s3cretProxyPass", source, "key-source"} { if strings.Contains(string(masked), secret) { t.Errorf("masked export prints %q:\n%s", secret, masked) @@ -262,7 +267,7 @@ func TestCtxExport_MasksProxyAndKeySource(t *testing.T) { t.Errorf("nopass proxy-url = %q, want it as it is", p) } - revealed, _, _ := commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "export", "--reveal"); return nil }) + revealed, _, _ := commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "export", "--reveal") }) for _, want := range []string{exportLongKey, proxy, source} { if !strings.Contains(string(revealed), want) { t.Errorf("export --reveal lacks %q:\n%s", want, revealed) @@ -276,7 +281,7 @@ func TestCtxExport_MasksProxyAndKeySource(t *testing.T) { if err := os.WriteFile(file, masked, 0o600); err != nil { t.Fatal(err) } - stdout, _, _ := commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "import", "--file", file); return nil }) + stdout, _, _ := commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "import", "--file", file) }) if p := loadConfig().Contexts["lab"].ProxyURL; p != "http://alice@proxy.corp:3128" { t.Errorf("imported proxy-url = %q, want it without the masked password", p) } @@ -290,7 +295,7 @@ func TestCtxExport_MasksProxyAndKeySource(t *testing.T) { func TestCtxShow_ShortKey(t *testing.T) { t.Setenv("HOME", t.TempDir()) mustSaveConfig(t, exportTestConfig()) - stdout, _, _ := commandOutput(t, func() error { runCtxCommandLine(t, "ctx", "show"); return nil }) + stdout, _, _ := commandOutput(t, func() error { return runCtxCommandLine(t, "ctx", "show") }) out := stripAnsi(string(stdout)) if !tableHasRow(out, []string{"tiny", "http://localhost:30083", "json", "****"}) || !tableHasRow(out, []string{"lab", "http://localhost:8080", "table", "Zk3v****"}) { diff --git a/cli/cmd/root_test.go b/cli/cmd/root_test.go index 050308b1..ec7aac52 100644 --- a/cli/cmd/root_test.go +++ b/cli/cmd/root_test.go @@ -390,7 +390,9 @@ func TestCtxImport_ClearsKeySource(t *testing.T) { t.Cleanup(func() { ctxImportFile, output.Out, output.Err = origFile, origOut, origErr }) ctxImportFile = file output.ResetForTesting() - ctxImportCmd.Run(ctxImportCmd, nil) + if err := ctxImportCmd.RunE(ctxImportCmd, nil); err != nil { + t.Fatal(err) + } got := loadConfig().Contexts[portsContextName] if got.APIKey != "imported-key" || got.KeySource != "" { @@ -414,6 +416,7 @@ func TestCtxCommands(t *testing.T) { wantNames []string wantOut string unchanged bool // the file must be left byte for byte as it was + wantErr bool // the command must fail, so its exit code is 1 }{ { name: "set adds a context without switching to it", @@ -426,28 +429,28 @@ func TestCtxCommands(t *testing.T) { }, { name: "use switches the current context", - run: func() error { ctxUseCmd.Run(ctxUseCmd, []string{"mcp"}); return nil }, + run: func() error { return ctxUseCmd.RunE(ctxUseCmd, []string{"mcp"}) }, wantCurrent: "mcp", wantNames: []string{"local", "mcp"}, wantOut: "Switched to 'mcp' → http://localhost:8080", }, { name: "use of a missing context changes nothing", - run: func() error { ctxUseCmd.Run(ctxUseCmd, []string{"nope"}); return nil }, + run: func() error { return ctxUseCmd.RunE(ctxUseCmd, []string{"nope"}) }, wantCurrent: "local", wantNames: []string{"local", "mcp"}, - wantOut: "Context 'nope' not found", unchanged: true, + wantOut: "Context 'nope' not found", unchanged: true, wantErr: true, }, { name: "delete of the current context picks another", - run: func() error { ctxDeleteCmd.Run(ctxDeleteCmd, []string{"local"}); return nil }, + run: func() error { return ctxDeleteCmd.RunE(ctxDeleteCmd, []string{"local"}) }, wantNames: []string{"mcp"}, wantOut: "Context 'local' deleted", wantCurrent: "", // any remaining context; checked below }, { name: "delete of a missing context changes nothing", - run: func() error { ctxDeleteCmd.Run(ctxDeleteCmd, []string{"nope"}); return nil }, + run: func() error { return ctxDeleteCmd.RunE(ctxDeleteCmd, []string{"nope"}) }, wantCurrent: "local", wantNames: []string{"local", "mcp"}, - wantOut: "Context 'nope' not found", unchanged: true, + wantOut: "Context 'nope' not found", unchanged: true, wantErr: true, }, } for _, tt := range tests { @@ -463,8 +466,8 @@ func TestCtxCommands(t *testing.T) { t.Fatal(err) } - if err := tt.run(); err != nil { - t.Fatalf("command: %v", err) + if err := tt.run(); (err != nil) != tt.wantErr { + t.Fatalf("command: err = %v, want an error: %v", err, tt.wantErr) } if out := stripAnsi(buf.String()); !strings.Contains(out, tt.wantOut) { diff --git a/docs/book/10-cli-reference.md b/docs/book/10-cli-reference.md index 756d053b..dd819f4e 100644 --- a/docs/book/10-cli-reference.md +++ b/docs/book/10-cli-reference.md @@ -2487,7 +2487,7 @@ Consequences worth knowing in scripts: Some commands report a failure on screen and still exit `0`, so a script cannot rely on their status. Among them: -- `kates ctx use`, `kates ctx delete` and `kates ctx export --name` with a context that does not exist, `kates ctx import` with a file it cannot read or parse, and a mistyped subcommand under a command group — `kates ctx list`, for instance — which prints the group's help. +- A mistyped subcommand under a command group — `kates ctx list`, for instance — which prints the group's help. - `kates status` when the API does not answer or rejects the API key: it prints `unreachable`. - `kates ports` when it finds no Kates services, some forwards fail, or the API rejects a key it checks. - `kates snapshot create` when the API fails: it saves a snapshot of zeros. diff --git a/eval/mcp/kates_api.py b/eval/mcp/kates_api.py index 18c99061..fbe44660 100644 --- a/eval/mcp/kates_api.py +++ b/eval/mcp/kates_api.py @@ -120,7 +120,8 @@ def read_context(kates_bin: str, name: str, env: dict[str, str] | None = None) - ) except (OSError, subprocess.TimeoutExpired) as e: raise ContextError(f"cannot run {kates_bin} ctx export: {e}") from e - # ctx export exits 0 and prints nothing when the context does not exist. + # ctx export exits 1 when the context does not exist; CLIs before that + # exited 0 with nothing on stdout, so both are checked. if proc.returncode != 0 or not proc.stdout.strip(): detail = proc.stderr.strip().splitlines()[-1] if proc.stderr.strip() else "no output" raise ContextError(f"context {name!r} is not in the kates config ({detail}); " diff --git a/eval/mcp/testdata/fake_kates.py b/eval/mcp/testdata/fake_kates.py index dbfdb99b..13e21105 100644 --- a/eval/mcp/testdata/fake_kates.py +++ b/eval/mcp/testdata/fake_kates.py @@ -20,9 +20,9 @@ def main(argv): if argv[:2] == ["ctx", "export"]: name = argv[argv.index("--name") + 1] if name not in KEYS: - # kates ctx export exits 0 with nothing on stdout for a missing context. + # kates ctx export exits 1 for a missing context. print(f" ✖ Context '{name}' not found", file=sys.stderr) - return 0 + return 1 print(f"current-context: {name}\ncontexts:\n {name}:\n url: {url}\n" f" output: table\n api-key: \"{KEYS[name]}\"") return 0