From 2a415a9ab5a3b8b9381d4b93d58b1c4e19e9a2c1 Mon Sep 17 00:00:00 2001 From: mendezr Date: Sun, 4 Oct 2026 19:32:02 +0000 Subject: [PATCH 1/6] =?UTF-8?q?=F0=9F=90=9B=20fix:=20refuse=20automated=20?= =?UTF-8?q?claims=20on=20needs-human=20issues?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gate automated ledger claims, renewals and takeovers on current GitHub labels. Apply the same gate to claim requests and relay claim markers, and release blocked automated claims on the cleanup tick while preserving human claims and concurrent takeovers. Hive-Run: hivecommons/hive#10526 Hive-Plan: needs-human-claim-gate Hive-Spec: issue-10526#acceptance Signed-off-by: mendezr Signed-off-by: Andrew Anderson Signed-off-by: Andy Anderson <407614+clubanderson@users.noreply.github.com> --- changelog.d/fixed-needs-human-issue-claims.md | 1 + src/cmd/hive/claims_admission_test.go | 50 ++++++++++ src/cmd/hive/claimswire.go | 14 +++ src/cmd/hive/main.go | 1 + src/pkg/claims/admission_test.go | 93 +++++++++++++++++++ src/pkg/claims/claims.go | 64 +++++++++++++ .../dashboard/contribute_claim_gate_test.go | 28 ++++++ src/pkg/dashboard/contribute_claims.go | 11 +++ src/pkg/dashboard/contribute_ws.go | 5 + src/pkg/github/issue_claim_gate.go | 29 ++++++ src/pkg/github/issue_claim_gate_test.go | 91 ++++++++++++++++++ src/pkg/github/issue_request_watcher.go | 9 ++ src/pkg/github/issue_request_watcher_test.go | 4 + 13 files changed, 400 insertions(+) create mode 100644 changelog.d/fixed-needs-human-issue-claims.md create mode 100644 src/cmd/hive/claims_admission_test.go create mode 100644 src/pkg/claims/admission_test.go create mode 100644 src/pkg/dashboard/contribute_claim_gate_test.go create mode 100644 src/pkg/github/issue_claim_gate.go create mode 100644 src/pkg/github/issue_claim_gate_test.go diff --git a/changelog.d/fixed-needs-human-issue-claims.md b/changelog.d/fixed-needs-human-issue-claims.md new file mode 100644 index 0000000000..a08e59b5bd --- /dev/null +++ b/changelog.d/fixed-needs-human-issue-claims.md @@ -0,0 +1 @@ +- Refuse automated claims on `needs-human` or held issues and release existing automated claims on the next cleanup tick, without disturbing human claims ([#10526](https://github.com/hivecommons/hive/issues/10526)). diff --git a/src/cmd/hive/claims_admission_test.go b/src/cmd/hive/claims_admission_test.go new file mode 100644 index 0000000000..ed8c9d5861 --- /dev/null +++ b/src/cmd/hive/claims_admission_test.go @@ -0,0 +1,50 @@ +package main + +import ( + "context" + "fmt" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/hivecommons/hive/pkg/claims" + "github.com/hivecommons/hive/pkg/config" + "github.com/hivecommons/hive/pkg/dashboard" + "github.com/hivecommons/hive/pkg/github" +) + +func TestKickClaimsRespectNeedsHumanAndReleaseOnTick(t *testing.T) { + var label atomic.Value + label.Store("needs-human") + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, `{"number":1,"labels":[{"name":%q}]}`, label.Load()) + })) + defer api.Close() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + client := github.NewClientForTest(api.URL, "o", []string{"r"}, logger) + ledger := testClaimsLedger(t) + installClaimAdmissionCheck(context.Background(), ledger, func() *github.Client { return client }) + srv := dashboard.NewServer(0, logger) + srv.RegisterAPI(&dashboard.Dependencies{Config: &config.Config{}, IssueClaims: ledger}) + t.Cleanup(srv.CloseContributeHub) + + recordAgentKickClaims(srv, "o", "scanner", []string{"r#1"}, logger) + if _, ok := ledger.Lookup("o/r", 1); ok { + t.Fatal("needs-human issue claimed by kick") + } + label.Store("bug") + recordAgentKickClaims(srv, "o", "scanner", []string{"r#1"}, logger) + if c, ok := ledger.Lookup("o/r", 1); !ok || c.Kind != claims.KindAgent { + t.Fatal("ordinary issue not claimed") + } + label.Store("needs-human") + if n := ledger.ReleaseBlocked(); n != 1 { + t.Fatalf("cleanup tick released %d claims, want 1", n) + } + if _, held := claimsInflightLookup(ledger, "o")(github.Issue{Repo: "r", Number: 1}); held { + t.Fatal("released claim still reported In Flight") + } +} diff --git a/src/cmd/hive/claimswire.go b/src/cmd/hive/claimswire.go index d5d7cba118..7b2b96460c 100644 --- a/src/cmd/hive/claimswire.go +++ b/src/cmd/hive/claimswire.go @@ -56,6 +56,20 @@ func buildClaimsLedger(cfg *config.Config, logger *slog.Logger) *claims.Ledger { return ledger } +// installClaimAdmissionCheck gates every automated ledger producer on current +// labels, including renewals and forced takeovers. Resolve the client per call +// because App setup can finish after boot. +func installClaimAdmissionCheck(ctx context.Context, ledger *claims.Ledger, client func() *github.Client) { + if ledger == nil || client == nil { + return + } + ledger.SetAdmissionCheck(func(req claims.Request) (string, error) { + ctx, cancel := context.WithTimeout(ctx, claimsGitHubTimeout) + defer cancel() + return client().IssueClaimBlockReason(ctx, req.Repo, req.Issue) + }) +} + // githubClaimHooks returns the hooks that mirror ledger transitions onto the // GitHub issue. Every call is best-effort: a failed comment or label never // affects the ledger, which is authoritative for the hub's own decisions. diff --git a/src/cmd/hive/main.go b/src/cmd/hive/main.go index 050af363e9..f3364a7bb7 100644 --- a/src/cmd/hive/main.go +++ b/src/cmd/hive/main.go @@ -3391,6 +3391,7 @@ func (b *boot) bootDashboardAPIWith(deps bootDashboardAPIDeps) { wireSpektacularRunner(b.cfg, b.dashSrv, b.logger) // #8380: chain GitHub comments/labels behind the relay yank on takeover. b.dashSrv.InstallClaimHooks(githubClaimHooks(b.ctx, b.cfg, func() *github.Client { return b.ghClient }, b.logger)) + installClaimAdmissionCheck(b.ctx, b.issueClaims, func() *github.Client { return b.ghClient }) // Forge App tab inventory: the resolved active key path and the per-app-id // PVC keys live here in cmd/hive, so they are injected as a provider (the // SetGitHubAppRecheckFn pattern). Fingerprints and paths only — the diff --git a/src/pkg/claims/admission_test.go b/src/pkg/claims/admission_test.go new file mode 100644 index 0000000000..5c27dd2b29 --- /dev/null +++ b/src/pkg/claims/admission_test.go @@ -0,0 +1,93 @@ +package claims + +import ( + "errors" + "testing" +) + +func TestAdmissionRefusesAutomatedClaims(t *testing.T) { + for _, reason := range []string{"hold", "needs-human"} { + for _, kind := range []Kind{KindAgent, KindContributor, KindExternal} { + t.Run(reason+"/"+string(kind), func(t *testing.T) { + writes := 0 + l, _ := newTestLedger(t, Hooks{OnClaimed: func(Claim, Outcome) { writes++ }}) + l.SetAdmissionCheck(func(Request) (string, error) { return reason, nil }) + res, err := l.Claim(Request{Repo: "o/r", Issue: 1, Holder: "worker", Kind: kind, Force: true}) + if err != nil || res.Outcome != OutcomeRefused || res.Reason != reason { + t.Fatalf("claim = %+v, %v", res, err) + } + if len(l.List()) != 0 || writes != 0 { + t.Fatal("refused claim mutated ledger or fired write hook") + } + }) + } + } +} + +func TestAdmissionRefusesRenewalAndTakeover(t *testing.T) { + l, _ := newTestLedger(t, Hooks{}) + req := Request{Repo: "o/r", Issue: 1, Holder: "worker", Kind: KindContributor} + first, _ := l.Claim(req) + l.SetAdmissionCheck(func(Request) (string, error) { return "needs-human", nil }) + for _, next := range []Request{req, {Repo: "o/r", Issue: 1, Holder: "scanner", Kind: KindAgent, Force: true}} { + res, err := l.Claim(next) + if err != nil || res.Outcome != OutcomeRefused { + t.Fatalf("claim = %+v, %v", res, err) + } + if got, _ := l.Lookup("o/r", 1); got != first.Claim { + t.Fatal("refusal changed existing claim") + } + } +} + +func TestReleaseBlockedClaims(t *testing.T) { + released := 0 + l, _ := newTestLedger(t, Hooks{OnReleased: func(c Claim, reason string) { + if c.Kind == KindHuman || reason != "needs-human" { + t.Errorf("unexpected release: %+v, %q", c, reason) + } + released++ + }}) + for i, kind := range []Kind{KindAgent, KindContributor, KindExternal, KindHuman} { + _, _ = l.Claim(Request{Repo: "o/r", Issue: i + 1, Holder: "worker", Kind: kind}) + } + l.SetAdmissionCheck(func(req Request) (string, error) { + if req.Issue == 3 { + return "", errors.New("forge unavailable") + } + return "needs-human", nil + }) + if n := l.ReleaseBlocked(); n != 2 || released != 2 { + t.Fatalf("released %d, hooks %d; want 2", n, released) + } + if _, ok := l.Lookup("o/r", 3); !ok { + t.Fatal("lookup error released a claim") + } + if _, ok := l.Lookup("o/r", 4); !ok { + t.Fatal("needs-human released a human claim") + } + res, err := l.Claim(Request{Repo: "o/r", Issue: 5, Holder: "person", Kind: KindHuman}) + if err != nil || res.Outcome != OutcomeClaimed { + t.Fatalf("human claim refused: %+v, %v", res, err) + } + res, err = l.Claim(Request{Repo: "o/r", Issue: 3, Holder: "other", Kind: KindAgent}) + if err == nil || res.Outcome.Changed() { + t.Fatalf("lookup failure admitted claim: %+v, %v", res, err) + } +} + +func TestReleaseBlockedPreservesConcurrentTakeover(t *testing.T) { + l, _ := newTestLedger(t, Hooks{}) + _, _ = l.Claim(Request{Repo: "o/r", Issue: 1, Holder: "scanner", Kind: KindAgent}) + l.SetAdmissionCheck(func(req Request) (string, error) { + // Human takeover while the forge check is in progress. + _, _ = l.Claim(Request{Repo: req.Repo, Issue: req.Issue, Holder: "person", Kind: KindHuman}) + return "needs-human", nil + }) + if n := l.ReleaseBlocked(); n != 0 { + t.Fatalf("released %d concurrent claims", n) + } + if c, ok := l.Lookup("o/r", 1); !ok || c.Kind != KindHuman { + t.Fatalf("human takeover lost: %+v, %v", c, ok) + } +} diff --git a/src/pkg/claims/claims.go b/src/pkg/claims/claims.go index 6926d51ba6..e9b09b1585 100644 --- a/src/pkg/claims/claims.go +++ b/src/pkg/claims/claims.go @@ -237,6 +237,8 @@ type Result struct { // Previous is the displaced (taken_over) or blocking (held/refused) // holder's claim, when there was one. Previous *Claim + // Reason explains an admission refusal, which has no blocking holder. + Reason string } // Hooks are the hub-side side effects, all optional. They run after the @@ -346,6 +348,7 @@ type Ledger struct { hooks Hooks now func() time.Time hive string + admit func(Request) (string, error) } // ErrInvalid is returned for requests missing a repo, issue, holder or kind. @@ -388,6 +391,15 @@ func (l *Ledger) SetHooks(h Hooks) { l.mu.Unlock() } +// SetAdmissionCheck installs a non-human claim gate. An empty reason admits +// the request; an error leaves the ledger unchanged. The check runs outside +// the ledger lock so a forge lookup never blocks other ledger operations. +func (l *Ledger) SetAdmissionCheck(check func(Request) (string, error)) { + l.mu.Lock() + l.admit = check + l.mu.Unlock() +} + // SetNow overrides the clock. Intended for tests. func (l *Ledger) SetNow(fn func() time.Time) { l.mu.Lock() @@ -404,6 +416,18 @@ func (l *Ledger) Claim(req Request) (Result, error) { return Result{}, ErrInvalid } l.mu.Lock() + check := l.admit + l.mu.Unlock() + if check != nil && req.Kind != KindHuman { + reason, err := check(req) + if err != nil { + return Result{}, err + } + if reason != "" { + return Result{Outcome: OutcomeRefused, Reason: reason}, nil + } + } + l.mu.Lock() now := l.now() expired := l.expireLocked(now) key := Key(req.Repo, req.Issue) @@ -551,6 +575,46 @@ func (l *Ledger) ForceRelease(repo string, issue int, reason string) (Claim, boo return c, ok } +// ReleaseBlocked rechecks non-human claims against the admission gate. Failed +// lookups preserve claims. A concurrent renewal or takeover also preserves the +// new claim: only the exact snapshot checked outside the lock may be removed. +func (l *Ledger) ReleaseBlocked() int { + if l == nil { + return 0 + } + l.mu.Lock() + check := l.admit + l.mu.Unlock() + if check == nil { + return 0 + } + n := 0 + for _, c := range l.List() { + if c.Kind == KindHuman { + continue + } + reason, err := check(Request{Repo: c.Repo, Issue: c.Issue, Holder: c.Holder, HolderID: c.HolderID, Kind: c.Kind}) + if err != nil || reason == "" { + continue + } + l.mu.Lock() + current, ok := l.claims[c.Key()] + if !ok || current != c { + l.mu.Unlock() + continue + } + delete(l.claims, c.Key()) + _ = l.saveLocked() + hooks := l.hooks + l.mu.Unlock() + n++ + if hooks.OnReleased != nil { + hooks.OnReleased(c, reason) + } + } + return n +} + // ReleaseByHolderID drops every claim whose HolderID matches — used when a // relay lease is revoked or expires so the claim goes with it. Returns the // released claims. A non-empty onlyKey restricts the release to that one diff --git a/src/pkg/dashboard/contribute_claim_gate_test.go b/src/pkg/dashboard/contribute_claim_gate_test.go new file mode 100644 index 0000000000..e2e8d852b7 --- /dev/null +++ b/src/pkg/dashboard/contribute_claim_gate_test.go @@ -0,0 +1,28 @@ +package dashboard + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/hivecommons/hive/pkg/github" +) + +func TestRelayClaimMarkerRefusesNeedsHuman(t *testing.T) { + hub, s := covK2Hub(t) + enableIssueClaims(s) + recorder := &claimRecorder{} + hub.claimCommenter = recorder + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, `{"number":42,"labels":[{"name":"needs-human"}]}`) + })) + defer api.Close() + s.deps.GHClient = github.NewClientForTest(api.URL, "o", []string{"r"}, hub.logger) + claim, posted := hub.recordAgentClaim(context.Background(), issueClaimConn("contributor"), "task-1", "o/r", 42, time.Now()) + if posted || claim.Identity != "" || len(recorder.all()) != 0 { + t.Fatalf("needs-human received legacy claim marker: %+v, posted=%v", claim, posted) + } +} diff --git a/src/pkg/dashboard/contribute_claims.go b/src/pkg/dashboard/contribute_claims.go index befa2796bd..55aa8d78fd 100644 --- a/src/pkg/dashboard/contribute_claims.go +++ b/src/pkg/dashboard/contribute_claims.go @@ -125,6 +125,17 @@ func (h *ContributeWSHub) recordAgentClaim(ctx context.Context, c *ContributorCo if !h.claimsEnabled() || c == nil || repoFull == "" || number <= 0 { return ghpkg.IssueClaimMark{}, false } + // The queue may predate a label change. Recheck before posting the + // legacy claim marker too, not only when writing the worker ledger. + if h.server.deps.GHClient != nil { + checkCtx, cancel := context.WithTimeout(ctx, claimCommentTimeout) + reason, err := h.server.deps.GHClient.IssueClaimBlockReason(checkCtx, repoFull, number) + cancel() + if err != nil || reason != "" { + h.logger.Warn("[contribute-ws] issue claim refused", "repo", repoFull, "number", number, "reason", reason, "error", err) + return ghpkg.IssueClaimMark{}, false + } + } claim := ghpkg.IssueClaimMark{ Identity: claimIdentityFor(c), StartedAt: now, diff --git a/src/pkg/dashboard/contribute_ws.go b/src/pkg/dashboard/contribute_ws.go index f8b02b3d8d..003918e62b 100644 --- a/src/pkg/dashboard/contribute_ws.go +++ b/src/pkg/dashboard/contribute_ws.go @@ -4217,6 +4217,11 @@ func (h *ContributeWSHub) cleanupLoop() { if n := h.claimsLedger().Expire(); n > 0 { h.logger.Info("[claims] expired claims released", "count", n) } + // A needs-human/hold label added after assignment releases the + // automated claim rather than leaving a false In Flight state. + if n := h.claimsLedger().ReleaseBlocked(); n > 0 { + h.logger.Info("[claims] blocked claims released", "count", n) + } // Deregister under the lock; CLOSE outside it. // diff --git a/src/pkg/github/issue_claim_gate.go b/src/pkg/github/issue_claim_gate.go new file mode 100644 index 0000000000..b9d589018a --- /dev/null +++ b/src/pkg/github/issue_claim_gate.go @@ -0,0 +1,29 @@ +package github + +import ( + "context" + "fmt" +) + +// IssueClaimBlockReason checks current forge labels before an automated claim. +// Like a hold, needs-human prevents new claims and renewals, even when forced. +// Lookup errors are returned so callers cannot mistake unknown labels for +// permission to claim. Human claims do not use this gate. +func (c *Client) IssueClaimBlockReason(ctx context.Context, repo string, number int) (string, error) { + if c == nil || c.client == nil { + return "", ErrNoGitHubClient + } + owner, name := c.splitRepo(repo) + issue, _, err := c.client.Issues.Get(ctx, owner, name, number) + if err != nil { + return "", fmt.Errorf("checking issue claim labels: %w", err) + } + labels := extractLabels(issue.Labels) + if hasIssueNeedsHumanLabel(labels) { + return "needs-human", nil + } + if c.isHeld(labels) { + return "hold", nil + } + return "", nil +} diff --git a/src/pkg/github/issue_claim_gate_test.go b/src/pkg/github/issue_claim_gate_test.go new file mode 100644 index 0000000000..7b59d900b8 --- /dev/null +++ b/src/pkg/github/issue_claim_gate_test.go @@ -0,0 +1,91 @@ +package github + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestIssueClaimBlockReason(t *testing.T) { + for _, tc := range []struct { + label, want string + }{ + {"hold", "hold"}, + {"on-hold", "hold"}, + {"needs-human", "needs-human"}, + {" NEEDS-HUMAN ", "needs-human"}, + {"needs-human-followup", ""}, + {"bug", ""}, + } { + t.Run(tc.label, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != "/repos/o/r/issues/42" { + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + } + fmt.Fprintf(w, `{"number":42,"labels":[{"name":%q}]}`, tc.label) + })) + defer srv.Close() + got, err := issueTestClient(t, srv.URL).IssueClaimBlockReason(context.Background(), "o/r", 42) + if err != nil || got != tc.want { + t.Fatalf("reason = %q, %v; want %q", got, err, tc.want) + } + }) + } +} + +func TestIssueRequestWatcher_ClaimRefusesHeldIssue(t *testing.T) { + for _, label := range []string{"hold", "needs-human"} { + t.Run(label, func(t *testing.T) { + writes := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + fmt.Fprintf(w, `{"number":42,"labels":[{"name":%q}]}`, label) + return + } + writes++ + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + c := issueTestClient(t, srv.URL) + dir := withIssueDir(t) + path, err := WriteIssueRequest(dir, IssueRequest{Kind: "claim", Repo: "o/r", Number: 42, Agent: "scanner"}) + if err != nil { + t.Fatal(err) + } + c.ProcessIssueRequestsOnce(context.Background()) + res := readIssueResultFile(t, path) + if res.OK || !strings.Contains(res.Error, label) || writes != 0 { + t.Fatalf("blocked claim: result %+v, writes %d", res, writes) + } + }) + } +} + +// The shared enumerator supplies every kick and contributor work list. Human +// escalation stays excluded even when mixed with ordinary work labels. +func TestNeedsHumanExcludedFromActionableWork(t *testing.T) { + for _, label := range []string{"needs-human", " NEEDS-HUMAN "} { + t.Run(label, func(t *testing.T) { + issues := []wireIssue{{Number: 42, Title: "human-only repair", User: wireUser{"alice"}, CreatedAt: hoursAgo(1), Labels: []wireLabel{{Name: "bug"}, {Name: label}}}} + srv := httptest.NewServer(buildMux(t, "o", "r", issues, nil)) + defer srv.Close() + result, err := newTestClient(t, srv, "o", []string{"r"}).EnumerateActionable(context.Background()) + if err != nil || result.Issues.Count != 0 || len(result.Issues.Items) != 0 { + t.Fatalf("human work leaked into actionable list: %+v, %v", result.Issues, err) + } + }) + } +} + +func TestIssueClaimBlockReasonLookupFailure(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + if _, err := issueTestClient(t, srv.URL).IssueClaimBlockReason(context.Background(), "o/r", 42); err == nil { + t.Fatal("failed lookup treated as permission to claim") + } +} diff --git a/src/pkg/github/issue_request_watcher.go b/src/pkg/github/issue_request_watcher.go index 96a2a3af66..17aa7c98b9 100644 --- a/src/pkg/github/issue_request_watcher.go +++ b/src/pkg/github/issue_request_watcher.go @@ -441,6 +441,15 @@ func (c *Client) handleOneIssueRequest(ctx context.Context, path string, nowFn f closedPR := false switch kind { case "claim": + var reason string + reason, err = c.IssueClaimBlockReason(ctx, req.Repo, req.Number) + if err != nil { + break // Retry a failed lookup; never claim on unknown labels. + } + if reason != "" { + c.denyIssueRequest(path, req, "issue claim refused: "+reason, nowFn) + return + } // Apply a namespaced ownership label (App bots can't be assignees). label := claimLabelPrefix + sanitizeAgentName(req.Agent) err = c.AddLabels(ctx, req.Repo, req.Number, []string{label}) diff --git a/src/pkg/github/issue_request_watcher_test.go b/src/pkg/github/issue_request_watcher_test.go index f7ee96a84d..b9d8730a38 100644 --- a/src/pkg/github/issue_request_watcher_test.go +++ b/src/pkg/github/issue_request_watcher_test.go @@ -815,6 +815,10 @@ func TestIssueRequestWatcher_RateLimitBackoffUsesReset(t *testing.T) { func TestIssueRequestWatcher_ClaimAppliesLabelAndAudits(t *testing.T) { labeled := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/repos/o/r/issues/42" { + _, _ = io.WriteString(w, `{"number":42,"labels":[]}`) + return + } if r.Method == "POST" && strings.HasSuffix(r.URL.Path, "/labels") { labeled++ w.Header().Set("Content-Type", "application/json") From e96fec1208cffe5ab6a3e4c533376e9abdf1bc1d Mon Sep 17 00:00:00 2001 From: mendezr Date: Mon, 5 Oct 2026 17:00:46 +0000 Subject: [PATCH 2/6] fix: isolate blocked-claim polling and update watcher fixture Signed-off-by: mendezr Signed-off-by: Andrew Anderson Signed-off-by: Andy Anderson <407614+clubanderson@users.noreply.github.com> --- src/pkg/dashboard/contribute_ws.go | 24 +++++++++++++++++++----- src/pkg/github/repo_pause_test.go | 2 ++ 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/pkg/dashboard/contribute_ws.go b/src/pkg/dashboard/contribute_ws.go index 003918e62b..5a715a67aa 100644 --- a/src/pkg/dashboard/contribute_ws.go +++ b/src/pkg/dashboard/contribute_ws.go @@ -4181,7 +4181,26 @@ func (h *ContributeWSHub) checkEffortAllowed(backend, effort string) (bool, stri return true, "" } +// blockedClaimsLoop polls labels separately from connection cleanup. Five minutes +// bounds steady-state traffic to 12 reads per claim/hour; slow GitHub calls +// cannot delay heartbeat reaping, and passes never overlap. +func (h *ContributeWSHub) blockedClaimsLoop(interval time.Duration) { + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-h.stopCh: + return + case <-ticker.C: + if n := h.claimsLedger().ReleaseBlocked(); n > 0 { + h.logger.Info("[claims] blocked claims released", "count", n) + } + } + } +} + func (h *ContributeWSHub) cleanupLoop() { + go h.blockedClaimsLoop(5 * time.Minute) if h.doneCh != nil { defer close(h.doneCh) } @@ -4217,11 +4236,6 @@ func (h *ContributeWSHub) cleanupLoop() { if n := h.claimsLedger().Expire(); n > 0 { h.logger.Info("[claims] expired claims released", "count", n) } - // A needs-human/hold label added after assignment releases the - // automated claim rather than leaving a false In Flight state. - if n := h.claimsLedger().ReleaseBlocked(); n > 0 { - h.logger.Info("[claims] blocked claims released", "count", n) - } // Deregister under the lock; CLOSE outside it. // diff --git a/src/pkg/github/repo_pause_test.go b/src/pkg/github/repo_pause_test.go index a9578e9692..d7f1cc535d 100644 --- a/src/pkg/github/repo_pause_test.go +++ b/src/pkg/github/repo_pause_test.go @@ -220,6 +220,8 @@ func TestIssueRequestWatcher_RepoPause(t *testing.T) { switch { case r.Method == http.MethodGet && r.URL.Path == "/repos/o/r/issues": _, _ = io.WriteString(w, `[]`) + case r.Method == http.MethodGet && r.URL.Path == "/repos/o/r/issues/42": + _, _ = io.WriteString(w, `{"number":42,"labels":[]}`) case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/repos/o/r/labels/"): _, _ = io.WriteString(w, `{"name":"agent/security"}`) case r.Method == http.MethodPost && r.URL.Path == tc.endpoint: From 936850345df98b9ba8096902fe177f9d75e1f5f9 Mon Sep 17 00:00:00 2001 From: mendezr Date: Mon, 5 Oct 2026 17:00:46 +0000 Subject: [PATCH 3/6] test: cover blocked-claim poller release and shutdown Signed-off-by: mendezr Signed-off-by: Andrew Anderson Signed-off-by: Andy Anderson <407614+clubanderson@users.noreply.github.com> --- src/pkg/dashboard/blocked_claims_loop_test.go | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 src/pkg/dashboard/blocked_claims_loop_test.go diff --git a/src/pkg/dashboard/blocked_claims_loop_test.go b/src/pkg/dashboard/blocked_claims_loop_test.go new file mode 100644 index 0000000000..149577ff6c --- /dev/null +++ b/src/pkg/dashboard/blocked_claims_loop_test.go @@ -0,0 +1,37 @@ +package dashboard + +import ( + "testing" + "time" + + "github.com/hivecommons/hive/pkg/claims" +) + +func TestBlockedClaimsLoopReleasesAndStops(t *testing.T) { + hub, s, ledger := claimsHub(t) + if _, err := ledger.Claim(claims.Request{Repo: "o/r", Issue: 1, Holder: "worker", Kind: claims.KindAgent}); err != nil { + t.Fatal(err) + } + ledger.SetAdmissionCheck(func(claims.Request) (string, error) { return "needs-human", nil }) + worker := &ContributeWSHub{server: s, logger: hub.logger, stopCh: make(chan struct{})} + done := make(chan struct{}) + go func() { defer close(done); worker.blockedClaimsLoop(time.Millisecond) }() + defer func() { + close(worker.stopCh) + select { + case <-done: + case <-time.After(time.Second): + t.Error("poller did not stop") + } + }() + deadline := time.Now().Add(time.Second) + for { + if _, ok := ledger.Lookup("o/r", 1); !ok { + break + } + if time.Now().After(deadline) { + t.Fatal("blocked claim was not released") + } + time.Sleep(time.Millisecond) + } +} From 3f186f6e4196272336e3172c55f3c0a97fe42f4c Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Mon, 5 Oct 2026 21:33:44 -0400 Subject: [PATCH 4/6] =?UTF-8?q?=F0=9F=90=9B=20fix:=20adapt=20needs-human?= =?UTF-8?q?=20admission=20to=20v5=20listings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep kick delivery as a non-commenting listing on v5 and let the start-signal path perform the gated automated claim. Treat a missing GitHub client as a failed lookup so automated claims never admit on unknown labels. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Andrew Anderson Signed-off-by: Andy Anderson <407614+clubanderson@users.noreply.github.com> --- src/cmd/hive/claims_admission_test.go | 10 +++++++--- src/cmd/hive/claimswire.go | 6 +++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/cmd/hive/claims_admission_test.go b/src/cmd/hive/claims_admission_test.go index ed8c9d5861..09b6bb9162 100644 --- a/src/cmd/hive/claims_admission_test.go +++ b/src/cmd/hive/claims_admission_test.go @@ -31,12 +31,16 @@ func TestKickClaimsRespectNeedsHumanAndReleaseOnTick(t *testing.T) { srv.RegisterAPI(&dashboard.Dependencies{Config: &config.Config{}, IssueClaims: ledger}) t.Cleanup(srv.CloseContributeHub) - recordAgentKickClaims(srv, "o", "scanner", []string{"r#1"}, logger) + recordAgentKickListings(srv, "o", "scanner", []string{"r#1"}, logger) + if _, listed := ledger.Listed("o/r", 1); !listed { + t.Fatal("kick did not record listing") + } + recordAgentStart(ledger, "o", "scanner", "r", 1, "comment", logger) if _, ok := ledger.Lookup("o/r", 1); ok { - t.Fatal("needs-human issue claimed by kick") + t.Fatal("needs-human issue claimed on start") } label.Store("bug") - recordAgentKickClaims(srv, "o", "scanner", []string{"r#1"}, logger) + recordAgentStart(ledger, "o", "scanner", "r", 1, "comment", logger) if c, ok := ledger.Lookup("o/r", 1); !ok || c.Kind != claims.KindAgent { t.Fatal("ordinary issue not claimed") } diff --git a/src/cmd/hive/claimswire.go b/src/cmd/hive/claimswire.go index 7b2b96460c..ff0b61e211 100644 --- a/src/cmd/hive/claimswire.go +++ b/src/cmd/hive/claimswire.go @@ -66,7 +66,11 @@ func installClaimAdmissionCheck(ctx context.Context, ledger *claims.Ledger, clie ledger.SetAdmissionCheck(func(req claims.Request) (string, error) { ctx, cancel := context.WithTimeout(ctx, claimsGitHubTimeout) defer cancel() - return client().IssueClaimBlockReason(ctx, req.Repo, req.Issue) + gh := client() + if gh == nil { + return "", github.ErrNoGitHubClient + } + return gh.IssueClaimBlockReason(ctx, req.Repo, req.Issue) }) } From 982112b1c437ab29310573a11c82097f9ebb866d Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Mon, 5 Oct 2026 21:37:21 -0400 Subject: [PATCH 5/6] test: cover failed needs-human claim lookups Exercise the no-client admission path and the issue-request retry path so unknown forge labels never become write permission. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Andrew Anderson Signed-off-by: Andy Anderson <407614+clubanderson@users.noreply.github.com> --- src/pkg/github/issue_claim_gate_test.go | 8 +++++ src/pkg/github/issue_request_watcher_test.go | 33 ++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/src/pkg/github/issue_claim_gate_test.go b/src/pkg/github/issue_claim_gate_test.go index 7b59d900b8..1854a088d2 100644 --- a/src/pkg/github/issue_claim_gate_test.go +++ b/src/pkg/github/issue_claim_gate_test.go @@ -89,3 +89,11 @@ func TestIssueClaimBlockReasonLookupFailure(t *testing.T) { t.Fatal("failed lookup treated as permission to claim") } } + +func TestIssueClaimBlockReasonNoClient(t *testing.T) { + for _, c := range []*Client{nil, &Client{}} { + if _, err := c.IssueClaimBlockReason(context.Background(), "o/r", 42); err == nil { + t.Fatal("missing client treated as permission to claim") + } + } +} diff --git a/src/pkg/github/issue_request_watcher_test.go b/src/pkg/github/issue_request_watcher_test.go index b9d8730a38..b5bd6f8eb4 100644 --- a/src/pkg/github/issue_request_watcher_test.go +++ b/src/pkg/github/issue_request_watcher_test.go @@ -856,6 +856,39 @@ func TestIssueRequestWatcher_ClaimAppliesLabelAndAudits(t *testing.T) { } } +func TestIssueRequestWatcher_ClaimLookupFailureRetriesWithoutWriting(t *testing.T) { + writes := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/repos/o/r/issues/42" { + w.WriteHeader(http.StatusInternalServerError) + return + } + writes++ + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + c := issueTestClient(t, srv.URL) + dir := withIssueDir(t) + reqPath, err := WriteIssueRequest(dir, IssueRequest{ + Kind: "claim", Repo: "o/r", Number: 42, Agent: "scanner", + }) + if err != nil { + t.Fatal(err) + } + + c.ProcessIssueRequestsOnce(context.Background()) + + if writes != 0 { + t.Fatalf("claim lookup failure performed %d write requests", writes) + } + if _, err := os.Stat(reqPath); err != nil { + t.Fatalf("claim request should remain queued for retry: %v", err) + } + if _, err := os.Stat(reqPath + ".result"); !os.IsNotExist(err) { + t.Fatalf("lookup failure wrote result file: %v", err) + } +} + // A claim request missing number/agent is quarantined, not retried forever. func TestIssueRequestWatcher_ClaimMalformed(t *testing.T) { c := issueTestClient(t, "http://127.0.0.1:0") From 39d5024f44f7f8cf6a9d566b4f869ee1f3a60868 Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Mon, 5 Oct 2026 21:40:57 -0400 Subject: [PATCH 6/6] test: use Eventually for blocked claim poller Avoid increasing the fixed-sleep test ratchet while preserving the blocked-claim cleanup assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Andrew Anderson Signed-off-by: Andy Anderson <407614+clubanderson@users.noreply.github.com> --- src/pkg/dashboard/blocked_claims_loop_test.go | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/pkg/dashboard/blocked_claims_loop_test.go b/src/pkg/dashboard/blocked_claims_loop_test.go index 149577ff6c..522c066f59 100644 --- a/src/pkg/dashboard/blocked_claims_loop_test.go +++ b/src/pkg/dashboard/blocked_claims_loop_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + "github.com/hivecommons/hive/internal/testutil" "github.com/hivecommons/hive/pkg/claims" ) @@ -24,14 +25,8 @@ func TestBlockedClaimsLoopReleasesAndStops(t *testing.T) { t.Error("poller did not stop") } }() - deadline := time.Now().Add(time.Second) - for { - if _, ok := ledger.Lookup("o/r", 1); !ok { - break - } - if time.Now().After(deadline) { - t.Fatal("blocked claim was not released") - } - time.Sleep(time.Millisecond) - } + testutil.Eventually(t, time.Second, func() bool { + _, ok := ledger.Lookup("o/r", 1) + return !ok + }, "blocked claim was not released") }