fix: show clear error and non-zero exit on non-JSON API responses

main.go

@@ -221,6 +221,7 @@ func main() {
 func run() (exitCode int) {
 	// Reset per-invocation state so repeated calls (e.g. in tests) start clean.
 	customerContextFlagValue = ""
+	nonJSONErrorResponse = false
 
 	// Resolve agent mode once up front. Downstream behavior — color, default
 	// output format, stderr routing, and the User-Agent mode token — all key off
@@ -261,6 +262,7 @@ func run() (exitCode int) {
 	cli.Init("dci", version)
 	cli.Defaults()
 	overrideTableOutput()
+	installResponseGuard()
 	registerAgentFlags()
 	printFirstRunOnboarding(configured)
 	maybeHintAgentMode()
@@ -313,6 +315,10 @@ func run() (exitCode int) {
 		return 1
 	}
 	code := cli.GetExitCode()
+	// Force a non-zero exit when a 2xx response carried an error page/body.
+	if code == 0 && nonJSONErrorResponse {
+		code = 1
+	}
 	maybeHintDoerContext(code, cli.GetLastStatus(), configDir)
 	return code
 }
@@ -1222,14 +1228,15 @@ func registerAuthCommands(configDir string) {
 			if os.Getenv("DCI_API_KEY") != "" {
 				return fmt.Errorf("login is not needed when DCI_API_KEY is set")
 			}
-			// Trigger the OAuth flow by calling a lightweight endpoint.
-			// Suppress the validate response body — login only needs the OAuth
-			// side effect (token cached), not the API output.
+			// Trigger the OAuth flow via validate. This call is internal (login
+			// only needs the cached token), so isolate all its observable effects
+			// — stdout, guard stderr, guard exit-code flag — from the login result.
 			os.Args = []string{os.Args[0], "dci", "validate"}
-			oldOut := cli.Stdout
-			cli.Stdout = io.Discard
+			oldOut, oldErr := cli.Stdout, cli.Stderr
+			cli.Stdout, cli.Stderr = io.Discard, io.Discard
 			err := cli.Run()
-			cli.Stdout = oldOut
+			cli.Stdout, cli.Stderr = oldOut, oldErr
+			nonJSONErrorResponse = false
 
 			// Auto-configure DoiT employees who have no customer context set.
 			// The validate endpoint requires customerContext for @doit.com accounts,
@@ -1418,6 +1425,153 @@ func overrideTableOutput() {
 	cli.AddContentType("toon", "", -1, &dciToonContentType{})
 }
 
+// nonJSONErrorResponse flags that the last API response was an error page/body
+// so run() can force a non-zero exit even on a 2xx status. Reset each run().
+var nonJSONErrorResponse bool
+
+// dciResponseGuard wraps restish's formatter to catch error responses the DCI
+// API can leak as non-JSON HTML (e.g. a Cloudflare 524 maintenance page) or as
+// a JSON error body under a locked 2xx status, replacing restish's success
+// handling with a clear message and a non-zero exit code.
+type dciResponseGuard struct {
+	next cli.ResponseFormatter
+}
+
+func (g dciResponseGuard) Format(resp cli.Response) error {
+	if isHTMLErrorPage(resp) {
+		nonJSONErrorResponse = true
+		printNonJSONError(resp)
+		return nil
+	}
+	if msg, ok := jsonApplicationError(resp); ok {
+		nonJSONErrorResponse = true
+		if err := g.next.Format(resp); err != nil {
+			return err
+		}
+		// cli.Stderr (not os.Stderr) so callers like login can suppress it; don't revert.
+		fmt.Fprintf(cli.Stderr, "Error: the DoiT API returned an application error: %s\n", msg)
+		return nil
+	}
+	return g.next.Format(resp)
+}
+
+// installResponseGuard wraps the active restish formatter. It must run after
+// cli.Defaults() (which sets cli.Formatter) and overrideTableOutput().
+func installResponseGuard() {
+	if cli.Formatter == nil {
+		return
+	}
+	cli.Formatter = dciResponseGuard{next: cli.Formatter}
+}
+
+// isHTMLErrorPage reports whether a response is HTML rather than JSON, checking
+// the Content-Type and sniffing the body (edge error pages can carry a
+// misleading or absent type). JSON and SSE responses are left untouched.
+func isHTMLErrorPage(resp cli.Response) bool {
+	if strings.Contains(strings.ToLower(headerValue(resp.Headers, "Content-Type")), "text/html") {
+		return true
+	}
+	switch b := resp.Body.(type) {
+	case string:
+		return bodyLooksLikeHTML(b)
+	case []byte:
+		return bodyLooksLikeHTML(string(b))
+	}
+	return false
+}
+
+func bodyLooksLikeHTML(s string) bool {
+	s = strings.TrimSpace(strings.TrimPrefix(s, "\ufeff")) // strip UTF-8 BOM some proxies prepend
+	if len(s) > 16 {
+		s = s[:16] // only the prefix matters; avoid lowercasing the whole body
+	}
+	s = strings.ToLower(s)
+	return strings.HasPrefix(s, "<!doctype html") ||
+		strings.HasPrefix(s, "<html") ||
+		strings.HasPrefix(s, "<head") ||
+		strings.HasPrefix(s, "<body") ||
+		strings.HasPrefix(s, "<?xml") ||
+		strings.HasPrefix(s, "<!--")
+}
+
+// jsonApplicationError reports whether a 2xx response carries a non-empty
+// top-level JSON `error` field (e.g. an AVA askSync failure under a locked 2xx
+// status), returning a human-readable message. Such a body is not a valid DCI
+// success shape, so it is treated as a failure.
+//
+// The object-only, top-level-only scoping is load-bearing, verified against the
+// OpenAPI spec: the only other 2xx body carrying a top-level `error` is
+// POST /insights/v1/results, whose 200 is a top-level *array* of per-row error
+// items. The map assertion below lets that array pass through untouched. Do not
+// recurse into arrays or nested objects here, or it will false-positive on
+// legitimate partial-result responses.
+func jsonApplicationError(resp cli.Response) (string, bool) {
+	if resp.Status < 200 || resp.Status >= 300 {
+		return "", false
+	}
+	body, ok := resp.Body.(map[string]interface{})
+	if !ok {
+		return "", false
+	}
+	switch v := body["error"].(type) {
+	case string:
+		if strings.TrimSpace(v) == "" {
+			return "", false
+		}
+		return v, true
+	case map[string]interface{}:
+		// Defensive only: no current DCI endpoint returns an object-typed error (AVA returns a string).
+		if len(v) == 0 {
+			return "", false
+		}
+		if m, ok := v["message"].(string); ok && strings.TrimSpace(m) != "" {
+			return m, true
+		}
+		return "application error", true
+	}
+	return "", false
+}
+
+func printNonJSONError(resp cli.Response) {
+	var b strings.Builder
+	b.WriteString("Error: the DoiT API returned a non-JSON (HTML) response.\n")
+	b.WriteString("This usually means an upstream timeout or maintenance window rather than a problem with your request.\n")
+	if resp.Status != 0 {
+		fmt.Fprintf(&b, "HTTP status: %d\n", resp.Status)
+	}
+	if trace := traceID(resp.Headers); trace != "" {
+		fmt.Fprintf(&b, "Trace: %s\n", trace)
+	}
+	b.WriteString("Please retry in a moment. If it persists, contact DoiT support with the trace above.\n")
+	// cli.Stderr (not os.Stderr) so internal callers like login can redirect it.
+	fmt.Fprint(cli.Stderr, b.String())
+}
+
+// traceID returns the first available upstream trace identifier so users can
+// reference it in a support request.
+func traceID(headers map[string]string) string {
+	for _, name := range []string{"Cf-Ray", "X-Doit-Trace", "X-Request-Id", "X-Cloud-Trace-Context", "Traceparent"} {
+		if v := strings.TrimSpace(headerValue(headers, name)); v != "" {
+			return name + "=" + v
+		}
+	}
+	return ""
+}
+
+// headerValue looks up a header case-insensitively. Restish canonicalizes
+// header keys, but sniffing defensively keeps this robust across sources.
+func headerValue(headers map[string]string, name string) string {
+	if v, ok := headers[name]; ok {
+		return v
+	}
+	for k, v := range headers {
+		if strings.EqualFold(k, name) {
+			return v
+		}
+	}
+	return ""
+}
+
 func (t dciTableContentType) Detect(contentType string) bool { return false }
 
 func (t dciTableContentType) Marshal(value interface{}) ([]byte, error) {