[hivecommons-hive]

Closes #17300

Why

At 15:27:45 the claim ledger released Karibhou's claim on #17243 with reason lease revoked. That was 40 s after their PR #17248 had covered the issue. The lease ended because the work was done, not abandoned. Even so, releaseClaimForLease deleted the claim, so every reader that checks only the claim ledger (dispatch, the PR-request gate, the claim listing) saw #17243 as unclaimed. ledgerCoveringPR only works around this for the stage deriver.

What changed

• releaseClaimForLease (covers every lease-revoke and lease-expiry path) first asks the duplicate-PR claim ledger (Dependencies.IssueClaimed) whether an open PR by the claim's holder covers the issue (CoversOpenIssue). If one does, the claim is kept as a PR-held claim for the same principal through the new claims.(*Ledger).HoldForPR, which sets PRStarted plus the new additive PRRepo/PRNumber fields and holds for the policy max TTL. If there is no covering PR, or the PR is someone else's, the claim is still released as before.
• Before this change, nothing released a claim when its PR merged or closed; claims only lapsed when their TTL ran out. The hub cleanup tick now runs sweepPRHeldClaims. While the same PR still covers the issue, the sweep renews the hold without calling GitHub. Once the PR merges, closes or stops covering the issue, the sweep releases the claim with claims.ReasonCoveringPRDone. If the PR ledger isn't wired up, the hold simply expires at its TTL.
• The ledgerCoveringPR comment now notes that this case is handled upstream.

Tests

• src/pkg/claims: TestHoldForPRKeepsClaimWithPREvidence
• src/pkg/dashboard: TestClaims_LeaseRevokeKeepsClaimCoveredByHoldersPR (claim kept with PR evidence, then released by the sweep once the PR closes) and TestClaims_LeaseRevokeWithoutHoldersPRReleasesClaim (no covering PR, or a PR by someone else, still releases)

Changelog: changelog.d/fixed-lease-revoke-keeps-covered-claim.md

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign hanthor for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hivecommons-hive]

Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor.

hive/awaiting-ci: CI pending — sweep will merge when green. Pushed 869b698: task_complete now verifies the reported PR before revoking the lease and records it in the duplicate-PR ledger, so the claim is held for the contributor's PR even when no scan has seen it yet (the sweep also re-holds for a replacement PR by the same holder).

---

🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

Review findings from adjudicator-2 (folded here because this head already carries its hive review):

Security re-review (delta bec0481…→869b698) — no blocker; one advisory.

• src/pkg/dashboard/claims_api.go:244-265 recordCompletionPRClaim writes a client-reported PR URL into the duplicate-PR ledger gated only on VerifyReportedPR (src/pkg/github/pullrequest_verify.go:133-142), which checks that the PR exists, its base repo matches the assignment, and the contributor authored it — but not that the PR references the leased issue. A contributor can therefore task_complete with any open PR they authored on the assigned repo and have issue N recorded as "claimed by PR X" (ExternalAuthor, open, unsettled), which keeps their contributor claim past revoke and suppresses dispatch. Bounded: the entry is unsettled, so the next authoritative ReconcileScan drops it unless the PR actually lists the issue (src/pkg/github/prclaims.go:1703-1721), and sweepPRHeldClaims then releases the hold. The same unrelated-PR already earns the long cooldown/trust credit at this head, so this is a one-scan-interval extension of an existing trust assumption, not a new hole. Advisory only; a later tightening could require PRCoversIssue-style evidence before RecordIssueClaim.

Prior docs-currency note (contributor-relay.md says a revoked/completed lease releases the claim, e.g. line 1228) is unchanged by this delta — no docs file in the compare.

Review findings from reviewer-2 (folded here because this head already carries its hive review):

Read the diff against the tree at v5 plus the callers (contribute_leases.go:714/1131, contribute_flap.go:236, claims.go renew path, prclaims.go reconcile). This looks correct to me; no defects found.

What I verified:

