🐛 fix(review): A review the GitHub proxy cut short at the agent cap is no verdict and is asked again, whatever its wording

changelog.d/fixed-review-agent-cap-no-verdict.md

@@ -0,0 +1 @@
+- A `requires_human` from a review whose GitHub reads the hive proxy refused (the reviewer hit `agents.github_api_hourly_cap`, or the shared bucket fell below `github.agent_reserve_floor`) is no longer recorded as a verdict, whatever words the reviewer used. The proxy now remembers when it last refused each agent, and the review relay sets aside a `requires_human` with no in-scope finding, no high-risk marker and no already-landed PR when its agent was refused after the perspective was dispatched. The perspective is asked again after `review.pending_ttl`, the same path as the rate-limited "could not read" case. Before, summaries such as "GitHub reads stopped at the user's request after the hourly cap; … could not be verified" or "not assessed: GitHub API cap prevented reading" were not recognised, and projectbluefin/utah-packages#959, projectbluefin/utah#1161 and projectbluefin/finpilot#659 parked at `stage/085-needs-decision` on 2026-11-09. The expiry that follows such a set-aside, or any expiry whose every row went to an agent the proxy refused after the row was sent, no longer spends `review.max_redispatch`: the planner waits for the refusal's `Retry-After` (at most an hour past `pending_ttl`), re-asks, and counts it in the dispatch state's `cap_redispatches[]`, bounded separately at 8 before the usual `review-dispatch-exhausted` hold. Before, a reviewer capped for about 2h15m on one head still ended in that hold and a person was asked.

src/cmd/hive/main.go

