diff --git a/CLAUDE.md b/CLAUDE.md index c88152d..318cd3d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -58,3 +58,9 @@ Config directory is resolved as: `OMNI_CONFIG_DIR` > `XDG_CONFIG_HOME/omni-cli` ## Output All output is JSON to stdout. Errors go to stderr as JSON. Use `--compact` for non-indented output (good for piping to `jq`). + +Nothing is written to stdout on failure: HTTP ≥400 bodies, error messages, and subcommand suggestions all go to stderr, and the exit code is non-zero. A failed API call leaves exactly one JSON document on stderr — `{"error": , "status": , "body": }` — so `2>err.json` stays parseable; nothing else is printed alongside it. Runtime errors don't print the usage block (flag-parse errors still do). + +A 2xx body that isn't JSON (e.g. `query run`'s `text/ndjson` stream, or CSV/XLSX with `--result-type`) is passed through to stdout unchanged and counts as success. The body is read in full before anything is written, so a truncated response never leaves a partial payload on stdout. + +A group command with no subcommand (`omni models`) prints its help to stderr and exits 1; an unknown subcommand errors with suggestions, `--help` or not (`omni models list-branches --help` is a typo, not a help request). diff --git a/README.md b/README.md index 0a19704..1219cc0 100644 --- a/README.md +++ b/README.md @@ -142,6 +142,20 @@ Config file lives at `~/.config/omni-cli/config.json`. All output is JSON to stdout. Errors go to stderr as JSON. Use `--compact` for non-indented output (good for piping to `jq`). +Failures write nothing to stdout — the API's error body, the error message, and any subcommand suggestions go to stderr, and the exit code is non-zero. An empty stdout therefore always means "no data", which keeps `omni ... | jq` from choking on error JSON. + +A failed API call leaves exactly one JSON document on stderr, so `omni ... 2>err.json` stays parseable: + +```json +{ + "error": "bad model id", + "status": 400, + "body": { "detail": "bad model id", "code": "INVALID" } +} +``` + +`body` holds the API's own payload and is omitted when the response wasn't JSON. A **successful** response that isn't JSON — `query run` streams `text/ndjson`, and returns CSV or XLSX with a result type — is passed through to stdout unchanged. + ## Environment variables | Variable | Description | diff --git a/cmd/omni/agent_help.go b/cmd/omni/agent_help.go index 32fbe30..2bd4ba0 100644 --- a/cmd/omni/agent_help.go +++ b/cmd/omni/agent_help.go @@ -14,6 +14,21 @@ const agentHelpText = `# Omni CLI — Agent Guide All output is JSON to stdout. Errors are JSON to stderr. Use --compact for non-indented output (good for piping to jq). +## Streams and exit codes +On failure nothing is written to stdout — the API's error body, the error +message, and any suggestions all go to stderr, and the exit code is non-zero. +So an empty stdout always means "no data", never "parse this". + +A failed API call leaves exactly one JSON document on stderr: + {"error": "", "status": 400, "body": {}} +"body" is omitted when the response wasn't JSON. A successful response that +isn't JSON (query run streams text/ndjson) passes through to stdout unchanged. + +A group with no subcommand ("omni models") prints its help to stderr and exits +1; an unrecognized subcommand ("omni models list-branches") is an error with +suggestions, with or without --help. Use "omni --help" to see the real +subcommand names. + ## Auth Set OMNI_API_TOKEN env var, or run: omni config init diff --git a/cmd/omni/branch_commands.go b/cmd/omni/branch_commands.go index 27f2472..4aa672f 100644 --- a/cmd/omni/branch_commands.go +++ b/cmd/omni/branch_commands.go @@ -33,6 +33,9 @@ func createBranchCmd(exec openapi.Executor) *cobra.Command { Long: "Create a new branch of an existing model. The model-id is the base model to branch from.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // Runtime failures past this point shouldn't drag the usage block along. + cmd.SilenceUsage = true + baseModelID := args[0] name, _ := cmd.Flags().GetString("name") diff --git a/cmd/omni/config_commands.go b/cmd/omni/config_commands.go index c54f7d7..9b8f674 100644 --- a/cmd/omni/config_commands.go +++ b/cmd/omni/config_commands.go @@ -11,6 +11,7 @@ import ( "github.com/exploreomni/omni-cli/internal/config" "github.com/exploreomni/omni-cli/internal/oauth" + "github.com/exploreomni/omni-cli/internal/openapi" "github.com/spf13/cobra" "golang.org/x/oauth2" "golang.org/x/term" @@ -25,10 +26,8 @@ func applyOAuthToken(p *config.Profile, tok *oauth2.Token) { } func addConfigCommands(root *cobra.Command) { - configCmd := &cobra.Command{ - Use: "config", - Short: "Manage CLI configuration profiles", - } + // Same unknown-subcommand handling as the generated groups. + configCmd := openapi.NewGroupCommand("config", "Manage CLI configuration profiles") configCmd.AddCommand(configInitCmd()) configCmd.AddCommand(configShowCmd()) @@ -68,6 +67,9 @@ accepted as a flag, so it can't leak into shell history.`, # API key (prompts securely for the key) omni config init --name prod --endpoint https://myorg.omniapp.co --auth api-key`, RunE: func(cmd *cobra.Command, args []string) error { + // Args are validated: failures below are runtime errors, not usage errors. + cmd.SilenceUsage = true + reader := bufio.NewReader(os.Stdin) if !cmd.Flags().Changed("name") { @@ -163,6 +165,9 @@ func configShowCmd() *cobra.Command { Use: "show", Short: "Display current configuration", RunE: func(cmd *cobra.Command, args []string) error { + // Args are validated: failures below are runtime errors, not usage errors. + cmd.SilenceUsage = true + cfg, err := config.Load() if err != nil { return fmt.Errorf("no config found at %s — run `omni config init`", config.ConfigPath()) @@ -198,6 +203,9 @@ func configSetFormatCmd() *cobra.Command { Short: "Set the default output format", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // Args are validated: failures below are runtime errors, not usage errors. + cmd.SilenceUsage = true + format := strings.ToLower(strings.TrimSpace(args[0])) if !config.ValidOutputFormat(format) { return fmt.Errorf("invalid format %q — must be one of: json, human, auto", args[0]) @@ -228,6 +236,9 @@ func configUseCmd() *cobra.Command { Long: "Switch the default profile.\n\nIf the profile name contains spaces, quote it: `omni config use \"My Profile\"`. Run `omni config list` to see profile names.", Args: profileNameArgs(1, 1), RunE: func(cmd *cobra.Command, args []string) error { + // Args are validated: failures below are runtime errors, not usage errors. + cmd.SilenceUsage = true + cfg, err := config.Load() if err != nil { return fmt.Errorf("no config found — run `omni config init`") @@ -285,6 +296,9 @@ func configListCmd() *cobra.Command { Short: "List configured profiles", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { + // Args are validated: failures below are runtime errors, not usage errors. + cmd.SilenceUsage = true + cfg, err := config.Load() if err != nil { return fmt.Errorf("no config found — run `omni config init`") @@ -317,6 +331,9 @@ func configRenameCmd() *cobra.Command { Short: "Rename a profile", Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { + // Args are validated: failures below are runtime errors, not usage errors. + cmd.SilenceUsage = true + oldName, newName := args[0], args[1] cfg, err := config.Load() @@ -355,6 +372,9 @@ func configDeleteCmd() *cobra.Command { Short: "Delete a profile", Args: profileNameArgs(1, 1), RunE: func(cmd *cobra.Command, args []string) error { + // Args are validated: failures below are runtime errors, not usage errors. + cmd.SilenceUsage = true + name := args[0] cfg, err := config.Load() if err != nil { @@ -396,6 +416,9 @@ func configLoginCmd() *cobra.Command { Short: "Log in via OAuth browser flow", Args: profileNameArgs(0, 1), RunE: func(cmd *cobra.Command, args []string) error { + // Args are validated: failures below are runtime errors, not usage errors. + cmd.SilenceUsage = true + cfg, err := config.Load() if err != nil { return fmt.Errorf("no config found — run `omni config init` first") @@ -444,6 +467,9 @@ func configLogoutCmd() *cobra.Command { Short: "Clear OAuth tokens from a profile", Args: profileNameArgs(0, 1), RunE: func(cmd *cobra.Command, args []string) error { + // Args are validated: failures below are runtime errors, not usage errors. + cmd.SilenceUsage = true + cfg, err := config.Load() if err != nil { return fmt.Errorf("no config found — run `omni config init` first") diff --git a/cmd/omni/config_commands_test.go b/cmd/omni/config_commands_test.go index 4ccbc58..5fdefab 100644 --- a/cmd/omni/config_commands_test.go +++ b/cmd/omni/config_commands_test.go @@ -1,6 +1,7 @@ package main import ( + "bytes" "io" "os" "path/filepath" @@ -9,6 +10,7 @@ import ( "time" "github.com/exploreomni/omni-cli/internal/config" + "github.com/spf13/cobra" "golang.org/x/oauth2" ) @@ -81,6 +83,48 @@ func TestApplyOAuthToken_CopiesAllFields(t *testing.T) { } } +// Runtime failures in the hand-written config commands must not dump the usage +// block: same contract the generated API commands follow, so an error message +// isn't buried under a wall of flags. +func TestConfigCommands_NoUsageOnRuntimeError(t *testing.T) { + // An empty (absent) config file makes every command that loads config fail. + withConfig(t, nil) + + cases := []struct { + name string + cmd func() *cobra.Command + args []string + }{ + {"init", configInitCmd, []string{"--name", "prod", "--endpoint", "https://myorg.omniapp.co", "--auth", "magic"}}, + {"show", configShowCmd, nil}, + {"list", configListCmd, nil}, + {"use", configUseCmd, []string{"ghost"}}, + {"rename", configRenameCmd, []string{"ghost", "other"}}, + {"delete", configDeleteCmd, []string{"ghost", "--yes"}}, + {"login", configLoginCmd, []string{"ghost"}}, + {"logout", configLogoutCmd, []string{"ghost"}}, + {"set-format", configSetFormatCmd, []string{"yaml"}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var out bytes.Buffer + cmd := tc.cmd() + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceErrors = true + cmd.SetArgs(tc.args) + + if err := cmd.Execute(); err == nil { + t.Fatalf("expected a runtime error for %s", tc.name) + } + if strings.Contains(out.String(), "Usage:") { + t.Errorf("output = %q, want no usage block after a runtime error", out.String()) + } + }) + } +} + // --- config init (non-interactive flags) --- // // Note: there is deliberately no --api-key flag. Accepting a secret on the diff --git a/cmd/omni/main.go b/cmd/omni/main.go index 5db17fd..b8cbeac 100644 --- a/cmd/omni/main.go +++ b/cmd/omni/main.go @@ -2,6 +2,7 @@ package main import ( "embed" + "errors" "fmt" "os" "runtime/debug" @@ -81,7 +82,12 @@ func main() { addBranchCommands(root, executeAPICall) addUserCommands(root, executeAPICall) - if err := root.Execute(); err != nil { + // ExecuteC, not Execute: cobra returns a nil error whenever it answers the + // help flag, including for `omni models list-branches --help`, where the + // "help" is really an unknown-subcommand error. UnknownSubcommand asks the + // command that ran whether that's what happened. + cmd, err := root.ExecuteC() + if err != nil || openapi.UnknownSubcommand(cmd) { os.Exit(1) } } @@ -124,7 +130,16 @@ func executeAPICall(req openapi.APIRequest) error { } defer resp.Body.Close() - return outputResponse(resp, format, compact) + err = outputResponse(resp, format, compact) + var apiErr *apiError + if errors.As(err, &apiErr) { + // outputResponse already wrote a complete error message to stderr — + // a JSON envelope carrying the status, or the human-mode one-liner. + // Letting cobra append its own line would say it twice, and would + // leave two documents on stderr for JSON consumers to trip over. + req.Cmd.SilenceErrors = true + } + return err } // resolveConfig builds the runtime config from flags, env, and config file. diff --git a/cmd/omni/output.go b/cmd/omni/output.go index 501738b..ed06a88 100644 --- a/cmd/omni/output.go +++ b/cmd/omni/output.go @@ -6,54 +6,115 @@ import ( "fmt" "io" "net/http" + "os" "github.com/exploreomni/omni-cli/internal/config" "github.com/exploreomni/omni-cli/internal/output" ) +// apiError reports a failed invocation whose message has already been written +// to stderr in full. The caller silences cobra's own one-line report off the +// back of this type, so the failure isn't announced twice. +type apiError struct { + status int + detail string +} + +func (e *apiError) Error() string { + if e.detail != "" { + return e.detail + } + return fmt.Sprintf("API returned HTTP %d", e.status) +} + func outputResponse(resp *http.Response, format string, compact bool) error { + return outputResponseTo(os.Stdout, os.Stderr, resp, format, compact) +} + +// outputResponseTo writes a response to explicit streams, upholding two +// contracts: a failure writes nothing to stdout and leaves exactly one JSON +// document on stderr (in JSON mode), and a 2xx body that isn't JSON is data, +// not an error, so it passes through to stdout byte for byte. The body is read +// in full before anything is written, so a truncated read can't leave half a +// payload on stdout ahead of a non-zero exit. +func outputResponseTo(stdout, stderr io.Writer, resp *http.Response, format string, compact bool) error { + data, err := io.ReadAll(resp.Body) + if err != nil { + // Still an envelope: JSON-mode stderr has to stay parseable even when + // the failure is ours rather than the API's. + detail := fmt.Sprintf("reading response: %v", err) + writeError(stderr, format, resp.StatusCode, detail, nil, compact) + return &apiError{status: resp.StatusCode, detail: detail} + } + if resp.StatusCode >= 400 { - if format == config.FormatHuman { - body, _ := io.ReadAll(resp.Body) - output.HumanError(resp.StatusCode, extractErrorDetail(body, resp.StatusCode)) - } else { - if err := output.JSON(resp.Body, compact); err != nil { - output.Error(resp.StatusCode, fmt.Sprintf("HTTP %d", resp.StatusCode)) - } - } - return fmt.Errorf("API returned HTTP %d", resp.StatusCode) + body := jsonBody(data) + detail := extractErrorDetail(body, data, resp.StatusCode) + writeError(stderr, format, resp.StatusCode, detail, body, compact) + return &apiError{status: resp.StatusCode} } // 204 No Content if resp.StatusCode == 204 { if format == config.FormatHuman { - fmt.Println("✓ ok") + fmt.Fprintln(stdout, "✓ ok") } else { - fmt.Println("{}") + fmt.Fprintln(stdout, "{}") } return nil } + // Non-JSON payloads (`query run`'s text/ndjson stream, CSV/XLSX with + // --result-type) go out unchanged: no re-indenting, no appended newline, + // so a redirect to a file reproduces the response byte for byte. + if trimmed := bytes.TrimSpace(data); len(trimmed) > 0 && !json.Valid(trimmed) { + _, err := stdout.Write(data) + return err + } + + if format == config.FormatHuman { + return output.HumanBytes(stdout, data) + } + return output.JSONBytes(stdout, data, compact) +} + +// writeError reports a failed call on stderr in the requested format. +func writeError(stderr io.Writer, format string, status int, detail string, body json.RawMessage, compact bool) { if format == config.FormatHuman { - return output.Human(resp.Body) + output.HumanErrorTo(stderr, status, detail) + return } - return output.JSON(resp.Body, compact) + output.APIErrorTo(stderr, status, detail, body, compact) } -// extractErrorDetail pulls a readable message out of a JSON error body. -// Falls back to the raw body or an HTTP status string. -func extractErrorDetail(body []byte, status int) string { +// jsonBody returns the body as raw JSON for embedding in an error envelope, or +// nil when it isn't JSON (an HTML error page from a proxy, say) or is a literal +// null, which carries no more information than omitting the field. +func jsonBody(body []byte) json.RawMessage { trimmed := bytes.TrimSpace(body) - if len(trimmed) == 0 { - return fmt.Sprintf("HTTP %d", status) - } - var obj map[string]any - if err := json.Unmarshal(trimmed, &obj); err == nil { - for _, key := range []string{"detail", "message", "error"} { - if s, ok := obj[key].(string); ok && s != "" { - return s + if len(trimmed) == 0 || !json.Valid(trimmed) || string(trimmed) == "null" { + return nil + } + return json.RawMessage(trimmed) +} + +// extractErrorDetail pulls a readable message out of a JSON error body, given +// the same body already validated by jsonBody. It falls back to the raw body, +// or to an HTTP status string when the body is empty or carries no message. +func extractErrorDetail(body json.RawMessage, raw []byte, status int) string { + if body != nil { + var obj map[string]any + if err := json.Unmarshal(body, &obj); err == nil { + for _, key := range []string{"detail", "message", "error"} { + if s, ok := obj[key].(string); ok && s != "" { + return s + } } } + return string(body) + } + if trimmed := bytes.TrimSpace(raw); len(trimmed) > 0 && string(trimmed) != "null" { + return string(trimmed) } - return string(trimmed) + return fmt.Sprintf("HTTP %d", status) } diff --git a/cmd/omni/output_test.go b/cmd/omni/output_test.go index 32af684..93b97e2 100644 --- a/cmd/omni/output_test.go +++ b/cmd/omni/output_test.go @@ -1,6 +1,10 @@ package main import ( + "bytes" + "encoding/json" + "errors" + "fmt" "io" "net/http" "strings" @@ -50,3 +54,283 @@ func TestOutputResponse_Error_Human(t *testing.T) { t.Fatal("expected error for 404 status") } } + +// Stream hygiene: an API error body must never land on stdout. A caller doing +// `omni ... | jq` should see either well-formed data or an empty stream — +// never error JSON mixed into the payload. +func TestOutputResponseTo_ErrorBodyGoesToStderr(t *testing.T) { + for _, compact := range []bool{true, false} { + var stdout, stderr bytes.Buffer + resp := &http.Response{ + StatusCode: 400, + Body: io.NopCloser(strings.NewReader(`{"detail":"bad request"}`)), + } + + err := outputResponseTo(&stdout, &stderr, resp, "json", compact) + if err == nil { + t.Fatalf("compact=%v: expected error for 400 status", compact) + } + if stdout.Len() != 0 { + t.Errorf("compact=%v: stdout should be empty, got %q", compact, stdout.String()) + } + if !strings.Contains(stderr.String(), "bad request") { + t.Errorf("compact=%v: stderr missing error body, got %q", compact, stderr.String()) + } + } +} + +// Human-mode errors go to stderr too, as a one-line message with the status. +func TestOutputResponseTo_HumanErrorGoesToStderr(t *testing.T) { + var stdout, stderr bytes.Buffer + resp := &http.Response{ + StatusCode: 404, + Body: io.NopCloser(strings.NewReader(`{"detail":"not found"}`)), + } + + if err := outputResponseTo(&stdout, &stderr, resp, "human", false); err == nil { + t.Fatal("expected error for 404 status") + } + if stdout.Len() != 0 { + t.Errorf("stdout should be empty, got %q", stdout.String()) + } + if !strings.Contains(stderr.String(), "not found") || !strings.Contains(stderr.String(), "404") { + t.Errorf("stderr = %q, want detail and status", stderr.String()) + } +} + +// The whole stderr capture must be one valid JSON document in JSON mode — +// `omni ... 2>err.json` has to produce a parseable file, which it doesn't if +// anything (like cobra's own "Error: ..." line) is appended to the envelope. +func TestOutputResponseTo_StderrIsSingleJSONDocument(t *testing.T) { + for _, compact := range []bool{true, false} { + var stdout, stderr bytes.Buffer + resp := &http.Response{ + StatusCode: 400, + Body: io.NopCloser(strings.NewReader(`{"detail":"bad model id","code":"INVALID"}`)), + } + + err := outputResponseTo(&stdout, &stderr, resp, "json", compact) + + // The caller silences cobra's duplicate line off the back of this type. + var apiErr *apiError + if !errors.As(err, &apiErr) { + t.Fatalf("compact=%v: error = %v, want *apiError", compact, err) + } + if apiErr.status != 400 { + t.Errorf("compact=%v: status = %d, want 400", compact, apiErr.status) + } + + var envelope struct { + Error string `json:"error"` + Status int `json:"status"` + Body json.RawMessage `json:"body"` + } + if err := json.Unmarshal(stderr.Bytes(), &envelope); err != nil { + t.Fatalf("compact=%v: stderr is not a single JSON document (%v): %q", compact, err, stderr.String()) + } + if envelope.Error != "bad model id" { + t.Errorf("compact=%v: error = %q, want the API's detail", compact, envelope.Error) + } + if envelope.Status != 400 { + t.Errorf("compact=%v: status = %d, want 400", compact, envelope.Status) + } + if !strings.Contains(string(envelope.Body), `"INVALID"`) { + t.Errorf("compact=%v: body = %q, want the API's payload verbatim", compact, string(envelope.Body)) + } + } +} + +// A non-JSON error body (an HTML error page from a proxy, say) must still +// leave valid JSON on stderr. +func TestOutputResponseTo_NonJSONErrorBodyStillJSON(t *testing.T) { + var stdout, stderr bytes.Buffer + resp := &http.Response{ + StatusCode: 502, + Body: io.NopCloser(strings.NewReader("Bad Gateway")), + } + + if err := outputResponseTo(&stdout, &stderr, resp, "json", true); err == nil { + t.Fatal("expected error for 502 status") + } + var envelope struct { + Error string `json:"error"` + Status int `json:"status"` + Body json.RawMessage `json:"body"` + } + if err := json.Unmarshal(stderr.Bytes(), &envelope); err != nil { + t.Fatalf("stderr is not valid JSON (%v): %q", err, stderr.String()) + } + if !strings.Contains(envelope.Error, "Bad Gateway") { + t.Errorf("error = %q, want the raw body as the detail", envelope.Error) + } + if envelope.Body != nil { + t.Errorf("body = %q, want it omitted when the payload isn't JSON", string(envelope.Body)) + } + if stdout.Len() != 0 { + t.Errorf("stdout should be empty, got %q", stdout.String()) + } +} + +// Not every 2xx body is JSON: `query run` streams text/ndjson by default and +// returns CSV/XLSX with a result type. Those pass through to stdout byte for +// byte — no re-indenting, no appended newline — and count as success; an +// un-parseable payload is data, not an error. +func TestOutputResponseTo_NonJSONSuccessPassesThrough(t *testing.T) { + bodies := []struct { + name string + body string + }{ + {"ndjson", "{\"kind\":\"jobs_submitted\"}\n{\"kind\":\"job\"}\n"}, + {"csv", "id,name\n1,widget\n"}, + // A CSV whose last row has no trailing newline: appending one would + // change the file the user redirected to disk. + {"csv without trailing newline", "id,name\n1,widget"}, + // Binary payloads (XLSX with --result-type) must survive verbatim too. + {"binary", "PK\x03\x04\x14\x00\x00\x00\x08\x00"}, + } + for _, tc := range bodies { + for _, compact := range []bool{true, false} { + var stdout, stderr bytes.Buffer + resp := &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(tc.body)), + } + + if err := outputResponseTo(&stdout, &stderr, resp, "json", compact); err != nil { + t.Fatalf("%s compact=%v: non-JSON 2xx body should succeed, got %v", tc.name, compact, err) + } + if stdout.String() != tc.body { + t.Errorf("%s compact=%v: stdout = %q, want the body byte for byte (%q)", tc.name, compact, stdout.String(), tc.body) + } + if stderr.Len() != 0 { + t.Errorf("%s compact=%v: stderr should be empty, got %q", tc.name, compact, stderr.String()) + } + } + } +} + +// Human mode passes non-JSON payloads through unchanged as well — the same +// bytes, just on a terminal. +func TestOutputResponseTo_NonJSONSuccessPassesThroughHuman(t *testing.T) { + const body = "id,name\n1,widget" + var stdout, stderr bytes.Buffer + resp := &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(body)), + } + + if err := outputResponseTo(&stdout, &stderr, resp, "human", false); err != nil { + t.Fatalf("non-JSON 2xx body should succeed, got %v", err) + } + if stdout.String() != body { + t.Errorf("stdout = %q, want the body byte for byte", stdout.String()) + } +} + +// A literal `null` error body is valid JSON but says nothing, so the envelope +// omits "body" rather than carrying a JSON null that means the same thing. +func TestOutputResponseTo_NullErrorBodyOmitted(t *testing.T) { + for _, compact := range []bool{true, false} { + var stdout, stderr bytes.Buffer + resp := &http.Response{ + StatusCode: 500, + Body: io.NopCloser(strings.NewReader("null")), + } + + if err := outputResponseTo(&stdout, &stderr, resp, "json", compact); err == nil { + t.Fatalf("compact=%v: expected error for 500 status", compact) + } + var envelope map[string]any + if err := json.Unmarshal(stderr.Bytes(), &envelope); err != nil { + t.Fatalf("compact=%v: stderr is not valid JSON (%v): %q", compact, err, stderr.String()) + } + if _, ok := envelope["body"]; ok { + t.Errorf("compact=%v: envelope = %q, want no \"body\" field for a literal null", compact, stderr.String()) + } + if envelope["error"] != "HTTP 500" { + t.Errorf("compact=%v: error = %v, want the status fallback", compact, envelope["error"]) + } + } +} + +// A body that fails mid-read (a truncated response) must not leave a partial +// payload on stdout ahead of the non-zero exit. +func TestOutputResponseTo_ReadFailureWritesNothing(t *testing.T) { + var stdout, stderr bytes.Buffer + resp := &http.Response{ + StatusCode: 200, + Body: io.NopCloser(&truncatedReader{data: []byte(`{"records":[`)}), + } + + err := outputResponseTo(&stdout, &stderr, resp, "json", false) + var apiErr *apiError + if !errors.As(err, &apiErr) { + t.Fatalf("error = %v, want *apiError so cobra's duplicate line is silenced", err) + } + if stdout.Len() != 0 { + t.Errorf("stdout should be empty, got %q", stdout.String()) + } + // The one-JSON-document contract holds for our own failures too. + var envelope map[string]any + if err := json.Unmarshal(stderr.Bytes(), &envelope); err != nil { + t.Fatalf("stderr is not a single JSON document (%v): %q", err, stderr.String()) + } + detail, _ := envelope["error"].(string) + if !strings.Contains(detail, "unexpected EOF") { + t.Errorf("error = %q, want the read failure", detail) + } + if _, ok := envelope["body"]; ok { + t.Errorf("envelope = %q, want no body when the body couldn't be read", stderr.String()) + } +} + +// truncatedReader yields some bytes and then fails, like a connection dropped +// mid-response. +type truncatedReader struct { + data []byte + done bool +} + +func (r *truncatedReader) Read(p []byte) (int, error) { + if r.done { + return 0, fmt.Errorf("unexpected EOF") + } + r.done = true + n := copy(p, r.data) + return n, nil +} + +// The mirror image: successful payloads stay on stdout and leave stderr clean. +func TestOutputResponseTo_SuccessGoesToStdout(t *testing.T) { + cases := []struct { + name string + status int + body string + format string + want string + }{ + {"json", 200, `{"records":[]}`, "json", "records"}, + {"no content", 204, "", "json", "{}"}, + {"human", 200, `{"name":"widget"}`, "human", "widget"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + resp := &http.Response{ + StatusCode: tc.status, + Body: io.NopCloser(strings.NewReader(tc.body)), + } + + if err := outputResponseTo(&stdout, &stderr, resp, tc.format, true); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(stdout.String(), tc.want) { + t.Errorf("stdout = %q, want it to contain %q", stdout.String(), tc.want) + } + if stderr.Len() != 0 { + t.Errorf("stderr should be empty, got %q", stderr.String()) + } + }) + } +} diff --git a/cmd/omni/user_commands.go b/cmd/omni/user_commands.go index 2356a8c..b51a6ec 100644 --- a/cmd/omni/user_commands.go +++ b/cmd/omni/user_commands.go @@ -64,6 +64,9 @@ changed; the user's other attributes are left untouched. Values given with attribute. Use --attr-json to set numeric or multi-value (array) attributes.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // Runtime failures past this point shouldn't drag the usage block along. + cmd.SilenceUsage = true + attrs, _ := cmd.Flags().GetStringArray("attr") attrJSON, _ := cmd.Flags().GetString("attr-json") diff --git a/internal/openapi/generate.go b/internal/openapi/generate.go index 04bf96e..da169e5 100644 --- a/internal/openapi/generate.go +++ b/internal/openapi/generate.go @@ -4,6 +4,7 @@ package openapi import ( + "errors" "fmt" "io" "net/url" @@ -61,10 +62,7 @@ func GenerateCommands(specData []byte, exec Executor) ([]*cobra.Command, error) for _, tag := range tagNames { ops := groups[tag] - tagCmd := &cobra.Command{ - Use: slugify(tag), - Short: fmt.Sprintf("%s commands", tag), - } + tagCmd := NewGroupCommand(slugify(tag), fmt.Sprintf("%s commands", tag)) for _, op := range ops { if err := validateFlagNames(op); err != nil { @@ -79,6 +77,146 @@ func GenerateCommands(specData []byte, exec Executor) ([]*cobra.Command, error) return cmds, nil } +const ( + // groupAnnotation marks a command as a subcommand group, so the shared + // help func knows which commands to apply unknown-subcommand handling to. + groupAnnotation = "omni/group" + + // unknownSubcommandAnnotation records that a command printed an + // unknown-subcommand error instead of help. Cobra's help path always + // returns nil from Execute, so the caller has to read this to exit + // non-zero — see UnknownSubcommand. + unknownSubcommandAnnotation = "omni/unknown-subcommand" +) + +// NewGroupCommand builds a command that only groups subcommands (e.g. `omni +// models`), with the handling that keeps a mistyped or missing subcommand from +// looking like success: an unknown subcommand is a cobra-style error with +// suggestions, and a bare group prints its help to stderr — both exit non-zero +// with nothing on stdout. That covers `omni models list-branches --help`, which +// is a typo rather than a help request; plain `omni --help` is +// unaffected (help on stdout, exit 0). +func NewGroupCommand(use, short string) *cobra.Command { + cmd := &cobra.Command{ + Use: use, + Short: short, + Annotations: map[string]string{groupAnnotation: "true"}, + RunE: GroupRunE, + } + cmd.SetHelpFunc(groupHelpFunc) + return cmd +} + +// GroupRunE is the RunE for a command built by NewGroupCommand. +func GroupRunE(cmd *cobra.Command, args []string) error { + // Past flag parsing: from here on, errors are runtime errors, not usage + // errors, so don't bury the message under the usage block. + cmd.SilenceUsage = true + + if len(args) == 0 { + // The group produced no data, so its help goes to stderr and stdout + // stays empty for the pipe. The help text is the whole error report: + // letting cobra append its own "Error: ..." line would state the same + // failure twice, so silence it and let the non-zero exit speak. + renderHelp(cmd.ErrOrStderr(), cmd) + cmd.SilenceErrors = true + return fmt.Errorf("%q requires a subcommand", cmd.CommandPath()) + } + + return errors.New(unknownSubcommandMessage(cmd, args[0])) +} + +// groupHelpFunc backs --help for group commands and, by inheritance, for their +// subcommands. It only diverges from cobra's default when a group is asked for +// help with an unresolved positional argument left over — `omni models +// list-branches --help` — which is a typo, not a help request. +func groupHelpFunc(cmd *cobra.Command, _ []string) { + leftover := cmd.Flags().Args() + if !isGroup(cmd) || len(leftover) == 0 { + renderHelp(cmd.OutOrStdout(), cmd) + return + } + + cmd.Annotations[unknownSubcommandAnnotation] = leftover[0] + fmt.Fprintf(cmd.ErrOrStderr(), "%s %s\n", cmd.ErrPrefix(), unknownSubcommandMessage(cmd, leftover[0])) +} + +// UnknownSubcommand reports whether cmd's help output was really an +// unknown-subcommand error. Cobra returns a nil error from Execute whenever it +// handles the help flag, so main has to ask before choosing an exit code. +func UnknownSubcommand(cmd *cobra.Command) bool { + if cmd == nil { + return false + } + _, ok := cmd.Annotations[unknownSubcommandAnnotation] + return ok +} + +func isGroup(cmd *cobra.Command) bool { + return cmd != nil && cmd.Annotations[groupAnnotation] == "true" +} + +func unknownSubcommandMessage(cmd *cobra.Command, typed string) string { + var msg strings.Builder + fmt.Fprintf(&msg, "unknown subcommand %q for %q", typed, cmd.CommandPath()) + if names := subcommandSuggestions(cmd, typed); len(names) > 0 { + msg.WriteString("\n\nDid you mean this?") + for _, n := range names { + fmt.Fprintf(&msg, "\n\t%s", n) + } + } + fmt.Fprintf(&msg, "\n\nRun '%s --help' for a list of available subcommands", cmd.CommandPath()) + return msg.String() +} + +// renderHelp writes cobra's stock help text for cmd to w. It mirrors cobra's +// defaultHelpFunc rather than calling cmd.Help(), which dispatches back through +// HelpFunc — i.e. straight back into groupHelpFunc, forever. +func renderHelp(w io.Writer, cmd *cobra.Command) { + desc := cmd.Long + if desc == "" { + desc = cmd.Short + } + if desc = strings.TrimRight(desc, " \t\n"); desc != "" { + fmt.Fprintf(w, "%s\n\n", desc) + } + if cmd.Runnable() || cmd.HasSubCommands() { + fmt.Fprint(w, cmd.UsageString()) + } +} + +// subcommandSuggestions returns close matches for a mistyped subcommand. It +// extends cobra's Levenshtein/prefix matching with a reverse-prefix pass so an +// over-specified guess like `models list-branches` still points at `list` +// (the real answer being `list --model-kind BRANCH`). +func subcommandSuggestions(cmd *cobra.Command, typed string) []string { + if cmd.DisableSuggestions { + return nil + } + if cmd.SuggestionsMinimumDistance <= 0 { + cmd.SuggestionsMinimumDistance = 2 + } + + seen := map[string]bool{} + var names []string + for _, s := range cmd.SuggestionsFor(typed) { + if !seen[s] { + seen[s] = true + names = append(names, s) + } + } + for _, sub := range cmd.Commands() { + if !sub.IsAvailableCommand() || seen[sub.Name()] { + continue + } + if strings.HasPrefix(strings.ToLower(typed), strings.ToLower(sub.Name())+"-") { + seen[sub.Name()] = true + names = append(names, sub.Name()) + } + } + return names +} + type paramInfo struct { Name string In string // path, query, header @@ -210,6 +348,12 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { Deprecated: deprecatedMsg(op), Args: cobra.ExactArgs(len(op.PathParams)), RunE: func(cmd *cobra.Command, args []string) error { + // Flags parsed and args validated: anything that fails from here on + // (bad body, HTTP 4xx/5xx) is a runtime error, and dumping the usage + // block after it just buries the message — and, when the caller is + // piping, mixes prose into the stream. + cmd.SilenceUsage = true + // Substitute path params path := op.Path for i, p := range op.PathParams { diff --git a/internal/openapi/generate_test.go b/internal/openapi/generate_test.go index b8ce76b..8254821 100644 --- a/internal/openapi/generate_test.go +++ b/internal/openapi/generate_test.go @@ -1,6 +1,7 @@ package openapi import ( + "bytes" "encoding/json" "fmt" "os" @@ -409,6 +410,257 @@ func TestBuildCommand_WrongArgCount(t *testing.T) { } } +// A command that fails at runtime (e.g. the API returned HTTP 400) should not +// print the usage block after the error — the message is the useful part, and +// the usage text is noise that buries it. +func TestBuildCommand_RuntimeErrorSilencesUsage(t *testing.T) { + exec := func(req APIRequest) error { return fmt.Errorf("API returned HTTP 400") } + + op := &operationInfo{ + Tag: "test", + OperationID: "testListItems", + Method: "GET", + Path: "/api/v1/items", + } + + var out bytes.Buffer + cmd := buildCommand(op, exec) + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceErrors = true + cmd.SetArgs([]string{}) + + if err := cmd.Execute(); err == nil { + t.Fatal("expected the executor error to propagate") + } + if !cmd.SilenceUsage { + t.Error("SilenceUsage should be set once RunE is entered") + } + if strings.Contains(out.String(), "Usage:") { + t.Errorf("usage block should be suppressed, got %q", out.String()) + } +} + +// Flag-parse errors happen before RunE, so they're genuine usage errors and +// should still print the usage block. +func TestBuildCommand_FlagErrorKeepsUsage(t *testing.T) { + exec := func(req APIRequest) error { return nil } + + op := &operationInfo{ + Tag: "test", + OperationID: "testListItems", + Method: "GET", + Path: "/api/v1/items", + } + + var out bytes.Buffer + cmd := buildCommand(op, exec) + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceErrors = true + cmd.SetArgs([]string{"--nope"}) + + if err := cmd.Execute(); err == nil { + t.Fatal("expected error for unknown flag") + } + if !strings.Contains(out.String(), "Usage:") { + t.Errorf("usage block should be printed for flag errors, got %q", out.String()) + } +} + +// newTestGroup builds a tag group shaped like the ones GenerateCommands makes, +// hung off a root command so cobra routes args to the group the way it does in +// the real binary. +func newTestGroup(out, errOut *bytes.Buffer) (*cobra.Command, *cobra.Command) { + root := &cobra.Command{Use: "omni"} + root.SilenceErrors = true + group := NewGroupCommand("models", "models commands") + group.AddCommand(&cobra.Command{Use: "list", Short: "list models", RunE: func(*cobra.Command, []string) error { return nil }}) + group.AddCommand(&cobra.Command{Use: "get ", Short: "get a model", RunE: func(*cobra.Command, []string) error { return nil }}) + root.AddCommand(group) + root.SetOut(out) + root.SetErr(errOut) + return root, group +} + +// A mistyped subcommand must be a hard error with suggestions — not a silent +// help dump on stdout that a piped consumer would try to parse as data. +func TestGroupRunE_UnknownSubcommand(t *testing.T) { + var out, errOut bytes.Buffer + root, group := newTestGroup(&out, &errOut) + root.SetArgs([]string{"models", "list-branches"}) + + err := root.Execute() + if err == nil { + t.Fatal("expected an error for an unknown subcommand") + } + if !strings.Contains(err.Error(), `unknown subcommand "list-branches" for "omni models"`) { + t.Errorf("error = %q, want an unknown-subcommand message", err.Error()) + } + // "list-branches" is too far from "list" for Levenshtein, but the + // reverse-prefix pass should still point there. + if !strings.Contains(err.Error(), "Did you mean this?") || !strings.Contains(err.Error(), "list") { + t.Errorf("error = %q, want a suggestion for 'list'", err.Error()) + } + if !strings.Contains(err.Error(), "omni models --help") { + t.Errorf("error = %q, want a --help hint", err.Error()) + } + if out.Len() != 0 { + t.Errorf("stdout should be empty, got %q", out.String()) + } + if !group.SilenceUsage { + t.Error("SilenceUsage should be set so the error isn't buried in usage text") + } +} + +// Nothing similar to suggest: still an error, still nothing on stdout. +func TestGroupRunE_UnknownSubcommandNoSuggestions(t *testing.T) { + var out, errOut bytes.Buffer + root, _ := newTestGroup(&out, &errOut) + root.SetArgs([]string{"models", "zzzzzzzz"}) + + err := root.Execute() + if err == nil { + t.Fatal("expected an error for an unknown subcommand") + } + if strings.Contains(err.Error(), "Did you mean this?") { + t.Errorf("error = %q, want no suggestion block", err.Error()) + } + if out.Len() != 0 { + t.Errorf("stdout should be empty, got %q", out.String()) + } +} + +// A bare group produced no data, so its help goes to stderr and the exit code +// is non-zero — stdout stays empty for the pipe. +func TestGroupRunE_NoSubcommand(t *testing.T) { + var out, errOut bytes.Buffer + root, _ := newTestGroup(&out, &errOut) + root.SetArgs([]string{"models"}) + + err := root.Execute() + if err == nil { + t.Fatal("expected a non-nil error so the CLI exits non-zero") + } + if !strings.Contains(err.Error(), "requires a subcommand") { + t.Errorf("error = %q, want a 'requires a subcommand' message", err.Error()) + } + if out.Len() != 0 { + t.Errorf("stdout should be empty, got %q", out.String()) + } + if !strings.Contains(errOut.String(), "Available Commands") { + t.Errorf("stderr = %q, want the group help", errOut.String()) + } +} + +// The bare-group help IS the error report, so cobra must not append its own +// "Error: ..." line on top of it — one failure, one surface. +func TestGroupRunE_NoSubcommandReportsErrorOnce(t *testing.T) { + var out, errOut bytes.Buffer + root := &cobra.Command{Use: "omni"} + group := NewGroupCommand("models", "models commands") + group.AddCommand(&cobra.Command{Use: "list", Short: "list models", RunE: func(*cobra.Command, []string) error { return nil }}) + root.AddCommand(group) + root.SetOut(&out) + root.SetErr(&errOut) + root.SetArgs([]string{"models"}) + + if err := root.Execute(); err == nil { + t.Fatal("expected a non-nil error so the CLI exits non-zero") + } + if strings.Contains(errOut.String(), "Error:") { + t.Errorf("stderr = %q, want no duplicate cobra error line alongside the help", errOut.String()) + } + if n := strings.Count(errOut.String(), "Usage:"); n != 1 { + t.Errorf("stderr has %d usage blocks, want exactly 1: %q", n, errOut.String()) + } + if out.Len() != 0 { + t.Errorf("stdout should be empty, got %q", out.String()) + } +} + +// --help is not an error: it still prints to stdout and exits zero. +func TestGroupRunE_HelpFlagStaysOnStdout(t *testing.T) { + var out, errOut bytes.Buffer + root, _ := newTestGroup(&out, &errOut) + root.SetArgs([]string{"models", "--help"}) + + if err := root.Execute(); err != nil { + t.Fatalf("--help should not error: %v", err) + } + if !strings.Contains(out.String(), "Available Commands") { + t.Errorf("stdout = %q, want the group help", out.String()) + } + if errOut.Len() != 0 { + t.Errorf("stderr should be empty, got %q", errOut.String()) + } +} + +// A typo plus --help is still a typo. Cobra answers the help flag before RunE, +// so this is the one path that could still print help to stdout and exit 0. +func TestGroupHelp_UnknownSubcommandWithHelpFlag(t *testing.T) { + var out, errOut bytes.Buffer + root, group := newTestGroup(&out, &errOut) + root.SetArgs([]string{"models", "list-branches", "--help"}) + + // Cobra always returns nil once it has handled the help flag, which is why + // the caller has to consult UnknownSubcommand for the exit code. + if err := root.Execute(); err != nil { + t.Fatalf("unexpected error from Execute: %v", err) + } + if !UnknownSubcommand(group) { + t.Error("UnknownSubcommand should report the typo so the CLI exits non-zero") + } + if out.Len() != 0 { + t.Errorf("stdout should be empty, got %q", out.String()) + } + if !strings.Contains(errOut.String(), `unknown subcommand "list-branches"`) { + t.Errorf("stderr = %q, want an unknown-subcommand error", errOut.String()) + } + if !strings.Contains(errOut.String(), "Did you mean this?") { + t.Errorf("stderr = %q, want a suggestion", errOut.String()) + } +} + +// The group's help func is inherited by its subcommands, so `--help` on a real +// subcommand must still render normal help on stdout (and not recurse). +func TestGroupHelp_SubcommandHelpUnaffected(t *testing.T) { + var out, errOut bytes.Buffer + root, group := newTestGroup(&out, &errOut) + root.SetArgs([]string{"models", "list", "--help"}) + + if err := root.Execute(); err != nil { + t.Fatalf("--help should not error: %v", err) + } + if UnknownSubcommand(group) { + t.Error("a valid subcommand should not be reported as unknown") + } + if !strings.Contains(out.String(), "list models") || !strings.Contains(out.String(), "Usage:") { + t.Errorf("stdout = %q, want the subcommand's help", out.String()) + } + if errOut.Len() != 0 { + t.Errorf("stderr should be empty, got %q", errOut.String()) + } +} + +// Every generated group gets the unknown-subcommand handling, not just the +// ones someone remembered to wire up. +func TestGenerateCommands_GroupsAreRunnable(t *testing.T) { + specData := loadSpec(t) + cmds, err := GenerateCommands(specData, func(req APIRequest) error { return nil }) + if err != nil { + t.Fatalf("GenerateCommands: %v", err) + } + for _, tagCmd := range cmds { + if tagCmd.RunE == nil { + t.Errorf("group %q has no RunE; an unknown subcommand would exit 0", tagCmd.Use) + } + if !isGroup(tagCmd) { + t.Errorf("group %q is missing the group annotation; --help after a typo would exit 0", tagCmd.Use) + } + } +} + // Every generated command must accept --schema (and its --field/--depth // refinements), including bodyless GET/DELETE ones. Agents reach for --schema // first, so a command missing the flag costs a wasted call. diff --git a/internal/output/human.go b/internal/output/human.go index 9003d1e..7cf2338 100644 --- a/internal/output/human.go +++ b/internal/output/human.go @@ -1,6 +1,7 @@ package output import ( + "bytes" "encoding/json" "fmt" "io" @@ -26,7 +27,14 @@ func HumanTo(w io.Writer, body io.Reader) error { if err != nil { return fmt.Errorf("reading response: %w", err) } - if len(strings.TrimSpace(string(data))) == 0 { + return HumanBytes(w, data) +} + +// HumanBytes renders an already-read JSON body to w. Callers that have the +// bytes in hand use this so a large payload isn't read — and allocated — a +// second time. +func HumanBytes(w io.Writer, data []byte) error { + if len(bytes.TrimSpace(data)) == 0 { fmt.Fprintln(w, "✓ ok") return nil } diff --git a/internal/output/output.go b/internal/output/output.go index 8a39832..26af716 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -2,6 +2,7 @@ package output import ( + "bytes" "encoding/json" "fmt" "io" @@ -19,38 +20,69 @@ func JSONTo(w io.Writer, body io.Reader, compact bool) error { if err != nil { return fmt.Errorf("reading response: %w", err) } + return JSONBytes(w, data, compact) +} +// JSONBytes writes an already-read body as formatted JSON to w. Callers that +// have the bytes in hand use this so a large payload isn't read — and +// allocated — a second time. +func JSONBytes(w io.Writer, data []byte, compact bool) error { if compact { - _, err = w.Write(data) - if err != nil { + if _, err := w.Write(data); err != nil { return err } fmt.Fprintln(w) return nil } - // Pretty-print - var raw json.RawMessage - if err := json.Unmarshal(data, &raw); err != nil { - // Not JSON — print raw - _, err = w.Write(data) + // Pretty-print. json.Indent reformats the bytes in place rather than + // building a value first, so the body is parsed once. + var pretty bytes.Buffer + if err := json.Indent(&pretty, data, "", " "); err != nil { + // Not JSON — print raw. + _, err := w.Write(data) fmt.Fprintln(w) return err } - pretty, err := json.MarshalIndent(raw, "", " ") - if err != nil { - _, err = w.Write(data) - fmt.Fprintln(w) + if _, err := w.Write(bytes.TrimRight(pretty.Bytes(), "\n")); err != nil { return err } + fmt.Fprintln(w) + return nil +} + +// APIErrorTo writes a single JSON document describing a failed API call to w. +// The envelope carries the human-readable detail and the HTTP status, plus the +// API's own error payload verbatim under "body" when it was JSON. +// +// A failed invocation must leave exactly one JSON document on stderr, so +// callers must not print anything else alongside it — a trailing "Error: ..." +// line would make `omni ... 2>err.json` unparseable. +func APIErrorTo(w io.Writer, statusCode int, detail string, body json.RawMessage, compact bool) { + msg := struct { + Error string `json:"error"` + Status int `json:"status,omitempty"` + Body json.RawMessage `json:"body,omitempty"` + }{ + Error: detail, + Status: statusCode, + Body: body, + } - _, err = w.Write(pretty) + data, err := json.Marshal(msg) if err != nil { - return err + // The detail came from the response body and is always a valid string, + // so this is unreachable in practice; fall back to the plain envelope. + ErrorTo(w, statusCode, fmt.Sprintf("HTTP %d", statusCode)) + return } - fmt.Fprintln(w) - return nil + if !compact { + if pretty, err := json.MarshalIndent(msg, "", " "); err == nil { + data = pretty + } + } + fmt.Fprintln(w, string(data)) } // Error prints a JSON error to stderr. diff --git a/internal/output/output_test.go b/internal/output/output_test.go index 5a0a249..52f5e4a 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -2,6 +2,7 @@ package output import ( "bytes" + "encoding/json" "strings" "testing" ) @@ -54,6 +55,51 @@ func TestJSONTo_InvalidJSON(t *testing.T) { } } +// A failed API call leaves exactly one JSON document: the detail, the status, +// and the API's own payload verbatim under "body". +func TestAPIErrorTo_SingleJSONDocument(t *testing.T) { + for _, compact := range []bool{true, false} { + var buf bytes.Buffer + APIErrorTo(&buf, 400, "bad request", json.RawMessage(`{"detail":"bad request","code":"X"}`), compact) + + var got struct { + Error string `json:"error"` + Status int `json:"status"` + Body json.RawMessage `json:"body"` + } + if err := json.Unmarshal(buf.Bytes(), &got); err != nil { + t.Fatalf("compact=%v: output is not a single JSON document (%v): %q", compact, err, buf.String()) + } + if got.Error != "bad request" || got.Status != 400 { + t.Errorf("compact=%v: got %+v, want detail and status", compact, got) + } + var body map[string]any + if err := json.Unmarshal(got.Body, &body); err != nil { + t.Fatalf("compact=%v: body is not JSON (%v): %q", compact, err, string(got.Body)) + } + if body["code"] != "X" { + t.Errorf("compact=%v: body = %q, want the API's payload", compact, string(got.Body)) + } + // Pretty mode indents; compact mode stays on one line. + if indented := strings.Contains(buf.String(), "\n "); indented == compact { + t.Errorf("compact=%v: unexpected formatting: %q", compact, buf.String()) + } + } +} + +// With no JSON payload to embed, "body" is omitted rather than emitted as null. +func TestAPIErrorTo_OmitsMissingBody(t *testing.T) { + var buf bytes.Buffer + APIErrorTo(&buf, 502, "Bad Gateway", nil, true) + if strings.Contains(buf.String(), "body") { + t.Errorf("expected no body field, got %q", buf.String()) + } + var got map[string]any + if err := json.Unmarshal(buf.Bytes(), &got); err != nil { + t.Fatalf("output is not valid JSON (%v): %q", err, buf.String()) + } +} + // Error responses are written to stderr as JSON with "error" and "status" fields. func TestErrorTo_Format(t *testing.T) { var buf bytes.Buffer