Merge pull request 'Name the cause when a watch stops on an auth failure' (#18) from benvin/watchpr-auth-failure into main

Reviewed-on: #18

AGENTS.md

@@ -176,11 +176,22 @@ wrapped per stage (login / read denied / write denied) via `ErrVaultDenied`.
 ## Gotchas
 
 - `watchpr` exits 0 with no output changes on `--once` (just prints state).
-- Gitea tokens expire in ~1h, shorter than a watch: the client re-mints once on a
-  401/403 and replays the request. If the fresh token is rejected too, `watchpr`
-  exits non-zero rather than polling blind.
+- Gitea tokens expire in ~1h, shorter than a watch: the client re-mints once when
+  the credential it sent was rejected and replays the request. If the fresh token
+  is rejected too, `watchpr` exits non-zero rather than polling blind.
+- A 401/403 is classified before anything is re-minted, because only one of the
+  three cases is a stale token: `ErrNoCredential` (the request carried no token —
+  anonymous access to something not public), `IsPermissionDenied` (a 403 whose
+  body names no credential, so the identity is simply not allowed) and
+  `IsCredentialRejected` (any 401, or a 403 mentioning a token/scope/sign-in,
+  which is what Gitea returns for a token missing a scope). Only the last
+  re-mints; the others abort immediately, since a fresh token cannot fix them and
+  blaming one hides the real cause. `IsAuthError` stays "any 401/403" — all three
+  end a watch.
 - `watchpr` polls anonymously when no token can be minted (public repos work
-  fine); only a real 401/403 reaches for Vault.
+  fine); an anonymous run never reaches for Vault, on any status code.
+- A re-mint that hands back an empty token is an error: replaying with it would
+  drop the Authorization header and silently continue as an anonymous watcher.
 - The token cache is process-wide (mutex-guarded); `RefreshGiteaToken` replaces
   it. Tests call the unexported `fetchGiteaToken` to avoid the cache.
 - `agentvault` never puts a secret in an error string: Vault decode failures and

README.md

@@ -99,6 +99,25 @@ watchpr --once --json unkin/argocd-apps#42
 On a meaningful change `watchpr` prints the reason and the PR's current state,
 then exits 0. Use `--json` for machine-readable output.
 
+### Exit behaviour
+
+A watcher that sees nothing must not look healthy, so every terminal failure
+exits non-zero naming its cause:
+
+| Cause | Message |
+|---|---|
+| the token was rejected and a fresh one was too | `gitea rejected the token and re-minting did not recover it` |
+| authenticated but not allowed (Gitea 403, no token named) | `gitea denied access to <login>` |
+| polling anonymously and the PR is not public | `gitea requires authentication and no token could be minted` |
+| the PR lookup 404s (repo deleted, renamed, made private) | `PR no longer visible` |
+
+Gitea tokens expire in ~1h, far shorter than a watch, so a rejected token is
+re-minted once and the request replayed; only a failure that survives that
+re-mint ends the watch. Anonymous polling of a public repo is unaffected — with
+no token there is nothing to reject and Vault is never reached for one.
+Transient failures (5xx, network errors, rate limiting) are warned about and
+retried, and give up after 20 consecutive failures of the same PR.
+
 ## agentws
 
 `agentws` gives an agent an isolated git worktree per branch without disturbing

cmd/watchpr/main.go

