From 375aaff4697bb70c238ff010c06b1dd1259711e1 Mon Sep 17 00:00:00 2001 From: Said Boudjelda Date: Sat, 17 Oct 2026 19:18:51 +0200 Subject: [PATCH] fix(cli): kates ctx use, delete, export, import and current exit 1 when they fail In a step like "kates ctx import --file ci-contexts.yaml && kates ctx use lab && kates test create --type STRESS ...", a file with no lab context made ctx use print "Context 'lab' not found" and exit 0, so the test went to whichever context was current. ctx delete and ctx export --name did the same for a missing context (export > prod.yaml left an empty file), ctx import for a file it could not read, parse or save, and ctx current for an unknown --context or no active context. Only ctx set had been moved to RunE. These commands now use RunE and return cmdErr, which prints the same message and exits 1. TestCtxFailuresExitOne runs each failing case through the real command tree and checks the exit code; TestCtxCommands expects an error from the missing-context rows, and the ctx export tests' helper calls RunE and returns its error to commandOutput. The book no longer lists these commands under the ones that exit 0 on failure, and the MCP eval harness's comment and its fake CLI say ctx export exits 1 for a missing context. --- cli/cmd/config.go | 51 ++++++++++++++++----------------- cli/cmd/context_test.go | 31 ++++++++++++++++++++ cli/cmd/ctx_export_test.go | 35 ++++++++++++---------- cli/cmd/root_test.go | 21 ++++++++------ docs/book/10-cli-reference.md | 2 +- eval/mcp/kates_api.py | 3 +- eval/mcp/testdata/fake_kates.py | 4 +-- 7 files changed, 92 insertions(+), 55 deletions(-) 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