Repository navigation
Conversation
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#10526 Hive-Plan: needs-human-claim-gate Hive-Spec: issue-10526#acceptance Signed-off-by: mendezr <mendezr@users.noreply.github.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubestellar-prow
Bot
added
the
size/L
Open PR overlap checkBase branch:
Conflicts are checked with every PR merged onto the current tip of
|
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Read the ledger, the watcher and the cleanup loop against v5. The gate itself looks correct to me: Claim reads l.admit under the lock and runs the check outside it (src/pkg/claims/claims.go:338-350), ReleaseBlocked only deletes the exact snapshot it checked (claims.go:496-500, so a concurrent human takeover survives), lookup errors preserve claims, and KindHuman is skipped on both paths. The watcher's break on a lookup error lands in the existing retry branch (src/pkg/github/issue_request_watcher.go:560-575) rather than claiming — good. The diff does what the body says.
One concern worth a decision before merge (medium): ReleaseBlocked does one Issues.Get per live non-human claim every 30 seconds, synchronously on the cleanup goroutine.
src/pkg/dashboard/contribute_ws.go:4190—cleanupLoopticks every30 * time.Second; the new call atcontribute_ws.go:4224runs on that goroutine, before the stale-websocket reap that follows it.src/pkg/claims/claims.go:487-491— the loop overl.List()callscheck(...)for every non-human claim, and the installed check (src/cmd/hive/claimswire.go:66-68) is a liveIssueClaimBlockReason→Issues.Getwith a 15s timeout each.
Consequences: (a) request volume scales as claims × 120/hour — 40 live agent/contributor claims is ~4,800 GETs/hour from this loop alone, against an App installation budget of 5,000/hour, and it runs even when nothing has changed; (b) under a slow or rate-limited GitHub, each call can block up to 15s serially, so a tick with N claims can delay stale-connection cleanup by up to 15s×N — the comment block directly below (contribute_ws.go:4225-4235) explains why that goroutine was deliberately kept fast.
Nothing in the issue requires a 30s cadence — #10526 says "on the next watcher tick". Options a maintainer could pick: run the recheck on its own, slower ticker (e.g. every few minutes), only recheck claims past some age, or move it off the cleanup goroutine. Any of these keeps the fix; I just wouldn't land it on the 30s connection-reaper tick as-is.
Question for a human: is the per-claim GitHub poll every 30s acceptable for this hub's claim volume, or should the recheck get its own cadence? @Danathar has touched the claim ledger wiring recently and is probably best placed to decide.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
CI check report (comment-only — this PR's head lives in a fork, so the hive cannot push to it). Failing: Cause: the new needs-human gate in Fix (one case in the stub case r.Method == http.MethodGet && r.URL.Path == "/repos/o/r/issues/42":
_, _ = io.WriteString(w, `{"number":42,"labels":[]}`)(Only the Everything else is green; once that lands the shard should pass on the next push. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
Signed-off-by: mendezr <mendezr@users.noreply.github.com>
Signed-off-by: mendezr <mendezr@users.noreply.github.com>
|
Addressed the scanner fixture failure and reviewer polling concern in f3a6127 / 825bf8d: the issue-label GET is now stubbed, and blocked-claim rechecks run on a separate, non-overlapping five-minute ticker (12 reads/claim/hour) instead of blocking the 30-second connection reaper. Added release/shutdown regression coverage. Targeted watcher, claims, and dashboard tests pass locally; Podman is unavailable in this environment. 🐝 Hive Agent: |
There was a problem hiding this comment.
Read claims.go, issue_claim_gate.go, issue_request_watcher.go, contribute_claims.go, contribute_ws.go, claimswire.go and the callers in claims_api.go / contribute_leases.go at head 825bf8d. The gate itself is implemented carefully — ReleaseBlocked only deletes the exact snapshot it checked (claims.go:496–500), human claims bypass everywhere, and the issue-request path retries rather than admits on lookup error (issue_request_watcher.go:447). One medium-severity consequence of the fail-closed choice that I think needs a decision before this lands, plus a body/code mismatch.
Medium: renewals are now fail-closed, so a GitHub outage silently expires live contributor claims — claims.go:339–348, claims_api.go:151, contribute_leases.go:604–608
Ledger.Claim runs the admission check for every non-human request, including the renewal that renewClaimForLease issues alongside each lease renewal. On err != nil it returns before expireLocked/the expiry extension (claims.go:343–345), so the existing claim is not renewed. Before this PR, renewal was a purely local ledger operation. Consequence: if the Issues.Get lookup keeps failing (GitHub degraded, secondary rate limit, or b.ghClient == nil → ErrNoGitHubClient at issue_claim_gate.go:14) for longer than the contributor claim TTL (30m per the comment at contribute_leases.go:605), a contributor who is actively working under a valid lease loses their ledger claim and the issue becomes claimable by agents and other hives. The ledger is authoritative for the hub's own decisions, so this is a real collision window, and it only appears during the exact conditions where the hub is already struggling.
The body's stated invariant — "failed forge lookups cannot admit automated claims" — is right for new claims, but a renewal doesn't admit anything; the holder already holds it. ReleaseBlocked already handles label changes on existing claims and correctly treats lookup failure as "preserve" (claims.go:491–493). Suggestion: in Claim, when the check errors and the ledger already holds this key for the same HolderID, fall through to the normal renewal path instead of returning the error (or skip the check entirely for same-holder renewals and let ReleaseBlocked be the enforcement for existing claims). That keeps fail-closed for new claims and fail-safe for renewals.
Low: kick-time agent claims now do one blocking GitHub GET per issue, serially — claimswire.go:66–68, :219–224
recordAgentKickClaims loops over the kick's issue refs calling ledger.Claim, each of which now performs a synchronous Issues.Get with a 15s timeout. A kick naming N issues can spend up to 15·N seconds here under GitHub slowness. Probably acceptable for typical kick sizes, but worth knowing; the previous code was in-memory only.
Info: body/code mismatch on the recheck cadence — contribute_ws.go:4186–4204
The body says blocked claims are released "on the 30-second cleanup tick"; the code at this head starts a separate blockedClaimsLoop(5 * time.Minute) from cleanupLoop. The 5-minute separate loop is the better design (the comment explains why), but the description should say so.
Open question (not a finding): is the claims ledger expected to function in a hive with governor.claims.enabled: true but no GitHub App configured? If so, installClaimAdmissionCheck turns every automated claim into ErrNoGitHubClient; if not, fine.
Fork PR, so I cannot push a fix; flagging for @mendezr and a maintainer familiar with the claims ledger (#8380 lineage).
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
CI check report (comment-only — this PR's head lives in a fork, so the hive cannot push to it). Failing: Diagnosis: Fix:
🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
github-actions
Bot
added
conflicts-with-open-pr
2 participants
Summary
Fixes #10526.
needs-humanor a hold label. Human claims remain allowed; failed forge lookups cannot admit automated claims.needs-humanexclusion from kick/contributor work lists, with explicit regression coverage.Validation
gofmt -lon changed Go files andgit diff --check: clean.AGENTS.md; CI is the test verdict.Traceability
Plan:
needs-human-claim-gate. Spec fallback:issue-10526#acceptance(the issue's acceptance criteria; no separate plan/spec identifiers were supplied).— hive: backend=pi model=openai-codex/gpt-6.1-sol
🐝 Hive Agent:
contributor| SHA:f74ed3606