trim inline comment

Author: ektasawant

main.go

@@ -315,8 +315,7 @@ 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.
+	// Force a non-zero exit when a 2xx response carried an error page/body.
 	if code == 0 && nonJSONErrorResponse {
 		code = 1
 	}
@@ -1425,17 +1424,14 @@ 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().
+// 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 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.
+// 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
 }
@@ -1446,6 +1442,15 @@ func (g dciResponseGuard) Format(resp cli.Response) error {
 		printNonJSONError(resp)
 		return nil
 	}
+	if msg, ok := jsonApplicationError(resp); ok {
+		// Print the structured error body as usual, then flag a non-zero exit.
+		nonJSONErrorResponse = true
+		if err := g.next.Format(resp); err != nil {
+			return err
+		}
+		fmt.Fprintf(os.Stderr, "Error: the DoiT API returned an application error: %s\n", msg)
+		return nil
+	}
 	return g.next.Format(resp)
 }
 
@@ -1458,11 +1463,9 @@ func installResponseGuard() {
 	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.
+// 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
@@ -1484,6 +1487,36 @@ func bodyLooksLikeHTML(s string) bool {
 		strings.HasPrefix(s, "<body")
 }
 
+// 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.
+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{}:
+		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")

main_test.go

@@ -2170,4 +2170,107 @@ func TestResponseGuardFormat(t *testing.T) {
 			t.Fatal("expected nonJSONErrorResponse to stay false for JSON")
 		}
 	})
+
+	t.Run("2xx json error body prints body and flags non-zero exit", 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:  200,
+			Headers: map[string]string{"Content-Type": "application/json"},
+			Body:    map[string]interface{}{"error": "generation failed midway"},
+		})
+
+		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 to still print the structured error body via the wrapped formatter")
+		}
+		if !nonJSONErrorResponse {
+			t.Fatal("expected nonJSONErrorResponse to be set for a 2xx JSON error body")
+		}
+		if !strings.Contains(output, "application error") || !strings.Contains(output, "generation failed midway") {
+			t.Errorf("expected stderr to frame the application error, got:\n%s", output)
+		}
+	})
+}
+
+func TestJSONApplicationError(t *testing.T) {
+	tests := []struct {
+		name    string
+		resp    cli.Response
+		wantMsg string
+		wantOK  bool
+	}{
+		{
+			name:    "2xx with string error",
+			resp:    cli.Response{Status: 200, Body: map[string]interface{}{"error": "boom"}},
+			wantMsg: "boom",
+			wantOK:  true,
+		},
+		{
+			name:    "2xx with object error and message",
+			resp:    cli.Response{Status: 200, Body: map[string]interface{}{"error": map[string]interface{}{"message": "nested boom"}}},
+			wantMsg: "nested boom",
+			wantOK:  true,
+		},
+		{
+			name:    "2xx with object error without message",
+			resp:    cli.Response{Status: 200, Body: map[string]interface{}{"error": map[string]interface{}{"code": 42}}},
+			wantMsg: "application error",
+			wantOK:  true,
+		},
+		{
+			name:   "2xx success body is ignored",
+			resp:   cli.Response{Status: 200, Body: map[string]interface{}{"answer": "hi"}},
+			wantOK: false,
+		},
+		{
+			name:   "2xx empty string error is ignored",
+			resp:   cli.Response{Status: 200, Body: map[string]interface{}{"error": ""}},
+			wantOK: false,
+		},
+		{
+			name:   "2xx null error is ignored",
+			resp:   cli.Response{Status: 200, Body: map[string]interface{}{"error": nil}},
+			wantOK: false,
+		},
+		{
+			name:   "non-2xx error is left to restish",
+			resp:   cli.Response{Status: 400, Body: map[string]interface{}{"error": "bad request"}},
+			wantOK: false,
+		},
+		{
+			name:   "non-object body is ignored",
+			resp:   cli.Response{Status: 200, Body: "just a string"},
+			wantOK: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			msg, ok := jsonApplicationError(tt.resp)
+			if ok != tt.wantOK {
+				t.Fatalf("jsonApplicationError() ok = %v, want %v", ok, tt.wantOK)
+			}
+			if ok && msg != tt.wantMsg {
+				t.Errorf("jsonApplicationError() msg = %q, want %q", msg, tt.wantMsg)
+			}
+		})
+	}
 }