@@ -105,7 +105,7 @@ func runOnce(c *agent.GiteaClient, refs []agent.PRRef, jsonMode bool) error {
 	for _, ref := range refs {
 		st, err := agent.FetchState(c, ref, login)
 		if err != nil {
-			return err
+			return describeFailure(err)
 		}
 		states = append(states, st)
 	}
@@ -137,18 +137,30 @@ func runWatch(c *agent.GiteaClient, refs []agent.PRRef, interval time.Duration,
 
 	res, err := agent.Watch(c, refs, login, ticker.C, onBaseline, onError)
 	if err != nil {
-		if agent.IsAuthError(err) {
-			return fmt.Errorf("gitea authentication failed after re-minting the token, watch aborted: %w", err)
-		}
-		if agent.IsPRGone(err) {
-			return fmt.Errorf("PR no longer visible (repo deleted, renamed, or made private), watch aborted: %w", err)
-		}
-		return err
+		return describeFailure(err)
 	}
 	report(res.Ref.String(), res.Reason, res.State, jsonMode)
 	return nil
 }
 
+// describeFailure names the cause of a terminal failure so a watcher that stops
+// says why. An anonymous rejection, a permission boundary and a token that
+// outlived its Vault lease are three different problems and only the last is
+// fixed by a fresh token.
+func describeFailure(err error) error {
+	switch {
+	case agent.IsNoCredential(err):
+		return fmt.Errorf("gitea requires authentication and no token could be minted, aborted: %w", err)
+	case agent.IsPermissionDenied(err):
+		return fmt.Errorf("gitea denied access to %s (a fresh token will not help), aborted: %w", agent.AgentLogin(), err)
+	case agent.IsAuthError(err):
+		return fmt.Errorf("gitea rejected the token and re-minting did not recover it, aborted: %w", err)
+	case agent.IsPRGone(err):
+		return fmt.Errorf("PR no longer visible (repo deleted, renamed, or made private), aborted: %w", err)
+	}
+	return err
+}
+
 // report emits the change that ended the watch.
 func report(key, reason string, st agent.PRState, jsonMode bool) {
 	if jsonMode {

cmd/watchpr/main_test.go

@@ -1,11 +1,15 @@
 package main
 
 import (
+	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
+
+	"git.unkin.net/unkin/agent-tools/internal/agent"
 )
 
 // A bad PR reference must fail the command (so main exits non-zero) rather than
@@ -105,3 +109,76 @@ func TestExecuteBadIntervalErrors(t *testing.T) {
 		}
 	}
 }
+
+// failingVault serves an AppRole login that never issues a token, so the
+// command falls back to anonymous polling exactly as it does when Vault is
+// unreachable.
+func failingVault(t *testing.T) *httptest.Server {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusForbidden)
+	}))
+	t.Cleanup(srv.Close)
+	return srv
+}
+
+// An anonymous run against a repo that is not public must exit non-zero saying
+// no token was available — not claim a token expired, and not keep going.
+func TestOnceAnonymousRejectionNamesTheMissingToken(t *testing.T) {
+	vault := failingVault(t)
+
+	requests := 0
+	gitea := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		requests++
+		w.WriteHeader(http.StatusUnauthorized)
+		_, _ = io.WriteString(w, `{"message":"invalid username, password or token"}`)
+	}))
+	defer gitea.Close()
+
+	t.Setenv("VAULT_ADDR", vault.URL)
+	t.Setenv("GITEA_URL", gitea.URL)
+
+	cmd := newRootCmd()
+	cmd.SetArgs([]string{"--once", "unkin/repo#7"})
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatal("Execute() = nil, want a non-zero exit when the poll is rejected")
+	}
+	if !strings.Contains(err.Error(), "no token could be minted") {
+		t.Errorf("Execute() error = %q, want it to name the missing token", err)
+	}
+	if requests != 1 {
+		t.Errorf("gitea requests = %d, want 1 (no replay without a credential)", requests)
+	}
+}
+
+// describeFailure must tell the four terminal causes apart: each one sends the
+// reader somewhere different, and a watcher that stops without saying why is
+// the failure this names.
+func TestDescribeFailureNamesTheCause(t *testing.T) {
+	rejected := &agent.APIError{Method: "GET", Path: "/p", StatusCode: 401, Body: `{"message":"invalid username, password or token"}`}
+	forbidden := &agent.APIError{Method: "GET", Path: "/p", StatusCode: 403, Body: `{"message":"Forbidden"}`}
+	tests := []struct {
+		name string
+		err  error
+		want string
+	}{
+		{"anonymous", fmt.Errorf("%w: %w", agent.ErrNoCredential, rejected), "no token could be minted"},
+		{"permission boundary", error(forbidden), "denied access"},
+		{"rejected token", error(rejected), "re-minting did not recover it"},
+		{"other", errors.New("dial tcp: timeout"), "dial tcp: timeout"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := describeFailure(tt.err)
+			if got == nil || !strings.Contains(got.Error(), tt.want) {
+				t.Errorf("describeFailure = %v, want it to mention %q", got, tt.want)
+			}
+			if !errors.Is(got, tt.err) {
+				t.Errorf("describeFailure dropped the underlying error %v", tt.err)
+			}
+		})
+	}
+}

