From 0363c4a2315adfa59251f0911598df753e9d1d46 Mon Sep 17 00:00:00 2001 From: Danathar Date: Mon, 9 Nov 2026 06:39:51 -0500 Subject: [PATCH 1/2] fix(review): a review the GitHub proxy cut short is no verdict, whatever its wording When the reviewer reached its agents.github_api_hourly_cap, the proxy refused its reads and the session finished with requires_human in its own words: "GitHub reads stopped at the user's request after the hourly cap; ... could not be verified" (projectbluefin/utah-packages#959), "not assessed: GitHub API cap prevented reading" (projectbluefin/utah#1161). review.UnreadableReviewReason (#16994) only knows "could not read"-style wording, so these were recorded and parked the PRs at 085. The proxy now remembers, per agent, when it last refused a request for the hourly cap or the reserve floor (AgentGitHubRefusedSince). The review relay asks it: a requires_human with no in-scope finding, no high-risk marker and no already-landed PR, from an agent refused at or after the perspective's dispatch time, is not recorded. The dispatch row stays open and the perspective is re-asked after review.pending_ttl, as in the #16994 case. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1 Signed-off-by: Danathar --- .../fixed-review-agent-cap-no-verdict.md | 1 + src/cmd/hive/main.go | 2 + src/crux/proxy/agent_github_budget.go | 60 ++++++- src/crux/proxy/agent_github_budget_test.go | 57 ++++++ src/docs/review-swarm.md | 2 + src/pkg/github/review_verdict_agent_cap.go | 98 ++++++++++ .../github/review_verdict_agent_cap_test.go | 169 ++++++++++++++++++ src/pkg/github/review_verdict_relay.go | 13 ++ src/pkg/review/unreadable.go | 44 +++++ src/pkg/review/unreadable_test.go | 72 ++++++++ 10 files changed, 514 insertions(+), 4 deletions(-) create mode 100644 changelog.d/fixed-review-agent-cap-no-verdict.md create mode 100644 src/pkg/github/review_verdict_agent_cap.go create mode 100644 src/pkg/github/review_verdict_agent_cap_test.go diff --git a/changelog.d/fixed-review-agent-cap-no-verdict.md b/changelog.d/fixed-review-agent-cap-no-verdict.md new file mode 100644 index 0000000000..a3a57f78a1 --- /dev/null +++ b/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. diff --git a/src/cmd/hive/main.go b/src/cmd/hive/main.go index 8aa123d530..0f83795437 100644 --- a/src/cmd/hive/main.go +++ b/src/cmd/hive/main.go @@ -4330,6 +4330,8 @@ 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 (projectbluefin/utah-packages#959). + github.SetAgentGitHubRefusedSinceFunc(b.githubProxy.AgentGitHubRefusedSince) if b.agentMgr != nil { b.githubProxy.SetAgentGitHubBudgetNudgeFunc(b.agentMgr.SendGitHubAPIBudgetNudge) } diff --git a/src/crux/proxy/agent_github_budget.go b/src/crux/proxy/agent_github_budget.go index 3f00143cc0..b2226e628d 100644 --- a/src/crux/proxy/agent_github_budget.go +++ b/src/crux/proxy/agent_github_budget.go @@ -26,6 +26,11 @@ type agentGitHubBudget struct { events map[string][]time.Time warned map[string]bool nudged map[string]time.Time + // refused is the last time each agent's GitHub request was refused 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. + refused map[string]time.Time } func newAgentGitHubBudget(now func() time.Time) *agentGitHubBudget { @@ -33,10 +38,11 @@ func newAgentGitHubBudget(now func() time.Time) *agentGitHubBudget { 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]time.Time), } } @@ -90,6 +96,32 @@ func (b *agentGitHubBudget) markNudged(agentName string) bool { return true } +// markRefused records that agentName was just refused a GitHub request. +func (b *agentGitHubBudget) markRefused(agentName string) { + if b == nil || strings.TrimSpace(agentName) == "" { + return + } + now := b.now() + b.mu.Lock() + defer b.mu.Unlock() + if b.refused == nil { + b.refused = make(map[string]time.Time) + } + b.refused[agentName] = now +} + +// refusedSince reports whether agentName was refused a GitHub request at or +// after since. +func (b *agentGitHubBudget) refusedSince(agentName string, since time.Time) bool { + if b == nil || strings.TrimSpace(agentName) == "" { + return false + } + b.mu.Lock() + defer b.mu.Unlock() + last, ok := b.refused[agentName] + return ok && !last.Before(since) +} + func (b *agentGitHubBudget) pruneLocked(agentName string, now time.Time) []time.Time { events := b.events[agentName] cutoff := now.Add(-agentGitHubBudgetWindow) @@ -140,10 +172,30 @@ 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 { + if p == nil || p.githubBudget == nil { + return 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) + } + }() cap := p.agentGitHubCap(agentName) if cap > 0 && p.githubBudget != nil { count, retryAfter, over := p.githubBudget.check(agentName, cap) diff --git a/src/crux/proxy/agent_github_budget_test.go b/src/crux/proxy/agent_github_budget_test.go index 102409c4d8..c17a9d446f 100644 --- a/src/crux/proxy/agent_github_budget_test.go +++ b/src/crux/proxy/agent_github_budget_test.go @@ -148,3 +148,60 @@ 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") + } + 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 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") + } +} diff --git a/src/docs/review-swarm.md b/src/docs/review-swarm.md index 7b256296b2..64302ece64 100644 --- a/src/docs/review-swarm.md +++ b/src/docs/review-swarm.md @@ -611,6 +611,8 @@ 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`. + 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`. diff --git a/src/pkg/github/review_verdict_agent_cap.go b/src/pkg/github/review_verdict_agent_cap.go new file mode 100644 index 0000000000..754ba8a3b3 --- /dev/null +++ b/src/pkg/github/review_verdict_agent_cap.go @@ -0,0 +1,98 @@ +package github + +import ( + "strings" + "sync" + "time" + + "github.com/hivecommons/hive/pkg/review" +) + +// agentCapSessionLookback bounds how far back a refusal counts against a +// review when no dispatch row says when the session was asked. It is the +// proxy's cap window: a refusal older than that came from a window the +// session cannot have shared. +const agentCapSessionLookback = time.Hour + +var ( + agentGitHubRefusedSinceMu sync.RWMutex + agentGitHubRefusedSinceFn func(agent string, since time.Time) bool +) + +// SetAgentGitHubRefusedSinceFunc installs (or, with nil, removes) the +// process-wide answer to "did the hive's GitHub proxy refuse this agent a +// request at or after since?", because the agent reached +// agents.github_api_hourly_cap or the shared bucket fell below +// github.agent_reserve_floor. The proxy is one per process, built after the +// GitHub client, so main.go installs its AgentGitHubRefusedSince once the +// proxy exists. Unset, the review relay sets nothing aside on that ground. +func SetAgentGitHubRefusedSinceFunc(fn func(agent string, since time.Time) bool) { + agentGitHubRefusedSinceMu.Lock() + agentGitHubRefusedSinceFn = fn + agentGitHubRefusedSinceMu.Unlock() +} + +func agentGitHubRefusedSince() func(agent string, since time.Time) bool { + agentGitHubRefusedSinceMu.RLock() + defer agentGitHubRefusedSinceMu.RUnlock() + return agentGitHubRefusedSinceFn +} + +// capStoppedReview reports whether report is a requires_human from a review +// session the proxy cut short: the posting agent was refused a GitHub read +// after this perspective was dispatched (review.CapStoppedReviewReason has +// which reports qualify). It returns the reviewer's summary as the reason. +// +// The test is the proxy's own record of the refusal, not the reviewer's +// wording: projectbluefin/utah-packages#959, utah#1161, finpilot#659 and +// fsdk-containers#491 all parked at 085 on 2026-11-09 with summaries +// review.UnreadableReviewReason did not recognise. +// +// It errs toward asking again. A reviewer lane runs one session at a time, +// so a refusal between the dispatch and the verdict may have hit the +// session before this one; the cost of that is one more review, never a +// verdict nobody read for. +func (c *Client) capStoppedReview(req ReviewRequest, report review.PerspectiveReport, now time.Time) (string, bool) { + refusedSince := agentGitHubRefusedSince() + if refusedSince == nil { + return "", false + } + agent := strings.TrimSpace(req.Agent) + if agent == "" { + return "", false + } + reason, ok := review.CapStoppedReviewReason(report) + if !ok { + return "", false + } + since := reviewPerspectiveDispatchedAt(report) + if since.IsZero() || since.After(now) { + since = now.Add(-agentCapSessionLookback) + } + if !refusedSince(agent, since) { + return "", false + } + return reason, true +} + +// reviewPerspectiveDispatchedAt is when the open dispatch row for report's +// PR and perspective was sent: the start of the session that answered it. +// With several rows (an older head still open) the latest wins. It is zero +// when the state cannot be read or holds no such row. +func reviewPerspectiveDispatchedAt(report review.PerspectiveReport) time.Time { + state, err := review.LoadDispatchState("") + if err != nil { + return time.Time{} + } + var latest time.Time + for _, p := range state.Pending { + if p.Number != report.Number || p.Perspective != report.Perspective || + !strings.EqualFold(strings.TrimSpace(p.Repo), strings.TrimSpace(report.Repo)) { + continue + } + if p.Dispatched.After(latest) { + latest = p.Dispatched + } + } + return latest +} diff --git a/src/pkg/github/review_verdict_agent_cap_test.go b/src/pkg/github/review_verdict_agent_cap_test.go new file mode 100644 index 0000000000..c566a5022a --- /dev/null +++ b/src/pkg/github/review_verdict_agent_cap_test.go @@ -0,0 +1,169 @@ +package github + +import ( + "context" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/hivecommons/hive/pkg/review" +) + +// The three summaries the reviewer wrote on projectbluefin/utah-packages#959 +// at head d09267d (2026-11-09), after the proxy refused its GitHub reads at +// its 300/h cap (01:33Z). None says "could not read", so the wording match +// (review.UnreadableReviewReason) let them through and the PR parked at 085. +var capStoppedSummaries959 = map[review.Perspective]string{ + review.PerspectiveCorrectness: "Previous review had no findings to re-check. Read the full PR diff and 11-file delta; commit history identifies the delta as a merge of main containing #957. No concrete new correctness defect found. GitHub reads stopped at the user's request after the hourly cap; head-file surroundings, source-lock consumers, build behavior and head-specific tests could not be verified. Requires completion of those checks before correctness approval.", + review.PerspectiveDocsCurrency: "Previous review had no findings to re-check. Delta documentation distinguishes recipe import from build, publication and runtime proof, consistent with the backlog record. No verified documentation defect found. Claims about HTTP rejection, source fallback behavior and build-root dependencies could not be checked against their consumers after GitHub reads stopped; documentation assessment remains incomplete.", + review.PerspectiveSecurity: "Previous review had no findings to re-check. Delta adds pinned firmware sources and a driver options file through a merge of main. No verified secret, injection or permission regression found in the diff. Source verification consumers and driver-option behavior could not be inspected after GitHub reads were stopped at the user's request; security assessment remains incomplete, not evidence of a vulnerability.", +} + +type capRefusalRecord struct { + agent string + at time.Time + asked []time.Time +} + +func (r *capRefusalRecord) refusedSince(agent string, since time.Time) bool { + r.asked = append(r.asked, since) + return agent == r.agent && !r.at.Before(since) +} + +func withAgentGitHubRefusals(t *testing.T, fn func(agent string, since time.Time) bool) { + t.Helper() + old := agentGitHubRefusedSince() + SetAgentGitHubRefusedSinceFunc(fn) + t.Cleanup(func() { SetAgentGitHubRefusedSinceFunc(old) }) +} + +func capStoppedReport(t *testing.T, repo string, num int, head string, perspectives []review.Perspective, verdict string) string { + t.Helper() + var elems []string + for _, p := range perspectives { + elems = append(elems, verdictJSONWithSummary(t, repo, num, p, verdict, head, capStoppedSummaries959[p])) + } + return "[" + strings.Join(elems, ",") + "]" +} + +var capStoppedPerspectives = []review.Perspective{review.PerspectiveCorrectness, review.PerspectiveDocsCurrency, review.PerspectiveSecurity} + +func capStoppedDispatch(t *testing.T, repo string, num int, head string, dispatched time.Time) { + t.Helper() + var pending []review.PendingReview + for _, p := range capStoppedPerspectives { + pending = append(pending, review.PendingReview{Repo: repo, Number: num, HeadSHA: head, Perspective: p, Agent: "reviewer", Dispatched: dispatched}) + } + withVerdictDispatchState(t, review.DispatchState{Pending: pending}) +} + +// projectbluefin/utah-packages#959: the proxy refused the reviewer's reads +// after the review was dispatched, and the reviewer said so in its own +// words. The relay must take the proxy's word for it, not the reviewer's: +// nothing recorded, no requires_human, the perspectives asked again after +// the pending TTL. +func TestRecordReviewVerdictCapStoppedSessionIsNoVerdict(t *testing.T) { + const ( + repo = "projectbluefin/utah-packages" + num = 959 + head = "d09267d5f3b0c2a1e4d6b8a9c7e5f3a1b2c4d6e8" + ) + for p, summary := range capStoppedSummaries959 { + r := review.PerspectiveReport{Perspective: p, Verdict: review.VerdictRequiresHuman} + r.Summary = summary + if _, ok := review.UnreadableReviewReason(r); ok { + t.Fatalf("test premise: the wording match already recognises %s: %q", p, summary) + } + } + now := time.Now().UTC() + dispatched := now.Add(-30 * time.Minute) + capStoppedDispatch(t, repo, num, head, dispatched) + dir := t.TempDir() + refusals := &capRefusalRecord{agent: "reviewer", at: now.Add(-20 * time.Minute)} + c := &Client{logger: testLogger()} + withAgentGitHubRefusals(t, refusals.refusedSince) + + recorded := c.recordReviewVerdict(context.Background(), ReviewRequest{ + Repo: repo, Number: num, Agent: "reviewer", Event: ReviewEventRecordVerdict, + Report: capStoppedReport(t, repo, num, head, capStoppedPerspectives, "requires_human"), + }, dir) + + if len(recorded) != 0 { + t.Fatalf("recorded %d verdicts from a session the proxy cut short: %+v", len(recorded), recorded) + } + if matches, _ := filepath.Glob(filepath.Join(dir, review.ReviewReportFilePrefix+"*")); len(matches) != 0 { + t.Fatalf("cut-short review was written where the collector reads verdicts: %v", matches) + } + if len(refusals.asked) == 0 || !refusals.asked[0].Equal(dispatched) { + t.Fatalf("refusal window started at %v, want the dispatch time %v", refusals.asked, dispatched) + } + + state, err := review.LoadDispatchState("") + if err != nil { + t.Fatalf("load dispatch state: %v", err) + } + artifact, err := review.Collect(dir, review.AggregateOptions{}) + if err != nil { + t.Fatalf("Collect: %v", err) + } + pr := review.PullRequest{Repo: repo, Number: num, Title: "firmware sources", Author: "hanthor", HeadSHA: head} + opts := review.DispatchOptions{ + RequireApproval: true, + FanOut: true, + CombinedPerspectives: true, + MaxParallelReviews: 5, + Agents: []review.AgentCapability{{Name: "reviewer", Enabled: true, UsesKick: true, Role: "reviewer"}}, + Now: dispatched.Add(review.DefaultPendingReviewTTL + time.Minute), + } + retry := review.PlanDispatch([]review.PullRequest{pr}, artifact, state, opts) + if len(retry.NewHuman) != 0 || len(retry.State.Human) != 0 { + t.Fatalf("a cut-short review asked a person: new=%+v state=%+v", retry.NewHuman, retry.State.Human) + } + if len(retry.ReviewKicks) != 1 { + t.Fatalf("review kicks after the TTL = %d, want one retry", len(retry.ReviewKicks)) + } +} + +// The same requires_human is a verdict when the proxy refused nothing during +// the session: a refusal before the dispatch belongs to an earlier session, +// one for another agent is not this reviewer's, and an approve is a +// judgement whatever the proxy did. +func TestRecordReviewVerdictKeepsRequiresHumanWithoutARefusalInTheSession(t *testing.T) { + const ( + repo = "o/r" + num = 11 + head = "abc123" + ) + now := time.Now().UTC() + dispatched := now.Add(-30 * time.Minute) + cases := []struct { + name string + refusals *capRefusalRecord + verdict string + }{ + {"no proxy wired", nil, "requires_human"}, + {"refused before the dispatch", &capRefusalRecord{agent: "reviewer", at: dispatched.Add(-time.Minute)}, "requires_human"}, + {"another agent refused", &capRefusalRecord{agent: "scanner", at: now.Add(-time.Minute)}, "requires_human"}, + {"approve is a judgement", &capRefusalRecord{agent: "reviewer", at: now.Add(-time.Minute)}, "approve"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + capStoppedDispatch(t, repo, num, head, dispatched) + dir := t.TempDir() + c := &Client{logger: testLogger()} + if tc.refusals != nil { + withAgentGitHubRefusals(t, tc.refusals.refusedSince) + } else { + withAgentGitHubRefusals(t, nil) + } + recorded := c.recordReviewVerdict(context.Background(), ReviewRequest{ + Repo: repo, Number: num, Agent: "reviewer", + Report: capStoppedReport(t, repo, num, head, capStoppedPerspectives, tc.verdict), + }, dir) + if len(recorded) != len(capStoppedPerspectives) { + t.Fatalf("recorded %d of %d verdicts: %+v", len(recorded), len(capStoppedPerspectives), recorded) + } + }) + } +} diff --git a/src/pkg/github/review_verdict_relay.go b/src/pkg/github/review_verdict_relay.go index a6c5e9e94d..f81bffdcb2 100644 --- a/src/pkg/github/review_verdict_relay.go +++ b/src/pkg/github/review_verdict_relay.go @@ -123,6 +123,19 @@ func (c *Client) recordReviewVerdict(ctx context.Context, req ReviewRequest, dir slog.String("agent", req.Agent), slog.String("reason", reason)) continue } + if reason, stopped := c.capStoppedReview(req, report, time.Now().UTC()); stopped { + // The proxy refused this agent's GitHub reads after the + // perspective was asked (its hourly cap or the reserve floor), + // so the requires_human is what a cut-short session says, in + // whatever words, not a judgement. Set aside like the + // unreadable case above: re-asked after review.pending_ttl + // (projectbluefin/utah-packages#959). + c.logger.Info("review-request watcher: requires_human from a review the GitHub proxy refused reads to is not recorded as a verdict; the perspective is re-asked after the pending TTL", + slog.String("repo", req.Repo), slog.Int("number", req.Number), + slog.String("perspective", string(report.Perspective)), + slog.String("agent", req.Agent), slog.String("reason", reason)) + continue + } if written, ok, dispatched := c.writeOneVerdict(ctx, req, report, dir, prHead, prSnapshot); ok { recorded = append(recorded, written) wake = wake || dispatched diff --git a/src/pkg/review/unreadable.go b/src/pkg/review/unreadable.go index 9d206ac779..a8caa83eef 100644 --- a/src/pkg/review/unreadable.go +++ b/src/pkg/review/unreadable.go @@ -78,3 +78,47 @@ func UnreadableReviewReason(r PerspectiveReport) (string, bool) { } return "", false } + +// CapStoppedReviewReason reports whether r is a requires_human that the +// relay should set aside because the hive proxy refused the reviewer's +// GitHub reads during the session (its agents.github_api_hourly_cap or the +// shared reserve floor). The caller supplies that fact from the proxy's own +// record; this decides only whether the report is the kind such a refusal +// can produce. It returns the reviewer's summary as the reason. +// +// UnreadableReviewReason needs the reviewer to say, in words it knows, that +// a rate limit kept it from reading. Reviewers do not keep to those words: +// on projectbluefin/utah-packages#959 (2026-11-09) the reviewer wrote +// "GitHub reads stopped at the user's request after the hourly cap; … +// could not be verified", and on projectbluefin/utah#1161 "not assessed: +// GitHub API cap prevented reading". Each parked its PR at 085. The cap +// event itself is the evidence, so the wording does not matter here. +// +// The report must: +// - be requires_human (an approve or changes_requested is a judgement); +// - not carry the reviewer's explicit high-risk marker; +// - raise no in-scope finding (a finding is something it read and judged); +// - not name a PR that already carries the change (RedundantOf), which the +// hive's own already-landed close acts on. +// +// A decision question does not keep it: a session the proxy cut short asks +// for "a review with access to the changes" (projectbluefin/finpilot#659), +// and a real question is asked again by the re-review that reads the PR. +func CapStoppedReviewReason(r PerspectiveReport) (string, bool) { + if r.Verdict != VerdictRequiresHuman || r.DecisionRiskHigh || r.RedundantOf > 0 { + return "", false + } + for _, f := range r.Findings { + if InScopeFinding(f) { + return "", false + } + } + reason := strings.TrimSpace(r.Summary) + if reason == "" { + reason = strings.TrimSpace(r.DecisionQuestion) + } + if reason == "" { + reason = "requires_human from a review session the hive proxy refused GitHub reads" + } + return reason, true +} diff --git a/src/pkg/review/unreadable_test.go b/src/pkg/review/unreadable_test.go index 9636823193..e45e7b0e37 100644 --- a/src/pkg/review/unreadable_test.go +++ b/src/pkg/review/unreadable_test.go @@ -94,3 +94,75 @@ func unreadableTestReport(p Perspective, v Verdict, summary string, findings []o r.Findings = findings return r } + +// Summaries the reviewer wrote on 2026-11-09 after the proxy refused its +// GitHub reads at its 300/h cap. Each parked its PR at 085, because none says +// "could not read" in the words UnreadableReviewReason knows. +var capStoppedSummaries = []string{ + // projectbluefin/utah-packages#959 @d09267d + "Previous review had no findings to re-check. Read the full PR diff and 11-file delta; commit history identifies the delta as a merge of main containing #957. No concrete new correctness defect found. GitHub reads stopped at the user's request after the hourly cap; head-file surroundings, source-lock consumers, build behavior and head-specific tests could not be verified. Requires completion of those checks before correctness approval.", + "Previous review had no findings to re-check. Delta documentation distinguishes recipe import from build, publication and runtime proof, consistent with the backlog record. No verified documentation defect found. Claims about HTTP rejection, source fallback behavior and build-root dependencies could not be checked against their consumers after GitHub reads stopped; documentation assessment remains incomplete.", + "Previous review had no findings to re-check. Delta adds pinned firmware sources and a driver options file through a merge of main. No verified secret, injection or permission regression found in the diff. Source verification consumers and driver-option behavior could not be inspected after GitHub reads were stopped at the user's request; security assessment remains incomplete, not evidence of a vulnerability.", + // projectbluefin/utah#1161 @615fe80 + "Correctness not assessed: GitHub returned HTTP 429 before PR body or diff could be read. Available checkout is utah-packages, lacks requested head, and no Utah checkout was located. Requires review with repository access; no defect or CI claim asserted.", + "Documentation currency not assessed: GitHub API cap prevented reading changes and related documentation; requested head unavailable locally. Cannot determine whether operator-facing descriptions remain accurate.", + "Security not assessed: GitHub API hourly cap prevented reading PR body, diff, and review threads; requested repository and head unavailable locally. Cannot evaluate secret exposure or trust-boundary changes without code.", +} + +func TestCapStoppedReviewReasonDoesNotDependOnWording(t *testing.T) { + for _, summary := range capStoppedSummaries { + r := unreadableTestReport(PerspectiveCorrectness, VerdictRequiresHuman, summary, nil) + if _, ok := UnreadableReviewReason(r); ok { + t.Fatalf("test premise: the wording match recognises %q", summary) + } + reason, ok := CapStoppedReviewReason(r) + if !ok { + t.Errorf("requires_human from a cut-short session not set aside: %q", summary) + continue + } + if reason != summary { + t.Errorf("reason = %q, want the reviewer's summary", reason) + } + } + // projectbluefin/finpilot#659: the cut-short session asked the owner for + // "a review with access to the proposed changes". + asked := unreadableTestReport(PerspectiveCorrectness, VerdictRequiresHuman, "", nil) + asked.DecisionRequested = true + asked.DecisionQuestion = "obtain a review with access to the proposed changes before deciding whether to merge." + if reason, ok := CapStoppedReviewReason(asked); !ok || reason != asked.DecisionQuestion { + t.Errorf("decision question from a cut-short session = %q, %v; want set aside with the question as reason", reason, ok) + } +} + +func TestCapStoppedReviewReasonKeepsJudgements(t *testing.T) { + summary := capStoppedSummaries[0] + cases := []struct { + name string + report PerspectiveReport + }{ + {"approve is a judgement", unreadableTestReport(PerspectiveSecurity, VerdictApprove, summary, nil)}, + {"changes_requested is a judgement", unreadableTestReport(PerspectiveSecurity, VerdictChangesRequested, summary, nil)}, + {"an in-scope finding means it read and judged something", unreadableTestReport(PerspectiveSecurity, VerdictRequiresHuman, summary, + []outputschema.Finding{{Title: "secret leaks into logs", Severity: outputschema.SeverityHigh, File: "a.go", Line: 3}})}, + } + for _, tc := range cases { + if reason, ok := CapStoppedReviewReason(tc.report); ok { + t.Errorf("%s: set aside (%q), want kept", tc.name, reason) + } + } + marked := unreadableTestReport(PerspectiveSecurity, VerdictRequiresHuman, summary, nil) + marked.DecisionRiskHigh = true + if _, ok := CapStoppedReviewReason(marked); ok { + t.Error("the reviewer's explicit high-risk marker was overridden") + } + redundant := unreadableTestReport(PerspectiveIntentAlignment, VerdictRequiresHuman, summary, nil) + redundant.RedundantOf = 957 + if _, ok := CapStoppedReviewReason(redundant); ok { + t.Error("an already-landed report was set aside") + } + outOfScope := unreadableTestReport(PerspectiveSecurity, VerdictRequiresHuman, summary, + []outputschema.Finding{{Title: "pre-existing", Severity: outputschema.SeverityLow, ReviewScope: "out-of-scope"}}) + if _, ok := CapStoppedReviewReason(outOfScope); !ok { + t.Error("an out-of-scope note kept a cut-short requires_human as a verdict") + } +} From 81bcae9bface2796dac9bb6babe3503b17cda7cd Mon Sep 17 00:00:00 2001 From: Danathar Date: Mon, 9 Nov 2026 07:01:23 -0500 Subject: [PATCH 2/2] fix(review): an expiry the agent GitHub API cap caused does not spend max_redispatch The previous commit sets aside a requires_human from a session the proxy refused at the agent cap, and the perspective is asked again after review.pending_ttl. But the expiry that follows still counted against review.max_redispatch (3), so a reviewer capped for about 2h15m on one head (three 45m TTLs) still parked it as review-dispatch-exhausted and a person was asked about a PR nobody had read. The proxy now also remembers when each refusal said the agent could read again (its Retry-After: the sliding hour freeing a slot, or the shared bucket's reset), and exposes it as GitHubProxy.AgentGitHubRefusal. The same process-wide hook feeds the relay and, through review.DispatchOptions.AgentGitHubRefusal, the planner. When every row of an expiry went to an agent the proxy refused after the row was sent, the planner waits for the refusal to clear (at most an hour past the TTL), then re-asks and counts the expiry in the new cap_redispatches[] instead of redispatches[]. That count is bounded separately (DefaultMaxCapRedispatch = 8) and ends in the same review-dispatch-exhausted hold, so a reviewer that can never read still reaches a person instead of looping. Any other expiry counts as before. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1 Signed-off-by: Danathar --- .../fixed-review-agent-cap-no-verdict.md | 2 +- src/cmd/hive/main.go | 5 +- src/cmd/hive/main_sweeps.go | 3 +- src/crux/proxy/agent_github_budget.go | 58 +++-- src/crux/proxy/agent_github_budget_test.go | 12 + src/docs/review-swarm.md | 4 +- src/pkg/github/review_verdict_agent_cap.go | 39 +-- .../github/review_verdict_agent_cap_test.go | 114 ++++++++- src/pkg/review/dispatch.go | 46 +++- src/pkg/review/dispatch_agent_cap.go | 127 +++++++++ src/pkg/review/dispatch_agent_cap_test.go | 242 ++++++++++++++++++ 11 files changed, 606 insertions(+), 46 deletions(-) create mode 100644 src/pkg/review/dispatch_agent_cap.go create mode 100644 src/pkg/review/dispatch_agent_cap_test.go diff --git a/changelog.d/fixed-review-agent-cap-no-verdict.md b/changelog.d/fixed-review-agent-cap-no-verdict.md index a3a57f78a1..150137f5c8 100644 --- a/changelog.d/fixed-review-agent-cap-no-verdict.md +++ b/changelog.d/fixed-review-agent-cap-no-verdict.md @@ -1 +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. +- 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. diff --git a/src/cmd/hive/main.go b/src/cmd/hive/main.go index 0f83795437..21d1a9cc53 100644 --- a/src/cmd/hive/main.go +++ b/src/cmd/hive/main.go @@ -4330,8 +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 (projectbluefin/utah-packages#959). - github.SetAgentGitHubRefusedSinceFunc(b.githubProxy.AgentGitHubRefusedSince) + // 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) } diff --git a/src/cmd/hive/main_sweeps.go b/src/cmd/hive/main_sweeps.go index b77563f449..5df2e7e7df 100644 --- a/src/cmd/hive/main_sweeps.go +++ b/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 { diff --git a/src/crux/proxy/agent_github_budget.go b/src/crux/proxy/agent_github_budget.go index b2226e628d..d76bd28647 100644 --- a/src/crux/proxy/agent_github_budget.go +++ b/src/crux/proxy/agent_github_budget.go @@ -26,11 +26,21 @@ type agentGitHubBudget struct { events map[string][]time.Time warned map[string]bool nudged map[string]time.Time - // refused is the last time each agent's GitHub request was refused by - // its hourly cap or the shared reserve floor. The review relay reads it + // 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. - refused map[string]time.Time + // 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 { @@ -42,7 +52,7 @@ func newAgentGitHubBudget(now func() time.Time) *agentGitHubBudget { events: make(map[string][]time.Time), warned: make(map[string]bool), nudged: make(map[string]time.Time), - refused: make(map[string]time.Time), + refused: make(map[string]agentGitHubRefusal), } } @@ -96,30 +106,37 @@ func (b *agentGitHubBudget) markNudged(agentName string) bool { return true } -// markRefused records that agentName was just refused a GitHub request. -func (b *agentGitHubBudget) markRefused(agentName string) { +// 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]time.Time) + b.refused = make(map[string]agentGitHubRefusal) } - b.refused[agentName] = now + 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. -func (b *agentGitHubBudget) refusedSince(agentName string, since time.Time) bool { +// 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 false + return time.Time{}, false } b.mu.Lock() defer b.mu.Unlock() last, ok := b.refused[agentName] - return ok && !last.Before(since) + 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 { @@ -181,8 +198,19 @@ func (p *GitHubProxy) agentGitHubReserveSnapshot() (remaining int, reset time.Ti // (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 false + return time.Time{}, false } return p.githubBudget.refusedSince(agentName, since) } @@ -193,7 +221,7 @@ func (p *GitHubProxy) githubBudgetRefusal(agentName, method, path string, isRead } defer func() { if status != nil { - p.githubBudget.markRefused(agentName) + p.githubBudget.markRefused(agentName, status.RetryAfter) } }() cap := p.agentGitHubCap(agentName) diff --git a/src/crux/proxy/agent_github_budget_test.go b/src/crux/proxy/agent_github_budget_test.go index c17a9d446f..2583a40896 100644 --- a/src/crux/proxy/agent_github_budget_test.go +++ b/src/crux/proxy/agent_github_budget_test.go @@ -183,6 +183,12 @@ func TestAgentGitHubRefusedSinceRecordsCapAndReserveRefusals(t *testing.T) { 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") } @@ -197,6 +203,9 @@ func TestAgentGitHubRefusedSinceRecordsCapAndReserveRefusals(t *testing.T) { 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") } @@ -204,4 +213,7 @@ func TestAgentGitHubRefusedSinceRecordsCapAndReserveRefusals(t *testing.T) { 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") + } } diff --git a/src/docs/review-swarm.md b/src/docs/review-swarm.md index 64302ece64..cb3bae6976 100644 --- a/src/docs/review-swarm.md +++ b/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. @@ -613,6 +613,8 @@ Expiry also covers a head whose aggregate is `incomplete`: the delivered perspec **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`. diff --git a/src/pkg/github/review_verdict_agent_cap.go b/src/pkg/github/review_verdict_agent_cap.go index 754ba8a3b3..4574ab7ad6 100644 --- a/src/pkg/github/review_verdict_agent_cap.go +++ b/src/pkg/github/review_verdict_agent_cap.go @@ -15,27 +15,32 @@ import ( const agentCapSessionLookback = time.Hour var ( - agentGitHubRefusedSinceMu sync.RWMutex - agentGitHubRefusedSinceFn func(agent string, since time.Time) bool + agentGitHubRefusalMu sync.RWMutex + agentGitHubRefusalFn review.AgentGitHubRefusal ) -// SetAgentGitHubRefusedSinceFunc installs (or, with nil, removes) the +// SetAgentGitHubRefusalFunc installs (or, with nil, removes) the // process-wide answer to "did the hive's GitHub proxy refuse this agent a -// request at or after since?", because the agent reached +// request at or after since, and when did it say the agent could read +// again?", for refusals because the agent reached // agents.github_api_hourly_cap or the shared bucket fell below // github.agent_reserve_floor. The proxy is one per process, built after the -// GitHub client, so main.go installs its AgentGitHubRefusedSince once the -// proxy exists. Unset, the review relay sets nothing aside on that ground. -func SetAgentGitHubRefusedSinceFunc(fn func(agent string, since time.Time) bool) { - agentGitHubRefusedSinceMu.Lock() - agentGitHubRefusedSinceFn = fn - agentGitHubRefusedSinceMu.Unlock() +// GitHub client, so main.go installs its AgentGitHubRefusal once the proxy +// exists. Unset, the review relay sets nothing aside on that ground and the +// planner counts every expiry against review.max_redispatch. +func SetAgentGitHubRefusalFunc(fn review.AgentGitHubRefusal) { + agentGitHubRefusalMu.Lock() + agentGitHubRefusalFn = fn + agentGitHubRefusalMu.Unlock() } -func agentGitHubRefusedSince() func(agent string, since time.Time) bool { - agentGitHubRefusedSinceMu.RLock() - defer agentGitHubRefusedSinceMu.RUnlock() - return agentGitHubRefusedSinceFn +// AgentGitHubRefusalFunc is the installed proxy record, or nil. The review +// planner passes it as review.DispatchOptions.AgentGitHubRefusal, so an +// expiry the cap caused does not spend review.max_redispatch. +func AgentGitHubRefusalFunc() review.AgentGitHubRefusal { + agentGitHubRefusalMu.RLock() + defer agentGitHubRefusalMu.RUnlock() + return agentGitHubRefusalFn } // capStoppedReview reports whether report is a requires_human from a review @@ -53,8 +58,8 @@ func agentGitHubRefusedSince() func(agent string, since time.Time) bool { // session before this one; the cost of that is one more review, never a // verdict nobody read for. func (c *Client) capStoppedReview(req ReviewRequest, report review.PerspectiveReport, now time.Time) (string, bool) { - refusedSince := agentGitHubRefusedSince() - if refusedSince == nil { + refusal := AgentGitHubRefusalFunc() + if refusal == nil { return "", false } agent := strings.TrimSpace(req.Agent) @@ -69,7 +74,7 @@ func (c *Client) capStoppedReview(req ReviewRequest, report review.PerspectiveRe if since.IsZero() || since.After(now) { since = now.Add(-agentCapSessionLookback) } - if !refusedSince(agent, since) { + if _, refused := refusal(agent, since); !refused { return "", false } return reason, true diff --git a/src/pkg/github/review_verdict_agent_cap_test.go b/src/pkg/github/review_verdict_agent_cap_test.go index c566a5022a..20fe59f797 100644 --- a/src/pkg/github/review_verdict_agent_cap_test.go +++ b/src/pkg/github/review_verdict_agent_cap_test.go @@ -26,16 +26,19 @@ type capRefusalRecord struct { asked []time.Time } -func (r *capRefusalRecord) refusedSince(agent string, since time.Time) bool { +func (r *capRefusalRecord) refusedSince(agent string, since time.Time) (time.Time, bool) { r.asked = append(r.asked, since) - return agent == r.agent && !r.at.Before(since) + if agent != r.agent || r.at.Before(since) { + return time.Time{}, false + } + return r.at.Add(10 * time.Minute), true } -func withAgentGitHubRefusals(t *testing.T, fn func(agent string, since time.Time) bool) { +func withAgentGitHubRefusals(t *testing.T, fn review.AgentGitHubRefusal) { t.Helper() - old := agentGitHubRefusedSince() - SetAgentGitHubRefusedSinceFunc(fn) - t.Cleanup(func() { SetAgentGitHubRefusedSinceFunc(old) }) + old := AgentGitHubRefusalFunc() + SetAgentGitHubRefusalFunc(fn) + t.Cleanup(func() { SetAgentGitHubRefusalFunc(old) }) } func capStoppedReport(t *testing.T, repo string, num int, head string, perspectives []review.Perspective, verdict string) string { @@ -167,3 +170,102 @@ func TestRecordReviewVerdictKeepsRequiresHumanWithoutARefusalInTheSession(t *tes }) } } + +// One cap-stopped session is fine; the gap was what came after. The relay +// set each requires_human aside, the row expired, and every expiry spent one +// of review.max_redispatch's three tries, so a reviewer capped for about +// 2h15m on projectbluefin/utah-packages#959's head still parked it as +// review-dispatch-exhausted and asked a person. Four capped sessions in a row +// now leave the head unheld and asked again; the same four sessions set +// aside on wording alone, with no proxy refusal behind them, still exhaust +// after three. +func TestCapStoppedSessionsInARowDoNotExhaustTheRedispatchBudget(t *testing.T) { + const ( + repo = "projectbluefin/utah-packages" + num = 959 + head = "d09267d5f3b0c2a1e4d6b8a9c7e5f3a1b2c4d6e8" + ) + pr := review.PullRequest{Repo: repo, Number: num, Title: "firmware sources", Author: "hanthor", HeadSHA: head} + cases := []struct { + name string + summaries map[review.Perspective]string + refused bool + wantHold int // the session after which the head is held; 0 = never within the run + }{ + {"proxy refused every session", capStoppedSummaries959, true, 0}, + {"set aside on wording, no proxy refusal", rateLimitedSummaries1116, false, review.DefaultMaxRedispatch}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Start far enough back that every session in the run is in + // the past, as the relay's lookback expects. + dispatched := time.Now().UTC().Add(-6 * time.Hour) + capStoppedDispatch(t, repo, num, head, dispatched) + var lastRefusal time.Time + refusal := func(agent string, since time.Time) (time.Time, bool) { + if !tc.refused || agent != "reviewer" || lastRefusal.Before(since) { + return time.Time{}, false + } + return lastRefusal.Add(10 * time.Minute), true + } + withAgentGitHubRefusals(t, refusal) + c := &Client{logger: testLogger()} + dir := t.TempDir() + for session := 1; session <= review.DefaultMaxRedispatch+1; session++ { + // The proxy refuses the session ten minutes in, and the + // session reports requires_human in its own words. + lastRefusal = dispatched.Add(10 * time.Minute) + var elems []string + for _, p := range capStoppedPerspectives { + elems = append(elems, verdictJSONWithSummary(t, repo, num, p, "requires_human", head, tc.summaries[p])) + } + recorded := c.recordReviewVerdict(context.Background(), ReviewRequest{ + Repo: repo, Number: num, Agent: "reviewer", Event: ReviewEventRecordVerdict, + Report: "[" + strings.Join(elems, ",") + "]", + }, dir) + if len(recorded) != 0 { + t.Fatalf("session %d: recorded %d verdicts from a set-aside session", session, len(recorded)) + } + + state, err := review.LoadDispatchState("") + if err != nil { + t.Fatalf("load dispatch state: %v", err) + } + artifact, err := review.Collect(dir, review.AggregateOptions{}) + if err != nil { + t.Fatalf("Collect: %v", err) + } + now := dispatched.Add(review.DefaultPendingReviewTTL + time.Minute) + plan := review.PlanDispatch([]review.PullRequest{pr}, artifact, state, review.DispatchOptions{ + RequireApproval: true, + FanOut: true, + CombinedPerspectives: true, + MaxParallelReviews: 5, + Agents: []review.AgentCapability{{Name: "reviewer", Enabled: true, UsesKick: true, Role: "reviewer"}}, + AgentGitHubRefusal: AgentGitHubRefusalFunc(), + Now: now, + }) + held := len(plan.Exhausted) > 0 + if tc.wantHold == session { + if !held || len(plan.NewHuman) != 1 { + t.Fatalf("session %d: want the review-dispatch-exhausted hold, got exhausted=%+v new=%+v", session, plan.Exhausted, plan.NewHuman) + } + return + } + if held || len(plan.NewHuman) != 0 || len(plan.State.Human) != 0 { + t.Fatalf("session %d: a person was asked: exhausted=%+v human=%+v", session, plan.Exhausted, plan.State.Human) + } + if len(plan.ReviewKicks) != 1 { + t.Fatalf("session %d: review kicks = %d, want the head asked again", session, len(plan.ReviewKicks)) + } + if err := review.WriteDispatchState("", plan.State); err != nil { + t.Fatalf("write dispatch state: %v", err) + } + dispatched = now + } + if tc.wantHold != 0 { + t.Fatalf("never reached the hold expected after session %d", tc.wantHold) + } + }) + } +} diff --git a/src/pkg/review/dispatch.go b/src/pkg/review/dispatch.go index f39b88efd7..f52751e269 100644 --- a/src/pkg/review/dispatch.go +++ b/src/pkg/review/dispatch.go @@ -235,6 +235,16 @@ type DispatchOptions struct { // before the PR is held for a human instead of re-kicked. Zero means // DefaultMaxRedispatch. MaxRedispatch int + // AgentGitHubRefusal, when set, is the hive GitHub proxy's record of the + // agents it refused for agents.github_api_hourly_cap or + // github.agent_reserve_floor. An expiry whose every row went to an agent + // refused after the row was sent is the cap's doing, not the + // reviewer's: it waits for the refusal to clear and counts against + // MaxCapRedispatch instead of MaxRedispatch (see expireCapCaused). + AgentGitHubRefusal AgentGitHubRefusal + // MaxCapRedispatch caps the expiries the agent cap caused for one head. + // Zero means DefaultMaxCapRedispatch. + MaxCapRedispatch int // PendingFixTTL is how long a pending fix row stays authoritative with // no new head before the next bounded attempt replaces it // (review.pending_fix_ttl). Zero means @@ -285,6 +295,11 @@ type DispatchState struct { // bounded number of retries and then a human hold, rather than either an // indefinite stall or an endless re-kick. Redispatches []RedispatchCount `json:"redispatches,omitempty"` + // CapRedispatches counts, per head, the expiries the agent GitHub API + // cap caused (expireCapCaused). They are kept apart from Redispatches so + // a reviewer capped for a few hours does not spend max_redispatch and + // ask a person, while a reviewer capped for good still ends in a hold. + CapRedispatches []RedispatchCount `json:"cap_redispatches,omitempty"` // Review heads (hivecommons/hive#13542). HeadRetries counts each // (PR head, perspective, head)'s expiries against review.head_max_retries; // MissingHeads are the heads that ran out while another head answered, @@ -370,8 +385,12 @@ type ExpiredPending struct { Agent string Dispatched time.Time Age time.Duration - // Count is the head's expiry count after this one. + // Count is the head's expiry count after this one: its max_redispatch + // count, or its cap count when CapCaused. Count int + // CapCaused marks an expiry the agent GitHub API cap caused; it counts + // against MaxCapRedispatch, not MaxRedispatch. + CapCaused bool } // WithheldFix is a fix kick the planner declined to dispatch. It carries what @@ -1421,7 +1440,9 @@ func (o DispatchOptions) effectiveMaxRedispatch() int { // them, and reports whether it dropped any. Each cycle that drops something // counts one expiry against the head; once the count reaches MaxRedispatch // the entries are left in place (so nothing is re-kicked) and the PR is held -// for a human with a HoldReasonDispatchExhausted reason. +// for a human with a HoldReasonDispatchExhausted reason. An expiry the agent +// GitHub API cap caused waits for the cap and counts against +// MaxCapRedispatch instead (capCausedExpiry, expireCapCaused). // // An entry with no dispatched_at has no measurable age and is never expired. func (p *DispatchPlan) expirePending(pr PullRequest, opts DispatchOptions, now time.Time) bool { @@ -1443,6 +1464,12 @@ func (p *DispatchPlan) expirePending(pr PullRequest, opts DispatchOptions, now t if len(expired) == 0 { return false } + if capCaused, wait := capCausedExpiry(expired, opts, ttl, now); capCaused { + if wait { + return false + } + return p.expireCapCaused(pr, expired, opts, ttl, now) + } idx := redispatchIndex(p.State.Redispatches, pr) if idx < 0 { p.State.Redispatches = append(p.State.Redispatches, RedispatchCount{Repo: pr.Repo, Number: pr.Number, HeadSHA: pr.HeadSHA}) @@ -1466,6 +1493,13 @@ func (p *DispatchPlan) expirePending(pr PullRequest, opts DispatchOptions, now t p.Exhausted = append(p.Exhausted, hold) return false } + p.dropExpiredPending(pr, ttl, now) + return true +} + +// dropExpiredPending removes this head's pending entries that outlived ttl, +// so pendingMissingPerspectives asks those perspectives again. +func (p *DispatchPlan) dropExpiredPending(pr PullRequest, ttl time.Duration, now time.Time) { kept := p.State.Pending[:0] for _, e := range p.State.Pending { if samePRHead(e.Repo, e.Number, e.HeadSHA, pr.Repo, pr.Number, pr.HeadSHA) && !e.Dispatched.IsZero() && now.Sub(e.Dispatched) >= ttl { @@ -1474,7 +1508,6 @@ func (p *DispatchPlan) expirePending(pr PullRequest, opts DispatchOptions, now t kept = append(kept, e) } p.State.Pending = kept - return true } // redispatchExhausted reports whether this head already spent its @@ -2214,6 +2247,13 @@ func pruneState(state DispatchState, prs []PullRequest, org string) DispatchStat } } state.Redispatches = redispatches + var capRedispatches []RedispatchCount + for _, r := range state.CapRedispatches { + if openHeads[reviewKey(r.Repo, r.Number, r.HeadSHA, "")] { + capRedispatches = append(capRedispatches, r) + } + } + state.CapRedispatches = capRedispatches var starved []RevisitStarvation for _, s := range state.RevisitStarved { if openHeads[reviewKey(s.Repo, s.Number, s.HeadSHA, "")] { diff --git a/src/pkg/review/dispatch_agent_cap.go b/src/pkg/review/dispatch_agent_cap.go new file mode 100644 index 0000000000..ab4f43a358 --- /dev/null +++ b/src/pkg/review/dispatch_agent_cap.go @@ -0,0 +1,127 @@ +package review + +import ( + "fmt" + "strings" + "time" +) + +// AgentGitHubRefusal answers whether the hive's GitHub proxy refused one of +// agent's requests at or after since, because the agent reached +// agents.github_api_hourly_cap or the shared bucket fell below +// github.agent_reserve_floor, and when the last refusal said the agent could +// read again (its Retry-After). crux/proxy's GitHubProxy.AgentGitHubRefusal +// is the live answer; nil means the planner has no proxy record. +type AgentGitHubRefusal func(agent string, since time.Time) (clears time.Time, refused bool) + +const ( + // DefaultMaxCapRedispatch is how many expiries the agent GitHub API cap + // may cause on one head before the planner stops asking and holds the PR + // for a human with HoldReasonDispatchExhausted. Each one lasts at least + // review.pending_ttl and waits for the cap to clear, so eight is about a + // working day of a reviewer that cannot read: generous for a busy hour + // or two, still finite for a reviewer whose cap is set too low to ever + // finish a review. + DefaultMaxCapRedispatch = 8 + // agentCapWaitLimit bounds how long past pending_ttl an expiry the cap + // caused waits for the refusal to clear. It is the proxy's cap window: + // the sliding hour frees a slot within it, and the shared bucket resets + // within it, so a refusal that says later than that is not waited for. + agentCapWaitLimit = time.Hour +) + +func (o DispatchOptions) effectiveMaxCapRedispatch() int { + if o.MaxCapRedispatch <= 0 { + return DefaultMaxCapRedispatch + } + return o.MaxCapRedispatch +} + +// capCausedExpiry reports whether the agent GitHub API cap caused this +// expiry, and if so whether to wait before asking again. +// +// The cap caused it when the proxy refused the agent of every expired row at +// or after that row was sent: the session that should have answered could +// not read (projectbluefin/utah-packages#959). The relay sets aside the +// requires_human such a session writes (capStoppedReview in pkg/github), so +// the row expires instead; a session the cap stopped before it posted at all +// expires the same way. One expired row whose agent was not refused makes it +// an ordinary expiry, which spends max_redispatch as before. +// +// It waits while the refusal has not cleared (the sliding hour has no free +// slot yet, or the bucket has not reset), up to agentCapWaitLimit past the +// TTL: asking again sooner would only be refused again. The rows stay in +// place meanwhile, so the perspective is not asked twice. +func capCausedExpiry(expired []ExpiredPending, opts DispatchOptions, ttl time.Duration, now time.Time) (capCaused, wait bool) { + refusal := opts.AgentGitHubRefusal + if refusal == nil || len(expired) == 0 { + return false, false + } + var clears, earliest time.Time + for _, e := range expired { + agent := strings.TrimSpace(e.Agent) + if agent == "" { + return false, false + } + at, refused := refusal(agent, e.Dispatched) + if !refused { + return false, false + } + if at.After(clears) { + clears = at + } + if earliest.IsZero() || e.Dispatched.Before(earliest) { + earliest = e.Dispatched + } + } + waitUntil := clears + if limit := earliest.Add(ttl + agentCapWaitLimit); waitUntil.After(limit) { + waitUntil = limit + } + return true, now.Before(waitUntil) +} + +// expireCapCaused is expirePending for an expiry the agent cap caused: the +// rows are dropped and asked again like any expiry, but the count goes to +// CapRedispatches, not Redispatches. Before, each such expiry spent one of +// review.max_redispatch, so a reviewer capped for about 2h15m on one head +// (three 45m TTLs) ended in a review-dispatch-exhausted hold and a person was +// asked about a PR nobody had read. Once the cap count reaches +// MaxCapRedispatch the head is held the same way, so a reviewer that can +// never read still reaches a person instead of looping. +func (p *DispatchPlan) expireCapCaused(pr PullRequest, expired []ExpiredPending, opts DispatchOptions, ttl time.Duration, now time.Time) bool { + idx := redispatchIndex(p.State.CapRedispatches, pr) + if idx < 0 { + p.State.CapRedispatches = append(p.State.CapRedispatches, RedispatchCount{Repo: pr.Repo, Number: pr.Number, HeadSHA: pr.HeadSHA}) + idx = len(p.State.CapRedispatches) - 1 + } + p.State.CapRedispatches[idx].Count++ + p.State.CapRedispatches[idx].LastExpired = now + count := p.State.CapRedispatches[idx].Count + for i := range expired { + expired[i].Count = count + expired[i].CapCaused = true + } + p.Expired = append(p.Expired, expired...) + if count >= opts.effectiveMaxCapRedispatch() { + hold := HumanReviewHold{Repo: pr.Repo, Number: pr.Number, HeadSHA: pr.HeadSHA, + Reason: fmt.Sprintf("%s: %d review dispatches for this head ended at the agent GitHub API cap without a verdict", HoldReasonDispatchExhausted, count), + UpdatedAt: now} + p.addHuman(pr, hold.Reason, now) + p.Exhausted = append(p.Exhausted, hold) + return false + } + p.dropExpiredPending(pr, ttl, now) + return true +} + +// CapRedispatchCountFor is the number of this head's expiries the agent +// GitHub API cap caused. +func (s DispatchState) CapRedispatchCountFor(repo string, number int, headSHA string) int { + for _, r := range s.CapRedispatches { + if samePRHead(r.Repo, r.Number, r.HeadSHA, repo, number, headSHA) { + return r.Count + } + } + return 0 +} diff --git a/src/pkg/review/dispatch_agent_cap_test.go b/src/pkg/review/dispatch_agent_cap_test.go new file mode 100644 index 0000000000..823a6ed489 --- /dev/null +++ b/src/pkg/review/dispatch_agent_cap_test.go @@ -0,0 +1,242 @@ +package review + +import ( + "strings" + "testing" + "time" +) + +// capRefusals is a stand-in for the proxy's refusal record: the last time +// one of agents was refused, and when that refusal said it would clear. +type capRefusals struct { + agents map[string]bool + at time.Time + clears time.Time +} + +func (r *capRefusals) refusal(agent string, since time.Time) (time.Time, bool) { + if !r.agents[agent] || r.at.Before(since) { + return time.Time{}, false + } + return r.clears, true +} + +func capPendingFor(t *testing.T, state DispatchState) time.Time { + t.Helper() + var dispatched time.Time + for _, p := range state.Pending { + if p.Number != 7 || p.HeadSHA != "sha1" { + continue + } + if dispatched.IsZero() { + dispatched = p.Dispatched + } else if !p.Dispatched.Equal(dispatched) { + t.Fatalf("pending rows were sent at different times: %+v", state.Pending) + } + } + if dispatched.IsZero() { + t.Fatalf("no pending rows for the head: %+v", state.Pending) + } + return dispatched +} + +func holdFor(state DispatchState) *HumanReviewHold { + for i := range state.Human { + if state.Human[i].Number == 7 && state.Human[i].HeadSHA == "sha1" { + return &state.Human[i] + } + } + return nil +} + +// projectbluefin/utah-packages#959 at the planner: the reviewer kept hitting +// its 300/h cap, so every session on the head was refused mid-review and the +// relay set its requires_human aside. Each set-aside used to expire into one +// of review.max_redispatch's three tries, so about 2h15m of being capped +// parked the head as review-dispatch-exhausted and asked a person. Expiries +// the cap caused must count on their own, larger budget: still asked again +// after the third, held only after DefaultMaxCapRedispatch. +func TestPlanDispatchCapCausedExpiriesDoNotSpendRedispatchBudget(t *testing.T) { + start := time.Date(2026, 11, 9, 1, 0, 0, 0, time.UTC) + refusals := &capRefusals{agents: map[string]bool{"r1": true, "r2": true}} + state := DispatchState{Pending: pendingForHead("sha1", start)} + for cycle := 1; cycle <= DefaultMaxCapRedispatch; cycle++ { + dispatched := capPendingFor(t, state) + // Refused ten minutes into the session; the sliding hour frees a + // slot before the TTL is up, so nothing waits. + refusals.at = dispatched.Add(10 * time.Minute) + refusals.clears = dispatched.Add(30 * time.Minute) + now := dispatched.Add(DefaultPendingReviewTTL + time.Minute) + opts := pendingTTLOpts(now) + opts.AgentGitHubRefusal = refusals.refusal + plan := PlanDispatch([]PullRequest{dispatchPR("sha1")}, Artifact{}, state, opts) + + if got := plan.State.RedispatchCountFor("acme/hive", 7, "sha1"); got != 0 { + t.Fatalf("cycle %d: max_redispatch count = %d, want 0 for expiries the cap caused", cycle, got) + } + if got := plan.State.CapRedispatchCountFor("acme/hive", 7, "sha1"); got != cycle { + t.Fatalf("cycle %d: cap count = %d, want %d", cycle, got, cycle) + } + if len(plan.Expired) != len(DefaultPerspectives) { + t.Fatalf("cycle %d: expired = %d, want %d", cycle, len(plan.Expired), len(DefaultPerspectives)) + } + for _, e := range plan.Expired { + if !e.CapCaused || e.Count != cycle { + t.Fatalf("cycle %d: expired entry %+v, want cap-caused with count %d", cycle, e, cycle) + } + } + if cycle < DefaultMaxCapRedispatch { + if hold := holdFor(plan.State); hold != nil || len(plan.NewHuman) != 0 || len(plan.Exhausted) != 0 { + t.Fatalf("cycle %d: a head the cap kept from being read asked a person: hold=%+v new=%+v", cycle, hold, plan.NewHuman) + } + if len(plan.ReviewKicks) != 1 { + t.Fatalf("cycle %d: review kicks = %d, want the head asked again", cycle, len(plan.ReviewKicks)) + } + } else { + hold := holdFor(plan.State) + if hold == nil || !strings.HasPrefix(hold.Reason, HoldReasonDispatchExhausted) || !strings.Contains(hold.Reason, "agent GitHub API cap") { + t.Fatalf("cycle %d: hold = %+v, want a %s hold naming the cap", cycle, hold, HoldReasonDispatchExhausted) + } + if len(plan.ReviewKicks) != 0 || len(plan.NewHuman) != 1 || len(plan.Exhausted) != 1 { + t.Fatalf("cycle %d: kicks=%d new=%d exhausted=%d, want the head held once and not re-kicked", cycle, len(plan.ReviewKicks), len(plan.NewHuman), len(plan.Exhausted)) + } + } + state = plan.State + } +} + +// The counter-case: a head whose sessions end with no verdict for any reason +// but the cap still reaches the review-dispatch-exhausted hold after +// max_redispatch, as before. That covers no proxy record at all (none wired, +// or a restart forgot it), a refusal that belongs to an earlier session, a +// refusal of another agent, and a combined expiry where only one row's agent +// was refused. +func TestPlanDispatchOrdinaryExpiriesStillExhaust(t *testing.T) { + start := time.Date(2026, 11, 9, 1, 0, 0, 0, time.UTC) + cases := []struct { + name string + refusals func(dispatched time.Time) AgentGitHubRefusal + // mixed sends one of the head's rows to r2 each cycle, the rest to r1. + mixed bool + }{ + { + name: "no proxy record", + refusals: func(time.Time) AgentGitHubRefusal { return nil }, + }, + { + name: "refused before the row was sent", + refusals: func(dispatched time.Time) AgentGitHubRefusal { + r := &capRefusals{agents: map[string]bool{"r1": true, "r2": true}, at: dispatched.Add(-time.Minute), clears: dispatched} + return r.refusal + }, + }, + { + name: "another agent refused", + refusals: func(dispatched time.Time) AgentGitHubRefusal { + r := &capRefusals{agents: map[string]bool{"scanner": true}, at: dispatched.Add(10 * time.Minute), clears: dispatched.Add(20 * time.Minute)} + return r.refusal + }, + }, + { + name: "only one expired row's agent refused", + refusals: func(dispatched time.Time) AgentGitHubRefusal { + r := &capRefusals{agents: map[string]bool{"r1": true}, at: dispatched.Add(10 * time.Minute), clears: dispatched.Add(20 * time.Minute)} + return r.refusal + }, + mixed: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + state := DispatchState{Pending: pendingForHead("sha1", start)} + for cycle := 1; cycle <= DefaultMaxRedispatch; cycle++ { + if tc.mixed { + first := true + for i := range state.Pending { + if state.Pending[i].Number == 7 && state.Pending[i].HeadSHA == "sha1" { + state.Pending[i].Agent = "r1" + if first { + state.Pending[i].Agent = "r2" + first = false + } + } + } + } + dispatched := capPendingFor(t, state) + now := dispatched.Add(DefaultPendingReviewTTL + time.Minute) + opts := pendingTTLOpts(now) + opts.AgentGitHubRefusal = tc.refusals(dispatched) + plan := PlanDispatch([]PullRequest{dispatchPR("sha1")}, Artifact{}, state, opts) + if got := plan.State.CapRedispatchCountFor("acme/hive", 7, "sha1"); got != 0 { + t.Fatalf("cycle %d: cap count = %d for expiries the cap did not cause", cycle, got) + } + if got := plan.State.RedispatchCountFor("acme/hive", 7, "sha1"); got != cycle { + t.Fatalf("cycle %d: max_redispatch count = %d, want %d", cycle, got, cycle) + } + hold := holdFor(plan.State) + if cycle < DefaultMaxRedispatch { + if hold != nil { + t.Fatalf("cycle %d: held early: %+v", cycle, hold) + } + } else if hold == nil || !strings.HasPrefix(hold.Reason, HoldReasonDispatchExhausted) || strings.Contains(hold.Reason, "API cap") { + t.Fatalf("cycle %d: hold = %+v, want the ordinary %s hold", cycle, hold, HoldReasonDispatchExhausted) + } + state = plan.State + } + }) + } +} + +// An expiry the cap caused is asked again only once the refusal has +// cleared: sooner, the proxy would just refuse the new session too. The wait +// is bounded at agentCapWaitLimit past the TTL. +func TestPlanDispatchCapCausedExpiryWaitsForTheRefusalToClear(t *testing.T) { + dispatched := time.Date(2026, 11, 9, 1, 0, 0, 0, time.UTC) + ttl := DefaultPendingReviewTTL + run := func(clears, now time.Time) DispatchPlan { + r := &capRefusals{agents: map[string]bool{"r1": true}, at: dispatched.Add(40 * time.Minute), clears: clears} + opts := pendingTTLOpts(now) + opts.AgentGitHubRefusal = r.refusal + return PlanDispatch([]PullRequest{dispatchPR("sha1")}, Artifact{}, DispatchState{Pending: pendingForHead("sha1", dispatched)}, opts) + } + + clears := dispatched.Add(ttl + 30*time.Minute) + waiting := run(clears, dispatched.Add(ttl+time.Minute)) + if len(waiting.ReviewKicks) != 0 || len(waiting.Expired) != 0 { + t.Fatalf("asked again before the cap cleared: kicks=%d expired=%d", len(waiting.ReviewKicks), len(waiting.Expired)) + } + if len(waiting.State.Pending) != len(DefaultPerspectives) || !capPendingFor(t, waiting.State).Equal(dispatched) { + t.Fatalf("waiting rows were not kept as asked: %+v", waiting.State.Pending) + } + if waiting.State.RedispatchCountFor("acme/hive", 7, "sha1") != 0 || waiting.State.CapRedispatchCountFor("acme/hive", 7, "sha1") != 0 { + t.Fatal("a wait was counted as an expiry") + } + if holdFor(waiting.State) != nil { + t.Fatalf("a wait held the head: %+v", waiting.State.Human) + } + + cleared := run(clears, clears.Add(time.Minute)) + if len(cleared.ReviewKicks) != 1 || cleared.State.CapRedispatchCountFor("acme/hive", 7, "sha1") != 1 { + t.Fatalf("after the cap cleared: kicks=%d cap count=%d, want one re-ask counted as cap-caused", len(cleared.ReviewKicks), cleared.State.CapRedispatchCountFor("acme/hive", 7, "sha1")) + } + + far := dispatched.Add(ttl + 3*time.Hour) + bounded := run(far, dispatched.Add(ttl+agentCapWaitLimit+time.Minute)) + if len(bounded.ReviewKicks) != 1 { + t.Fatalf("waited past agentCapWaitLimit for a refusal that clears at %v", far) + } +} + +// A new push starts the head over: the cap count goes with the old head, +// like max_redispatch's. +func TestPlanDispatchCapRedispatchesResetOnNewHead(t *testing.T) { + now := time.Date(2026, 11, 9, 1, 0, 0, 0, time.UTC) + state := DispatchState{ + Pending: pendingForHead("old", now.Add(-time.Hour)), + CapRedispatches: []RedispatchCount{{Repo: "acme/hive", Number: 7, HeadSHA: "old", Count: DefaultMaxCapRedispatch - 1}}, + } + plan := PlanDispatch([]PullRequest{dispatchPR("new")}, Artifact{}, state, pendingTTLOpts(now)) + if len(plan.State.CapRedispatches) != 0 { + t.Fatalf("old head's cap count survived the push: %+v", plan.State.CapRedispatches) + } +}