@@ -4330,6 +4330,9 @@ func (b *boot) bootProxyWith(deps bootProxyDeps) {
 		stopWriteCounters := b.githubProxy.StartWriteCounterPersistence(proxy.WriteCountersPath)
 		b.preShutdownHooks.add("flush-write-counters", stopWriteCounters)
 		b.githubProxy.SetAgentGitHubBudgetConfig(b.cfg.AgentGitHubAPIHourlyCap, b.cfg.GitHubAgentReserveFloor)
+		// A review whose reads this proxy refused is no verdict, and its
+		// expiry does not spend review.max_redispatch (projectbluefin/utah-packages#959).
+		github.SetAgentGitHubRefusalFunc(b.githubProxy.AgentGitHubRefusal)
 		if b.agentMgr != nil {
 			b.githubProxy.SetAgentGitHubBudgetNudgeFunc(b.agentMgr.SendGitHubAPIBudgetNudge)
 		}

src/cmd/hive/main_sweeps.go

@@ -2667,6 +2667,7 @@ func planReviewDispatchWith(ctx context.Context, cfg *config.Config, actionable
 		PendingTTL:            cfg.Review.PendingReviewTTL(),
 		PendingFixTTL:         cfg.Review.PendingReviewFixTTL(),
 		MaxRedispatch:         cfg.Review.EffectiveMaxRedispatch(),
+		AgentGitHubRefusal:    github.AgentGitHubRefusalFunc(),
 		HeadMaxRetries:        cfg.Review.HeadMaxRetries,
 		ProjectOrg:            cfg.Project.Org,
 		AIAuthor:              cfg.EffectiveAIAuthor(),
@@ -2705,7 +2706,7 @@ func planReviewDispatchWith(ctx context.Context, cfg *config.Config, actionable
 		// Warn once per PR while it keeps expiring (#13687).
 		key := prWarnKey(e.Repo, e.Number)
 		expiredNow[key] = struct{}{}
-		logger.Log(context.Background(), reviewExpiredWarn.level(key), "review pending expired", "repo", e.Repo, "pr", e.Number, "head", e.HeadSHA, "perspective", e.Perspective, "agent", e.Agent, "age", e.Age.Round(time.Second).String(), "expiries", e.Count)
+		logger.Log(context.Background(), reviewExpiredWarn.level(key), "review pending expired", "repo", e.Repo, "pr", e.Number, "head", e.HeadSHA, "perspective", e.Perspective, "agent", e.Agent, "age", e.Age.Round(time.Second).String(), "expiries", e.Count, "cap_caused", e.CapCaused)
 	}
 	reviewExpiredWarn.retain(expiredNow)
 	for _, h := range plan.Exhausted {

src/crux/proxy/agent_github_budget.go

@@ -26,17 +26,33 @@ type agentGitHubBudget struct {
 	events map[string][]time.Time
 	warned map[string]bool
 	nudged map[string]time.Time
+	// refused is the last refusal of each agent's GitHub request by its
+	// hourly cap or the shared reserve floor. The review relay reads it
+	// (AgentGitHubRefusedSince) to tell a review the proxy cut short from
+	// one that read the PR, and the review planner reads it
+	// (AgentGitHubRefusal) to tell an expiry the cap caused from one it did
+	// not, and to wait for the window to free before asking again.
+	refused map[string]agentGitHubRefusal
+}
+
+// agentGitHubRefusal is one refusal: when it happened, and when the
+// refusal's Retry-After said the agent could read again (the sliding hour
+// freeing a slot, or the shared bucket's reset).
+type agentGitHubRefusal struct {
+	at     time.Time
+	clears time.Time
 }
 
 func newAgentGitHubBudget(now func() time.Time) *agentGitHubBudget {
 	if now == nil {
 		now = time.Now
 	}
 	return &agentGitHubBudget{
-		now:    now,
-		events: make(map[string][]time.Time),
-		warned: make(map[string]bool),
-		nudged: make(map[string]time.Time),
+		now:     now,
+		events:  make(map[string][]time.Time),
+		warned:  make(map[string]bool),
+		nudged:  make(map[string]time.Time),
+		refused: make(map[string]agentGitHubRefusal),
 	}
 }
 
@@ -90,6 +106,39 @@ func (b *agentGitHubBudget) markNudged(agentName string) bool {
 	return true
 }
 
+// markRefused records that agentName was just refused a GitHub request
+// whose Retry-After was retryAfter seconds.
+func (b *agentGitHubBudget) markRefused(agentName string, retryAfter int) {
+	if b == nil || strings.TrimSpace(agentName) == "" {
+		return
+	}
+	now := b.now()
+	if retryAfter < 0 {
+		retryAfter = 0
+	}
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	if b.refused == nil {
+		b.refused = make(map[string]agentGitHubRefusal)
+	}
+	b.refused[agentName] = agentGitHubRefusal{at: now, clears: now.Add(time.Duration(retryAfter) * time.Second)}
+}
+
+// refusedSince reports whether agentName was refused a GitHub request at or
+// after since and, if so, when the last refusal said it would clear.
+func (b *agentGitHubBudget) refusedSince(agentName string, since time.Time) (clears time.Time, refused bool) {
+	if b == nil || strings.TrimSpace(agentName) == "" {
+		return time.Time{}, false
+	}
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	last, ok := b.refused[agentName]
+	if !ok || last.at.Before(since) {
+		return time.Time{}, false
+	}
+	return last.clears, true
+}
+
 func (b *agentGitHubBudget) pruneLocked(agentName string, now time.Time) []time.Time {
 	events := b.events[agentName]
 	cutoff := now.Add(-agentGitHubBudgetWindow)
@@ -140,10 +189,41 @@ func (p *GitHubProxy) agentGitHubReserveSnapshot() (remaining int, reset time.Ti
 	return 0, time.Time{}, false
 }
 
+// AgentGitHubRefusedSince reports whether the proxy refused one of
+// agentName's GitHub requests at or after since, because the agent reached
+// agents.github_api_hourly_cap or the shared bucket fell below
+// github.agent_reserve_floor. The review relay uses it to set aside a
+// requires_human from a review session the proxy cut short: the session
+// could not read what it needed, so its verdict is no verdict
+// (projectbluefin/utah-packages#959). The record lives in memory only; a
+// restart forgets it, as it forgets the counts.
+func (p *GitHubProxy) AgentGitHubRefusedSince(agentName string, since time.Time) bool {
+	_, refused := p.AgentGitHubRefusal(agentName, since)
+	return refused
+}
+
+// AgentGitHubRefusal is AgentGitHubRefusedSince plus when the last refusal
+// said the agent could read again: its Retry-After, which is when the
+// sliding hour frees a slot under the cap, or the shared bucket's reset
+// under the reserve floor. The review planner uses it so an expiry the cap
+// caused does not spend review.max_redispatch, and so the perspective is
+// asked again only once the agent can read.
+func (p *GitHubProxy) AgentGitHubRefusal(agentName string, since time.Time) (clears time.Time, refused bool) {
+	if p == nil || p.githubBudget == nil {
+		return time.Time{}, false
+	}
+	return p.githubBudget.refusedSince(agentName, since)
+}
+
 func (p *GitHubProxy) githubBudgetRefusal(agentName, method, path string, isRead bool) (status *agentGitHubBudgetStatus) {
 	if p == nil || agentName == "" || agentName == internalCallerName {
 		return nil
 	}
+	defer func() {
+		if status != nil {
+			p.githubBudget.markRefused(agentName, status.RetryAfter)
+		}
+	}()
 	cap := p.agentGitHubCap(agentName)
 	if cap > 0 && p.githubBudget != nil {
 		count, retryAfter, over := p.githubBudget.check(agentName, cap)

src/crux/proxy/agent_github_budget_test.go

@@ -148,3 +148,72 @@ func TestAgentGitHubBudgetCapWarnOncePerWindow(t *testing.T) {
 		t.Fatalf("WARNs after window reset = %d, want 2", got)
 	}
 }
+
+// The review relay sets aside a requires_human from a session the proxy cut
+// short (projectbluefin/utah-packages#959). It can only do that if the proxy
+// remembers, per agent, when it last refused a request: for the hourly cap
+// and for the reserve floor, and for nobody else.
+func TestAgentGitHubRefusedSinceRecordsCapAndReserveRefusals(t *testing.T) {
+	now := time.Date(2026, 11, 9, 1, 33, 0, 0, time.UTC)
+	remaining := 5000
+	p := &GitHubProxy{
+		githubBudget:       newAgentGitHubBudget(func() time.Time { return now }),
+		githubBudgetCap:    func(string) int { return 3 },
+		githubReserveFloor: func() int { return 400 },
+		githubReserveSnapshot: func() (int, time.Time, bool) {
+			return remaining, now.Add(10 * time.Minute), true
+		},
+	}
+	dispatched := now.Add(-20 * time.Minute)
+	for i := 0; i < 3; i++ {
+		if st := p.githubBudgetRefusal("reviewer", http.MethodGet, "/repos/o/r/pulls/1", true); st != nil {
+			t.Fatalf("request %d refused under the cap: %+v", i, st)
+		}
+		p.recordAgentGitHubBudget("reviewer", "/repos/o/r/pulls/1", http.StatusOK, 3)
+	}
+	if p.AgentGitHubRefusedSince("reviewer", dispatched) {
+		t.Fatal("an agent under its cap reads as refused")
+	}
+	if st := p.githubBudgetRefusal("reviewer", http.MethodGet, "/repos/o/r/pulls/1/files", true); st == nil || st.Reason != "cap" {
+		t.Fatalf("fourth request = %+v, want a cap refusal", st)
+	}
+	if !p.AgentGitHubRefusedSince("reviewer", dispatched) {
+		t.Fatal("the cap refusal was not recorded against the session that was running")
+	}
+	if !p.AgentGitHubRefusedSince("reviewer", now) {
+		t.Fatal("a refusal at the session start must count")
+	}
+	// The planner waits for the cap window to free before asking again, so
+	// the refusal carries its Retry-After: here the first of the three
+	// requests (all at now) leaves the sliding hour at now+1h.
+	if clears, refused := p.AgentGitHubRefusal("reviewer", dispatched); !refused || !clears.Equal(now.Add(agentGitHubBudgetWindow)) {
+		t.Fatalf("AgentGitHubRefusal = %v, %v, want refused until %v", clears, refused, now.Add(agentGitHubBudgetWindow))
+	}
+	if p.AgentGitHubRefusedSince("reviewer", now.Add(time.Second)) {
+		t.Fatal("a session asked after the refusal reads as refused")
+	}
+	if p.AgentGitHubRefusedSince("scanner", dispatched) {
+		t.Fatal("another agent's refusal leaked")
+	}
+
+	remaining = 100
+	if st := p.githubBudgetRefusal("scanner", http.MethodGet, "/repos/o/r/issues", true); st == nil || st.Reason != "reserve" {
+		t.Fatalf("scanner read below the floor = %+v, want a reserve refusal", st)
+	}
+	if !p.AgentGitHubRefusedSince("scanner", dispatched) {
+		t.Fatal("the reserve-floor refusal was not recorded")
+	}
+	if clears, _ := p.AgentGitHubRefusal("scanner", dispatched); !clears.Equal(now.Add(10 * time.Minute)) {
+		t.Fatalf("reserve refusal clears at %v, want the bucket reset %v", clears, now.Add(10*time.Minute))
+	}
+	if p.AgentGitHubRefusedSince(internalCallerName, dispatched) {
+		t.Fatal("the hive's own calls are never refused or recorded")
+	}
+	var nilProxy *GitHubProxy
+	if nilProxy.AgentGitHubRefusedSince("reviewer", dispatched) {
+		t.Fatal("a nil proxy reads as refused")
+	}
+	if _, refused := nilProxy.AgentGitHubRefusal("reviewer", dispatched); refused {
+		t.Fatal("a nil proxy's AgentGitHubRefusal reads as refused")
+	}
+}

src/docs/review-swarm.md

@@ -601,7 +601,7 @@ review:
 
 ### Pending-review expiry (`review.pending_ttl`, `review.max_redispatch`)
 
-A pending entry is the planner's only record that a perspective was asked, so a reviewer whose kick turn dies — model timeout, proxy read timeout, pod restart — leaves entries that post no verdict and, with no expiry, made every perspective look covered until someone pushed a new commit (hivecommons/hive#12045 and #12025 sat at queue position 1 for hours with CI green and the contributor lane stuck at `no-verdict`). Each cycle the planner therefore expires pending entries older than `pending_ttl` (Go duration string, default `45m`; a negative value disables expiry) whose head still has no verdict in the artifact: the expired perspectives are dropped from state and re-dispatched in the same cycle, the governor logs `review pending expired repo=… pr=… head=… perspective=… age=… expiries=N`, and the state file's `redispatches[]` counts expiries per head. Entries younger than the TTL are left alone, so a review genuinely in flight is never re-kicked, and a verdict for the head settles it however old the entries are. The same expiry releases a `revise_verdicts_before` revisit whose reviewer died.
+A pending entry is the planner's only record that a perspective was asked, so a reviewer whose kick turn dies — model timeout, proxy read timeout, pod restart — leaves entries that post no verdict and, with no expiry, made every perspective look covered until someone pushed a new commit (hivecommons/hive#12045 and #12025 sat at queue position 1 for hours with CI green and the contributor lane stuck at `no-verdict`). Each cycle the planner therefore expires pending entries older than `pending_ttl` (Go duration string, default `45m`; a negative value disables expiry) whose head still has no verdict in the artifact: the expired perspectives are dropped from state and re-dispatched in the same cycle, the governor logs `review pending expired repo=… pr=… head=… perspective=… age=… expiries=N cap_caused=…`, and the state file's `redispatches[]` counts expiries per head (`cap_redispatches[]` counts the ones the agent GitHub API cap caused; see below). Entries younger than the TTL are left alone, so a review genuinely in flight is never re-kicked, and a verdict for the head settles it however old the entries are. The same expiry releases a `revise_verdicts_before` revisit whose reviewer died.
 
 The retry is bounded. Once a head's expiry count reaches `max_redispatch` (default 3) the planner stops re-kicking it, leaves the entries in place, and records a `requires_human` hold whose reason starts `review-dispatch-exhausted`; like every other review hold it sends one `decision` escalation event per head, `review.human_decision_label` is applied the same way as for a reviewer's own requires_human verdict, the PR shows as "Your turn" (`review_dispatch_hold`), and the governor logs `review dispatch exhausted; holding PR for a human`. A new push clears the counter and the hold along with the head's pending entries.
 
@@ -611,6 +611,10 @@ Expiry also covers a head whose aggregate is `incomplete`: the delivered perspec
 
 **The same rule applies to report files already on disk.** A report written before the relay learned to skip these is still read by every verdict collect, so the collect applies the same check (`UnreadableReviewReason`) and sets such a file aside as missing; the refresh log line counts them as `unreadable_reports_skipped`. A stored `requires_human` for that head is withdrawn when those set-aside reports alone produced it: every perspective it holds at `requires_human`, and every human summary and security hold it quotes, must come from one of them, and it must carry no decision question, no in-scope finding and no exhausted fix cycle. The head then reads `incomplete` (or has no verdict yet), and the planner asks the missing perspectives again on the normal path, under `pending_ttl` and `max_redispatch`, not on every sweep. While a set-aside perspective is owed, the head is never `approve`: the contributor rule's hands-free approve waits for full coverage instead. A stored hold that anything else backs, including a real report no longer on disk, stays as it is (rule 6 above). On projectbluefin/testsuite#1116 the five pre-upgrade files kept the head at 085 hours after the upgrade until this check.
 
+**A review the proxy cut short is no verdict either, whatever it says.** Reviewers do not keep to the words the check above looks for. After the reviewer hit its 300/h cap on 2026-11-09 it wrote "GitHub reads stopped at the user's request after the hourly cap; … could not be verified" (projectbluefin/utah-packages#959) and "not assessed: GitHub API cap prevented reading" (projectbluefin/utah#1161), and both PRs parked at 085. So the relay also asks the GitHub proxy, which remembers the last time it refused each agent a request for its `agents.github_api_hourly_cap` or for `github.agent_reserve_floor` (`AgentGitHubRefusedSince`, `src/crux/proxy/agent_github_budget.go`). When the posting agent was refused at or after the time the perspective's open dispatch row was sent (or within the last hour, if no row says), a `requires_human` with no in-scope finding, no `hive-decision-risk: high` marker and no already-landed PR is not recorded (`review.CapStoppedReviewReason`); it is re-asked after `pending_ttl` like the case above. A decision question does not keep it, because a cut-short session asks for "a review with access to the changes" (projectbluefin/finpilot#659); a real question comes back from the re-review that reads the PR. An `approve` or `changes_requested` is still recorded. The test errs toward asking again: a refusal that hit the lane's previous session after this row was dispatched also counts. The record is in memory, so a restart forgets it, along with the counts. The watcher logs `requires_human from a review the GitHub proxy refused reads to is not recorded as a verdict`.
+
+**An expiry the cap caused does not spend `max_redispatch`.** Setting the report aside only moves the problem if the expiry that follows counts like a dead reviewer: three 45-minute TTLs is about 2h15m, and a reviewer capped that long on one head would still end in a `review-dispatch-exhausted` hold and ask a person about a PR nobody read. So the planner asks the same proxy record when a head's pending entries expire (`DispatchOptions.AgentGitHubRefusal`, `src/pkg/review/dispatch_agent_cap.go`). When every expired entry went to an agent the proxy refused at or after the entry was sent, whether or not the session posted anything, the expiry is the cap's doing. The planner then waits until the last refusal's `Retry-After` has passed (the sliding hour has a free slot, or the shared bucket has reset), for at most one hour past `pending_ttl`, because asking sooner would only be refused again; the entries stay in place meanwhile, so the perspective is not asked twice. Then it drops and re-asks them as usual, but counts the expiry in `cap_redispatches[]`, not `redispatches[]`, and logs it with `cap_caused=true`. That count has its own, larger bound, `DefaultMaxCapRedispatch` (8, about a working day at the default TTL, not a config key): a reviewer whose cap is too low to ever finish a review still ends in a `review-dispatch-exhausted` hold, with the reason `… ended at the agent GitHub API cap without a verdict`, instead of looping. One expired entry whose agent was not refused makes it an ordinary expiry, so a reviewer that dies or reports unreadable for any other reason still reaches the `max_redispatch` hold after three. The two counts are separate: cap-caused expiries neither add to nor reset `redispatches[]`. A restart forgets the proxy record, so an expiry right after a restart counts as ordinary.
+
 The PR review queue (`GET /api/review/queue`) shows this state on unreviewed items: `pending_since` (first dispatch for the current head still pending) and `redispatch_count`, plus a reason such as `review dispatched 32m ago, no verdict yet (re-dispatched 2x after expiry)` or `review dispatch exhausted: …; waiting on a human` after the generic `not reviewed at this head` line.
 
 When an aggregate verdict is `changes_requested`, Hive builds a review-fix kick containing the aggregate findings and sends it to `review.fixer_agent`, the classified PR lane, or `scanner`.