From aebc30e5fa4344a94811899c369d2ab71ba97d26 Mon Sep 17 00:00:00 2001
From: Marko Bevc <marko@kosli.com>
Date: Fri, 6 Nov 2026 20:53:30 +0000
Subject: [PATCH 1/7] fix: report the HTTP status when retries run out

When the retry client exhausted its retries on a 409, 429 or 5xx, its
default error handler drained the last response and returned only
"giving up after N attempt(s)". A lock conflict, a rate limit and a
server failure were indistinguishable from the CLI output, unlike every
other non-2xx path, which already reports the status.

A custom ErrorHandler now appends "HTTP <status>" to the giving-up
error. It also drops the method and URL the default handler repeated,
because the *url.Error that http.Client wraps the error in already
names them. Transport errors keep their underlying cause.
---
 internal/evaluations/client_test.go |  5 +++--
 internal/requests/requests.go       | 17 +++++++++++++++++
 internal/requests/requests_test.go  | 21 +++++++++++++++++++--
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/internal/evaluations/client_test.go b/internal/evaluations/client_test.go
index dab5e7543..b6c6eb6bf 100644
--- a/internal/evaluations/client_test.go
+++ b/internal/evaluations/client_test.go
@@ -221,8 +221,8 @@ func TestCreateSurfacesTheServerMessageAndStatus(t *testing.T) {
 }
 
 // A 5xx is retryable, so the shared HTTP client consumes it: it exhausts the