internal/agent/client_test.go

@@ -9,6 +9,7 @@ import (
 	"net/http/httptest"
 	"strings"
 	"testing"
+	"time"
 )
 
 // fakeVault serves the AppRole login and the gitea creds secret at credsPath
@@ -619,6 +620,197 @@ func TestAnonymousPollingNeverMints(t *testing.T) {
 	}
 }
 
+// Anonymous access to something that is not public is not a stale credential:
+// nothing was sent to be rejected, so the client must not burn a Vault mint on
+// every poll, and the error must say a token was missing rather than expired.
+func TestAnonymousAuthFailureNeverMints(t *testing.T) {
+	requests := 0
+	mux := http.NewServeMux()
+	mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) {
+		requests++
+		w.WriteHeader(http.StatusUnauthorized)
+		_, _ = io.WriteString(w, `{"message":"invalid username, password or token"}`)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	refreshes := 0
+	c := &GiteaClient{BaseURL: srv.URL, HTTP: srv.Client(),
+		Refresh: func() (string, error) { refreshes++; return "fresh", nil }}
+
+	_, err := c.GetPR("unkin/repo", 7)
+	if err == nil {
+		t.Fatal("expected an error on an anonymous 401")
+	}
+	if !IsNoCredential(err) {
+		t.Errorf("IsNoCredential(%v) = false, want true", err)
+	}
+	if !IsAuthError(err) {
+		t.Errorf("IsAuthError(%v) = false; an anonymous rejection still ends a watch", err)
+	}
+	if refreshes != 0 {
+		t.Errorf("refreshes = %d, want 0 (nothing was rejected)", refreshes)
+	}
+	if requests != 1 {
+		t.Errorf("requests = %d, want 1 (no replay)", requests)
+	}
+}
+
+// Gitea's bare "Forbidden" is a permission boundary, not an expired token. A
+// re-mint cannot grant a permission the identity lacks, so the client must not
+// spend one, and the failure must not be reported as an auth expiry.
+func TestPermissionDeniedIsNotRemintedOrRetried(t *testing.T) {
+	requests := 0
+	mux := http.NewServeMux()
+	mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) {
+		requests++
+		w.WriteHeader(http.StatusForbidden)
+		_, _ = io.WriteString(w, `{"errors":null,"message":"Forbidden","url":"https://git.unkin.net/api/swagger"}`)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	refreshes := 0
+	c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client(),
+		Refresh: func() (string, error) { refreshes++; return "fresh", nil }}
+
+	_, err := c.GetPR("unkin/repo", 7)
+	if err == nil {
+		t.Fatal("expected an error on a 403")
+	}
+	if !IsPermissionDenied(err) {
+		t.Errorf("IsPermissionDenied(%v) = false, want true", err)
+	}
+	if IsCredentialRejected(err) {
+		t.Errorf("a bare Forbidden must not read as a rejected credential")
+	}
+	if refreshes != 0 || requests != 1 {
+		t.Errorf("refreshes = %d, requests = %d, want 0 and 1", refreshes, requests)
+	}
+}
+
+// A 403 that names the token is a credential problem after all (Gitea reports a
+// missing scope this way), so it keeps the re-mint-and-replay path.
+func TestScopeForbiddenIsRemintedAndRetried(t *testing.T) {
+	var seen []string
+	mux := http.NewServeMux()
+	mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) {
+		tok := strings.TrimPrefix(r.Header.Get("Authorization"), "token ")
+		seen = append(seen, tok)
+		if tok != "fresh" {
+			w.WriteHeader(http.StatusForbidden)
+			_, _ = io.WriteString(w, `{"message":"token does not have at least one of required scope(s): [read:repository]"}`)
+			return
+		}
+		_, _ = io.WriteString(w, `{"number":7,"state":"open","mergeable":true,"head":{"sha":"cafebabe"}}`)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	refreshes := 0
+	c := &GiteaClient{BaseURL: srv.URL, Token: "stale", HTTP: srv.Client(),
+		Refresh: func() (string, error) { refreshes++; return "fresh", nil }}
+
+	pr, err := c.GetPR("unkin/repo", 7)
+	if err != nil {
+		t.Fatalf("GetPR after re-mint: %v", err)
+	}
+	if pr.Number != 7 || refreshes != 1 {
+		t.Errorf("PR = %+v, refreshes = %d, want PR 7 and 1 re-mint", pr, refreshes)
+	}
+	if len(seen) != 2 || seen[1] != "fresh" {
+		t.Errorf("tokens seen = %v, want [stale fresh]", seen)
+	}
+}
+
+// A re-mint that hands back an empty token must fail loudly. Replaying with it
+// would drop the Authorization header, and on a public repo that anonymous
+// replay succeeds — the watch would carry on having quietly lost its identity.
+func TestEmptyRemintedTokenFailsInsteadOfGoingAnonymous(t *testing.T) {
+	var seen []string
+	mux := http.NewServeMux()
+	mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) {
+		tok := strings.TrimPrefix(r.Header.Get("Authorization"), "token ")
+		seen = append(seen, tok)
+		if tok == "stale" {
+			w.WriteHeader(http.StatusUnauthorized)
+			_, _ = io.WriteString(w, `{"message":"invalid username, password or token"}`)
+			return
+		}
+		_, _ = io.WriteString(w, `{"number":7,"state":"open","mergeable":true,"head":{"sha":"cafebabe"}}`)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	c := &GiteaClient{BaseURL: srv.URL, Token: "stale", HTTP: srv.Client(),
+		Refresh: func() (string, error) { return "", nil }}
+
+	_, err := c.GetPR("unkin/repo", 7)
+	if err == nil {
+		t.Fatal("an empty re-minted token must be an error, not an anonymous retry")
+	}
+	if !IsAuthError(err) {
+		t.Errorf("IsAuthError(%v) = false, want true", err)
+	}
+	if len(seen) != 1 {
+		t.Errorf("requests = %d, want 1 (no anonymous replay)", len(seen))
+	}
+}
+
+// The real-world rotation case end to end: the watch runs happily for several
+// polls, then the Vault lease expires and Gitea rejects every token, the fresh
+// one included. Watch must end with a named auth failure instead of polling on.
+func TestWatchAbortsWhenTokenExpiresMidWatch(t *testing.T) {
+	polls := 0
+	mux := http.NewServeMux()
+	mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) {
+		polls++
+		if polls > 3 {
+			w.WriteHeader(http.StatusUnauthorized)
+			_, _ = io.WriteString(w, `{"message":"invalid username, password or token"}`)
+			return
+		}
+		_, _ = io.WriteString(w, `{"number":7,"state":"open","mergeable":true,"head":{"sha":"cafebabe"}}`)
+	})
+	mux.HandleFunc("/api/v1/repos/unkin/repo/commits/cafebabe/status", func(w http.ResponseWriter, r *http.Request) {
+		_, _ = io.WriteString(w, `{"state":"success"}`)
+	})
+	mux.HandleFunc("/api/v1/repos/unkin/repo/issues/7/comments", func(w http.ResponseWriter, r *http.Request) {
+		_, _ = io.WriteString(w, `[]`)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	refreshes := 0
+	c := &GiteaClient{BaseURL: srv.URL, Token: "t1", HTTP: srv.Client(),
+		Refresh: func() (string, error) { refreshes++; return "t2", nil }}
+
+	ref := PRRef{Owner: "unkin", Repo: "repo", Number: 7}
+	ticks := make(chan time.Time, 5)
+	for i := 0; i < 5; i++ {
+		ticks <- time.Now()
+	}
+	close(ticks)
+
+	warned := 0
+	_, err := Watch(c, []PRRef{ref}, "unkin-agent", ticks, nil, func(PRRef, error) { warned++ })
+	if err == nil {
+		t.Fatal("Watch returned nil: an expired token must end the watch, not be polled past")
+	}
+	if !IsAuthError(err) {
+		t.Errorf("Watch error = %v, want an auth error", err)
+	}
+	if IsNoCredential(err) {
+		t.Errorf("a rejected token must not be reported as a missing one: %v", err)
+	}
+	if refreshes != 1 {
+		t.Errorf("refreshes = %d, want 1", refreshes)
+	}
+	if warned != 0 {
+		t.Errorf("auth failure logged as a warning %d time(s); it must abort", warned)
+	}
+}
+
 func TestCreateIssueRequestBody(t *testing.T) {
 	var gotPath, gotMethod, gotAuth string
 	var gotBody CreateIssueOptions

internal/agent/gitea.go

@@ -32,14 +32,61 @@ func IsNotFound(err error) bool {
 	return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound
 }
 
-// IsAuthError reports whether err is a Gitea 401/403: the token is expired or
-// unauthorised, which retrying the same request cannot fix.
+// IsAuthError reports whether err is a Gitea 401/403. Both end a watch: neither
+// a rejected credential nor a permission boundary clears itself on a retry.
 func IsAuthError(err error) bool {
 	var apiErr *APIError
 	return errors.As(err, &apiErr) &&
 		(apiErr.StatusCode == http.StatusUnauthorized || apiErr.StatusCode == http.StatusForbidden)
 }
 
+// ErrNoCredential marks a 401/403 on a request that carried no token at all.
+// Anonymous polling of a public repo is supported, so this is not a rejected
+// credential: the resource simply is not public and no token was available.
+var ErrNoCredential = errors.New("gitea requires authentication and no token was available")
+
+// IsNoCredential reports whether err is an auth failure on an anonymous request.
+func IsNoCredential(err error) bool {
+	return errors.Is(err, ErrNoCredential)
+}
+
+// credentialHints are the fragments Gitea puts in a 403 body when the
+// credential itself is at fault ("token does not have at least one of required
+// scope(s)", "sign in required") rather than the identity's permissions, whose
+// body is a bare "Forbidden".
+var credentialHints = []string{"token", "sign in", "credential"}
+
+// IsCredentialRejected reports whether err means the credential that was sent
+// was refused, which a freshly minted token may fix. A 401 always qualifies.
+// Gitea 403s both for a token missing a scope and for an identity that may not
+// do this at all, so for a 403 the response body decides.
+func IsCredentialRejected(err error) bool {
+	var apiErr *APIError
+	if !errors.As(err, &apiErr) {
+		return false
+	}
+	switch apiErr.StatusCode {
+	case http.StatusUnauthorized:
+		return true
+	case http.StatusForbidden:
+		body := strings.ToLower(apiErr.Body)
+		for _, hint := range credentialHints {
+			if strings.Contains(body, hint) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// IsPermissionDenied reports a 403 that names no credential problem: the
+// identity is authenticated but not allowed, so re-minting cannot help.
+func IsPermissionDenied(err error) bool {
+	var apiErr *APIError
+	return errors.As(err, &apiErr) &&
+		apiErr.StatusCode == http.StatusForbidden && !IsCredentialRejected(err)
+}
+
 // GiteaClient talks to the Gitea REST API as the agent user.
 type GiteaClient struct {
 	BaseURL string
@@ -56,8 +103,10 @@ func NewGiteaClient(token string) *GiteaClient {
 	return &GiteaClient{BaseURL: GiteaURL(), Token: token, HTTP: httpClient, Refresh: RefreshGiteaToken}
 }
 
-// do sends the request and, if the token was rejected, re-mints it once and
-// replays the request with the fresh token.
+// do sends the request and, if the credential it carried was rejected, re-mints
+// the token once and replays the request. Anonymous requests and permission
+// denials are returned as they are: neither is fixed by a fresh token, and
+// re-minting on them would report a stale token as the cause of something else.
 func (c *GiteaClient) do(method, path string, body any, out any) error {
 	var payload []byte
 	if body != nil {
@@ -67,14 +116,24 @@ func (c *GiteaClient) do(method, path string, body any, out any) error {
 		}
 		payload = b
 	}
+	anonymous := c.Token == ""
 	err := c.attempt(method, path, payload, out)
-	if !IsAuthError(err) || c.Refresh == nil {
+	if !IsAuthError(err) {
+		return err
+	}
+	if anonymous {
+		return fmt.Errorf("%w: %w", ErrNoCredential, err)
+	}
+	if !IsCredentialRejected(err) || c.Refresh == nil {
 		return err
 	}
 	token, refreshErr := c.Refresh()
 	if refreshErr != nil {
 		return fmt.Errorf("%w; re-minting token: %v", err, refreshErr)
 	}
+	if token == "" {
+		return fmt.Errorf("%w; re-minting token yielded an empty token", err)
+	}
 	c.Token = token
 	return c.attempt(method, path, payload, out)
 }