diff --git a/changelog.d/fixed-10527-claim-escalation.md b/changelog.d/fixed-10527-claim-escalation.md new file mode 100644 index 0000000000..a8e8ae2e8c --- /dev/null +++ b/changelog.d/fixed-10527-claim-escalation.md @@ -0,0 +1,3 @@ +- An agent no longer re-claims the same issue forever when nothing moves between its claims ([#10527](https://github.com/hivecommons/hive/issues/10527)). An agent claim lapses after `agent_ttl_s`, the issue went back into the work lists, and the next kick claimed it again, so an issue only a person could unblock showed as "claimed" for hours (#10512: `scanner` claimed at 15:50Z and again at 17:57Z with no commit, PR or label change in between). The claim ledger now remembers each agent's claims on an issue. When one agent's last `governor.claims.escalate_after_claims` claims (default `2`; negative turns the gate off) on a free issue were followed by no linked or referencing pull request, no referencing commit, no label or assignee change and no close/reopen, the issue is left out of every kick instead of being claimed a third time, labelled `needs-human`, and given one comment listing the claims and how each ended (ADR-0019). The claim machinery's own `claimed`/`preempted:*` labels and the gate's own `needs-human` do not count as progress; a person removing `needs-human` (or `/hive approve`) does, so the next claim goes ahead. If the issue timeline cannot be read, the claim is allowed. +- A governor kick no longer claims, or posts a 🔒 claim comment on, every issue in the agent's work list every two hours ([#10527](https://github.com/hivecommons/hive/issues/10527)). The kick now only *lists* those issues: a 30-minute hold in the ledger, with no GitHub comment, that keeps relay contributors and other agents off them while the kick is live. The agent claim, with its comment and `claimed` label, is recorded on the agent's first start signal on an issue — its own comment, label or claim request through `hive-open-issue`, or a `hive-open-pr` request naming the issue — and only those claims count toward `escalate_after_claims`, so an issue that merely sat in a work list is never escalated. +- A `hive/likely-done` issue is verified once instead of every cycle ([#10527](https://github.com/hivecommons/hive/issues/10527), the #10509 pattern of eight near-identical verification comments). Once an agent has started on the issue after the merged pull request — its verification comment, then the reporter-confirmation request for a human-filed bug — the issue is withheld from kicks until a person (the reporter or a maintainer) comments, labels, assigns, closes or reopens it. If the issue timeline cannot be read, the issue is offered as before. diff --git a/src/cmd/hive/claimescalation.go b/src/cmd/hive/claimescalation.go new file mode 100644 index 0000000000..4bd93489fd --- /dev/null +++ b/src/cmd/hive/claimescalation.go @@ -0,0 +1,215 @@ +package main + +import ( + "context" + "fmt" + "log/slog" + "strings" + "sync" + "time" + + "github.com/hivecommons/hive/pkg/claims" + "github.com/hivecommons/hive/pkg/config" + "github.com/hivecommons/hive/pkg/escalation" + "github.com/hivecommons/hive/pkg/github" + "github.com/hivecommons/hive/pkg/scheduler" +) + +// Claim escalation gate (hivecommons/hive#10527). +// +// An agent claim expires after agent_ttl_s and the issue goes back into the +// work lists, so nothing stopped the same agent re-claiming an issue forever: +// each claim looked like progress while nothing moved. The gate sits in the +// scheduler's in-flight seam, in front of the kick that would record the next +// claim. When one agent's last escalate_after_claims claims on a free issue +// were followed by no linked PR, referencing commit, label or assignee change +// and no close/reopen, the issue is withheld from every work list, labelled +// needs-human, and given one comment summarising the claims (ADR-0019). The +// needs-human label then keeps it out of the issue enumeration until a person +// removes it — which is itself progress, so the next claim goes ahead. + +// claimProgressRecheck bounds how long a "nothing moved" verdict, or a failed +// timeline read, is reused before the timeline is read again. A verdict that +// something moved holds for as long as the run it was read for. +const claimProgressRecheck = 5 * time.Minute + +// claimEscalationGate decides, per issue, whether a stalled run of agent +// claims withholds it. +type claimEscalationGate struct { + ctx context.Context + ledger *claims.Ledger + org string + threshold int + // events reads the issue timeline after since. + events func(ctx context.Context, repo string, issue int, since time.Time) ([]github.IssueEvent, error) + // escalate applies the needs-human label and posts the summary; called + // once per stalled run. + escalate func(claims.Stall) + now func() time.Time + logger *slog.Logger + + mu sync.Mutex + verdicts map[string]progressVerdict +} + +// progressVerdict caches one timeline read for one stalled run. +type progressVerdict struct { + since time.Time // Since of the run the verdict was read for + allow bool + until time.Time // zero: valid until the run changes +} + +// claimEscalationLookup returns the gate as a scheduler in-flight lookup, or +// nil when the ledger is off or escalate_after_claims is negative. +func claimEscalationLookup(ctx context.Context, cfg *config.Config, ledger *claims.Ledger, client func() *github.Client, logger *slog.Logger) scheduler.InflightLookup { + if ledger == nil || cfg == nil || client == nil { + return nil + } + threshold := cfg.Governor.Claims.EscalationThreshold() + if threshold <= 0 { + return nil + } + g := &claimEscalationGate{ + ctx: ctx, + ledger: ledger, + org: cfg.Project.Org, + threshold: threshold, + events: func(ctx context.Context, repo string, issue int, since time.Time) ([]github.IssueEvent, error) { + gh := client() + if gh == nil { + return nil, github.ErrNoGitHubClient + } + return gh.IssueTimelineSince(ctx, repo, issue, since) + }, + escalate: func(s claims.Stall) { + go func() { + gh := client() + if gh == nil { + return + } + cctx, cancel := context.WithTimeout(ctx, claimsGitHubTimeout) + defer cancel() + if err := gh.AddLabels(cctx, s.Repo, s.Issue, []string{escalation.NeedsHumanLabel}); err != nil && logger != nil { + logger.Warn("issue-claims: escalation label failed", "issue", s.Key(), "error", err) + } + if err := gh.CreateIssueComment(cctx, s.Repo, s.Issue, claims.EscalationComment(s, escalation.NeedsHumanLabel)); err != nil && logger != nil { + logger.Warn("issue-claims: escalation comment failed", "issue", s.Key(), "error", err) + } + }() + }, + now: time.Now, + logger: logger, + } + return g.lookup +} + +// lookup is the scheduler.InflightLookup. An issue is withheld only when a +// stalled run exists and the timeline shows nothing moved since it began; a +// timeline read that fails lets the issue through, so a GitHub outage never +// parks work on its own. +func (g *claimEscalationGate) lookup(issue github.Issue) (string, bool) { + if issue.Number <= 0 { + return "", false + } + repo := issue.Repo + if g.org != "" && !strings.Contains(repo, "/") { + repo = g.org + "/" + repo + } + stall, ok := g.ledger.Stall(repo, issue.Number, g.threshold) + if !ok { + return "", false + } + since := stall.Since() + held := fmt.Sprintf("escalated to %s: %d claims by %s moved nothing since %s", + escalation.NeedsHumanLabel, len(stall.Attempts), stall.Holder, since.UTC().Format(time.RFC3339)) + now := g.now() + if v, ok := g.verdict(stall.Key(), since, now); ok { + if v.allow { + return "", false + } + return held, true + } + + ctx, cancel := context.WithTimeout(g.ctx, claimsGitHubTimeout) + events, err := g.events(ctx, repo, issue.Number, since) + cancel() + if err != nil { + if g.logger != nil { + g.logger.Warn("issue-claims: could not read timeline for escalation check, allowing the claim", + "issue", stall.Key(), "holder", stall.Holder, "error", err) + } + g.remember(stall.Key(), progressVerdict{since: since, allow: true, until: now.Add(claimProgressRecheck)}) + return "", false + } + if why, moved := claimProgress(events); moved { + g.remember(stall.Key(), progressVerdict{since: since, allow: true}) + if g.logger != nil { + g.logger.Debug("issue-claims: re-claim allowed, issue moved", "issue", stall.Key(), "holder", stall.Holder, "progress", why) + } + return "", false + } + g.remember(stall.Key(), progressVerdict{since: since, until: now.Add(claimProgressRecheck)}) + if g.ledger.MarkEscalated(stall) { + if g.logger != nil { + g.logger.Info("issue-claims: escalating instead of re-claiming", + "issue", stall.Key(), "holder", stall.Holder, "claims", len(stall.Attempts), "since", since) + } + g.escalate(stall) + } + return held, true +} + +func (g *claimEscalationGate) verdict(key string, since, now time.Time) (progressVerdict, bool) { + g.mu.Lock() + defer g.mu.Unlock() + v, ok := g.verdicts[key] + if !ok || !v.since.Equal(since) || (!v.until.IsZero() && !now.Before(v.until)) { + return progressVerdict{}, false + } + return v, true +} + +func (g *claimEscalationGate) remember(key string, v progressVerdict) { + g.mu.Lock() + defer g.mu.Unlock() + if g.verdicts == nil { + g.verdicts = map[string]progressVerdict{} + } + g.verdicts[key] = v +} + +// claimProgress reports the first timeline event that counts as the issue +// moving: a pull request referencing or linked to it, a commit referencing it, +// an assignee change, a close/reopen, or a label change. The claim machinery's +// own labels (claimed, preempted:*) and the gate's own needs-human label being +// ADDED do not count — otherwise every re-claim, and every escalation, would +// read as progress. needs-human being REMOVED does count: that is a person +// sending the issue back. +func claimProgress(events []github.IssueEvent) (string, bool) { + for _, e := range events { + switch e.Event { + case "cross-referenced": + if e.SourcePR > 0 { + return fmt.Sprintf("pull request #%d referenced it", e.SourcePR), true + } + case "connected": + return "a pull request was linked", true + case "referenced": + return "commit " + e.CommitID + " referenced it", true + case "assigned", "unassigned": + return "assignees changed", true + case "closed", "reopened": + return "it was " + e.Event, true + case "labeled", "unlabeled": + if strings.EqualFold(e.Label, claims.LabelClaimed) || + strings.HasPrefix(strings.ToLower(e.Label), claims.LabelPreemptedPrefix) { + continue + } + if e.Event == "labeled" && strings.EqualFold(e.Label, escalation.NeedsHumanLabel) { + continue + } + return fmt.Sprintf("label %q %s", e.Label, e.Event), true + } + } + return "", false +} diff --git a/src/cmd/hive/claimescalation_test.go b/src/cmd/hive/claimescalation_test.go new file mode 100644 index 0000000000..e92ca99fd2 --- /dev/null +++ b/src/cmd/hive/claimescalation_test.go @@ -0,0 +1,230 @@ +package main + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/hivecommons/hive/pkg/claims" + "github.com/hivecommons/hive/pkg/github" +) + +// escalationFixture is a ledger on a test clock plus a gate whose timeline +// reads and escalations are recorded instead of hitting GitHub. +type escalationFixture struct { + t *testing.T + ledger *claims.Ledger + now time.Time + gate *claimEscalationGate + timeline []github.IssueEvent // returned for every read, filtered by since + readErr error + reads int + escalated []claims.Stall +} + +func newEscalationFixture(t *testing.T) *escalationFixture { + t.Helper() + f := &escalationFixture{t: t, now: time.Date(2026, 10, 4, 15, 50, 0, 0, time.UTC)} + l, err := claims.New(filepath.Join(t.TempDir(), "claims.json"), claims.DefaultPolicy(), claims.Hooks{}) + if err != nil { + t.Fatal(err) + } + l.SetNow(func() time.Time { return f.now }) + f.ledger = l + f.gate = &claimEscalationGate{ + ctx: context.Background(), ledger: l, org: "myorg", threshold: 2, + events: func(_ context.Context, repo string, issue int, since time.Time) ([]github.IssueEvent, error) { + f.reads++ + if repo != "myorg/repo" || issue != 10512 { + t.Fatalf("timeline read for %s#%d", repo, issue) + } + if f.readErr != nil { + return nil, f.readErr + } + var out []github.IssueEvent + for _, e := range f.timeline { + if e.At.After(since) { + out = append(out, e) + } + } + return out, nil + }, + escalate: func(s claims.Stall) { f.escalated = append(f.escalated, s) }, + now: func() time.Time { return f.now }, + } + return f +} + +// kickClaim records the claim an agent kick naming the issue makes, then +// lets it lapse with nothing done — the #10512 pattern. +func (f *escalationFixture) kickClaim(agent string) { + f.t.Helper() + res, err := f.ledger.Claim(claims.Request{Repo: "myorg/repo", Issue: 10512, Holder: agent, HolderID: agent, Kind: claims.KindAgent}) + if err != nil || res.Outcome != claims.OutcomeClaimed { + f.t.Fatalf("claim by %s: outcome=%s err=%v", agent, res.Outcome, err) + } + f.now = f.now.Add(claims.DefaultAgentTTL + 7*time.Minute) +} + +func (f *escalationFixture) event(e github.IssueEvent) { + e.At = f.now.Add(-time.Minute) + f.timeline = append(f.timeline, e) +} + +// lookup asks the gate about the issue the way the scheduler does, with the +// bare repo name a default config's enumeration carries. +func (f *escalationFixture) lookup() (string, bool) { + return f.gate.lookup(github.Issue{Repo: "repo", Number: 10512}) +} + +func TestClaimEscalationRefusesThirdNoProgressClaim(t *testing.T) { + f := newEscalationFixture(t) + f.kickClaim("scanner") + if _, held := f.lookup(); held || f.reads != 0 { + t.Fatalf("one prior claim: held=%v reads=%d, want free without a timeline read", held, f.reads) + } + f.kickClaim("scanner") + // The claim machinery's own label churn and comments are not progress. + f.event(github.IssueEvent{Event: "unlabeled", Label: claims.LabelClaimed}) + f.event(github.IssueEvent{Event: "commented"}) + + holder, held := f.lookup() + if !held || !strings.Contains(holder, "needs-human") || !strings.Contains(holder, "scanner") { + t.Fatalf("third no-progress claim: held=%v holder=%q, want withheld and escalated", held, holder) + } + if len(f.escalated) != 1 || f.escalated[0].Holder != "scanner" || len(f.escalated[0].Attempts) != 2 { + t.Fatalf("escalations=%+v, want one for scanner's two claims", f.escalated) + } + + // The scheduler asks several times per cycle, and the stale enumeration + // keeps offering the issue until needs-human shows up in it: one + // escalation, one timeline read per recheck window. + f.event(github.IssueEvent{Event: "labeled", Label: "needs-human"}) + for range 3 { + if _, held := f.lookup(); !held { + t.Fatal("escalated issue offered again") + } + } + f.now = f.now.Add(claimProgressRecheck) + if _, held := f.lookup(); !held { + t.Fatal("the gate's own needs-human label counted as progress") + } + if len(f.escalated) != 1 || f.reads != 2 { + t.Fatalf("escalations=%d reads=%d, want 1 escalation and 2 reads", len(f.escalated), f.reads) + } + if _, ok := f.ledger.Lookup("myorg/repo", 10512); ok { + t.Fatal("refused claim left a live claim behind") + } + + // A person removing needs-human is progress: the next claim goes ahead. + f.now = f.now.Add(claimProgressRecheck) + f.event(github.IssueEvent{Event: "unlabeled", Label: "needs-human"}) + if _, held := f.lookup(); held { + t.Fatal("issue still withheld after needs-human was removed") + } +} + +func TestClaimEscalationAllowsClaimsWithAPullRequestBetween(t *testing.T) { + f := newEscalationFixture(t) + f.kickClaim("scanner") + f.event(github.IssueEvent{Event: "cross-referenced", SourcePR: 10530}) + f.kickClaim("scanner") + + if holder, held := f.lookup(); held { + t.Fatalf("claims with a PR between withheld: %q", holder) + } + if len(f.escalated) != 0 { + t.Fatalf("escalated despite a PR: %+v", f.escalated) + } + // The verdict holds for the run: no second read. + if _, held := f.lookup(); held || f.reads != 1 { + t.Fatalf("held=%v reads=%d, want free on one read", held, f.reads) + } + + // Two more claims with nothing between them form a new run, which + // escalates: the PR before it does not excuse it forever. + f.kickClaim("scanner") + f.kickClaim("scanner") + if _, held := f.lookup(); !held || len(f.escalated) != 1 { + t.Fatalf("held=%v escalations=%d, want the new no-progress run escalated", held, len(f.escalated)) + } +} + +func TestClaimEscalationSkipsWhileClaimedAndAcrossAgents(t *testing.T) { + f := newEscalationFixture(t) + f.kickClaim("ci-maintainer") + f.kickClaim("scanner") + if _, held := f.lookup(); held || f.reads != 0 { + t.Fatalf("one claim each by two agents: held=%v reads=%d", held, f.reads) + } + if _, err := f.ledger.Claim(claims.Request{Repo: "myorg/repo", Issue: 10512, Holder: "scanner", Kind: claims.KindAgent}); err != nil { + t.Fatal(err) + } + if _, held := f.lookup(); held || f.reads != 0 { + t.Fatalf("live claim: held=%v reads=%d — the claims lookup owns a held issue", held, f.reads) + } +} + +func TestClaimEscalationFailsOpenOnTimelineError(t *testing.T) { + f := newEscalationFixture(t) + f.kickClaim("scanner") + f.kickClaim("scanner") + f.readErr = errors.New("502 bad gateway") + if _, held := f.lookup(); held || len(f.escalated) != 0 { + t.Fatalf("timeline error: held=%v escalations=%d, want the claim allowed", held, len(f.escalated)) + } + if _, held := f.lookup(); held || f.reads != 1 { + t.Fatalf("held=%v reads=%d, want the failed read reused within the recheck window", held, f.reads) + } + f.readErr = nil + f.now = f.now.Add(claimProgressRecheck) + if _, held := f.lookup(); !held || len(f.escalated) != 1 { + t.Fatalf("after recovery: held=%v escalations=%d, want escalated", held, len(f.escalated)) + } +} + +func TestClaimProgress(t *testing.T) { + cases := []struct { + name string + event github.IssueEvent + moved bool + }{ + {"linked pr", github.IssueEvent{Event: "cross-referenced", SourcePR: 7}, true}, + {"issue cross-reference", github.IssueEvent{Event: "cross-referenced"}, false}, + {"development link", github.IssueEvent{Event: "connected"}, true}, + {"commit", github.IssueEvent{Event: "referenced", CommitID: "abc"}, true}, + {"assignee", github.IssueEvent{Event: "assigned"}, true}, + {"closed", github.IssueEvent{Event: "closed"}, true}, + {"other label", github.IssueEvent{Event: "labeled", Label: "priority/high"}, true}, + {"claimed label", github.IssueEvent{Event: "labeled", Label: "claimed"}, false}, + {"preempted label", github.IssueEvent{Event: "unlabeled", Label: "preempted:alice"}, false}, + {"needs-human added", github.IssueEvent{Event: "labeled", Label: "Needs-Human"}, false}, + {"needs-human removed", github.IssueEvent{Event: "unlabeled", Label: "needs-human"}, true}, + {"comment", github.IssueEvent{Event: "commented"}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if _, moved := claimProgress([]github.IssueEvent{tc.event}); moved != tc.moved { + t.Fatalf("claimProgress(%+v) moved=%v, want %v", tc.event, moved, tc.moved) + } + }) + } +} + +// #10527: kick listings are not claims, so an issue a kick only listed — the +// scanner's whole work list every cycle — never reaches the gate. +func TestClaimEscalationIgnoresKickListings(t *testing.T) { + f := newEscalationFixture(t) + for range 4 { + if _, ok := f.ledger.MarkListed("myorg/repo", 10512, "scanner"); !ok { + f.t.Fatal("listing refused") + } + f.now = f.now.Add(claims.DefaultAgentTTL + 7*time.Minute) + } + if holder, held := f.lookup(); held || f.reads != 0 || len(f.escalated) != 0 { + t.Fatalf("listed-only issue: held=%v holder=%q reads=%d escalations=%d, want free and unread", held, holder, f.reads, len(f.escalated)) + } +} diff --git a/src/cmd/hive/claimswire.go b/src/cmd/hive/claimswire.go index 4cc9a817c5..d5d7cba118 100644 --- a/src/cmd/hive/claimswire.go +++ b/src/cmd/hive/claimswire.go @@ -20,8 +20,9 @@ import ( // The ledger itself lives in pkg/claims; the dashboard owns the relay seam // (auto-claim on lease, exclusion in selectTask, takeover → yank). This file // wires the two remaining producers and consumers that only cmd/hive can see: -// - the scheduler: withhold claimed issues from kicks, and record an agent -// claim for every issue a kick hands one of the hive's own agents; and +// - the scheduler: withhold claimed and kick-listed issues from kicks, list +// every issue a kick hands one of the hive's own agents, and record the +// agent claim on its first start signal (#10527); and // - GitHub: post the claim / takeover / release comments and labels so a // human, a clanker polling the issue, or another hive can see the hold. @@ -136,9 +137,10 @@ func githubClaimHooks(ctx context.Context, cfg *config.Config, client func() *gi } // claimsInflightLookup makes the ledger a scheduler in-flight source: an -// issue with a live claim held by anyone is withheld from kicks. Agent claims -// are included on purpose — the agent that holds it already has the kick, and -// a second agent must not be handed the same issue. +// issue with a live claim held by anyone, or a live kick listing (#10527), is +// withheld from kicks. Agent claims and listings are included on purpose — the +// agent that holds it already has the kick, and a second agent must not be +// handed the same issue. func claimsInflightLookup(ledger *claims.Ledger, org string) scheduler.InflightLookup { if ledger == nil { return nil @@ -151,10 +153,17 @@ func claimsInflightLookup(ledger *claims.Ledger, org string) scheduler.InflightL if !ok && org != "" && !strings.Contains(issue.Repo, "/") { c, ok = ledger.Lookup(org+"/"+issue.Repo, issue.Number) } - if !ok { - return "", false + if ok { + return fmt.Sprintf("claimed by %s (%s) until %s", c.Holder, c.Kind, c.ExpiresAt.UTC().Format(time.RFC3339)), true + } + li, ok := ledger.Listed(issue.Repo, issue.Number) + if !ok && org != "" && !strings.Contains(issue.Repo, "/") { + li, ok = ledger.Listed(org+"/"+issue.Repo, issue.Number) } - return fmt.Sprintf("claimed by %s (%s) until %s", c.Holder, c.Kind, c.ExpiresAt.UTC().Format(time.RFC3339)), true + if ok { + return fmt.Sprintf("listed in a live kick to %s until %s", li.Holder, li.ExpiresAt.UTC().Format(time.RFC3339)), true + } + return "", false } } @@ -179,16 +188,19 @@ func composeInflight(lookups ...scheduler.InflightLookup) scheduler.InflightLook } } -// recordAgentKickClaims records an agent claim for every issue a delivered -// kick named. The agent's own earlier claim renews; a lower-ranked holder -// (contributor / external) is taken over and told to stop; a human's claim -// is left alone — the scheduler already withheld it, so a ref reaching here -// under a human claim is the rare race, logged and not escalated. +// recordAgentKickListings records a kick listing for every issue a delivered +// kick named (#10527). A listing posts nothing on GitHub and is not a claim +// attempt: a kick names up to governor.kick_limits.max_issues issues and the +// agent starts on few of them, so claiming the whole list put a 🔒 comment on +// every listed issue every agent TTL and made the escalation gate count +// issues nobody started. The listing holds the issue back from other agents +// and relay contributors while the kick is live; the agent's first start +// signal on it (recordAgentStart) records the real claim. // // Kick refs carry github.Issue.Repo, which is the bare project.repos entry on // a default config; the relay keys claims on owner/repo, so the repo is // qualified with org here or the two sides would never see each other. -func recordAgentKickClaims(dashSrv *dashboard.Server, org, agentName string, issueRefs []string, logger *slog.Logger) { +func recordAgentKickListings(dashSrv *dashboard.Server, org, agentName string, issueRefs []string, logger *slog.Logger) { ledger := dashSrv.IssueClaims() if ledger == nil || agentName == "" { return @@ -198,21 +210,44 @@ func recordAgentKickClaims(dashSrv *dashboard.Server, org, agentName string, iss if !ok || !ref.IsGitHubIssue() { continue } - repo := ref.Repo - if org != "" && !strings.Contains(repo, "/") { - repo = org + "/" + repo - } - res, err := ledger.Claim(claims.Request{ - Repo: repo, Issue: ref.Number, - Holder: agentName, HolderID: agentName, Kind: claims.KindAgent, - }) - if err != nil && logger != nil { - logger.Warn("issue-claims: agent claim not recorded", "agent", agentName, "issue", raw, "error", err) - continue - } - if res.Outcome == claims.OutcomeRefused && logger != nil { - logger.Warn("issue-claims: kick named an issue a higher-ranked holder claims", - "agent", agentName, "issue", raw, "holder", res.Claim.Holder, "kind", res.Claim.Kind) + if _, listed := ledger.MarkListed(qualifyClaimRepo(org, ref.Repo), ref.Number, agentName); !listed && logger != nil { + logger.Debug("issue-claims: kick named an issue someone else holds; not listed", "agent", agentName, "issue", raw) } } } + +// recordAgentStart is the start-signal seam (#10527): the agent's own request +// shows it working the issue — a comment, label or claim request, or a +// pull-request request naming it. The first such signal on an issue a kick +// listed to the agent records the agent claim (comment and label included) +// that the escalation gate counts; a later one renews it. A signal on an +// issue no kick listed to the agent records nothing. +func recordAgentStart(ledger *claims.Ledger, org, agentName, repo string, issue int, signal string, logger *slog.Logger) { + if ledger == nil || agentName == "" || issue <= 0 { + return + } + res, started, err := ledger.ClaimOnStart(qualifyClaimRepo(org, repo), issue, agentName) + if logger == nil || !started { + return + } + if err != nil { + logger.Warn("issue-claims: agent claim not recorded", "agent", agentName, "repo", repo, "issue", issue, "signal", signal, "error", err) + return + } + switch res.Outcome { + case claims.OutcomeRefused, claims.OutcomeHeld: + logger.Warn("issue-claims: agent started an issue another holder claims", + "agent", agentName, "repo", repo, "issue", issue, "signal", signal, "holder", res.Claim.Holder, "kind", res.Claim.Kind) + case claims.OutcomeClaimed, claims.OutcomeTakenOver: + logger.Info("issue-claims: agent claim recorded on start signal", "agent", agentName, "issue", res.Claim.Key(), "signal", signal) + } +} + +// qualifyClaimRepo qualifies a bare repo name with org. +func qualifyClaimRepo(org, repo string) string { + repo = strings.TrimSpace(repo) + if org != "" && repo != "" && !strings.Contains(repo, "/") { + return org + "/" + repo + } + return repo +} diff --git a/src/cmd/hive/claimswire_test.go b/src/cmd/hive/claimswire_test.go index 0e55f414d3..6b7cb5d8a9 100644 --- a/src/cmd/hive/claimswire_test.go +++ b/src/cmd/hive/claimswire_test.go @@ -4,6 +4,7 @@ import ( "io" "log/slog" "path/filepath" + "strings" "testing" "github.com/hivecommons/hive/pkg/claims" @@ -63,7 +64,7 @@ func TestClaimsInflightLookup_WithholdsClaimedIssues(t *testing.T) { } } -func TestRecordAgentKickClaims(t *testing.T) { +func TestRecordAgentKickListings(t *testing.T) { l := testClaimsLedger(t) _, _ = l.Claim(claims.Request{Repo: "myorg/repo", Issue: 2, Holder: "relay", HolderID: "relay#s", Kind: claims.KindContributor}) _, _ = l.Claim(claims.Request{Repo: "myorg/repo", Issue: 3, Holder: "alice", Kind: claims.KindHuman}) @@ -73,17 +74,60 @@ func TestRecordAgentKickClaims(t *testing.T) { srv.RegisterAPI(&dashboard.Dependencies{Config: &config.Config{}, IssueClaims: l}) t.Cleanup(srv.CloseContributeHub) - recordAgentKickClaims(srv, "myorg", "quality", []string{"repo#1", "myorg/repo#2", "myorg/repo#3", "myorg/repo#0", "garbage"}, logger) + recordAgentKickListings(srv, "myorg", "quality", []string{"repo#1", "myorg/repo#2", "myorg/repo#3", "myorg/repo#0", "garbage"}, logger) - if c, ok := l.Lookup("myorg/repo", 1); !ok || c.Kind != claims.KindAgent || c.Holder != "quality" { - t.Fatalf("bare-repo ref not claimed under org-qualified key: %+v", c) + // #10527: a kick lists, it does not claim — no 🔒 comment, no attempt. + if c, ok := l.Lookup("myorg/repo", 1); ok { + t.Fatalf("kick recorded a claim on a listed issue: %+v", c) } - if c, _ := l.Lookup("myorg/repo", 2); c.Holder != "quality" || c.TakenFrom != "relay" { - t.Fatalf("contributor claim not taken over by agent: %+v", c) + if li, ok := l.Listed("myorg/repo", 1); !ok || li.Holder != "quality" { + t.Fatalf("bare-repo ref not listed under org-qualified key: %+v %v", li, ok) + } + if c, _ := l.Lookup("myorg/repo", 2); c.Holder != "relay" { + t.Fatalf("a kick listing displaced a contributor claim: %+v", c) } if c, _ := l.Lookup("myorg/repo", 3); c.Holder != "alice" { t.Fatalf("human claim was overridden by agent: %+v", c) } + if !l.HeldKeys("relay#other")["myorg/repo#1"] { + t.Fatal("listed issue not withheld from relay contributors") + } + fn := claimsInflightLookup(l, "myorg") + if holder, held := fn(github.Issue{Repo: "repo", Number: 1}); !held || !strings.Contains(holder, "quality") { + t.Fatalf("listed issue not withheld from other kicks: %q %v", holder, held) + } // A nil ledger is a no-op, not a panic. - recordAgentKickClaims(dashboard.NewServer(0, logger), "o", "quality", []string{"o/r#1"}, logger) + recordAgentKickListings(dashboard.NewServer(0, logger), "o", "quality", []string{"o/r#1"}, logger) +} + +func TestRecordAgentStartClaimsOnFirstSignal(t *testing.T) { + comments := make(chan claims.Claim, 4) + l, err := claims.New(filepath.Join(t.TempDir(), "claims.json"), claims.DefaultPolicy(), claims.Hooks{ + OnClaimed: func(c claims.Claim, o claims.Outcome) { + if o == claims.OutcomeClaimed { + comments <- c + } + }, + }) + if err != nil { + t.Fatal(err) + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + recordAgentStart(l, "myorg", "quality", "repo", 1, github.AgentStartSignalComment, logger) + if _, ok := l.Lookup("myorg/repo", 1); ok { + t.Fatal("start signal on an issue no kick listed recorded a claim") + } + + l.MarkListed("myorg/repo", 1, "quality") + recordAgentStart(l, "myorg", "quality", "repo", 1, github.AgentStartSignalPRRequest, logger) + recordAgentStart(l, "myorg", "quality", "myorg/repo", 1, github.AgentStartSignalComment, logger) + c, ok := l.Lookup("myorg/repo", 1) + if !ok || c.Holder != "quality" || c.Kind != claims.KindAgent { + t.Fatalf("start signal did not record the agent claim: %+v %v", c, ok) + } + if len(comments) != 1 { + t.Fatalf("claim comments = %d, want exactly one for the first start signal", len(comments)) + } + recordAgentStart(nil, "myorg", "quality", "repo", 1, github.AgentStartSignalComment, logger) } diff --git a/src/cmd/hive/github_client_config.go b/src/cmd/hive/github_client_config.go index a037be9094..0d9d2fc2c4 100644 --- a/src/cmd/hive/github_client_config.go +++ b/src/cmd/hive/github_client_config.go @@ -281,6 +281,13 @@ func (b *boot) applyGitHubClientDashboardHooks(client *github.Client) { // dedupes with the audit-sink bridge by (ref, kind). recordPROpened(b.dashSrv, b.cfg.Project.Org, agentName, repo, number, url) }) + // #10527: an agent's own comment, label/claim or PR request on an issue a + // kick listed to it is its start signal; that is when the agent claim (and + // its 🔒 comment) is recorded. The ledger is read per call because the + // client can be configured before it is built. + client.SetAgentStartHook(func(agentName, repo string, issue int, signal string) { + recordAgentStart(b.issueClaims, b.cfg.Project.Org, agentName, repo, issue, signal, b.logger) + }) // PR follow-up session resume (#9583, default off): remember which live // CLI session authored this PR, and the PR's handoff note, so its // follow-ups can resume that conversation or brief a fresh one. The diff --git a/src/cmd/hive/main.go b/src/cmd/hive/main.go index cc7c76febb..050af363e9 100644 --- a/src/cmd/hive/main.go +++ b/src/cmd/hive/main.go @@ -3347,8 +3347,14 @@ func (b *boot) bootSupervision() { // the pr-request watcher narrates opened PRs into the session. // #8380: the worker-claim ledger is a second in-flight source — an issue a // human, contributor or another agent holds is withheld from kicks too. + // #10527: a free issue an agent already claimed escalate_after_claims + // times with nothing moving is withheld and escalated instead of + // re-claimed, and a likely-done issue an agent already verified after the + // merge is withheld until the reporter or a maintainer acts. b.sched.SetInflightLookup(composeInflight(b.dashSrv.LinearSessionHolder, - claimsInflightLookup(b.issueClaims, b.cfg.Project.Org))) + claimsInflightLookup(b.issueClaims, b.cfg.Project.Org), + verifyOnceLookup(b.ctx, b.cfg.Project.Org, b.issueClaims, func() *github.Client { return b.ghClient }, b.logger), + claimEscalationLookup(b.ctx, b.cfg, b.issueClaims, func() *github.Client { return b.ghClient }, b.logger))) // #9584: close answered question issues unless the author objects (default off). b.questionAutoclose.Store(wireQuestionAutoclose(b.ctx, b.cfg, b.sched, func() *github.Client { return b.ghClient }, b.logger)) // The pr-request watcher's PR-opened hook (Linear session narration, the @@ -6745,9 +6751,11 @@ func runEvalCycle( // Record issue-scoped kicks into the lifecycle timeline. Cheap, // guarded, and nil-safe (Record no-ops on a nil dashboard/store). recordKick(ctx, dashSrv, msg.Agent, msg.IssueRefs...) - // #8380: the kicked agent now holds these issues; record the - // claims so contributors and other hives back off. - recordAgentKickClaims(dashSrv, cfg.Project.Org, msg.Agent, msg.IssueRefs, logger) + // #8380/#10527: list these issues for the kicked agent so + // contributors and other agents back off while the kick is + // live; the claim itself is recorded on the agent's first + // start signal (recordAgentStart), not here. + recordAgentKickListings(dashSrv, cfg.Project.Org, msg.Agent, msg.IssueRefs, logger) // Log token state at time of kick for cost attribution if tokenCollector != nil { diff --git a/src/cmd/hive/verifyonce.go b/src/cmd/hive/verifyonce.go new file mode 100644 index 0000000000..cd130052cd --- /dev/null +++ b/src/cmd/hive/verifyonce.go @@ -0,0 +1,149 @@ +package main + +import ( + "context" + "fmt" + "log/slog" + "sync" + "time" + + "github.com/hivecommons/hive/pkg/claims" + "github.com/hivecommons/hive/pkg/github" + "github.com/hivecommons/hive/pkg/scheduler" +) + +// Verify-once gate (hivecommons/hive#10527, the #10509 pattern). +// +// An issue a merged pull request referenced without closing it is kept +// actionable with the hive/likely-done label, and the kick asks the agent to +// "verify once": close it if resolved, otherwise label it hive/verified-open. +// A human-filed bug cannot be closed by the hive before the reporter confirms, +// so the issue stayed likely-done and every later kick verified it again — #10509 +// collected eight near-identical verification comments. Once an agent has +// started on the issue after the merge (its claim, recorded on its first +// start signal, is the verification), the issue is withheld from kicks until +// a person — the reporter or a maintainer — comments, labels, assigns, closes +// or reopens it. That one verification plus the reporter-confirmation request +// is the single nudge; silence follows. + +// verifyOnceGate withholds a likely-done issue an agent already verified. +type verifyOnceGate struct { + ctx context.Context + ledger *claims.Ledger + org string + // events reads the issue timeline after since. + events func(ctx context.Context, repo string, issue int, since time.Time) ([]github.IssueEvent, error) + now func() time.Time + logger *slog.Logger + + mu sync.Mutex + verdicts map[string]progressVerdict +} + +// verifyOnceLookup returns the gate as a scheduler in-flight lookup, or nil +// when the claims ledger is off (it is where the verification is recorded). +func verifyOnceLookup(ctx context.Context, org string, ledger *claims.Ledger, client func() *github.Client, logger *slog.Logger) scheduler.InflightLookup { + if ledger == nil || client == nil { + return nil + } + g := &verifyOnceGate{ + ctx: ctx, + ledger: ledger, + org: org, + events: func(ctx context.Context, repo string, issue int, since time.Time) ([]github.IssueEvent, error) { + gh := client() + if gh == nil { + return nil, github.ErrNoGitHubClient + } + return gh.IssueTimelineSince(ctx, repo, issue, since) + }, + now: time.Now, + logger: logger, + } + return g.lookup +} + +// lookup is the scheduler.InflightLookup. It only ever looks at an issue +// carrying merged-PR context that is not yet hive/verified-open, and only +// withholds it when an agent claim on it postdates the merge and no person +// has acted on the issue since. A timeline read that fails lets the issue +// through: a duplicate verification is a smaller harm than parking work on a +// GitHub outage. +func (g *verifyOnceGate) lookup(issue github.Issue) (string, bool) { + cc := issue.ClaimContext + if issue.Number <= 0 || cc == nil || !cc.MergedPR || cc.MergedAt.IsZero() || hasLabel(issue.Labels, github.VerifiedOpenLabel) { + return "", false + } + repo := qualifyClaimRepo(g.org, issue.Repo) + attempt, ok := g.ledger.LastAttempt(repo, issue.Number) + if !ok || !attempt.ClaimedAt.After(cc.MergedAt) { + return "", false + } + key := claims.Key(repo, issue.Number) + since := attempt.ClaimedAt + held := fmt.Sprintf("verified once by %s at %s after %s#%d merged; waiting for the reporter or a maintainer — do not re-verify or comment", + attempt.Holder, since.UTC().Format(time.RFC3339), cc.PRRepo, cc.PRNumber) + now := g.now() + if v, ok := g.verdict(key, since, now); ok { + if v.allow { + return "", false + } + return held, true + } + + ctx, cancel := context.WithTimeout(g.ctx, claimsGitHubTimeout) + events, err := g.events(ctx, repo, issue.Number, since) + cancel() + if err != nil { + if g.logger != nil { + g.logger.Warn("issue-claims: could not read timeline for verify-once check, allowing the kick", + "issue", key, "error", err) + } + g.remember(key, progressVerdict{since: since, allow: true, until: now.Add(claimProgressRecheck)}) + return "", false + } + if why, acted := personActed(events); acted { + g.remember(key, progressVerdict{since: since, allow: true}) + if g.logger != nil { + g.logger.Debug("issue-claims: likely-done issue re-offered, a person acted", "issue", key, "action", why) + } + return "", false + } + g.remember(key, progressVerdict{since: since, until: now.Add(claimProgressRecheck)}) + return held, true +} + +func (g *verifyOnceGate) verdict(key string, since, now time.Time) (progressVerdict, bool) { + g.mu.Lock() + defer g.mu.Unlock() + v, ok := g.verdicts[key] + if !ok || !v.since.Equal(since) || (!v.until.IsZero() && !now.Before(v.until)) { + return progressVerdict{}, false + } + return v, true +} + +func (g *verifyOnceGate) remember(key string, v progressVerdict) { + g.mu.Lock() + defer g.mu.Unlock() + if g.verdicts == nil { + g.verdicts = map[string]progressVerdict{} + } + g.verdicts[key] = v +} + +// personActed reports the first timeline event in which a person (not a bot +// or App account, which is how the hive itself comments and labels) commented +// on, labelled, assigned, closed or reopened the issue. +func personActed(events []github.IssueEvent) (string, bool) { + for _, e := range events { + if e.Actor == "" || e.ActorIsBot { + continue + } + switch e.Event { + case "commented", "labeled", "unlabeled", "assigned", "unassigned", "closed", "reopened": + return fmt.Sprintf("%s %s", e.Actor, e.Event), true + } + } + return "", false +} diff --git a/src/cmd/hive/verifyonce_test.go b/src/cmd/hive/verifyonce_test.go new file mode 100644 index 0000000000..a943b3717c --- /dev/null +++ b/src/cmd/hive/verifyonce_test.go @@ -0,0 +1,182 @@ +package main + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/hivecommons/hive/pkg/claims" + "github.com/hivecommons/hive/pkg/github" +) + +// verifyOnceFixture is a ledger on a test clock plus a verify-once gate whose +// timeline reads are served from memory. +type verifyOnceFixture struct { + t *testing.T + ledger *claims.Ledger + now time.Time + mergedAt time.Time + gate *verifyOnceGate + timeline []github.IssueEvent + readErr error + reads int +} + +func newVerifyOnceFixture(t *testing.T) *verifyOnceFixture { + t.Helper() + f := &verifyOnceFixture{t: t, now: time.Date(2026, 10, 4, 22, 30, 0, 0, time.UTC)} + f.mergedAt = f.now.Add(-2 * time.Hour) + l, err := claims.New(filepath.Join(t.TempDir(), "claims.json"), claims.DefaultPolicy(), claims.Hooks{}) + if err != nil { + t.Fatal(err) + } + l.SetNow(func() time.Time { return f.now }) + f.ledger = l + f.gate = &verifyOnceGate{ + ctx: context.Background(), ledger: l, org: "myorg", + events: func(_ context.Context, repo string, issue int, since time.Time) ([]github.IssueEvent, error) { + f.reads++ + if repo != "myorg/repo" || issue != 10509 { + t.Fatalf("timeline read for %s#%d", repo, issue) + } + if f.readErr != nil { + return nil, f.readErr + } + var out []github.IssueEvent + for _, e := range f.timeline { + if e.At.After(since) { + out = append(out, e) + } + } + return out, nil + }, + now: func() time.Time { return f.now }, + } + return f +} + +// verify is one kick listing the issue followed by the agent's verification +// comment (its start signal), after which the claim lapses. +func (f *verifyOnceFixture) verify(agent string) { + f.t.Helper() + f.ledger.MarkListed("myorg/repo", 10509, agent) + if _, started, err := f.ledger.ClaimOnStart("myorg/repo", 10509, agent); !started || err != nil { + f.t.Fatalf("start signal by %s: started=%v err=%v", agent, started, err) + } + f.now = f.now.Add(claims.DefaultAgentTTL + time.Minute) +} + +func (f *verifyOnceFixture) event(e github.IssueEvent) { + e.At = f.now.Add(-time.Minute) + f.timeline = append(f.timeline, e) +} + +func (f *verifyOnceFixture) issue(labels ...string) github.Issue { + return github.Issue{Repo: "repo", Number: 10509, Labels: labels, ClaimContext: &github.IssueClaimContext{ + PRRepo: "myorg/repo", PRNumber: 10511, MergedPR: true, MergedAt: f.mergedAt, Reference: true, + }} +} + +func (f *verifyOnceFixture) lookup(labels ...string) (string, bool) { + return f.gate.lookup(f.issue(labels...)) +} + +func TestVerifyOnceWithholdsAfterTheFirstVerification(t *testing.T) { + f := newVerifyOnceFixture(t) + if _, held := f.lookup(github.LikelyDoneLabel); held || f.reads != 0 { + t.Fatalf("unverified likely-done issue: held=%v reads=%d, want offered once", held, f.reads) + } + f.verify("scanner") + // The hive's own verification and reporter-confirmation comments are the + // one nudge; they are not a person acting. + f.event(github.IssueEvent{Event: "commented", Actor: "hivecommons-hive[bot]", ActorIsBot: true}) + f.event(github.IssueEvent{Event: "labeled", Label: github.LikelyDoneLabel, Actor: "hivecommons-hive[bot]", ActorIsBot: true}) + + holder, held := f.lookup(github.LikelyDoneLabel) + if !held || !strings.Contains(holder, "verified once by scanner") || !strings.Contains(holder, "#10511") { + t.Fatalf("second kick after verification: held=%v holder=%q, want withheld", held, holder) + } + for range 3 { + if _, held := f.lookup(github.LikelyDoneLabel); !held { + t.Fatal("verified issue offered again") + } + } + if f.reads != 1 { + t.Fatalf("reads=%d, want the withhold verdict reused within the recheck window", f.reads) + } + f.now = f.now.Add(8 * time.Hour) + if _, held := f.lookup(github.LikelyDoneLabel); !held { + t.Fatal("verified issue offered again with no one acting") + } + + // The reporter replying is a person acting: the next kick may look again. + f.event(github.IssueEvent{Event: "commented", Actor: "clubanderson"}) + f.now = f.now.Add(claimProgressRecheck) + if _, held := f.lookup(github.LikelyDoneLabel); held { + t.Fatal("issue still withheld after the reporter replied") + } + // ... and that next verification starts a new silence. + f.verify("scanner") + if _, held := f.lookup(github.LikelyDoneLabel); !held { + t.Fatal("second verification not followed by silence") + } +} + +func TestVerifyOnceIgnoresOtherIssues(t *testing.T) { + f := newVerifyOnceFixture(t) + f.verify("scanner") + if _, held := f.lookup(github.VerifiedOpenLabel); held { + t.Fatal("hive/verified-open issue withheld: its remainder is real work") + } + plain := github.Issue{Repo: "repo", Number: 10509} + if _, held := f.gate.lookup(plain); held { + t.Fatal("issue without merged-PR context withheld") + } + // A claim from before the merge is not a verification of it. + f.mergedAt = f.now + if _, held := f.lookup(github.LikelyDoneLabel); held { + t.Fatal("claim that predates the merge treated as a verification") + } + if f.reads != 0 { + t.Fatalf("reads=%d, want none for issues the gate does not cover", f.reads) + } +} + +func TestVerifyOnceFailsOpenOnTimelineError(t *testing.T) { + f := newVerifyOnceFixture(t) + f.verify("scanner") + f.readErr = errors.New("502 bad gateway") + if _, held := f.lookup(github.LikelyDoneLabel); held { + t.Fatal("timeline error withheld the issue") + } + f.readErr = nil + f.now = f.now.Add(claimProgressRecheck) + if _, held := f.lookup(github.LikelyDoneLabel); !held { + t.Fatal("after recovery the verified issue was not withheld") + } +} + +func TestPersonActed(t *testing.T) { + cases := []struct { + name string + event github.IssueEvent + acted bool + }{ + {"reporter comment", github.IssueEvent{Event: "commented", Actor: "alice"}, true}, + {"maintainer label", github.IssueEvent{Event: "labeled", Label: "hive: reporter-confirmed", Actor: "bob"}, true}, + {"maintainer close", github.IssueEvent{Event: "closed", Actor: "bob"}, true}, + {"hive comment", github.IssueEvent{Event: "commented", Actor: "hivecommons-hive[bot]", ActorIsBot: true}, false}, + {"actorless", github.IssueEvent{Event: "commented"}, false}, + {"cross-reference", github.IssueEvent{Event: "cross-referenced", Actor: "alice", SourcePR: 1}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if _, acted := personActed([]github.IssueEvent{tc.event}); acted != tc.acted { + t.Fatalf("personActed(%+v)=%v, want %v", tc.event, acted, tc.acted) + } + }) + } +} diff --git a/src/docs/contributor-relay.md b/src/docs/contributor-relay.md index 0afd3b9a90..dd1d281ccb 100644 --- a/src/docs/contributor-relay.md +++ b/src/docs/contributor-relay.md @@ -770,7 +770,7 @@ Two admission behaviors are worth knowing when your relay seems idle: - **A closing keyword written as prose counts.** The claim parser used to require GitHub's own trailer shape, `keyword #N` (optionally with a colon), so a merged PR whose body read *"Resolves architect issue #1232 (first incremental step)"* produced **no claim at all** ([#7995](https://github.com/hivecommons/hive/issues/7995)). Since that issue the closing tier accepts the same bounded run of prose the non-closing reference tier already did — at most 40 characters, no `.`, newline or `#` — so `Resolves architect issue #1232` and `Fixes the bug in #12` are closing claims. GitHub will still not auto-close on those, and hive does not pretend it will: a claim here is about whether the **work landed**, and a merged PR that says it resolves an issue is the strongest evidence the hub gets, so it suppresses rather than being released for re-verification after every merge. The prose gap must begin with a space or tab, which is what keeps a Conventional Commits type prefix (`fix:`, `fix(scope):`) from claiming the first issue number in a title. The closing tier remains single-issue, matching GitHub's one-keyword-per-number rule; only the reference tier reads lists. - **A merged fix does not expire on a clock.** A strong claim from a merged PR — or a verified `no_work_needed` verdict — used to leave the ledger 72 hours after the merge, because the settle scan looks back 72h and the ledger TTL matched it. The PR was still merged; only the clock had moved. So the issue went straight back into the offer pool, an agent re-verified "already fixed on main", and the verdict claim it left sat on the same 72h clock — one wasted task cycle every three days for as long as nobody closed the issue ([#8003](https://github.com/hivecommons/hive/issues/8003): projectbluefin/chairlift#55, fixed by #133, re-offered three times with two "please close" agent comments on it). Settled claims are now carried forward across scans for 30 days from the merge; a merged *weak* claim (a `Refs #N`, or an external author's PR) is not — it never asserted it finished the issue, and is still released with context as before. And because the hold has by then outlasted every automatic bound, after **7 days** the contribute queue stops describing it as "an open pull request already claims this issue" and withholds it under `merged_claim_stale`, whose prose is the question a maintainer has to answer: *Fixed by merged PR #133 4 days ago; the issue is still open — close it or say what remains.* - **An issue that keeps consuming pull requests is withheld for a maintainer.** The claim ledger answers "is somebody on this right now?"; it could not answer "has this issue already cost seven pull requests?" ([#7995](https://github.com/hivecommons/hive/issues/7995)). projectbluefin/documentation#1232 — an incremental architecture issue — went three merged and four closed PRs deep and was still NEXT UP: every cycle an agent found remaining work, shipped the next slice, and the issue came back after the merge, because only a human can decide that an incremental issue is *done*. So the same two PR listings the claim scan already pages through now also feed a cumulative **churn history**: which pull requests have been observed against each issue, and how each one ended. A closed-without-merge PR still claims nothing — the issue stays released for work — it only counts. Once an issue reaches **2 merged** or **2 closed-unmerged** pull requests, the contribute queue stops offering it and withholds it with reason `issue_churn`, on the same operator-visible Withheld surface as an open-PR claim, carrying the counts and the PR numbers a maintainer needs in order to answer the question. Nothing is closed, commented on, or relabelled by the guard. It lapses on its own: records age out 14 days after a pull request was last seen (deliberately longer than the 72h settle scan, because the pattern plays out over a week or more), and a maintainer who has triaged the issue can clear the guard immediately by adding the label `hive: churn-triaged`. -- **An issue somebody has *claimed* is not offered — and a clanker already on it is pulled off when a higher rank claims it.** The PR-claim ledger above only knows about work that has reached a pull request. Since [#8380](https://github.com/hivecommons/hive/issues/8380) the hub also keeps a **worker-claim ledger**: who is on an issue *right now*, ranked **human > agent > contributor > external**. A person claims from the terminal (`hivectl claim owner/repo#N`) or the API (`POST /api/claims/{owner}/{repo}/{number}`); the hub records an **agent** claim for every issue a governor kick hands one of its own agents; and the relay records a **contributor** claim for your session the moment it leases you a task, releasing it when the task completes, is abandoned, is yanked or its lease expires. `selectTask` excludes every issue another holder claims exactly as it excludes leased items, and the scheduler withholds them from kicks under *In Flight*. Precedence is enforced on write: a higher rank **takes over** and the displaced holder is told to stop — for a relay session that means the hub yanks that one task (`task_revoke`, generation bumped so a late completion is fenced) and hands the session its next admissible item in the same call, leaving your other sessions alone; for an agent or an external bot it means a `🔁` comment addressed to it plus a `preempted:` label on the issue. Same rank warns unless forced; a lower rank is refused. Claims lapse on a per-kind TTL (`claims.human_ttl_s` 4h, `agent_ttl_s` 2h, `contributor_ttl_s` 30m — the relay lease length — capped by `max_ttl_s` 24h) and `claims.enabled: false` turns the whole mechanism off. Every transition is mirrored onto the issue as a comment carrying a `` marker (or `` on takeover) and a `claimed` label, so a relay that only reads GitHub can still see it was preempted. +- **An issue somebody has *claimed* is not offered — and a clanker already on it is pulled off when a higher rank claims it.** The PR-claim ledger above only knows about work that has reached a pull request. Since [#8380](https://github.com/hivecommons/hive/issues/8380) the hub also keeps a **worker-claim ledger**: who is on an issue *right now*, ranked **human > agent > contributor > external**. A person claims from the terminal (`hivectl claim owner/repo#N`) or the API (`POST /api/claims/{owner}/{repo}/{number}`); a governor kick *lists* each issue it hands one of the hive's own agents — no GitHub comment, no claim, just a 30-minute hold that keeps your session and other agents off it while the kick is live — and the hub records the **agent** claim on the agent's first start signal on that issue (its own comment, label or claim request through `hive-open-issue`, or a `hive-open-pr` request naming it; [#10527](https://github.com/hivecommons/hive/issues/10527)); and the relay records a **contributor** claim for your session the moment it leases you a task, releasing it when the task completes, is abandoned, is yanked or its lease expires. `selectTask` excludes every issue another holder claims exactly as it excludes leased items, and the scheduler withholds them from kicks under *In Flight*. Precedence is enforced on write: a higher rank **takes over** and the displaced holder is told to stop — for a relay session that means the hub yanks that one task (`task_revoke`, generation bumped so a late completion is fenced) and hands the session its next admissible item in the same call, leaving your other sessions alone; for an agent or an external bot it means a `🔁` comment addressed to it plus a `preempted:` label on the issue. Same rank warns unless forced; a lower rank is refused. Claims lapse on a per-kind TTL (`claims.human_ttl_s` 4h, `agent_ttl_s` 2h, `contributor_ttl_s` 30m — the relay lease length — capped by `max_ttl_s` 24h) and `claims.enabled: false` turns the whole mechanism off. Every transition is mirrored onto the issue as a comment carrying a `` marker (or `` on takeover) and a `claimed` label, so a relay that only reads GitHub can still see it was preempted. - **Claims expire.** Ledger entries live 72 hours — refreshed while the PR stays open, or anchored at the merge for a merged claim, so a settled issue is suppressed for 72h after its fix lands and nothing is stranded forever. An **open** claiming PR that goes red on a required check and stale releases the issue back to the queue; merged claims never take that valve — a fix already on `main` cannot go red. - **A repository the App cannot mint a token for is excluded, not fatal.** Since C4 the `task_assign` token is scoped to the chosen item's repository, so GitHub's `422 There is at least one repository that does not exist or is not accessible to the parent installation` means *that repo* — renamed, deleted, or removed from the installation — and nothing about the tier or the App key. It used to fail the whole selection with `token_mint_failed`, and because selection is deterministic the same doomed item was chosen on every 30 s retry: one stale entry in the project repo list (a renamed repo) wedged a hive for every contributor while `actionable_items` sat at 19 ([#7869](https://github.com/hivecommons/hive/issues/7869)). Now the claim is rolled back, the repo is excluded from selection for `mintFailureRepoCooldown` (10 min, so the fleet is not re-discovering it every retry, but the operator's fix takes effect without a restart), and the *same* call selects again from the remaining candidates (bounded at three such skips). If nothing else is admissible the reason is `repo_unmintable` rather than `no_matching_work` or `token_mint_failed`, and the hub log names the repo with a pointer at the repo list. Non-repo mint failures (5xx, bad key, an unaccepted permission) keep the `token_mint_failed` contract and exclude nothing — they would recur for any candidate. diff --git a/src/docs/labels-and-control-signals.md b/src/docs/labels-and-control-signals.md index b0aff05fa1..105c6b363f 100644 --- a/src/docs/labels-and-control-signals.md +++ b/src/docs/labels-and-control-signals.md @@ -41,7 +41,7 @@ The "respect hold labels" text in policy templates names the enforced set (`hold | `hold` from SHA-hold | Issues | SHA-hold sweep | Human-filed primary-repo `kind/bug` without a 7-40 hex SHA gets `hold` and a notice carrying ``; once a SHA appears in body/comments, Hive removes `hold` only if the current hold is its own (newest `hold` event labeled by the App bot, paired with an App-authored notice at or after it). A human's hold, a re-hold, or another subsystem's hold stays. | Hard issue hold | Hive | Eval tick SHA sweep | Add SHA evidence; sweep removes its own `hold`, a human removes any other | Primary repo and SHA-hold config (`src/pkg/github/client.go:2467-2532`, `src/pkg/github/client.go:2582-2604`) | | `do-not-merge` and `do-not-merge*` | Issues, PRs | Exempt filter, merge sweeps, task-list sweep | Permanently exempt; exact match is case-insensitive but prefix matching follows Go `strings.HasPrefix` on the original label. | Hard suppress | Human | Enumeration/sweeps | Human removes | Built-in `PermanentExemptLabels` (`src/pkg/github/client.go:750-755`, `src/pkg/github/client.go:2030-2048`) | | `governor.labels.exempt` entries | Issues, PRs | Exempt filter | Same exempt behavior as `do-not-merge`; wins over required-label admission. Defaults include `nightly-tests`, `LFX`, `meta-tracker`, `auto-qa-tuning-report`, `adopters`, `changes-requested`, `waiting-on-author`. | Hard suppress | Operator/dashboard | Enumeration/sweeps | Remove label or config entry | `governor.labels.exempt` (`src/pkg/config/config.go:3388-3396`, `src/pkg/config/config.go:5800-5808`) | -| `needs-human` | Issues | Enumeration, task-list sweep and un-park sweep | Issue is filtered, not held: no agent kick or contributor offer; task-list sweep can add it when a merged PR leaves human remainder work (never on a held issue). The un-park sweep keeps a "What to reply" comment on every parked issue listing the commands. See [Maintainer commands](maintainer-commands.md). | Hard suppress | Hive or human | Enumeration/task-list sweep | Human removes, or a maintainer with write/maintain/admin access replies `/hive approve` or `/hive decision ` on the issue, which also adds `approved-direction` and never touches `hold` (`src/pkg/github/issue_unpark_command.go`) | Fixed label (`src/pkg/github/client.go:1008-1024`, `src/pkg/github/task_list_sweep.go:611-641`, `src/pkg/github/task_list_sweep.go:797-815`) | +| `needs-human` | Issues | Enumeration, task-list sweep, claim escalation gate and un-park sweep | Issue is filtered, not held: no agent kick or contributor offer; task-list sweep can add it when a merged PR leaves human remainder work (never on a held issue), and the claim escalation gate adds it instead of a third no-progress claim by the same agent (`governor.claims.escalate_after_claims`, [#10527](https://github.com/hivecommons/hive/issues/10527)). The un-park sweep keeps a "What to reply" comment on every parked issue listing the commands. See [Maintainer commands](maintainer-commands.md). | Hard suppress | Hive or human | Enumeration/task-list sweep/kick building | Human removes, or a maintainer with write/maintain/admin access replies `/hive approve` or `/hive decision ` on the issue, which also adds `approved-direction` and never touches `hold` (`src/pkg/github/issue_unpark_command.go`) | Fixed label (`src/pkg/github/client.go:1008-1024`, `src/pkg/github/task_list_sweep.go:611-641`, `src/pkg/github/task_list_sweep.go:797-815`, `src/cmd/hive/claimescalation.go`) | | `needs-human` | PRs | Escalation ledger and reviewer lane | Applied when red-CI fix budget is exhausted; stops automated fix dispatch and sends the PR to the human/reviewer lane. It is not a PR enumeration filter. | Hard fix-dispatch gate | Hive | Escalation pass/kick building | Human removes after root cause or reviewer un-escalates; budget resets after grace | Fixed label (`src/pkg/escalation/escalation.go:400-470`, `src/pkg/escalation/escalation.go:1185-1205`) | | `hive/advisory` | Issues | Standing meta classifier | Hive advisory report is never actionable and never appears on the Hold list. | Hard exclusion | Hive | Before holds/exempts | Remove label, but exact advisory title still excludes | Fixed label/name (`src/pkg/github/standing_issues.go:1-57`, `src/pkg/github/client.go:983-1001`) | | `approved-direction` | Agent-filed issues | #5117 gate, ranking, kick tag | A PR opened after the label is present avoids #5117 hold; the issue ranks ahead of unacknowledged hive-filed backlog. | Gate input + soft ranking | Human | PR creation and issue ranking | Remove label; human assignee/comment can still acknowledge for #5117 | Fixed `HumanAckLabel` (`src/pkg/github/pr_self_authorization.go:14-226`, `src/pkg/github/client.go:2740-2832`) | diff --git a/src/docs/operator-reference.md b/src/docs/operator-reference.md index 3bb9d9dd1b..db7533495a 100644 --- a/src/docs/operator-reference.md +++ b/src/docs/operator-reference.md @@ -74,7 +74,7 @@ Top-level YAML keys accepted by `config.Config`: | `contribute.help_links` | Defaults to Hive contributor docs and the Hive issue tracker. | Up to 5 `{label, url}` links shown on `/contribute` Onboarding, Operations, and printed once by relays on connect. URLs must be absolute `http://` or `https://`; labels are plain text and length-capped. Prefer the Hive Commons Discord redirect (`https://hivecommons.dev/discord`) over channel URLs for contributors who may not already be server members. | | `variables` | Trusted variable resolver definitions. | Env-only substitution works without this block. | | `classification` | Go-consumed subset of `hive-project.yaml`'s `classification:` block — today only `review_bots`, the external review-bot logins whose inline threads on hive-authored PRs the hive addresses and resolves itself. | Off unless `review_bots.logins` names a bot. The same key in `hive-project.yaml` is read when `hive.yaml` has none. See [review-bot-threads.md](review-bot-threads.md). | -| `claims` | The worker-claim ledger ([#8380](https://github.com/hivecommons/hive/issues/8380)): who is on an issue right now, ranked human > agent > contributor > external, so a person can take an issue over from a hive agent or a relay contributor and the displaced holder is told to stop. | Default ON. `enabled: false` turns recording, enforcement and `/api/claims` off. `human_ttl_s` 4h, `agent_ttl_s` 2h, `contributor_ttl_s` 30m, `max_ttl_s` 24h; `comment: false` / `label: false` suppress the GitHub issue comment / `claimed` label mirror. Driven from `hivectl claim` — see [hivectl.md](hivectl.md#claim--unclaim--claims--issue-claims). | +| `claims` | The worker-claim ledger ([#8380](https://github.com/hivecommons/hive/issues/8380)): who is on an issue right now, ranked human > agent > contributor > external, so a person can take an issue over from a hive agent or a relay contributor and the displaced holder is told to stop. | Default ON. `enabled: false` turns recording, enforcement and `/api/claims` off. `human_ttl_s` 4h, `agent_ttl_s` 2h, `contributor_ttl_s` 30m, `max_ttl_s` 24h; `comment: false` / `label: false` suppress the GitHub issue comment / `claimed` label mirror. A governor kick only *lists* the issues it names (30-minute hold, no comment, not a claim); the agent claim, with its comment, is recorded on the agent's first start signal on an issue — its own comment, label or claim request through `hive-open-issue`, or a `hive-open-pr` request naming it. `escalate_after_claims` (default `2`, negative turns it off) counts those claims only: once one agent's last that-many claims on a free issue were followed by no linked PR, referencing commit, label or assignee change and no close/reopen, the issue is withheld from every kick, labelled `needs-human` and given one comment listing the claims, instead of being claimed again. A `hive/likely-done` issue an agent already started on after the merged PR (its verification) is withheld from kicks until a person comments, labels, assigns, closes or reopens it, so it is verified once rather than every cycle ([#10527](https://github.com/hivecommons/hive/issues/10527)). Driven from `hivectl claim` — see [hivectl.md](hivectl.md#claim--unclaim--claims--issue-claims). | | `removed_agents` | Persistent tombstones for deleted agents. | Dashboard/overlay-owned; do not seed casually. | ## Notable fields diff --git a/src/docs/scale-envelope.md b/src/docs/scale-envelope.md index 81f512197b..347662ddee 100644 --- a/src/docs/scale-envelope.md +++ b/src/docs/scale-envelope.md @@ -34,7 +34,7 @@ ones to: 2. **Truncation must preserve the prioritization signal, not cut an arbitrary slice.** This holds today and is easy to assume backwards: - Issues are sorted **oldest first** before any cap is applied - (`src/pkg/github/client.go:3249-3257`, descending `AgeMinutes`). + (`src/pkg/github/client.go:3250-3258`, descending `AgeMinutes`). - PRs are sorted by **review class then oldest-first within class** (`SortPullRequestsForReview`, `src/pkg/github/review_priority.go:181`; fixes → refactors/docs → tests). diff --git a/src/pkg/claims/claims.go b/src/pkg/claims/claims.go index d53a53c741..6926d51ba6 100644 --- a/src/pkg/claims/claims.go +++ b/src/pkg/claims/claims.go @@ -40,8 +40,8 @@ const ( // KindHuman is a person acting directly: dashboard, hivectl, an `/claim` // comment, or their own IDE/CLI session running under their login. KindHuman Kind = "human" - // KindAgent is one of this hive's own agents, claimed by the hub when the - // scheduler kicks it with the issue in IssueRefs. + // KindAgent is one of this hive's own agents, claimed by the hub on the + // agent's first start signal on an issue a kick listed to it (#10527). KindAgent Kind = "agent" // KindContributor is a relay contributor ("clanker") the hub dispatched // the issue to over the contribute WebSocket. @@ -123,6 +123,75 @@ func (c Claim) Expired(now time.Time) bool { return !now.Before(c.ExpiresAt) } // Key builds the canonical work-item key. func Key(repo string, issue int) string { return repo + "#" + fmt.Sprint(issue) } +// Attempt is one agent claim on an issue, remembered after the claim itself +// ends so the escalation gate (#10527) can tell an agent re-claiming an issue +// it already held, with nothing moving in between, from a healthy long fix. +type Attempt struct { + // Holder is the agent that claimed. + Holder string `json:"holder"` + // ClaimedAt is when the claim was taken. + ClaimedAt time.Time `json:"claimed_at"` + // EndedAt is when the claim ended; zero while it is live. + EndedAt time.Time `json:"ended_at,omitempty"` + // Ended is how it ended: "expired", a release reason, or "taken over by + // ". + Ended string `json:"ended,omitempty"` +} + +// AttemptRetention is how long an agent's claim attempts are remembered. A +// run of claims spread over more than this is not treated as one stall. +const AttemptRetention = 7 * 24 * time.Hour + +// Stall is the most recent run of claims one agent made on an issue that is +// free again — the shape the escalation gate (#10527) checks for progress. +type Stall struct { + Repo string + Issue int + Holder string + // Attempts are the holder's trailing claims on the issue, oldest first. + Attempts []Attempt +} + +// Key is the canonical "owner/repo#N" key of the stalled issue. +func (s Stall) Key() string { return Key(s.Repo, s.Issue) } + +// Since is when the run began. Progress on the issue after it breaks the run. +func (s Stall) Since() time.Time { + if len(s.Attempts) == 0 { + return time.Time{} + } + return s.Attempts[0].ClaimedAt +} + +// Listing is a kick naming an issue to one of the hive's agents (#10527). +// +// A kick lists up to governor.kick_limits.max_issues issues, and the agent +// starts on at most a few of them, so a listing is not a claim: it posts no +// GitHub comment and is not an Attempt. It holds the issue back from other +// agents and relay contributors for DefaultListedTTL, and lets the agent's +// first start signal on the issue (a comment, a label or claim request, a +// pull-request request) turn into a real claim for as long as the agent's +// claim TTL after the kick. +type Listing struct { + Repo string `json:"repo"` + Issue int `json:"issue"` + Holder string `json:"holder"` + // ListedAt is when the kick was delivered. + ListedAt time.Time `json:"listed_at"` + // ExpiresAt is when the hold on other workers lapses. + ExpiresAt time.Time `json:"expires_at"` +} + +// Key is the canonical "owner/repo#N" key of the listed issue. +func (li Listing) Key() string { return Key(li.Repo, li.Issue) } + +// Expired reports whether the listing's hold has lapsed at now. +func (li Listing) Expired(now time.Time) bool { return !now.Before(li.ExpiresAt) } + +// DefaultListedTTL is how long a kick listing holds an issue back from other +// workers. Short on purpose: it covers the kick being live, not the work. +const DefaultListedTTL = 30 * time.Minute + // Request is one attempt to claim an issue. type Request struct { Repo string @@ -264,6 +333,14 @@ func (p Policy) ttlFor(k Kind, requested time.Duration) time.Duration { type Ledger struct { mu sync.Mutex claims map[string]Claim + // attempts holds every agent claim per issue key, oldest first, pruned + // after AttemptRetention. + attempts map[string][]Attempt + // escalated maps an issue key to the Since of the last stall escalated + // on it, so one run is escalated once however often it is observed. + escalated map[string]time.Time + // listed holds the kick listings (#10527) by issue key. + listed map[string]Listing path string policy Policy hooks Hooks @@ -282,11 +359,14 @@ func New(path string, policy Policy, hooks Hooks) (*Ledger, error) { policy = DefaultPolicy() } l := &Ledger{ - claims: map[string]Claim{}, - path: path, - policy: policy, - hooks: hooks, - now: time.Now, + claims: map[string]Claim{}, + attempts: map[string][]Attempt{}, + escalated: map[string]time.Time{}, + listed: map[string]Listing{}, + path: path, + policy: policy, + hooks: hooks, + now: time.Now, } err := l.load() return l, err @@ -340,6 +420,7 @@ func (l *Ledger) Claim(req Request) (Result, error) { case !held: l.claims[key] = next res = Result{Outcome: OutcomeClaimed, Claim: next} + l.recordAttemptLocked(next) case sameHolder(prev, req): prev.ExpiresAt = now.Add(ttl) l.claims[key] = prev @@ -350,6 +431,8 @@ func (l *Ledger) Claim(req Request) (Result, error) { l.claims[key] = next p := prev res = Result{Outcome: OutcomeTakenOver, Claim: next, Previous: &p} + l.endAttemptLocked(prev, "taken over by "+req.Holder, now) + l.recordAttemptLocked(next) case req.Kind.rank() == prev.Kind.rank(): p := prev res = Result{Outcome: OutcomeHeld, Claim: prev, Previous: &p} @@ -357,6 +440,9 @@ func (l *Ledger) Claim(req Request) (Result, error) { p := prev res = Result{Outcome: OutcomeRefused, Claim: prev, Previous: &p} } + if res.Outcome.Changed() { + delete(l.listed, key) + } var saveErr error if res.Outcome.Changed() || len(expired) > 0 { saveErr = l.saveLocked() @@ -412,7 +498,8 @@ func (l *Ledger) Release(repo string, issue int, by string, byKind Kind, reason return Claim{}, false, nil } l.mu.Lock() - expired := l.expireLocked(l.now()) + now := l.now() + expired := l.expireLocked(now) key := Key(strings.TrimSpace(repo), issue) c, ok := l.claims[key] if !ok { @@ -431,6 +518,7 @@ func (l *Ledger) Release(repo string, issue int, by string, byKind Kind, reason return c, false, fmt.Errorf("claims: %s is held by %s (%s)", key, c.Holder, c.Kind) } delete(l.claims, key) + l.endAttemptLocked(c, reason, now) err := l.saveLocked() hooks := l.hooks l.mu.Unlock() @@ -452,6 +540,7 @@ func (l *Ledger) ForceRelease(repo string, issue int, reason string) (Claim, boo c, ok := l.claims[key] if ok { delete(l.claims, key) + l.endAttemptLocked(c, reason, l.now()) _ = l.saveLocked() } hooks := l.hooks @@ -471,6 +560,7 @@ func (l *Ledger) ReleaseByHolderID(holderID, reason string, onlyKey ...string) [ return nil } l.mu.Lock() + now := l.now() var out []Claim for k, c := range l.claims { if c.HolderID != holderID { @@ -481,6 +571,7 @@ func (l *Ledger) ReleaseByHolderID(holderID, reason string, onlyKey ...string) [ } out = append(out, c) delete(l.claims, k) + l.endAttemptLocked(c, reason, now) } if len(out) > 0 { _ = l.saveLocked() @@ -536,6 +627,12 @@ func (l *Ledger) HeldKeys(exceptHolderID string) map[string]bool { } out[k] = true } + for k, li := range l.listed { + if li.Expired(now) || (exceptHolderID != "" && li.Holder == exceptHolderID) { + continue + } + out[k] = true + } return out } @@ -596,16 +693,241 @@ func (l *Ledger) expireLocked(now time.Time) []Claim { if c.Expired(now) { dropped = append(dropped, c) delete(l.claims, k) + l.endAttemptLocked(c, "expired", c.ExpiresAt) } } + l.pruneLocked(now) return dropped } +// ---- attempt history (#10527) ---- + +// recordAttemptLocked remembers a new agent claim. Only agent claims are +// counted: the gate exists to stop the hive's own agents spinning on an issue, +// and a person's claim is never second-guessed. +func (l *Ledger) recordAttemptLocked(c Claim) { + if c.Kind != KindAgent { + return + } + key := c.Key() + l.attempts[key] = append(l.attempts[key], Attempt{Holder: c.Holder, ClaimedAt: c.ClaimedAt}) +} + +// endAttemptLocked records how an agent claim ended on its attempt entry. +func (l *Ledger) endAttemptLocked(c Claim, reason string, at time.Time) { + if c.Kind != KindAgent { + return + } + list := l.attempts[c.Key()] + for i := len(list) - 1; i >= 0; i-- { + if list[i].Holder == c.Holder && list[i].ClaimedAt.Equal(c.ClaimedAt) { + if list[i].EndedAt.IsZero() { + list[i].EndedAt, list[i].Ended = at, reason + } + return + } + } +} + +// pruneLocked forgets attempts and escalations older than AttemptRetention. +func (l *Ledger) pruneLocked(now time.Time) { + cutoff := now.Add(-AttemptRetention) + for k, list := range l.attempts { + keep := list[:0] + for _, a := range list { + if a.ClaimedAt.After(cutoff) { + keep = append(keep, a) + } + } + if len(keep) == 0 { + delete(l.attempts, k) + continue + } + l.attempts[k] = keep + } + for k, since := range l.escalated { + if !since.After(cutoff) { + delete(l.escalated, k) + } + } + window := l.listingWindow() + for k, li := range l.listed { + if li.Expired(now) && !now.Before(li.ListedAt.Add(window)) { + delete(l.listed, k) + } + } +} + +// ---- kick listings (#10527) ---- + +// listingWindow is how long after a kick the listed agent's first start +// signal still becomes a claim: the agent claim TTL, the hold a kick used to +// take on every issue it named. +func (l *Ledger) listingWindow() time.Duration { + return l.policy.ttlFor(KindAgent, 0) +} + +// MarkListed records that a delivered kick named the issue to the agent +// holder. It posts nothing and counts as no claim. An issue under a live claim +// or a live listing for another agent is left alone and reports false. +func (l *Ledger) MarkListed(repo string, issue int, holder string) (Listing, bool) { + repo, holder = strings.TrimSpace(repo), strings.TrimSpace(holder) + if l == nil || repo == "" || issue <= 0 || holder == "" { + return Listing{}, false + } + l.mu.Lock() + now := l.now() + expired := l.expireLocked(now) + key := Key(repo, issue) + _, held := l.claims[key] + prev, listed := l.listed[key] + var li Listing + ok := !held && (!listed || prev.Expired(now) || prev.Holder == holder) + if ok { + li = Listing{Repo: repo, Issue: issue, Holder: holder, ListedAt: now, ExpiresAt: now.Add(DefaultListedTTL)} + l.listed[key] = li + } + if ok || len(expired) > 0 { + _ = l.saveLocked() + } + hooks := l.hooks + l.mu.Unlock() + fireExpired(hooks, expired) + return li, ok +} + +// Listed returns the live kick listing on an issue, if any. +func (l *Ledger) Listed(repo string, issue int) (Listing, bool) { + if l == nil { + return Listing{}, false + } + l.mu.Lock() + defer l.mu.Unlock() + li, ok := l.listed[Key(strings.TrimSpace(repo), issue)] + if !ok || li.Expired(l.now()) { + return Listing{}, false + } + return li, true +} + +// ClaimOnStart turns the agent's first start signal on an issue into a real +// agent claim — comment, label and Attempt included — when a kick listed the +// issue to that agent within the listing window, or renews the agent's own +// live claim. Any other start signal (a comment on an issue nobody kicked the +// agent with, a request about its own pull request) reports false and +// changes nothing. +func (l *Ledger) ClaimOnStart(repo string, issue int, agent string) (Result, bool, error) { + repo, agent = strings.TrimSpace(repo), strings.TrimSpace(agent) + if l == nil || repo == "" || issue <= 0 || agent == "" { + return Result{}, false, nil + } + l.mu.Lock() + now := l.now() + key := Key(repo, issue) + c, held := l.claims[key] + li, listed := l.listed[key] + ok := (held && !c.Expired(now) && c.Kind == KindAgent && c.Holder == agent) || + (listed && li.Holder == agent && now.Before(li.ListedAt.Add(l.listingWindow()))) + l.mu.Unlock() + if !ok { + return Result{}, false, nil + } + res, err := l.Claim(Request{Repo: repo, Issue: issue, Holder: agent, HolderID: agent, Kind: KindAgent}) + return res, true, err +} + +// LastAttempt returns the most recent agent claim recorded on the issue, by +// any agent, within AttemptRetention. +func (l *Ledger) LastAttempt(repo string, issue int) (Attempt, bool) { + if l == nil { + return Attempt{}, false + } + l.mu.Lock() + defer l.mu.Unlock() + list := l.attempts[Key(strings.TrimSpace(repo), issue)] + if len(list) == 0 { + return Attempt{}, false + } + return list[len(list)-1], true +} + +// Stall reports the most recent run of threshold claims one agent made on the +// issue, when the issue has no live claim. Of several agents with such a run +// it returns the one whose run began last: if anything moved since that run +// began, it moved during every other agent's run too. threshold <= 0, a live +// claim, or no agent with threshold remembered claims reports false. Lapsed +// claims are expired first, so the run's last attempt carries its end. +func (l *Ledger) Stall(repo string, issue, threshold int) (Stall, bool) { + if l == nil || threshold <= 0 { + return Stall{}, false + } + repo = strings.TrimSpace(repo) + key := Key(repo, issue) + l.mu.Lock() + expired := l.expireLocked(l.now()) + if len(expired) > 0 { + _ = l.saveLocked() + } + var best Stall + if _, held := l.claims[key]; !held { + best = l.stallLocked(repo, issue, threshold) + } + hooks := l.hooks + l.mu.Unlock() + fireExpired(hooks, expired) + return best, best.Attempts != nil +} + +func (l *Ledger) stallLocked(repo string, issue, threshold int) Stall { + byHolder := map[string][]Attempt{} + for _, a := range l.attempts[Key(repo, issue)] { + byHolder[a.Holder] = append(byHolder[a.Holder], a) + } + var best Stall + for holder, list := range byHolder { + if len(list) < threshold { + continue + } + run := append([]Attempt(nil), list[len(list)-threshold:]...) + if best.Attempts == nil || run[0].ClaimedAt.After(best.Since()) || + (run[0].ClaimedAt.Equal(best.Since()) && holder < best.Holder) { + best = Stall{Repo: repo, Issue: issue, Holder: holder, Attempts: run} + } + } + return best +} + +// MarkEscalated records that the stall s was escalated and reports whether +// this call was the first for that run, so the escalation fires once per run +// however many times the gate observes it. A later run on the same issue (it +// began after s) escalates again. +func (l *Ledger) MarkEscalated(s Stall) bool { + if l == nil || len(s.Attempts) == 0 { + return false + } + key := s.Key() + since := s.Since() + l.mu.Lock() + defer l.mu.Unlock() + if prev, ok := l.escalated[key]; ok && prev.Equal(since) { + return false + } + l.escalated[key] = since + _ = l.saveLocked() + return true +} + // ---- persistence ---- type persisted struct { Version int `json:"version"` Claims []Claim `json:"claims"` + // Attempts and Escalated carry the #10527 escalation history; both are + // absent from files written before it, which load as no history. + Attempts map[string][]Attempt `json:"attempts,omitempty"` + Escalated map[string]time.Time `json:"escalated,omitempty"` + // Listed carries the kick listings (#10527). + Listed []Listing `json:"listed,omitempty"` } const persistVersion = 1 @@ -631,6 +953,21 @@ func (l *Ledger) load() error { } l.claims[c.Key()] = c } + for k, list := range p.Attempts { + for _, a := range list { + if a.Holder != "" && !a.ClaimedAt.IsZero() { + l.attempts[k] = append(l.attempts[k], a) + } + } + } + for k, since := range p.Escalated { + l.escalated[k] = since + } + for _, li := range p.Listed { + if li.Repo != "" && li.Issue > 0 && li.Holder != "" { + l.listed[li.Key()] = li + } + } return nil } @@ -646,6 +983,16 @@ func (l *Ledger) saveLocked() error { p.Claims = append(p.Claims, c) } sort.Slice(p.Claims, func(i, j int) bool { return p.Claims[i].Key() < p.Claims[j].Key() }) + if len(l.attempts) > 0 { + p.Attempts = l.attempts + } + if len(l.escalated) > 0 { + p.Escalated = l.escalated + } + for _, li := range l.listed { + p.Listed = append(p.Listed, li) + } + sort.Slice(p.Listed, func(i, j int) bool { return p.Listed[i].Key() < p.Listed[j].Key() }) data, err := json.MarshalIndent(p, "", " ") if err != nil { return err diff --git a/src/pkg/claims/comments.go b/src/pkg/claims/comments.go index 548a7f9152..f28a3c464f 100644 --- a/src/pkg/claims/comments.go +++ b/src/pkg/claims/comments.go @@ -18,9 +18,10 @@ const ( // Marker names used in the HTML comments the hub writes so tooling can parse // them without scraping prose. const ( - markerClaim = "hive:claim" - markerPreempt = "hive:preempt" - markerRelease = "hive:release" + markerClaim = "hive:claim" + markerPreempt = "hive:preempt" + markerRelease = "hive:release" + markerEscalation = "hive:claim-escalation" ) // PreemptedLabel is the label a displaced holder should watch for. @@ -69,6 +70,37 @@ func ReleaseComment(c Claim, reason string) string { return b.String() } +// EscalationComment renders the comment posted instead of another claim when +// an agent's last claims on an issue moved nothing (#10527, ADR-0019). label +// is the label the issue is parked with. +func EscalationComment(s Stall, label string) string { + agent := Claim{Holder: s.Holder, Kind: KindAgent} + var b strings.Builder + fmt.Fprintf(&b, "\n", + markerEscalation, s.Holder, KindAgent, len(s.Attempts), s.Since().UTC().Format(time.RFC3339)) + fmt.Fprintf(&b, "🚨 Not claiming this again: %s claimed it %d times and nothing moved in between — no linked pull request, referencing commit, label or assignee change, or close/reopen since %s UTC.\n\n", + mention(agent), len(s.Attempts), s.Since().UTC().Format("2006-01-02 15:04")) + b.WriteString("| Claimed | How it ended |\n| --- | --- |\n") + for _, a := range s.Attempts { + ended := "no end recorded" + if !a.EndedAt.IsZero() { + ended = fmt.Sprintf("%s at %s UTC", tableCell(a.Ended), a.EndedAt.UTC().Format("2006-01-02 15:04")) + } + fmt.Fprintf(&b, "| %s UTC | %s |\n", a.ClaimedAt.UTC().Format("2006-01-02 15:04"), ended) + } + fmt.Fprintf(&b, "\nLabelled `%s` and left out of every work list. Removing the label (or replying `/hive approve`) counts as progress, so the next claim goes ahead.", label) + return b.String() +} + +// tableCell keeps a free-text reason inside one markdown table cell. +func tableCell(s string) string { + s = strings.Join(strings.Fields(s), " ") + if s == "" { + return "ended" + } + return strings.ReplaceAll(s, "|", `\|`) +} + func mention(c Claim) string { switch c.Kind { case KindHuman, KindExternal, KindContributor: diff --git a/src/pkg/claims/listing_test.go b/src/pkg/claims/listing_test.go new file mode 100644 index 0000000000..c52aab48af --- /dev/null +++ b/src/pkg/claims/listing_test.go @@ -0,0 +1,149 @@ +package claims + +import ( + "path/filepath" + "testing" + "time" +) + +func TestMarkListedHoldsWithoutClaiming(t *testing.T) { + var claimed int + l, now := newTestLedger(t, Hooks{OnClaimed: func(Claim, Outcome) { claimed++ }}) + li, ok := l.MarkListed("o/r", 5, "scanner") + if !ok || li.Holder != "scanner" || !li.ExpiresAt.Equal(now.Add(DefaultListedTTL)) { + t.Fatalf("MarkListed = %+v, %v", li, ok) + } + if claimed != 0 { + t.Fatal("a kick listing fired OnClaimed (the 🔒 comment)") + } + if _, held := l.Lookup("o/r", 5); held { + t.Fatal("a kick listing created a claim") + } + if _, ok := l.LastAttempt("o/r", 5); ok { + t.Fatal("a kick listing counted as a claim attempt") + } + if keys := l.HeldKeys("relay#s"); !keys["o/r#5"] { + t.Fatalf("listing not excluded for a relay contributor: %v", keys) + } + if keys := l.HeldKeys("scanner"); keys["o/r#5"] { + t.Fatal("listing excluded for the agent it was listed to") + } + if _, ok := l.MarkListed("o/r", 5, "quality"); ok { + t.Fatal("a second agent's kick displaced a live listing") + } + + *now = now.Add(DefaultListedTTL) + if _, ok := l.Listed("o/r", 5); ok { + t.Fatal("listing still holds after DefaultListedTTL") + } + if keys := l.HeldKeys(""); keys["o/r#5"] { + t.Fatal("lapsed listing still excluded") + } + if li, ok := l.MarkListed("o/r", 5, "quality"); !ok || li.Holder != "quality" { + t.Fatalf("lapsed listing not replaced: %+v %v", li, ok) + } +} + +func TestMarkListedLeavesClaimedIssuesAlone(t *testing.T) { + l, _ := newTestLedger(t, Hooks{}) + if _, err := l.Claim(Request{Repo: "o/r", Issue: 5, Holder: "alice", Kind: KindHuman}); err != nil { + t.Fatal(err) + } + if _, ok := l.MarkListed("o/r", 5, "scanner"); ok { + t.Fatal("listed an issue a person claims") + } + if _, ok := l.MarkListed("", 5, "scanner"); ok { + t.Fatal("listed without a repo") + } + var nilLedger *Ledger + if _, ok := nilLedger.MarkListed("o/r", 5, "scanner"); ok { + t.Fatal("nil ledger listed") + } +} + +func TestClaimOnStartClaimsOnlyListedIssues(t *testing.T) { + var outcomes []Outcome + l, now := newTestLedger(t, Hooks{OnClaimed: func(_ Claim, o Outcome) { outcomes = append(outcomes, o) }}) + if _, started, _ := l.ClaimOnStart("o/r", 5, "scanner"); started { + t.Fatal("start signal on an issue no kick listed recorded a claim") + } + l.MarkListed("o/r", 5, "scanner") + if _, started, _ := l.ClaimOnStart("o/r", 5, "quality"); started { + t.Fatal("another agent's start signal claimed a listing") + } + + // The start signal can come after the listing's short hold lapsed, as + // long as it is within the agent claim TTL of the kick. + *now = now.Add(DefaultListedTTL + time.Minute) + res, started, err := l.ClaimOnStart("o/r", 5, "scanner") + if err != nil || !started || res.Outcome != OutcomeClaimed || res.Claim.Kind != KindAgent { + t.Fatalf("first start signal: res=%+v started=%v err=%v", res, started, err) + } + if _, ok := l.Listed("o/r", 5); ok { + t.Fatal("listing left behind once the claim was recorded") + } + a, ok := l.LastAttempt("o/r", 5) + if !ok || a.Holder != "scanner" || !a.ClaimedAt.Equal(*now) { + t.Fatalf("LastAttempt = %+v, %v", a, ok) + } + + // A second signal renews: no second 🔒 comment, no second attempt. + *now = now.Add(time.Minute) + if res, started, _ := l.ClaimOnStart("o/r", 5, "scanner"); !started || res.Outcome != OutcomeRenewed { + t.Fatalf("second start signal: %+v started=%v", res, started) + } + if len(outcomes) != 2 || outcomes[0] != OutcomeClaimed || outcomes[1] != OutcomeRenewed { + t.Fatalf("OnClaimed outcomes = %v, want [claimed renewed]", outcomes) + } + if s, _ := l.Stall("o/r", 5, 1); len(s.Attempts) > 0 { + t.Fatalf("live claim reported as a stall: %+v", s) + } +} + +func TestClaimOnStartAfterTheListingWindowRecordsNothing(t *testing.T) { + l, now := newTestLedger(t, Hooks{}) + l.MarkListed("o/r", 5, "scanner") + *now = now.Add(DefaultAgentTTL) + if _, started, _ := l.ClaimOnStart("o/r", 5, "scanner"); started { + t.Fatal("start signal past the agent claim TTL of the kick recorded a claim") + } + // Pruned once the window has passed. + l.Expire() + if _, ok := l.MarkListed("o/r", 5, "quality"); !ok { + t.Fatal("stale listing blocked a new one") + } +} + +func TestListedIssuesNeverStall(t *testing.T) { + l, now := newTestLedger(t, Hooks{}) + for range 5 { + l.MarkListed("o/r", 5, "scanner") + *now = now.Add(DefaultAgentTTL + time.Minute) + } + if s, ok := l.Stall("o/r", 5, 2); ok { + t.Fatalf("issues only listed, never started, reported as a stall: %+v", s) + } +} + +func TestListingsPersist(t *testing.T) { + path := filepath.Join(t.TempDir(), "claims.json") + now := time.Date(2026, 10, 5, 12, 0, 0, 0, time.UTC) + l, err := New(path, DefaultPolicy(), Hooks{}) + if err != nil { + t.Fatal(err) + } + l.SetNow(func() time.Time { return now }) + l.MarkListed("o/r", 5, "scanner") + + reloaded, err := New(path, DefaultPolicy(), Hooks{}) + if err != nil { + t.Fatal(err) + } + reloaded.SetNow(func() time.Time { return now.Add(time.Minute) }) + if li, ok := reloaded.Listed("o/r", 5); !ok || li.Holder != "scanner" { + t.Fatalf("listing lost across restart: %+v %v", li, ok) + } + if res, started, err := reloaded.ClaimOnStart("o/r", 5, "scanner"); err != nil || !started || res.Outcome != OutcomeClaimed { + t.Fatalf("start after restart: %+v started=%v err=%v", res, started, err) + } +} diff --git a/src/pkg/claims/stall_test.go b/src/pkg/claims/stall_test.go new file mode 100644 index 0000000000..98664cbffa --- /dev/null +++ b/src/pkg/claims/stall_test.go @@ -0,0 +1,166 @@ +package claims + +import ( + "path/filepath" + "strings" + "testing" + "time" +) + +// claimAndLapse records an agent claim at *now and moves the clock past its +// expiry, the way an agent kick's claim runs out with nothing done. +func claimAndLapse(t *testing.T, l *Ledger, now *time.Time, holder string) { + t.Helper() + if res, err := l.Claim(Request{Repo: "o/r", Issue: 5, Holder: holder, HolderID: holder, Kind: KindAgent}); err != nil || res.Outcome != OutcomeClaimed { + t.Fatalf("claim by %s: outcome=%s err=%v", holder, res.Outcome, err) + } + *now = now.Add(DefaultAgentTTL + time.Minute) +} + +func TestStallNeedsThresholdAgentClaimsOnAFreeIssue(t *testing.T) { + l, now := newTestLedger(t, Hooks{}) + first := *now + claimAndLapse(t, l, now, "scanner") + if _, ok := l.Stall("o/r", 5, 2); ok { + t.Fatal("one claim reported as a stall at threshold 2") + } + second := *now + if _, err := l.Claim(Request{Repo: "o/r", Issue: 5, Holder: "scanner", HolderID: "scanner", Kind: KindAgent}); err != nil { + t.Fatal(err) + } + if _, ok := l.Stall("o/r", 5, 2); ok { + t.Fatal("stall reported while the second claim is still live") + } + *now = now.Add(DefaultAgentTTL + time.Minute) + + s, ok := l.Stall("o/r", 5, 2) + if !ok || s.Holder != "scanner" || len(s.Attempts) != 2 || !s.Since().Equal(first) || !s.Attempts[1].ClaimedAt.Equal(second) { + t.Fatalf("Stall=%+v ok=%v, want scanner run since %v", s, ok, first) + } + for _, a := range s.Attempts { + if a.Ended != "expired" || !a.EndedAt.Equal(a.ClaimedAt.Add(DefaultAgentTTL)) { + t.Fatalf("attempt end not recorded: %+v", a) + } + } + if _, ok := l.Stall("o/r", 5, 3); ok { + t.Fatal("two claims reported as a stall at threshold 3") + } + if _, ok := l.Stall("o/r", 5, 0); ok { + t.Fatal("threshold 0 must disable the check") + } +} + +func TestStallIgnoresHumanClaimsAndCountsPerAgent(t *testing.T) { + l, now := newTestLedger(t, Hooks{}) + if _, err := l.Claim(Request{Repo: "o/r", Issue: 5, Holder: "alice", Kind: KindHuman}); err != nil { + t.Fatal(err) + } + *now = now.Add(DefaultHumanTTL + time.Minute) + claimAndLapse(t, l, now, "ci-maintainer") + claimAndLapse(t, l, now, "scanner") + if s, ok := l.Stall("o/r", 5, 2); ok { + t.Fatalf("claims by three different holders reported as a stall: %+v", s) + } + claimAndLapse(t, l, now, "ci-maintainer") + s, ok := l.Stall("o/r", 5, 2) + if !ok || s.Holder != "ci-maintainer" { + t.Fatalf("Stall=%+v ok=%v, want ci-maintainer", s, ok) + } +} + +func TestStallRecordsTakeoverAsTheAttemptEnd(t *testing.T) { + l, now := newTestLedger(t, Hooks{}) + if _, err := l.Claim(Request{Repo: "o/r", Issue: 5, Holder: "scanner", Kind: KindAgent}); err != nil { + t.Fatal(err) + } + *now = now.Add(time.Minute) + if res, _ := l.Claim(Request{Repo: "o/r", Issue: 5, Holder: "alice", Kind: KindHuman}); res.Outcome != OutcomeTakenOver { + t.Fatalf("outcome=%s want taken_over", res.Outcome) + } + if _, _, err := l.Release("o/r", 5, "alice", KindHuman, "released by alice"); err != nil { + t.Fatal(err) + } + claimAndLapse(t, l, now, "scanner") + s, ok := l.Stall("o/r", 5, 2) + if !ok || s.Attempts[0].Ended != "taken over by alice" { + t.Fatalf("Stall=%+v ok=%v, want first attempt ended by takeover", s, ok) + } +} + +func TestMarkEscalatedOncePerRunAndPersisted(t *testing.T) { + path := filepath.Join(t.TempDir(), "claims.json") + now := time.Date(2026, 10, 4, 12, 0, 0, 0, time.UTC) + l, err := New(path, DefaultPolicy(), Hooks{}) + if err != nil { + t.Fatal(err) + } + l.SetNow(func() time.Time { return now }) + claimAndLapse(t, l, &now, "scanner") + claimAndLapse(t, l, &now, "scanner") + s, ok := l.Stall("o/r", 5, 2) + if !ok { + t.Fatal("no stall after two lapsed claims") + } + if !l.MarkEscalated(s) { + t.Fatal("first MarkEscalated = false") + } + if l.MarkEscalated(s) { + t.Fatal("same run escalated twice") + } + + reloaded, err := New(path, DefaultPolicy(), Hooks{}) + if err != nil { + t.Fatal(err) + } + reloaded.SetNow(func() time.Time { return now }) + rs, ok := reloaded.Stall("o/r", 5, 2) + if !ok || !rs.Since().Equal(s.Since()) || rs.Attempts[1].Ended != "expired" { + t.Fatalf("reloaded Stall=%+v ok=%v, want %+v", rs, ok, s) + } + if reloaded.MarkEscalated(rs) { + t.Fatal("escalation forgotten across a restart: the same run would be escalated again") + } + + // A later run (it begins after the escalated one) escalates again. + claimAndLapse(t, reloaded, &now, "scanner") + next, ok := reloaded.Stall("o/r", 5, 2) + if !ok || !next.Since().After(s.Since()) { + t.Fatalf("next Stall=%+v ok=%v", next, ok) + } + if !reloaded.MarkEscalated(next) { + t.Fatal("a new run was suppressed as already escalated") + } +} + +func TestStallForgetsAttemptsPastRetention(t *testing.T) { + l, now := newTestLedger(t, Hooks{}) + claimAndLapse(t, l, now, "scanner") + *now = now.Add(AttemptRetention) + claimAndLapse(t, l, now, "scanner") + if s, ok := l.Stall("o/r", 5, 2); ok { + t.Fatalf("claims more than AttemptRetention apart reported as one run: %+v", s) + } +} + +func TestEscalationComment(t *testing.T) { + at := time.Date(2026, 10, 4, 15, 50, 0, 0, time.UTC) + s := Stall{Repo: "o/r", Issue: 5, Holder: "scanner", Attempts: []Attempt{ + {Holder: "scanner", ClaimedAt: at, EndedAt: at.Add(2 * time.Hour), Ended: "expired"}, + {Holder: "scanner", ClaimedAt: at.Add(127 * time.Minute), EndedAt: at.Add(247 * time.Minute), Ended: "a | b"}, + }} + got := EscalationComment(s, "needs-human") + for _, want := range []string{ + "", + "agent `scanner` claimed it 2 times", + "| 2026-10-04 15:50 UTC | expired at 2026-10-04 17:50 UTC |", + `| 2026-10-04 17:57 UTC | a \| b at 2026-10-04 19:57 UTC |`, + "Labelled `needs-human`", + } { + if !strings.Contains(got, want) { + t.Fatalf("EscalationComment missing %q:\n%s", want, got) + } + } + if strings.Contains(got, "