-// retries and reports giving up, discarding the response and the server's
-// message with it. Pinned because the enqueue failure the API documents at 503
+// retries and reports giving up with the status alone, discarding the server's
+// message. Pinned because the enqueue failure the API documents at 503
 // therefore cannot reach a user as its own sentence, and any caller wanting to
 // name that failure has to say something of its own instead.
 func TestCreateLosesTheServerMessageOnAServerError(t *testing.T) {
@@ -238,6 +238,7 @@ func TestCreateLosesTheServerMessageOnAServerError(t *testing.T) {
 	require.False(t, errors.As(err, &apiError), "a 5xx does not arrive as an API error")
 	require.NotContains(t, err.Error(), "could not be queued")
 	require.Contains(t, err.Error(), "giving up after")
+	require.Contains(t, err.Error(), "HTTP 503")
 }
 
 func TestCreateSendsNothingOnADryRun(t *testing.T) {
diff --git a/internal/requests/requests.go b/internal/requests/requests.go
index bc60ec893..fe95031bd 100644
--- a/internal/requests/requests.go
+++ b/internal/requests/requests.go
@@ -149,6 +149,7 @@ func NewKosliClient(httpProxyURL string, maxAPIRetries int, debug bool, logger *
 	retryClient := retryablehttp.NewClient()
 	retryClient.RetryMax = maxAPIRetries
 	retryClient.CheckRetry = customCheckRetry
+	retryClient.ErrorHandler = giveUpErrorHandler
 	if debug {
 		retryClient.Logger = &CustomLogger{
 			Logger: log.New(os.Stderr, "[debug]", log.Lmsgprefix),
@@ -546,6 +547,22 @@ func isSuccess(status int) bool {
 	return false
 }
 
+// giveUpErrorHandler reports retries running out. It names the status of the
+// last response, which the retry client's default handler drops, and leaves
+// the method and URL to the *url.Error that http.Client wraps it in.
+func giveUpErrorHandler(resp *http.Response, err error, numTries int) (*http.Response, error) {
+	if resp == nil {
+		return nil, fmt.Errorf("giving up after %d attempt(s): %w", numTries, err)
+	}
+	// Drained before closing so the connection can be reused.
+	_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
+	_ = resp.Body.Close()
+	if err != nil {
+		return nil, fmt.Errorf("giving up after %d attempt(s): HTTP %d: %w", numTries, resp.StatusCode, err)
+	}
+	return nil, fmt.Errorf("giving up after %d attempt(s): HTTP %d", numTries, resp.StatusCode)
+}
+
 func customCheckRetry(ctx context.Context, resp *http.Response, err error) (bool, error) {
 	// Get the default retry policy for errors and certain status codes.
 	// It will retry on 5xx, 429 (rate limit) and we add 409 (lock conflict) via a custom check.
diff --git a/internal/requests/requests_test.go b/internal/requests/requests_test.go
index 8cece4fb3..d95f0c112 100644
--- a/internal/requests/requests_test.go
+++ b/internal/requests/requests_test.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"net"
 	"net/http"
 	"strings"
 	"testing"
@@ -347,7 +348,7 @@ func (suite *RequestsTestSuite) TestDo() {
 				URL:    suite.fakeService.ResolveURL("/locked/"),
 			},
 			wantError:        true,
-			expectedErrorMsg: fmt.Sprintf("Get \"%s\": GET %s giving up after 2 attempt(s)", suite.fakeService.ResolveURL("/locked/"), suite.fakeService.ResolveURL("/locked/")),
+			expectedErrorMsg: fmt.Sprintf("Get \"%s\": giving up after 2 attempt(s): HTTP 409", suite.fakeService.ResolveURL("/locked/")),
 		},
 		{
 			name: "409 is not retried and is surfaced when DisableConflictRetry is set",
@@ -366,7 +367,7 @@ func (suite *RequestsTestSuite) TestDo() {
 				URL:    suite.fakeService.ResolveURL("/fail/"),
 			},
 			wantError:        true,
-			expectedErrorMsg: fmt.Sprintf("Get \"%s\": GET %s giving up after 2 attempt(s)", suite.fakeService.ResolveURL("/fail/"), suite.fakeService.ResolveURL("/fail/")),
+			expectedErrorMsg: fmt.Sprintf("Get \"%s\": giving up after 2 attempt(s): HTTP 500", suite.fakeService.ResolveURL("/fail/")),
 		},
 		{
 			name: "GET request with invalid URL causes an error",
@@ -676,6 +677,22 @@ func TestRequestsTestSuite(t *testing.T) {
 	suite.Run(t, new(RequestsTestSuite))
 }
 
+func TestDoKeepsTheTransportErrorWhenRetriesRunOut(t *testing.T) {
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	url := "http://" + listener.Addr().String() + "/closed/"
+	require.NoError(t, listener.Close())
+
+	client, err := NewKosliClient("", 1, false, logger.NewStandardLogger())
+	require.NoError(t, err)
+	_, err = client.Do(&RequestParams{Method: http.MethodGet, URL: url})
+
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "giving up after 2 attempt(s): ")
+	require.Contains(t, err.Error(), "connection refused")
+	require.NotContains(t, err.Error(), "HTTP ")
+}
+
 // A misspelled form item type must fail rather than silently drop the part.
 func TestCreateMultipartRequestBodyRejectsAnUnknownItemType(t *testing.T) {
 	_, _, _, err := createMultipartRequestBody([]FormItem{

From bfd65e11de8517a2508e400a1f157b317932a59b Mon Sep 17 00:00:00 2001
From: Marko Bevc <marko@kosli.com>
Date: Fri, 6 Nov 2026 20:53:32 +0000
Subject: [PATCH 2/7] feat: add upward jitter to the API retry backoff

Retries used retryablehttp's default exponential backoff, so clients
that failed together waited exactly as long as each other and collided
again on every retry. Parallel CI jobs attesting to one trail are the
common case: the v2.55.0 release failed when about half of its matrix
jobs gave up at once.

Each wait is now stretched by a random 0-50%, capped at the maximum
wait. Stretching rather than shrinking keeps the total time before
giving up at least what it was, so a server blip the CLI recovers from
today is still recovered from. A wait the server sets through
Retry-After is left untouched, and the number of attempts and the
statuses retried are unchanged.
---
 internal/requests/requests.go      | 15 ++++++++++++
 internal/requests/requests_test.go | 39 ++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/internal/requests/requests.go b/internal/requests/requests.go
index fe95031bd..093aaf2d6 100644
--- a/internal/requests/requests.go
+++ b/internal/requests/requests.go
@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"math/rand/v2"
 	"mime/multipart"
 	"net/http"
 	"net/url"
@@ -17,6 +18,7 @@ import (
 	"reflect"
 	"regexp"
 	"strings"
+	"time"
 
 	retryablehttp "github.com/hashicorp/go-retryablehttp"
 	"github.com/kosli-dev/cli/internal/logger"
@@ -150,6 +152,7 @@ func NewKosliClient(httpProxyURL string, maxAPIRetries int, debug bool, logger *
 	retryClient.RetryMax = maxAPIRetries
 	retryClient.CheckRetry = customCheckRetry
 	retryClient.ErrorHandler = giveUpErrorHandler
+	retryClient.Backoff = jitteredBackoff
 	if debug {
 		retryClient.Logger = &CustomLogger{
 			Logger: log.New(os.Stderr, "[debug]", log.Lmsgprefix),
@@ -563,6 +566,18 @@ func giveUpErrorHandler(resp *http.Response, err error, numTries int) (*http.Res
 	return nil, fmt.Errorf("giving up after %d attempt(s): HTTP %d", numTries, resp.StatusCode)
 }
 
+// jitteredBackoff stretches the default exponential wait by a random 0-50%, so
+// clients that failed together, such as parallel CI jobs writing to one trail,
+// do not retry in lockstep. Stretching rather than shrinking keeps the total
+// time before giving up at least what it was. A Retry-After wait is kept as is.
+func jitteredBackoff(minWait, maxWait time.Duration, attempt int, resp *http.Response) time.Duration {
+	wait := retryablehttp.DefaultBackoff(minWait, maxWait, attempt, resp)
+	if resp != nil && resp.Header.Get("Retry-After") != "" {
+		return wait
+	}
+	return min(wait+rand.N(wait/2+1), maxWait)
+}
+
 func customCheckRetry(ctx context.Context, resp *http.Response, err error) (bool, error) {
 	// Get the default retry policy for errors and certain status codes.
 	// It will retry on 5xx, 429 (rate limit) and we add 409 (lock conflict) via a custom check.
diff --git a/internal/requests/requests_test.go b/internal/requests/requests_test.go
index d95f0c112..b9077ca4c 100644
--- a/internal/requests/requests_test.go
+++ b/internal/requests/requests_test.go
@@ -9,7 +9,9 @@ import (
 	"net/http"
 	"strings"
 	"testing"
+	"time"
 
+	"github.com/hashicorp/go-retryablehttp"
 	"github.com/kosli-dev/cli/internal/logger"
 	"github.com/kosli-dev/cli/internal/version"
 	"github.com/maxcnunes/httpfake"
@@ -677,6 +679,43 @@ func TestRequestsTestSuite(t *testing.T) {
 	suite.Run(t, new(RequestsTestSuite))
 }
 
+func TestJitteredBackoff(t *testing.T) {
+	const minWait, maxWait = time.Second, 30 * time.Second
+
+	t.Run("never waits less than the default backoff", func(t *testing.T) {
+		for attempt := range 4 {
+			wait := retryablehttp.DefaultBackoff(minWait, maxWait, attempt, nil)
+			for range 1000 {
+				got := jitteredBackoff(minWait, maxWait, attempt, nil)
+				require.GreaterOrEqual(t, got, wait)
+				require.LessOrEqual(t, got, wait+wait/2)
+			}
+		}
+	})
+
+	t.Run("spreads the waits of clients that failed together", func(t *testing.T) {
+		seen := map[time.Duration]bool{}
+		for range 100 {
+			seen[jitteredBackoff(minWait, maxWait, 2, nil)] = true
+		}
+		require.Greater(t, len(seen), 1)
+	})
+
+	t.Run("never waits longer than the maximum", func(t *testing.T) {
+		for range 1000 {
+			require.Equal(t, maxWait, jitteredBackoff(minWait, maxWait, 10, nil))
+		}
+	})
+
+	t.Run("keeps the wait the server asked for", func(t *testing.T) {
+		resp := &http.Response{
+			StatusCode: http.StatusTooManyRequests,
+			Header:     http.Header{"Retry-After": []string{"7"}},
+		}
+		require.Equal(t, 7*time.Second, jitteredBackoff(minWait, maxWait, 0, resp))
+	})
+}
+
 func TestDoKeepsTheTransportErrorWhenRetriesRunOut(t *testing.T) {
 	listener, err := net.Listen("tcp", "127.0.0.1:0")
 	require.NoError(t, err)

From f72f2f96cd2f2cc435d6bad9dbeb7cf5e342620d Mon Sep 17 00:00:00 2001
From: Marko Bevc <marko@kosli.com>
Date: Fri, 6 Nov 2026 20:53:33 +0000
Subject: [PATCH 3/7] refactor: consolidate the giving-up handler and trim its
 comments

The handler had three return paths that each rebuilt the same message.
It now builds the message once, adding the status when there is a last
response and the cause when there is an error. The comments on the
handler and the backoff are cut to the facts the code cannot show.
---
 internal/evaluations/client_test.go |  8 ++++----
 internal/requests/requests.go       | 26 ++++++++++++--------------
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/internal/evaluations/client_test.go b/internal/evaluations/client_test.go
index b6c6eb6bf..071679135 100644
--- a/internal/evaluations/client_test.go
+++ b/internal/evaluations/client_test.go
@@ -221,10 +221,10 @@ func TestCreateSurfacesTheServerMessageAndStatus(t *testing.T) {
 }
 
 // A 5xx is retryable, so the shared HTTP client consumes it: it exhausts the
-// retries and reports giving up with the status alone, discarding the server's
-// message. Pinned because the enqueue failure the API documents at 503
-// therefore cannot reach a user as its own sentence, and any caller wanting to
-// name that failure has to say something of its own instead.
+// retries and reports giving up with only the status. Pinned because the
+// enqueue failure the API documents at 503 therefore cannot reach a user as its
+// own sentence, and any caller wanting to name that failure has to say
+// something of its own instead.
 func TestCreateLosesTheServerMessageOnAServerError(t *testing.T) {
 	server, _ := newFakeServer(t, http.StatusServiceUnavailable,
 		`{"message":"Evaluation '01JABCDEF' could not be queued"}`)
diff --git a/internal/requests/requests.go b/internal/requests/requests.go
index 093aaf2d6..ae9f2dff4 100644
--- a/internal/requests/requests.go
+++ b/internal/requests/requests.go
@@ -550,26 +550,24 @@ func isSuccess(status int) bool {
 	return false
 }
 
-// giveUpErrorHandler reports retries running out. It names the status of the
-// last response, which the retry client's default handler drops, and leaves
-// the method and URL to the *url.Error that http.Client wraps it in.
+// giveUpErrorHandler keeps the last response's status, which retryablehttp's
+// default handler drops. The *url.Error that http.Client wraps it in names the
+// method and URL.
 func giveUpErrorHandler(resp *http.Response, err error, numTries int) (*http.Response, error) {
-	if resp == nil {
-		return nil, fmt.Errorf("giving up after %d attempt(s): %w", numTries, err)
+	msg := fmt.Sprintf("giving up after %d attempt(s)", numTries)
+	if resp != nil {
+		_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
+		_ = resp.Body.Close()
+		msg += fmt.Sprintf(": HTTP %d", resp.StatusCode)
 	}
-	// Drained before closing so the connection can be reused.
-	_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
-	_ = resp.Body.Close()
 	if err != nil {
-		return nil, fmt.Errorf("giving up after %d attempt(s): HTTP %d: %w", numTries, resp.StatusCode, err)
+		return nil, fmt.Errorf("%s: %w", msg, err)
 	}
-	return nil, fmt.Errorf("giving up after %d attempt(s): HTTP %d", numTries, resp.StatusCode)
+	return nil, errors.New(msg)
 }
 
-// jitteredBackoff stretches the default exponential wait by a random 0-50%, so
-// clients that failed together, such as parallel CI jobs writing to one trail,
-// do not retry in lockstep. Stretching rather than shrinking keeps the total
-// time before giving up at least what it was. A Retry-After wait is kept as is.
+// jitteredBackoff stretches each wait by a random 0-50%, so clients that failed
+// together do not retry in lockstep, without shortening the time before giving up.
 func jitteredBackoff(minWait, maxWait time.Duration, attempt int, resp *http.Response) time.Duration {
 	wait := retryablehttp.DefaultBackoff(minWait, maxWait, attempt, resp)
 	if resp != nil && resp.Header.Get("Retry-After") != "" {

From 6260489ea33393cf7895d4dc11e570457e92dc11 Mon Sep 17 00:00:00 2001
From: Marko Bevc <marko@kosli.com>
Date: Fri, 6 Nov 2026 20:53:33 +0000
Subject: [PATCH 4/7] fix: jitter retries whose Retry-After the backoff ignores

The backoff skipped the jitter whenever a response carried a
Retry-After header. retryablehttp only honours that header on a 429 or
503, and only when it parses, so a 409 lock conflict from a proxy that
adds the header, or a 429 with an unparseable value, got the plain
exponential wait with no jitter.

The backoff now treats a wait as server-set only when it differs from
the plain exponential wait, which matches the library's own rules
without copying them.
---
 internal/requests/requests.go      |  8 +++++---
 internal/requests/requests_test.go | 17 +++++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/internal/requests/requests.go b/internal/requests/requests.go
index ae9f2dff4..1ecb6fc01 100644
--- a/internal/requests/requests.go
+++ b/internal/requests/requests.go
@@ -568,12 +568,14 @@ func giveUpErrorHandler(resp *http.Response, err error, numTries int) (*http.Res
 
 // jitteredBackoff stretches each wait by a random 0-50%, so clients that failed
 // together do not retry in lockstep, without shortening the time before giving up.
+// A wait that differs from the plain exponential one came from Retry-After and
+// is kept as is.
 func jitteredBackoff(minWait, maxWait time.Duration, attempt int, resp *http.Response) time.Duration {
-	wait := retryablehttp.DefaultBackoff(minWait, maxWait, attempt, resp)
-	if resp != nil && resp.Header.Get("Retry-After") != "" {
+	exponential := retryablehttp.DefaultBackoff(minWait, maxWait, attempt, nil)
+	if wait := retryablehttp.DefaultBackoff(minWait, maxWait, attempt, resp); wait != exponential {
 		return wait
 	}
-	return min(wait+rand.N(wait/2+1), maxWait)
+	return min(exponential+rand.N(exponential/2+1), maxWait)
 }
 
 func customCheckRetry(ctx context.Context, resp *http.Response, err error) (bool, error) {
diff --git a/internal/requests/requests_test.go b/internal/requests/requests_test.go
index b9077ca4c..993a58a9e 100644
--- a/internal/requests/requests_test.go
+++ b/internal/requests/requests_test.go
@@ -707,6 +707,23 @@ func TestJitteredBackoff(t *testing.T) {
 		}
 	})
 
+	for _, resp := range []*http.Response{
+		{StatusCode: http.StatusConflict, Header: http.Header{"Retry-After": []string{"7"}}},
+		{StatusCode: http.StatusTooManyRequests, Header: http.Header{"Retry-After": []string{"soon"}}},
+	} {
+		t.Run(fmt.Sprintf("jitters a %d with a Retry-After the backoff ignores", resp.StatusCode), func(t *testing.T) {
+			wait := retryablehttp.DefaultBackoff(minWait, maxWait, 1, nil)
+			seen := map[time.Duration]bool{}
+			for range 100 {
+				got := jitteredBackoff(minWait, maxWait, 1, resp)
+				require.GreaterOrEqual(t, got, wait)
+				require.LessOrEqual(t, got, wait+wait/2)
+				seen[got] = true
+			}
+			require.Greater(t, len(seen), 1)
+		})
+	}
+
 	t.Run("keeps the wait the server asked for", func(t *testing.T) {
 		resp := &http.Response{
 			StatusCode: http.StatusTooManyRequests,

From ede8bf244468f3d5e0b5a13cb72e8c918533f8a4 Mon Sep 17 00:00:00 2001
From: Marko Bevc <marko@kosli.com>
Date: Fri, 6 Nov 2026 20:53:33 +0000
Subject: [PATCH 5/7] fix: name the status in the giving-up error

"HTTP 409" leaves the reader to know what the code means. The error now
reads "HTTP 409 Conflict" or "HTTP 429 Too Many Requests", which tells a
lock conflict from a rate limit at a glance.

The text comes from http.StatusText rather than resp.Status, so the
server's reason phrase never reaches the output and a hand-built
response without a status line still renders.
---
 internal/requests/requests.go      | 8 +++++++-
 internal/requests/requests_test.go | 9 +++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/internal/requests/requests.go b/internal/requests/requests.go
index 1ecb6fc01..133f598fe 100644
--- a/internal/requests/requests.go
+++ b/internal/requests/requests.go
@@ -558,7 +558,7 @@ func giveUpErrorHandler(resp *http.Response, err error, numTries int) (*http.Res
 	if resp != nil {
 		_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
 		_ = resp.Body.Close()
-		msg += fmt.Sprintf(": HTTP %d", resp.StatusCode)
+		msg += ": " + httpStatus(resp.StatusCode)
 	}
 	if err != nil {
 		return nil, fmt.Errorf("%s: %w", msg, err)
@@ -566,6 +566,12 @@ func giveUpErrorHandler(resp *http.Response, err error, numTries int) (*http.Res
 	return nil, errors.New(msg)
 }
 
+// httpStatus renders a code as "HTTP 409 Conflict", or "HTTP 599" for a code
+// with no standard text.
+func httpStatus(code int) string {
+	return strings.TrimSpace(fmt.Sprintf("HTTP %d %s", code, http.StatusText(code)))
+}
+
 // jitteredBackoff stretches each wait by a random 0-50%, so clients that failed
 // together do not retry in lockstep, without shortening the time before giving up.
 // A wait that differs from the plain exponential one came from Retry-After and
diff --git a/internal/requests/requests_test.go b/internal/requests/requests_test.go
index 993a58a9e..158e169c1 100644
--- a/internal/requests/requests_test.go
+++ b/internal/requests/requests_test.go
@@ -350,7 +350,7 @@ func (suite *RequestsTestSuite) TestDo() {
 				URL:    suite.fakeService.ResolveURL("/locked/"),
 			},
 			wantError:        true,
-			expectedErrorMsg: fmt.Sprintf("Get \"%s\": giving up after 2 attempt(s): HTTP 409", suite.fakeService.ResolveURL("/locked/")),
+			expectedErrorMsg: fmt.Sprintf("Get \"%s\": giving up after 2 attempt(s): HTTP 409 Conflict", suite.fakeService.ResolveURL("/locked/")),
 		},
 		{
 			name: "409 is not retried and is surfaced when DisableConflictRetry is set",
@@ -369,7 +369,7 @@ func (suite *RequestsTestSuite) TestDo() {
 				URL:    suite.fakeService.ResolveURL("/fail/"),
 			},
 			wantError:        true,
-			expectedErrorMsg: fmt.Sprintf("Get \"%s\": giving up after 2 attempt(s): HTTP 500", suite.fakeService.ResolveURL("/fail/")),
+			expectedErrorMsg: fmt.Sprintf("Get \"%s\": giving up after 2 attempt(s): HTTP 500 Internal Server Error", suite.fakeService.ResolveURL("/fail/")),
 		},
 		{
 			name: "GET request with invalid URL causes an error",
@@ -679,6 +679,11 @@ func TestRequestsTestSuite(t *testing.T) {
 	suite.Run(t, new(RequestsTestSuite))
 }
 
+func TestHTTPStatus(t *testing.T) {
+	require.Equal(t, "HTTP 429 Too Many Requests", httpStatus(http.StatusTooManyRequests))
+	require.Equal(t, "HTTP 599", httpStatus(599))
+}
+
 func TestJitteredBackoff(t *testing.T) {
 	const minWait, maxWait = time.Second, 30 * time.Second
 

From 460d4da71f43e1522ba2c2d38abc68f2642a2a66 Mon Sep 17 00:00:00 2001
From: Marko Bevc <marko@kosli.com>
Date: Fri, 6 Nov 2026 21:11:31 +0000
Subject: [PATCH 6/7] fix: report non-JSON error responses by their HTTP status
 (#1350)

* fix: report non-JSON error responses by their HTTP status

An error response whose body is not JSON, such as a proxy's HTML page
or an empty 401 or 404, was reported by the JSON decoder's complaint
alone, e.g. "invalid character '<' looking for beginning of value". The
status was carried on the APIError but never reached the message, so
the user could not tell a firewall block from a missing resource.

The message is now the status, such as "HTTP 403 Forbidden". When the
body is not empty it adds that the response is not JSON and, unless
--debug is already on, that rerunning with --debug shows it, since the
body is logged at debug level only.

* fix: treat a whitespace-only error body as empty

Some proxies answer a 401 or 404 with a lone newline. That body counted
as non-empty, so the user was told to rerun with --debug to see it, and
--debug then showed nothing. The body is now trimmed before the check.

Also renames the test case for an unrouted GET, which gets an empty 404
rather than the HTML body its old name described.
---
 internal/requests/requests.go      | 19 +++++++++++++++----
 internal/requests/requests_test.go | 23 ++++++++++++++++++-----
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/internal/requests/requests.go b/internal/requests/requests.go
index 133f598fe..f094b3336 100644
--- a/internal/requests/requests.go
+++ b/internal/requests/requests.go
@@ -388,10 +388,7 @@ func (c *Client) Do(p *RequestParams) (*HTTPResponse, error) {
 			err := json.Unmarshal([]byte(body), &respBody)
 			if err != nil {
 				c.Logger.Debug("response body from %s (status %d):\n%s", req.URL, resp.StatusCode, string(body))
-				// Still carry the status code so callers can distinguish e.g. a 404
-				// with an empty or non-JSON body (a proxy/CDN page). Keep the
-				// (JSON parse) error text as the message.
-				return nil, &APIError{StatusCode: resp.StatusCode, Message: err.Error()}
+				return nil, &APIError{StatusCode: resp.StatusCode, Message: nonJSONErrorMessage(resp.StatusCode, body, c.Debug)}
 			}
 			cleanedErrorMessage := ""
 			hasServerMessage := false
@@ -550,6 +547,20 @@ func isSuccess(status int) bool {
 	return false
 }
 
+// nonJSONErrorMessage reports an error response whose body is not JSON, such as
+// a proxy's HTML page, by its status: the decoder's complaint about the body
+// tells the user nothing.
+func nonJSONErrorMessage(status int, body []byte, debug bool) string {
+	switch {
+	case len(bytes.TrimSpace(body)) == 0:
+		return httpStatus(status)
+	case debug:
+		return httpStatus(status) + ": the response is not JSON"
+	default:
+		return httpStatus(status) + ": the response is not JSON, rerun with --debug to see it"
+	}
+}
+
 // giveUpErrorHandler keeps the last response's status, which retryablehttp's
 // default handler drops. The *url.Error that http.Client wraps it in names the
 // method and URL.
diff --git a/internal/requests/requests_test.go b/internal/requests/requests_test.go
index 158e169c1..dd2bae34b 100644
--- a/internal/requests/requests_test.go
+++ b/internal/requests/requests_test.go
@@ -79,6 +79,10 @@ func (suite *RequestsTestSuite) SetupSuite() {
 		Get("/fail/").
 		Reply(500).
 		BodyString("server broken")
+	suite.fakeService.NewHandler().
+		Get("/blank/").
+		Reply(401).
+		BodyString("\r\n")
 	suite.fakeService.NewHandler().
 		Get("/proxy-block/").
 		Reply(403).
@@ -323,7 +327,7 @@ func (suite *RequestsTestSuite) TestDo() {
 				URL:    suite.fakeService.ResolveURL("/no-content/"),
 			},
 			wantError:        true,
-			expectedErrorMsg: "unexpected end of JSON input",
+			expectedErrorMsg: "HTTP 204 No Content",
 		},
 		{
 			name: "206 is not a success: a partial body is not the answer",
@@ -332,7 +336,7 @@ func (suite *RequestsTestSuite) TestDo() {
 				URL:    suite.fakeService.ResolveURL("/partial/"),
 			},
 			wantError:        true,
-			expectedErrorMsg: "unexpected end of JSON input",
+			expectedErrorMsg: "HTTP 206 Partial Content: the response is not JSON, rerun with --debug to see it",
 		},
 		{
 			name: "GET request to 404 endpoint",
@@ -430,13 +434,22 @@ func (suite *RequestsTestSuite) TestDo() {
 			expectedErrorMsg: "Denied",
 		},
 		{
-			name: "GET request to a PUT endpoint fails because of invalid response",
+			name: "GET to an unrouted path reports the status of the empty 404",
 			params: &RequestParams{
 				Method: http.MethodGet,
 				URL:    suite.fakeService.ResolveURL("/html"),
 			},
 			wantError:        true,
-			expectedErrorMsg: "unexpected end of JSON input",
+			expectedErrorMsg: "HTTP 404 Not Found",
+		},
+		{
+			name: "a whitespace-only body reports the status alone",
+			params: &RequestParams{
+				Method: http.MethodGet,
+				URL:    suite.fakeService.ResolveURL("/blank/"),
+			},
+			wantError:        true,
+			expectedErrorMsg: "HTTP 401 Unauthorized",
 		},
 		{
 			name: "non-JSON error response body is logged with debug enabled",
@@ -446,7 +459,7 @@ func (suite *RequestsTestSuite) TestDo() {
 			},
 			debug:            true,
 			wantError:        true,
-			expectedErrorMsg: "invalid character '<' looking for beginning of value",
+			expectedErrorMsg: "HTTP 403 Forbidden: the response is not JSON",
 			expectedLog:      `<!DOCTYPE html><html><body><h1>403 Forbidden</h1><p>Blocked by proxy</p></body></html>`,
 		},
 	} {

From 59ecb6cc5a0a2b38cad6c2cb0df4181d34c43dd7 Mon Sep 17 00:00:00 2001
From: Marko Bevc <marko@kosli.com>
Date: Fri, 6 Nov 2026 21:24:14 +0000
Subject: [PATCH 7/7] fix: name the HTTP status in upload, evaluate and
 Bitbucket errors (#1351)

* refactor: export HTTPStatus for use outside the requests package

Other packages report HTTP errors by bare code and are about to render
them the same way as the API client does, so the helper needs to be
reachable from them.

* fix: name the status in upload errors

An upload refused by the storage bucket read "upload refused with HTTP
403". It now reads "HTTP 403 Forbidden", like the API client's errors.

* fix: name the status in kosli evaluate errors

A refused policy fetch and a server-side evaluation the API turned down
were reported by bare code, e.g. "the Kosli server answered 403". They
now read "HTTP 403 Forbidden", like the API client's errors.

* fix: name the status in Bitbucket pull request errors

A pull request or commit list that came back with a success other than
200 was reported as "got HTTP status 202". It now reads "got HTTP 202
Accepted", like the API client's errors. The fake Bitbucket server in
the tests can now answer a reply with a chosen status, to reach these
paths.

* fix: say what Bitbucket should have answered instead of blaming permissions

The pull request details error reached only on a 201 or 202 told the
user to review repository permissions, which a success status does not
suggest. Both Bitbucket errors on that path now say the reply was not
the 200 OK the code reads. The tests match the whole message.

Also pins the evaluate error for a server message that is present but
empty, which reports the status alone.
---
 cmd/kosli/attestationSubmit_test.go    |  2 +-
 cmd/kosli/evaluateHelpers.go           |  6 +++---
 cmd/kosli/evaluateHelpers_test.go      | 24 ++++++++++++++++++++++++
 internal/bitbucket/bitbucket.go        |  4 ++--
 internal/bitbucket/odd_replies_test.go | 24 ++++++++++++++++++++++++
 internal/requests/requests.go          | 16 ++++++++--------
 internal/requests/requests_test.go     |  4 ++--
 7 files changed, 64 insertions(+), 16 deletions(-)

diff --git a/cmd/kosli/attestationSubmit_test.go b/cmd/kosli/attestationSubmit_test.go
index 2faaca538..325e72105 100644
--- a/cmd/kosli/attestationSubmit_test.go
+++ b/cmd/kosli/attestationSubmit_test.go
@@ -321,7 +321,7 @@ func TestAttestationSubmitterAttestsNothingWhenTheBucketRefusesTheFile(t *testin
 	err := submitter.submit(submission)
 
 	require.EqualError(t, err, "failed to upload attachment testdata/file1: "+
-		"upload refused with HTTP 403: BadDigest: The SHA256 you specified did not match what we received")
+		"upload refused with HTTP 403 Forbidden: BadDigest: The SHA256 you specified did not match what we received")
 	require.Empty(t, fake.recorded)
 }
 
diff --git a/cmd/kosli/evaluateHelpers.go b/cmd/kosli/evaluateHelpers.go
index 259723ec4..db8abab25 100644
--- a/cmd/kosli/evaluateHelpers.go
+++ b/cmd/kosli/evaluateHelpers.go
@@ -202,7 +202,7 @@ func fetchRemotePolicy(remoteURL string) ([]byte, error) {
 	}
 	defer func() { _ = resp.Body.Close() }()
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		return nil, fmt.Errorf("failed to fetch policy from %s: HTTP %d", remoteURL, resp.StatusCode)
+		return nil, fmt.Errorf("failed to fetch policy from %s: %s", remoteURL, requests.HTTPStatus(resp.StatusCode))
 	}
 
 	body, err := io.ReadAll(io.LimitReader(resp.Body, policyMaxBytes+1))
@@ -421,9 +421,9 @@ func serverSideRequestError(err error) error {
 	// Quoting what arrived instead would dress a proxy's page, or the decoder's
 	// complaint about one, as the server's own reason.
 	if !apiError.HasServerMessage || apiError.Message == "" {
-		return fmt.Errorf("the Kosli server answered %d", apiError.StatusCode)
+		return fmt.Errorf("the Kosli server answered %s", requests.HTTPStatus(apiError.StatusCode))
 	}
-	return fmt.Errorf("the Kosli server answered %d: %s", apiError.StatusCode, apiError.Message)
+	return fmt.Errorf("the Kosli server answered %s: %s", requests.HTTPStatus(apiError.StatusCode), apiError.Message)
 }
 
 // serverSideReadError reports a failure to read a verdict back. By this point
diff --git a/cmd/kosli/evaluateHelpers_test.go b/cmd/kosli/evaluateHelpers_test.go
index 5e331421d..cd107ebde 100644
--- a/cmd/kosli/evaluateHelpers_test.go
+++ b/cmd/kosli/evaluateHelpers_test.go
@@ -3,9 +3,12 @@ package main
 import (
 	"bytes"
 	"encoding/json"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/kosli-dev/cli/internal/evaluate"
+	"github.com/kosli-dev/cli/internal/requests"
 	"github.com/stretchr/testify/require"
 )
 
@@ -21,3 +24,24 @@ func TestEvaluateResultKeysListsEveryKeyTheOutputCanHave(t *testing.T) {
 		require.Contains(t, evaluateResultKeys, key)
 	}
 }
+
+func TestFetchRemotePolicyNamesTheStatusItWasRefusedWith(t *testing.T) {
+	server := httptest.NewServer(http.NotFoundHandler())
+	defer server.Close()
+
+	_, err := fetchRemotePolicy(server.URL + "/policy.rego")
+
+	require.EqualError(t, err, "failed to fetch policy from "+server.URL+"/policy.rego: HTTP 404 Not Found")
+}
+
+func TestServerSideRequestErrorNamesTheStatus(t *testing.T) {
+	require.EqualError(t,
+		serverSideRequestError(&requests.APIError{StatusCode: http.StatusForbidden, Message: "not entitled", HasServerMessage: true}),
+		"the Kosli server answered HTTP 403 Forbidden: not entitled")
+	require.EqualError(t,
+		serverSideRequestError(&requests.APIError{StatusCode: http.StatusNotFound}),
+		"the Kosli server answered HTTP 404 Not Found")
+	require.EqualError(t,
+		serverSideRequestError(&requests.APIError{StatusCode: http.StatusBadRequest, HasServerMessage: true}),
+		"the Kosli server answered HTTP 400 Bad Request")
+}
diff --git a/internal/bitbucket/bitbucket.go b/internal/bitbucket/bitbucket.go
index d412476e5..84d1b2f1f 100644
--- a/internal/bitbucket/bitbucket.go
+++ b/internal/bitbucket/bitbucket.go
@@ -244,7 +244,7 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit
 			}
 		}
 	} else {
-		return evidence, fmt.Errorf("failed to get PR details, got HTTP status %d. Please review repository permissions", response.Resp.StatusCode)
+		return evidence, fmt.Errorf("failed to get PR details, got %s instead of HTTP 200 OK", requests.HTTPStatus(response.Resp.StatusCode))
 	}
 	return evidence, nil
 }
@@ -332,7 +332,7 @@ func (c *Config) getPullRequestCommitsFromBitbucket(prID int) ([]types.Commit, e
 			return nil, err
 		}
 		if response.Resp.StatusCode != 200 {
-			return nil, fmt.Errorf("failed to get PR commits, got HTTP status %d", response.Resp.StatusCode)
+			return nil, fmt.Errorf("failed to get PR commits, got %s instead of HTTP 200 OK", requests.HTTPStatus(response.Resp.StatusCode))
 		}
 
 		var responseData map[string]any
diff --git a/internal/bitbucket/odd_replies_test.go b/internal/bitbucket/odd_replies_test.go
index 83df38da1..e093de5fd 100644
--- a/internal/bitbucket/odd_replies_test.go
+++ b/internal/bitbucket/odd_replies_test.go
@@ -160,11 +160,21 @@ func (rt redirectTransport) RoundTrip(req *http.Request) (*http.Response, error)
 // runOddReplies serves replies and collects evidence for oddCommit through
 // the given entry point: 1 for PREvidenceForCommitV1, 2 for V2.
 func runOddReplies(t *testing.T, version int, replies map[string]string) ([]*types.PREvidence, error) {
+	t.Helper()
+	return runOddRepliesWithStatus(t, version, replies, nil)
+}
+
+// runOddRepliesWithStatus is runOddReplies with the status of each named reply
+// set by statuses; any other reply is a 200.
+func runOddRepliesWithStatus(t *testing.T, version int, replies map[string]string, statuses map[string]int) ([]*types.PREvidence, error) {
 	t.Helper()
 	var base string
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		for name, route := range oddRoutes {
 			if r.URL.RequestURI() == route {
+				if status, ok := statuses[name]; ok {
+					w.WriteHeader(status)
+				}
 				_, _ = w.Write([]byte(strings.ReplaceAll(replies[name], "{{base}}", base)))
 				return
 			}
@@ -423,3 +433,17 @@ func TestOddReplies_CommitWithNoHashFails(t *testing.T) {
 		})
 	}
 }
+
+// A 201 or 202 passes the client as a success but is not the reply the code
+// reads, so it fails, naming the status.
+func TestOddReplies_ASuccessOtherThanOKIsNamed(t *testing.T) {
+	for name, want := range map[string]string{
+		"pullrequest":           "failed to get PR details, got HTTP 202 Accepted instead of HTTP 200 OK",
+		"pullrequest_commits_1": "failed to get PR commits, got HTTP 202 Accepted instead of HTTP 200 OK",
+	} {
+		t.Run(name, func(t *testing.T) {
+			_, err := runOddRepliesWithStatus(t, 2, oddBaselineReplies(), map[string]int{name: http.StatusAccepted})
+			require.EqualError(t, err, want)
+		})
+	}
+}
diff --git a/internal/requests/requests.go b/internal/requests/requests.go
index f094b3336..3589a55ec 100644
--- a/internal/requests/requests.go
+++ b/internal/requests/requests.go
@@ -498,9 +498,9 @@ func s3ErrorMessage(status int, body []byte) string {
 		Message string `xml:"Message"`
 	}
 	if err := xml.Unmarshal(body, &s3Error); err != nil || s3Error.Code == "" {
-		return fmt.Sprintf("upload refused with HTTP %d", status)
+		return "upload refused with " + HTTPStatus(status)
 	}
-	return fmt.Sprintf("upload refused with HTTP %d: %s: %s", status, s3Error.Code, s3Error.Message)
+	return fmt.Sprintf("upload refused with %s: %s: %s", HTTPStatus(status), s3Error.Code, s3Error.Message)
 }
 
 func (c *Client) PayloadOutput(req *http.Request, jsonFields map[string]any, message string) error {
@@ -553,11 +553,11 @@ func isSuccess(status int) bool {
 func nonJSONErrorMessage(status int, body []byte, debug bool) string {
 	switch {
 	case len(bytes.TrimSpace(body)) == 0:
-		return httpStatus(status)
+		return HTTPStatus(status)
 	case debug:
-		return httpStatus(status) + ": the response is not JSON"
+		return HTTPStatus(status) + ": the response is not JSON"
 	default:
-		return httpStatus(status) + ": the response is not JSON, rerun with --debug to see it"
+		return HTTPStatus(status) + ": the response is not JSON, rerun with --debug to see it"
 	}
 }
 
@@ -569,7 +569,7 @@ func giveUpErrorHandler(resp *http.Response, err error, numTries int) (*http.Res
 	if resp != nil {
 		_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
 		_ = resp.Body.Close()
-		msg += ": " + httpStatus(resp.StatusCode)
+		msg += ": " + HTTPStatus(resp.StatusCode)
 	}
 	if err != nil {
 		return nil, fmt.Errorf("%s: %w", msg, err)
@@ -577,9 +577,9 @@ func giveUpErrorHandler(resp *http.Response, err error, numTries int) (*http.Res
 	return nil, errors.New(msg)
 }
 
-// httpStatus renders a code as "HTTP 409 Conflict", or "HTTP 599" for a code
+// HTTPStatus renders a code as "HTTP 409 Conflict", or "HTTP 599" for a code
 // with no standard text.
-func httpStatus(code int) string {
+func HTTPStatus(code int) string {
 	return strings.TrimSpace(fmt.Sprintf("HTTP %d %s", code, http.StatusText(code)))
 }
 
diff --git a/internal/requests/requests_test.go b/internal/requests/requests_test.go
index dd2bae34b..39f1501e9 100644
--- a/internal/requests/requests_test.go
+++ b/internal/requests/requests_test.go
@@ -693,8 +693,8 @@ func TestRequestsTestSuite(t *testing.T) {
 }
 
 func TestHTTPStatus(t *testing.T) {
-	require.Equal(t, "HTTP 429 Too Many Requests", httpStatus(http.StatusTooManyRequests))
-	require.Equal(t, "HTTP 599", httpStatus(599))
+	require.Equal(t, "HTTP 429 Too Many Requests", HTTPStatus(http.StatusTooManyRequests))
+	require.Equal(t, "HTTP 599", HTTPStatus(599))
 }
 
 func TestJitteredBackoff(t *testing.T) {