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

ektasawant · ektasawant · commit dd9fbb5f155c · 2026-07-09T08:40:32.000+02:00
diff --git a/main.go b/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,11 @@ func run() (exitCode int) {
 		return 1
 	}
 	code := cli.GetExitCode()
+	// A non-JSON error page can arrive with a 2xx status, which restish would
+	// treat as success. Force a non-zero exit so callers detect the failure.
+	if code == 0 && nonJSONErrorResponse {
+		code = 1
+	}
 	maybeHintDoerContext(code, cli.GetLastStatus(), configDir)
 	return code
 }
@@ -1418,6 +1425,104 @@ func overrideTableOutput() {
 	cli.AddContentType("toon", "", -1, &dciToonContentType{})
 }
 
+// nonJSONErrorResponse records that the last API response was a non-JSON error
+// page (typically HTML) so run() can force a non-zero exit even when the HTTP
+// status was 2xx. Reset at the start of each run().
+var nonJSONErrorResponse bool
+
+// dciResponseGuard wraps restish's response formatter to catch non-JSON error
+// pages before they are printed. The DCI API is JSON-only, but an upstream
+// timeout or maintenance window can surface the Cloudflare edge's HTML error
+// page (e.g. a 524 rendered as the DoiT maintenance page). Restish would dump
+// that raw HTML to stdout, which is confusing and hard to script against, so we
+// replace it with a clear, actionable 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
+	}
+	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 parsed response is an HTML page rather than
+// the JSON the DCI API is expected to return. It checks the Content-Type header
+// and, because edge error pages sometimes carry a misleading or absent type,
+// sniffs the body for an HTML prefix. Streaming (text/event-stream) and JSON
+// responses are intentionally 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.ToLower(strings.TrimSpace(s))
+	return strings.HasPrefix(s, "<!doctype html") ||
+		strings.HasPrefix(s, "<html") ||
+		strings.HasPrefix(s, "<head") ||
+		strings.HasPrefix(s, "<body")
+}
+
+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")
+	fmt.Fprint(os.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) {
diff --git a/main_test.go b/main_test.go
@@ -2011,3 +2011,163 @@ func TestCustomerContextFlag(t *testing.T) {
 		}
 	})
 }
+
+func TestIsHTMLErrorPage(t *testing.T) {
+	tests := []struct {
+		name string
+		resp cli.Response
+		want bool
+	}{
+		{
+			name: "text/html content type",
+			resp: cli.Response{Headers: map[string]string{"Content-Type": "text/html; charset=utf-8"}, Body: "anything"},
+			want: true,
+		},
+		{
+			name: "doctype body without content type",
+			resp: cli.Response{Body: "<!DOCTYPE html><html><head><title>DoiT Console - Maintenance</title></head></html>"},
+			want: true,
+		},
+		{
+			name: "html body with leading whitespace",
+			resp: cli.Response{Body: "\n  <html><body>oops</body></html>"},
+			want: true,
+		},
+		{
+			name: "html body as bytes",
+			resp: cli.Response{Body: []byte("<head>...</head>")},
+			want: true,
+		},
+		{
+			name: "json object body is not html",
+			resp: cli.Response{Headers: map[string]string{"Content-Type": "application/json"}, Body: map[string]interface{}{"answer": "hi"}},
+			want: false,
+		},
+		{
+			name: "json string body is not html",
+			resp: cli.Response{Body: `{"answer":"hi"}`},
+			want: false,
+		},
+		{
+			name: "streaming SSE body is not html",
+			resp: cli.Response{Headers: map[string]string{"Content-Type": "text/event-stream"}, Body: "data: {\"chunk\":1}"},
+			want: false,
+		},
+		{
+			name: "empty response is not html",
+			resp: cli.Response{},
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isHTMLErrorPage(tt.resp); got != tt.want {
+				t.Errorf("isHTMLErrorPage() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestTraceID(t *testing.T) {
+	tests := []struct {
+		name    string
+		headers map[string]string
+		want    string
+	}{
+		{name: "cf-ray preferred", headers: map[string]string{"Cf-Ray": "a023e448ca101c99", "X-Request-Id": "abc"}, want: "Cf-Ray=a023e448ca101c99"},
+		{name: "case insensitive lookup", headers: map[string]string{"cf-ray": "deadbeef"}, want: "Cf-Ray=deadbeef"},
+		{name: "falls back to request id", headers: map[string]string{"X-Request-Id": "req-123"}, want: "X-Request-Id=req-123"},
+		{name: "none present", headers: map[string]string{"Content-Type": "text/html"}, want: ""},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := traceID(tt.headers); got != tt.want {
+				t.Errorf("traceID() = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}
+
+// recordingFormatter is a test double capturing whether the wrapped formatter
+// was delegated to.
+type recordingFormatter struct {
+	called bool
+	got    cli.Response
+}
+
+func (r *recordingFormatter) Format(resp cli.Response) error {
+	r.called = true
+	r.got = resp
+	return nil
+}
+
+func TestResponseGuardFormat(t *testing.T) {
+	t.Run("html page prints message and sets flag without delegating", func(t *testing.T) {
+		nonJSONErrorResponse = false
+		t.Cleanup(func() { nonJSONErrorResponse = false })
+
+		next := &recordingFormatter{}
+		guard := dciResponseGuard{next: next}
+
+		r, w, _ := os.Pipe()
+		oldStderr := os.Stderr
+		os.Stderr = w
+
+		err := guard.Format(cli.Response{
+			Status:  524,
+			Headers: map[string]string{"Content-Type": "text/html", "Cf-Ray": "a023e448ca101c99"},
+			Body:    "<!DOCTYPE html><html><head><title>DoiT Console - Maintenance</title></head></html>",
+		})
+
+		w.Close()
+		os.Stderr = oldStderr
+		buf := make([]byte, 4096)
+		n, _ := r.Read(buf)
+		output := string(buf[:n])
+		r.Close()
+
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if next.called {
+			t.Fatal("expected guard not to delegate to the wrapped formatter for HTML")
+		}
+		if !nonJSONErrorResponse {
+			t.Fatal("expected nonJSONErrorResponse to be set")
+		}
+		if !strings.Contains(output, "non-JSON") {
+			t.Errorf("expected message to mention non-JSON, got:\n%s", output)
+		}
+		if !strings.Contains(output, "HTTP status: 524") {
+			t.Errorf("expected message to include HTTP status, got:\n%s", output)
+		}
+		if !strings.Contains(output, "Cf-Ray=a023e448ca101c99") {
+			t.Errorf("expected message to include trace, got:\n%s", output)
+		}
+	})
+
+	t.Run("json response delegates to wrapped formatter", func(t *testing.T) {
+		nonJSONErrorResponse = false
+		t.Cleanup(func() { nonJSONErrorResponse = false })
+
+		next := &recordingFormatter{}
+		guard := dciResponseGuard{next: next}
+
+		resp := cli.Response{
+			Status:  200,
+			Headers: map[string]string{"Content-Type": "application/json"},
+			Body:    map[string]interface{}{"answer": "hello"},
+		}
+		if err := guard.Format(resp); err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if !next.called {
+			t.Fatal("expected guard to delegate to the wrapped formatter for JSON")
+		}
+		if nonJSONErrorResponse {
+			t.Fatal("expected nonJSONErrorResponse to stay false for JSON")
+		}
+	})
+}