• claims.go:523-527 — the sameHolder renewal path copies prev forward, so a later claimIssueForContributor/renewClaimForLease on a PR-held claim keeps PRRepo/PRNumber; the sweep's c.PRNumber <= 0 filter at claims_api.go:280 still sees it.
• prclaims.go:1703-1712 — an authoritative scan replaces the map, dropping claims whose PR closed; a merged PR comes back with MergedPR=true, so CoversOpenIssue() (prclaims.go:328) is false either way and sweepPRHeldClaims releases with ReasonCoveringPRDone. The merge/close release the body describes is real.
• recordCompletionPRClaim is gated on pr.Verified && !pr.Merged and on ParsePRURL(...).FullName() == task.Repo, matching the trust posture of the existing RecordIssueClaim writer in contribute_verdict_settle.go:78-135. Moving verifyReportedPRDetail ahead of revokeLease in handleTaskComplete is order-only; verifiedPR/prDetail are consumed unchanged below.
• Holder matching (coveringPRByHolder) compares pr.PRAuthor to c.Holder, which claimIssueForContributor sets to GitHubUsername (falling back to ContributorID); a login-less contributor simply never matches and is released as before.

One open question, not a defect: reconcileOpen keeps a recorded-after-listing claim only when c.ObservedAt >= listedAt (prclaims.go:1713-1721). If the completion record lands before a scan starts and the scan's PR enumeration has not yet indexed the relay-opened PR, the entry is dropped and the next sweepPRHeldClaims releases the claim while the PR is still open — the same symptom #17300 fixes, for one scan interval. Whether the enumeration path can lag like that is something I could not confirm from the tree.

Review findings from adjudicator (folded here because this head already carries its hive review):

Correctness perspective — delta re-review at head 869b698 (since bec0481). No blocker; one low advisory.

A closed-unmerged completion PR is recorded as open and holds the claim until the next authoritative scan (low, advisory). recordCompletionPRClaim (src/pkg/dashboard/claims_api.go:244-262) skips only pr.Merged and otherwise writes PRState: ghpkg.PRStateOpen. VerifyReportedPR (src/pkg/github/pullrequest_verify.go:131-144) verifies on base-repo + author and never checks open/closed, so a PR the contributor closed before task_complete is recorded as an open covering claim, coveringPRByHolder (claims_api.go:218-226, CoversOpenIssue at prclaims.go:328-333) accepts it, and sweepPRHeldClaims (claims_api.go:275-293) keeps re-holding it until the authoritative scan replaces the ledger map (prclaims.go:1702-1711). Narrow window; fix would be carrying the PR's state out of VerifyReportedPR (or skipping non-open PRs in recordCompletionPRClaim).

Previous docs-currency note (src/docs/contributor-relay.md:1243 — "releasing it when the task completes…") is still present at this head; the delta did not touch the doc.

Review findings from ci-maintainer (folded here because this head already carries its hive review):

intent-alignment (delta re-review, head 869b698) — approve.

The delta closes the real gap in the previous head: with the duplicate-PR ledger only refreshed by scans, releaseClaimForLease → coveringPRByHolder (claims_api.go:188-227) found nothing at revoke time for a PR the relay opened minutes earlier, so the #17300 path still released the claim. recordCompletionPRClaim (claims_api.go:246-265) now writes the GitHub-verified completion PR (VerifyReportedPR enforces author == contributor, pullrequest_verify.go:137-142) into that ledger before revokeLease runs (contribute_ws.go:3957-3965), and reconcileOpen carries a claim recorded after the scan start across the next authoritative scan (prclaims.go:1710-1720). TestClaims_CompletionPRHoldsClaimBeforeScanSeesIt covers both the hold and the unverified/merged negative cases.

Advisory (non-blocking): the PR body's "What changed" still describes only releaseClaimForLease/sweepPRHeldClaims; it does not mention recordCompletionPRClaim, the new Dependencies.RecordIssueClaim write from the contribute hub, or the sweep now re-holding for a replacement PR by the same holder (claims_api.go:283-287). Worth a one-line update so the body matches the diff.

