diff --git a/changelog.d/fixed-10706.md b/changelog.d/fixed-10706.md new file mode 100644 index 0000000000..21a01a499f --- /dev/null +++ b/changelog.d/fixed-10706.md @@ -0,0 +1 @@ +- suppress claims on already-done issues (#10706) diff --git a/src/cmd/hive/claimswire.go b/src/cmd/hive/claimswire.go index 4cc9a817c5..542a2d32e3 100644 --- a/src/cmd/hive/claimswire.go +++ b/src/cmd/hive/claimswire.go @@ -202,6 +202,15 @@ func recordAgentKickClaims(dashSrv *dashboard.Server, org, agentName string, iss if org != "" && !strings.Contains(repo, "/") { repo = org + "/" + repo } + ctx, cancel := context.WithTimeout(context.Background(), claimsGitHubTimeout) + refused, _ := dashSrv.RefuseAlreadyDoneClaim(ctx, repo, ref.Number) + cancel() + if refused { + if logger != nil { + logger.Info("issue-claims: agent claim refused for already-done issue", "agent", agentName, "issue", claims.Key(repo, ref.Number)) + } + continue + } res, err := ledger.Claim(claims.Request{ Repo: repo, Issue: ref.Number, Holder: agentName, HolderID: agentName, Kind: claims.KindAgent, diff --git a/src/cmd/hive/main.go b/src/cmd/hive/main.go index 5f269b3f4c..f3dda3e741 100644 --- a/src/cmd/hive/main.go +++ b/src/cmd/hive/main.go @@ -8486,6 +8486,12 @@ func applyDuplicatePRGuard( actionable *github.ActionableResult, logger *slog.Logger, ) { + // hive/already-done is display state, not a hold, but it is also positive + // evidence that a merged fix already covered the item. Keep it out of new + // kick prompts until a human removes the label. + if withheld := github.FilterAlreadyDoneIssues(actionable, logger); withheld > 0 { + logger.Info("already-done guard applied", "withheld", withheld) + } // #8380: a LIVE issue claim covers an issue the same way an open PR does, // for every kick prompt (the scanner's included). The claim fields are // only ever set while governor.claims.enabled is on, so with the feature diff --git a/src/docs/api-reference.md b/src/docs/api-reference.md index cacea72cce..5d0d7c6205 100644 --- a/src/docs/api-reference.md +++ b/src/docs/api-reference.md @@ -390,10 +390,10 @@ Read the result from `GET /api/kick/{agent}/status`, which returns `status` of ` | Method | Path | Auth | Purpose | Source | |---|---|---|---|---| -| `GET` | `/api/claims` | Dashboard auth | Every live worker claim (`{enabled, claims:[…]}`); `enabled:false` when `claims.enabled: false` | `pkg/dashboard/claims_api.go:53` | -| `GET` | `/api/claims/{owner}/{repo}/{number}` | Dashboard auth | `{held, claim}` for one issue | `pkg/dashboard/claims_api.go:54` | -| `POST` | `/api/claims/{owner}/{repo}/{number}` | Signed-in / hub-proxied / GitHub-token caller claims as a **human** under their login; a verified owner token claims as the owner | Body `{force, ttl_s, session}`. 200 with `outcome` `claimed`/`renewed`/`taken_over`; 409 with `outcome` `held` (same rank, retry with `force`) or `refused` (higher rank holds it) and a `hint` | `pkg/dashboard/claims_api.go:55` | -| `DELETE` | `/api/claims/{owner}/{repo}/{number}` | Same identity rules | Body `{force, reason}`. Releases your claim or a lower-ranked holder's; `force` with a verified owner role releases regardless of holder | `pkg/dashboard/claims_api.go:56` | +| `GET` | `/api/claims` | Dashboard auth | Every live worker claim (`{enabled, claims:[…]}`); `enabled:false` when `claims.enabled: false` | `pkg/dashboard/claims_api.go:56` | +| `GET` | `/api/claims/{owner}/{repo}/{number}` | Dashboard auth | `{held, claim}` for one issue | `pkg/dashboard/claims_api.go:57` | +| `POST` | `/api/claims/{owner}/{repo}/{number}` | Signed-in / hub-proxied / GitHub-token caller claims as a **human** under their login; a verified owner token claims as the owner | Body `{force, ttl_s, session}`. 200 with `outcome` `claimed`/`renewed`/`taken_over`; 409 with `outcome` `held` (same rank, retry with `force`) or `refused` (higher rank holds it) and a `hint` | `pkg/dashboard/claims_api.go:58` | +| `DELETE` | `/api/claims/{owner}/{repo}/{number}` | Same identity rules | Body `{force, reason}`. Releases your claim or a lower-ranked holder's; `force` with a verified owner role releases regardless of holder | `pkg/dashboard/claims_api.go:59` | ### `/api/v1` contributor subpaths diff --git a/src/docs/labels-and-control-signals.md b/src/docs/labels-and-control-signals.md index c77b7d69bd..287f9ea8eb 100644 --- a/src/docs/labels-and-control-signals.md +++ b/src/docs/labels-and-control-signals.md @@ -66,7 +66,7 @@ The "respect hold labels" text in policy templates names the enforced set (`hold | `needs-rebase` | PRs | Scanner/kick annotation | Fills mergeability annotation in kick data. | Informational | Scanner/Hive | PR status processing | Remove after rebase | Fixed label (`src/pkg/scheduler/pr_annotations.go:117-144`) | | `hive/covered-by-pr` | Issues | PR-claim label sync and dashboard | Open PR is verified as related; issue remains actionable. Labels are synced only for actionable issues, so stale labels can remain on held/exempt/filtered issues. | Display-only | Hive | Claim sync after enumeration | Hive removes when actionable issue no longer has open PR evidence | Fixed label (`src/pkg/github/prclaims.go:1380-1435`) | | `hive/likely-done` | Issues | PR-claim label sync and dashboard | Merged PR is verified as related while issue remains open; issue remains actionable until GitHub/operator closes or confirms. | Display-only | Hive | Claim sync after enumeration | Hive removes when actionable evidence no longer says likely done | Fixed label (`src/pkg/github/prclaims.go:1380-1435`) | -| `hive/already-done` | Issues | Contributor queue and dashboard | Contributor already-done verdict/confirmation; default contributor skip label and done band. Spoke agents can still receive the issue unless another gate also filters it. | Contributor hard skip; dashboard display | Hive/contributor flow | Contributor admission | Human removes or config changes; optional close behavior | `hub.contribute_already_done_label`, `contribute_close_already_done` (`src/pkg/config/config.go:4442-4572`, `src/pkg/config/config.go:4585-4685`) | +| `hive/already-done` | Issues | Contributor queue, agent kicks, claims, and dashboard | Contributor already-done verdict/confirmation; default contributor skip label and done band. It is display state, not a hold, but Hive soft-suppresses contributor offers, agent kick claims, and new worker claims while the label remains. | Soft suppress; dashboard display | Hive/contributor flow | Contributor admission, actionable filtering, claim create | Human removes or config changes; optional close behavior | `hub.contribute_already_done_label`, `contribute_close_already_done`, fixed label (`src/pkg/config/config.go:4701-4924`, `src/pkg/github/issue_claims.go`, `src/pkg/dashboard/claims_api.go`) | | `blocked` | Issues | Contributor queue and dashboard issue bands | Always included in contributor skip patterns and default waiting band. Does not by itself stop spoke-agent enumeration unless also exempt/held/filtered. | Contributor hard skip; display | Human | Contributor admission/dashboard render | Remove label | `hub.contribute_skip_labels`, dashboard bands (`src/pkg/config/config.go:4585-4685`, `src/docs/dashboard.md:105-140`) | | `needs-direction` | Issues | Enumeration, escalation labels, dashboard issue bands and un-park sweep | ADR-0019 escalation marker for work that needs a maintainer direction decision before continuing. Filtered from enumeration like `needs-human`: no agent kick, kick claim or contributor offer. The un-park sweep keeps the same "What to reply" notice as `needs-human`/`needs-decision` and accepts the same `/hive approve` or `/hive decision ` commands. See [Maintainer commands](maintainer-commands.md). | Hard suppress until directed | Hive/human | Dashboard render and un-park sweep | Human removes, or a maintainer replies `/hive approve` / `/hive decision `, which also adds `approved-direction` and never touches `hold` (`src/pkg/github/issue_unpark_command.go`) | Fixed escalation label (`src/pkg/github/labels.go`, `src/docs/adr/0019-escalation-over-stalling.md`) | | `needs-decision` | Issues | Enumeration, contributor relay, dashboard issue bands and un-park sweep | Relay can apply it when a maintainer decision is needed; filtered from enumeration like `needs-human`, so no agent kick, kick claim or contributor offer. The un-park sweep keeps the "What to reply" notice and command handling documented in [Maintainer commands](maintainer-commands.md). | Contributor hard skip; display | Relay/Hive/human | Contributor admission/dashboard render | Human removes, or a maintainer replies `/hive approve` / `/hive decision `, which also adds `approved-direction` and never touches `hold` (`src/pkg/github/issue_unpark_command.go`); empty config disables relay application | `hub.contribute_needs_decision_label` (`src/pkg/config/config.go:4397-4408`, `src/pkg/config/config.go:4645-4685`) | diff --git a/src/pkg/dashboard/claims_api.go b/src/pkg/dashboard/claims_api.go index 3ad5ab95b6..e54b2ae173 100644 --- a/src/pkg/dashboard/claims_api.go +++ b/src/pkg/dashboard/claims_api.go @@ -9,7 +9,10 @@ import ( "strings" "time" + gh "github.com/google/go-github/v72/github" + "github.com/hivecommons/hive/pkg/claims" + ghpkg "github.com/hivecommons/hive/pkg/github" "github.com/hivecommons/hive/pkg/hooks" "github.com/hivecommons/hive/pkg/worksource" ) @@ -290,6 +293,10 @@ func (s *Server) handleClaimCreate(w http.ResponseWriter, r *http.Request) { jsonError(w, "Sign in with GitHub (or use an owner token) to claim an issue.", http.StatusUnauthorized) return } + if refused, msg := s.RefuseAlreadyDoneClaim(r.Context(), repo, number); refused { + jsonError(w, msg, http.StatusConflict) + return + } var body claimRequestBody if r.Body != nil { _ = json.NewDecoder(r.Body).Decode(&body) @@ -313,6 +320,90 @@ func (s *Server) handleClaimCreate(w http.ResponseWriter, r *http.Request) { writeClaimsJSON(w, status, claimResultJSON(res)) } +const alreadyDoneClaimRefusalMarker = "" + +// RefuseAlreadyDoneClaim refuses to create a new worker claim on an issue Hive +// has already marked as fixed. This is a soft suppression: removing the display +// label makes the issue claimable again, and a GitHub read failure fails open +// so a transient API problem does not break otherwise-valid claims. +func (s *Server) RefuseAlreadyDoneClaim(ctx context.Context, repo string, number int) (bool, string) { + if s == nil || s.deps == nil || s.deps.GHClient == nil || number <= 0 || repo == "" { + return false, "" + } + gh := s.deps.GHClient.GoGitHub() + if gh == nil { + return false, "" + } + owner, name, ok := strings.Cut(repo, claimsRepoSep) + if !ok || strings.TrimSpace(owner) == "" || strings.TrimSpace(name) == "" { + return false, "" + } + issue, _, err := gh.Issues.Get(ctx, owner, name, number) + if err != nil { + if s.logger != nil { + s.logger.Warn("[claims] already-done claim guard failed open", "repo", repo, "issue", number, "error", err) + } + return false, "" + } + label, ok := s.alreadyDoneLabelOn(issue.Labels) + if !ok { + return false, "" + } + claim, hasClaim := s.mergedIssueClaim(repo, number) + body := alreadyDoneClaimRefusalComment(label, claim, hasClaim) + alreadyCommented := false + if ok, err := s.deps.GHClient.IssueCommentsContain(ctx, repo, number, alreadyDoneClaimRefusalMarker); err == nil { + alreadyCommented = ok + } + if !alreadyCommented { + if err := s.deps.GHClient.CreateIssueComment(ctx, repo, number, body); err != nil && s.logger != nil { + s.logger.Warn("[claims] already-done claim refusal comment failed", "repo", repo, "issue", number, "error", err) + } + } + return true, "issue is marked " + label + "; not claimable until the label is removed" +} + +func (s *Server) alreadyDoneLabelOn(labels []*gh.Label) (string, bool) { + want := ghpkg.AlreadyDoneLabel + if s != nil && s.deps != nil && s.deps.Config != nil { + want = s.deps.Config.Hub.ContributeAlreadyDoneLabelOrDefault() + } + for _, label := range labels { + name := strings.TrimSpace(label.GetName()) + if strings.EqualFold(name, ghpkg.AlreadyDoneLabel) || strings.EqualFold(name, want) { + return name, true + } + } + return "", false +} + +func (s *Server) mergedIssueClaim(repo string, number int) (ghpkg.IssueClaim, bool) { + if s == nil || s.deps == nil || s.deps.IssueClaimed == nil { + return ghpkg.IssueClaim{}, false + } + if claim, ok := s.deps.IssueClaimed(repo, number); ok && claim.MergedPR { + return claim, true + } + if _, name, ok := strings.Cut(repo, claimsRepoSep); ok { + if claim, ok := s.deps.IssueClaimed(name, number); ok && claim.MergedPR { + return claim, true + } + } + return ghpkg.IssueClaim{}, false +} + +func alreadyDoneClaimRefusalComment(label string, claim ghpkg.IssueClaim, hasClaim bool) string { + target := "a merged fix" + if hasClaim && claim.PRNumber > 0 { + target = fmt.Sprintf("merged PR #%d", claim.PRNumber) + if claim.PRURL != "" { + target = fmt.Sprintf("merged PR #%d (%s)", claim.PRNumber, claim.PRURL) + } + } + return fmt.Sprintf("%s\nNot claiming this issue because it is marked `%s`; %s already fixed it. Remove `%s` (or mark `%s`) if work remains.", + alreadyDoneClaimRefusalMarker, label, target, label, ghpkg.VerifiedOpenLabel) +} + func (s *Server) handleClaimRelease(w http.ResponseWriter, r *http.Request) { repo, number, err := claimPathRef(r) if err != nil { diff --git a/src/pkg/dashboard/claims_api_test.go b/src/pkg/dashboard/claims_api_test.go index 3ab2b9d3c6..6ffa6b7197 100644 --- a/src/pkg/dashboard/claims_api_test.go +++ b/src/pkg/dashboard/claims_api_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/hivecommons/hive/pkg/claims" + ghpkg "github.com/hivecommons/hive/pkg/github" "github.com/hivecommons/hive/pkg/hooks" ) @@ -69,6 +70,27 @@ func TestClaims_SelectTaskSkipsHumanClaimedIssue(t *testing.T) { } } +func TestClaims_SelectTaskSkipsAlreadyDoneIssue(t *testing.T) { + hub, s, l := claimsHub(t) + done := intgIssue(1, "already fixed", "someone", nil) + done["labels"] = []any{ghpkg.AlreadyDoneLabel} + free := intgIssue(2, "free", "someone", nil) + free["labels"] = []any{"bug"} + setStatusIssues(s, done, free) + c := &ContributorConnection{ + profile: &ContributorProfile{GitHubUsername: "worker", ContributorID: "c-worker", TrustTier: "contributor"}, + lastPong: time.Now(), + } + + msg := hub.selectTask(c) + if msg == nil || msg.Type != "task_assign" || msg.Number != 2 { + t.Fatalf("expected #2 (the not-already-done issue), got %+v", msg) + } + if _, ok := l.Lookup("myorg/repo1", 1); ok { + t.Fatal("already-done issue was claimed") + } +} + func TestClaims_TakeoverPreemptsOnlyTheDisplacedSession(t *testing.T) { hub, s, l := claimsHub(t) setStatusIssues(s, @@ -203,6 +225,51 @@ func TestClaims_APIRoutes(t *testing.T) { } } +func TestClaims_APIRouteRefusesAlreadyDoneIssue(t *testing.T) { + _, s, l := claimsHub(t) + var posted string + ghSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/repos/o/r/issues/5": + _, _ = w.Write([]byte(`{"number":5,"labels":[{"name":"hive/already-done"}]}`)) + case r.Method == http.MethodGet && r.URL.Path == "/repos/o/r/issues/5/comments": + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost && r.URL.Path == "/repos/o/r/issues/5/comments": + var body map[string]string + _ = json.NewDecoder(r.Body).Decode(&body) + posted = body["body"] + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":1}`)) + default: + t.Fatalf("unexpected GitHub request %s %s", r.Method, r.URL.Path) + } + })) + t.Cleanup(ghSrv.Close) + s.deps.GHClient = ghpkg.NewClientForTest(ghSrv.URL, "o", []string{"r"}, s.logger) + s.deps.IssueClaimed = func(repo string, number int) (ghpkg.IssueClaim, bool) { + if repo == "o/r" && number == 5 { + return ghpkg.IssueClaim{Repo: repo, Issue: number, PRNumber: 10617, PRURL: "https://github.com/hivecommons/hive/pull/10617", MergedPR: true}, true + } + return ghpkg.IssueClaim{}, false + } + + req := httptest.NewRequest(http.MethodPost, "/api/claims/o/r/5", strings.NewReader("")) + req.Header.Set("X-Hive-User", "alice") + w := httptest.NewRecorder() + s.mux.ServeHTTP(w, req) + if w.Code != http.StatusConflict { + t.Fatalf("claim code=%d body=%s, want 409", w.Code, w.Body.String()) + } + if _, ok := l.Lookup("o/r", 5); ok { + t.Fatal("already-done issue was claimed") + } + for _, want := range []string{"hive:claim-refused", "hive/already-done", "#10617", "https://github.com/hivecommons/hive/pull/10617"} { + if !strings.Contains(posted, want) { + t.Fatalf("refusal comment missing %q: %q", want, posted) + } + } +} + // A relay whose socket is down has no connection to yank, but its lease would // otherwise let it RESUME the item it lost on reconnect (#4260 resumability). // Takeover must revoke that lease too. diff --git a/src/pkg/github/issue_claims.go b/src/pkg/github/issue_claims.go index aa3f4979dd..4778c00484 100644 --- a/src/pkg/github/issue_claims.go +++ b/src/pkg/github/issue_claims.go @@ -188,6 +188,35 @@ func FilterLiveIssueClaims(result *ActionableResult, now time.Time, logger *slog return withheld } +// FilterAlreadyDoneIssues removes issues marked hive/already-done from the +// actionable set. The label is not a hard hold: it does not park the issue in +// the hold list or require an operator unhold, but it does mean Hive already has +// enough evidence that new contributor offers and agent kicks are duplicate +// work until a human removes the display label. +func FilterAlreadyDoneIssues(result *ActionableResult, logger *slog.Logger) int { + if result == nil || len(result.Issues.Items) == 0 { + return 0 + } + kept := make([]Issue, 0, len(result.Issues.Items)) + withheld := 0 + for _, issue := range result.Issues.Items { + if !issueHasLabel(issue.Labels, AlreadyDoneLabel) { + kept = append(kept, issue) + continue + } + withheld++ + if logger != nil { + logger.Info("withholding issue: already-done label", + "repo", issue.Repo, "issue", issue.Number, "label", AlreadyDoneLabel) + } + } + if withheld == 0 { + return 0 + } + result.Issues = IssueResultFromItems(kept) + return withheld +} + // The dashboard-facing seam for issue claims (hivecommons/hive#8380). // // pkg/dashboard already imports this package for the Issue envelope that diff --git a/src/pkg/github/issue_claims_test.go b/src/pkg/github/issue_claims_test.go index 123b2d2f87..570ee26bb4 100644 --- a/src/pkg/github/issue_claims_test.go +++ b/src/pkg/github/issue_claims_test.go @@ -311,6 +311,29 @@ func TestFilterLiveIssueClaims(t *testing.T) { } } +func TestFilterAlreadyDoneIssues(t *testing.T) { + items := []Issue{ + {Repo: "widgets", Number: 1, Labels: []string{AlreadyDoneLabel}}, + {Repo: "widgets", Number: 2, Labels: []string{LikelyDoneLabel}}, + {Repo: "widgets", Number: 3}, + } + result := &ActionableResult{Issues: IssueResultFromItems(items)} + if n := FilterAlreadyDoneIssues(result, slog.New(slog.NewTextHandler(io.Discard, nil))); n != 1 { + t.Fatalf("withheld %d, want 1", n) + } + if len(result.Issues.Items) != 2 || result.Issues.Count != 2 { + t.Fatalf("kept %d/%d, want 2", len(result.Issues.Items), result.Issues.Count) + } + for _, issue := range result.Issues.Items { + if issue.Number == 1 { + t.Fatal("already-done issue #1 was not withheld") + } + } + if n := FilterAlreadyDoneIssues(nil, nil); n != 0 { + t.Fatalf("nil result withheld %d", n) + } +} + // The SetIssueClaims setting is read live and a zero ttl takes the default. func TestIssueClaimsSetting(t *testing.T) { var c *Client diff --git a/src/pkg/github/prclaims.go b/src/pkg/github/prclaims.go index ec241a0c16..72ec25310f 100644 --- a/src/pkg/github/prclaims.go +++ b/src/pkg/github/prclaims.go @@ -102,6 +102,8 @@ const ( // LikelyDoneLabel marks an open issue with an API-verified merged PR relation // that is not yet a resolved/closed issue. LikelyDoneLabel = "hive/likely-done" + // AlreadyDoneLabel marks an issue that Hive verified is already fixed. + AlreadyDoneLabel = "hive/already-done" // VerifiedOpenLabel records that verification found the issue still open/not // done, suppressing renewed likely-done labeling from the same stale claim. VerifiedOpenLabel = "hive/verified-open"