Prior docs-currency note (contributor-relay.md:1243, "releasing it when the task completes…") is still present at this head; not re-filed here.

Evidence bundle `#173051b3aae4` sha256:4b4d0200eb3c74337cb11b9039835b3009d9f913733c70d98aec82a80b74daa7 — download from the dashboard or `hivectl review evidence #17305`.

[github-actions]

Open PR overlap check

Base branch: v5

Conflicts are checked with every PR merged onto the current tip of v5, so a PR that only needs a rebase is not reported as conflicting with its siblings.

No conflicting open PRs or same-file overlaps were found.

[hivecommons-hive]

Read src/pkg/claims/claims.go and src/pkg/dashboard/claims_api.go at 7b14b86, plus the callers (issueClaimedByOpenPR in contribute_admission.go:306, Ledger.Claim renew branch, InstallClaimHooks). This looks correct to me.

• HoldForPR (claims.go:738-767) only converts a live claim held by the same HolderID, so a claim already taken over (the preemptDisplacedHolder → revokeLeaseForKey path) falls through to the existing ReleaseByHolderID — no change in that path.
• The renew branch of Ledger.Claim mutates prev in place (claims.go:523-527), so a holder who re-leases the same issue later does not lose PRRepo/PRNumber; the sweep keeps tracking it.
• coveringPRByHolder (claims_api.go:218-233) requires PRAuthor to equal Claim.Holder. claimIssueForContributor (claims_api.go:135-138) uses GitHubUsername and falls back to ContributorID, so an unverified contributor never matches and gets today's release behaviour — the conservative side.
• OnClaimed fires only when PR evidence changed (changed at claims.go:750), so the cleanup-tick sweep renewing a hold does not re-run integrationClaimObserved every tick.
• sweepPRHeldClaims runs after Expire() in cleanupLoop (contribute_ws.go:4732-4737), so it never re-holds an already-lapsed claim.

Info, not a defect: with Dependencies.IssueClaimed unwired the hold lasts the policy MaxTTL (24h default) and then lapses; the body says so, and that is the same lifetime any claim has today.

Diff matches #17300's scope; the src/docs/api-reference/contribute.md change is citation-line drift only.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Review follow-ups (below review.backlog.min_severity: P2)

Non-blocking findings below the backlog filing line are listed here instead of as issues, one comment per PR, updated in place. Tick them off as they are fixed; set review.backlog.min_severity: P3 to file every finding.

• P3 info intent-alignment — Conversion depends on claim Holder being the GitHub login (src/pkg/dashboard/claims_api.go:131): coveringPRByHolder matches pr.PRAuthor against c.Holder with EqualFold. claimIssueForContributor sets Holder to profile.GitHubUsername, falling back to profile.ContributorID when no login is known (c…
• P3 low correctness — Sweep releases claim when the same holder's different PR now covers the issue (src/pkg/dashboard/claims_api.go:254): sweepPRHeldClaims re-holds only when pr.PRNumber == c.PRNumber; if the contributor replaced PR A with PR B for the same issue, coveringPRByHolder confirms B is theirs but the claim is released with R…
• P3 low correctness — sweepPRHeldClaims persists the ledger every 30s per PR-held claim (src/pkg/dashboard/claims_api.go:255): sweepPRHeldClaims calls HoldForPR on every cleanupLoop tick (30s, contribute_ws.go:4703), and HoldForPR unconditionally saveLocked()s (claims.go:757) even when only ExpiresAt advanced. One disk write…
• P3 low intent-alignment — Integration reservation still released on lease revoke even when the claim is kept (src/pkg/dashboard/contribute_leases.go:710): revokeLease fires h.server.integrationRelease(..., "lease revoked") for every released lease before calling releaseClaimForLease (contribute_leases.go:708-715). With this PR the claim may now survive…

[hivecommons-hive]

Correctness review @ 7b14b86 (read from the local checkout; the GitHub API cap prevented me from reading #17300 and the PR body, so I judged against the changelog fragment's stated contract).

1. The hold is gated on a ledger that usually hasn't seen the contributor's PR yet (in-scope, medium)

releaseClaimForLease only keeps the claim when coveringPRByHolder finds the PR in deps.IssueClaimed (src/pkg/dashboard/claims_api.go:194-201, :222). That hook is getClaimLedger().Lookup (src/cmd/hive/main.go:1145-1147), which the code itself documents as "one eval cycle old" (main.go:1148-1150). The only out-of-band writer, RecordOpenedPRClaims, is wired to SetPROpenedDetailHook on the hive's own GitHub client (src/cmd/hive/github_client_config.go:446-458) — a contributor relay opening a PR with its own token never triggers it.

On the primary path — relay reports task_complete with a just-opened PR — revokeLease runs before the server verifies the reported PR (src/pkg/dashboard/contribute_ws.go:3961-3969: revokeLease at :3963, verifyReportedPRDetail at :3968). So unless a scan happened to run between the PR open and the completion report, coveringPRByHolder returns false and the claim is still released with lease revoked — the exact symptom the changelog says is fixed. The hold will fire reliably only for later lease ends (abandon/expire/yank well after the PR opened), and nothing re-establishes the claim once the scan does see the PR.

Suggested fix: in the task_complete handler, after verifyReportedPRDetail succeeds, call l.HoldForPR(identity, key, prDetail.Repo, prDetail.Number) directly (or feed the verified PR into deps.RecordIssueClaim before revokeLease), so the hold does not depend on scan timing. Add a test where IssueClaimed is empty at revoke time but the completion carries a verified PR.

2. Sweep writes the ledger to disk every 30s per held claim (advisory, low)

sweepPRHeldClaims (claims_api.go:254-256) calls HoldForPR on every tick, and HoldForPR unconditionally saveLocked()s (src/pkg/claims/claims.go:757) even when only ExpiresAt moved. With cleanupLoop at 30s (contribute_ws.go:4703) that is one fsync per PR-held claim per tick. Consider skipping the save/extension when the remaining TTL is still far from expiry.

3. Same holder's different PR drops the claim instead of re-holding (edge, low)

sweepPRHeldClaims requires pr.PRNumber == c.PRNumber (claims_api.go:254); if the contributor closed PR A and opened PR B for the same issue, the claim is released with ReasonCoveringPRDone rather than re-held for B, even though coveringPRByHolder already confirmed B is theirs.

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Re-read at 869b698 (claims.go, claims_api.go, contribute_ws.go:3954-3975, plus VerifyReportedPR in pullrequest_verify.go:103-140 and ClaimLedger.Record/reconcileOpen in prclaims.go). The new head closes the gap the previous review raised: verifyReportedPRDetail now runs before revokeLease (contribute_ws.go:3957-3965), and recordCompletionPRClaim writes the verified PR into the ledger coveringPRByHolder reads, so the hold fires on the task_complete path even when no scan has seen the PR. The sweep's "any open PR by the holder re-holds" change (claims_api.go:289-294) also covers the replace-PR-A-with-PR-B case. Looks correct to me.

One low-severity edge, not a blocker:

• A closed-unmerged PR reported at completion is recorded as open. VerifyReportedPR checks existence, base repo and author only — it never reads pr.GetState() (pullrequest_verify.go:127-140; the doc at :63-67 says so explicitly). recordCompletionPRClaim then stamps PRState: ghpkg.PRStateOpen unconditionally (claims_api.go:259-260), and CoversOpenIssue is true for anything not MergedPR (prclaims.go:328-333). So a contributor whose PR was already closed when they reported task_complete gets their claim held, and the issue is suppressed from dispatch, until the next authoritative scan replaces the map (prclaims.go:~1705). Bounded to one scan interval, so low; surfacing pr.GetState() on PRVerification and skipping the record when it is not open would close it.

Still open from earlier reviews and unchanged here: HoldForPR calls saveLocked() on every 30 s sweep tick per held claim even when only ExpiresAt moved (claims.go:755-757).

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[github-actions]

Thank you for your contribution! Your PR has been merged.

We'd love to hear how your experience was: share feedback