Repository navigation
π fix(contribute): a merged Refs PR no longer holds its issue - #14454
hivecommons-hive[bot] merged 2 commits into
Conversation
The contribute queue treated a merged PR that only said `Refs #N` as a live claim, logging "skip: issue already claimed by a PR ... merged=true" for as long as the merged-PR scan listed it. The PR landed part of the work and left the issue open for the rest, and agents already get such issues back with context (hivecommons#7061), so contributors now do too. A merged reference claim now ranks below every open claim in the ledger, so releasing it never hides a person's open PR on the same issue. The contribute-ws admission skip lines are logged when an item is first withheld or its reason changes, not on every selectTask pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1 Signed-off-by: Danathar <Danathar@users.noreply.github.com>
|
[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 |
|
@Danathar this PR is back in your court ( Failing checks:
Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path. Your relay session received this as a |
There was a problem hiding this comment.
Looks correct to me, read against the v6 base.
issueClaimedByOpenPR(contribute_admission.go): theok && !mergedReferenceClaim(claim)guard on therepo.Fulllookup falls through to therepo.Namelookup, so an open claim keyed under the other spelling still holds the issue β the cross-repo case in the body is handled, not just skipped.claimRank(prclaims.go:385): withtier-2for merged references, the two merged-Refsgrades rank-1(hive) and-2(external) while every other claim is β₯ 2, so they sit strictly below all open claims and keep hive-over-external between themselves. Rank 0/1 are now unused; harmless.- Skip-log dedup (
contribute_select.goaround line 876): the earlyreturnhappens before the logswitch, and that switch did nothing but log andreturn, so no metrics or Withheld-panel path is bypassed.admissionSkipClearedruns on admission under its own mutex.
One thing worth a maintainer's eye, not a defect: the rank change also moves agent-side behaviour. An issue with a merged hive Refs #N plus a person's open Refs #N now resolves to the open PR, so FilterClaimedIssues defers agents under the #3768 external-PR rule rather than releasing the issue for verification. The body states this; I'm surfacing it so it is a conscious acceptance.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Check report: the red agent shard here is π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
|
hive/shared-ci: the π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
|
hive/awaiting-ci: π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
β¦14415) (#14494) Port 3540e2a from v5 to the v6 path src/pkg/agent/. Give the thrash-breaker wiring test its own tmux socket and a plain shell so co-sharded tests cannot change its server options or tear down its session; emit the blocked glyph with single octal escapes; cancel and drain the poller during cleanup. Unblocks the test (agent 4/5) shard on v6 fork PRs (#14454, #14481). Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com> Co-authored-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
Shared-CI incident #14506 is resolved: the π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
|
hive/awaiting-ci: branch synced with v6 (picks up #14494, which fixed the π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Read the tree at the PR head rather than just the diff; this looks correct to me.
- Rank change (
src/pkg/github/prclaims.go:403-405): every open claim rankstier*2+1 >= 1; a mergedRefsnow ranks-1(hive) /-2(external); a merged closing PR keeps4/6, still above an openRefsat3. So the three invariants in the body hold: merged closing > open Refs, every open > merged Refs, merged hive Refs > merged external Refs. - Admission (
src/pkg/dashboard/contribute_admission.go:286-303): a merged reference claim is skipped for both repo spellings and never reachesmerged_claim_stale, which is the intended outcome sinceSettled()would not include it anyway. - Skip log dedupe (
src/pkg/dashboard/contribute_select.go:876-879): the earlyreturnhappens before aswitchthat contains onlyh.logger.Infocalls, so nothing besides the log line is suppressed; the Withheld panel path is untouched.admissionSkipClearedat:935runs after admission and before the per-contributor yank filter, which is the right place for an item-keyed memory.
go build ./pkg/dashboard/ ./pkg/github/ passes locally at 5781c10; hub/agent CI shards are green at the time of reading.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the diff against the v6 tree and the callers. Looks correct to me.
src/pkg/github/prclaims.go:403β with tiers 1 (hive ref) / 0 (external ref), a mergedRefsranks β1 / β2, below the lowest open rank (external open ref = 1) and below merged closing claims (4 / 6); hive-merged-ref still beats external-merged-ref, so the #3768 precedence holds.TestClaimRankMergedReferenceBelowEveryOpenClaimcovers both ledger scan orders.src/pkg/dashboard/contribute_admission.go:288-293βissueClaimedByOpenPRis the onlydeps.IssueClaimedcaller in the contribute path, so themergedReferenceClaimfilter changes nothing else. The churn guard still sees the merged PR viaIssuePRRecord, so churn protection is unaffected.src/pkg/dashboard/contribute_select.go:876β the early return sits afterevaluateContributorNeutralAdmission, so the sweep's withheld recording still runs; only the log line is deduplicated.admissionSkipLoggedis only touched underadmissionSkipMu.- Builds clean (
go build ./pkg/dashboard/ ./pkg/github/...at 5781c10).
One non-blocking observation: agents get the released issue with the merged PR as context (#7061); a contributor offered the remainder here gets no pointer to the merged Refs PR. Not a defect in this fix β just the one asymmetry that remains.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Style/maintainability review (head 5781c10) β no blockers; two advisory notes.
- The "merged reference" predicate now lives in two packages.
claimRanktestsc.Reference && c.MergedPR(src/pkg/github/prclaims.go:403) and the contribute reader re-derives the same thing as a free functionmergedReferenceClaim(src/pkg/dashboard/contribute_admission.go:301). The existing idiom for claim-state predicates is a method onIssueClaimnext toSettled()/ReopenedAfterMerge()(prclaims.go:292,:319), and the PR body's whole argument is that the ranker and the reader must agree on this definition β so give it one home:
--- a/src/pkg/github/prclaims.go
+++ b/src/pkg/github/prclaims.go
@@ func (c IssueClaim) Settled() bool {
return c.MergedPR && !c.Reference && !c.ExternalAuthor
}
+// MergedReference reports whether the claim comes from a merged PR that only
+// referenced the issue (`Refs #N`, `Part of #N`). It never said the issue was
+// done, so readers release it (#7061) and claimRank puts it below every open
+// claim.
+func (c IssueClaim) MergedReference() bool {
+ return c.MergedPR && c.Reference
+}
+
@@ func claimRank(c IssueClaim) int {
- if c.Reference && c.MergedPR {
+ if c.MergedReference() {
return tier - 2
}
--- a/src/pkg/dashboard/contribute_admission.go
+++ b/src/pkg/dashboard/contribute_admission.go
- if claim, ok := h.server.deps.IssueClaimed(repoFull, number); ok && !mergedReferenceClaim(claim) {
+ if claim, ok := h.server.deps.IssueClaimed(repoFull, number); ok && !claim.MergedReference() {
...
- if claim, ok := h.server.deps.IssueClaimed(repoName, number); ok && !mergedReferenceClaim(claim) {
+ if claim, ok := h.server.deps.IssueClaimed(repoName, number); ok && !claim.MergedReference() {
...
-// mergedReferenceClaim reports whether a claim comes from a merged PR that
-// only referenced the issue (`Refs #N`, `Part of #N`).
-func mergedReferenceClaim(c ghpkg.IssueClaim) bool {
- return c.MergedPR && c.Reference
-}return tier - 2is correct but opaque (prclaims.go:403-405). It yields β1 for a hive mergedRefsand β2 for an external one, so both sit below the lowest open rank (external openRefs= 1) while keeping hive-over-external between them. The doc comment above explains the policy but not why-2specifically produces those values; a one-line note there (or inTestClaimRankMergedReferenceBelowEveryOpenClaim) saves the next reader the arithmetic.
β hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
Summary
When a PR that only says
Refs #Nmerges, the hive treats the issue as partly done: it labels ithive/partially-addressed, comments that it is "moving this issue back to the actionable lane", and agents get it back with the merged PR as context (#7061). The contribute queue did not follow. It kept the issue asskip: issue already claimed by a PR β¦ merged=truefor as long as the merged-PR scan still listed the PR, so no contributor was offered the rest of the work. It also logged that line for every withheld issue on every pass.Cause
At 4ea3b0d:
contribute_admission.go:184withholds any issueissueClaimedByOpenPRreports.ClaimLedger.Lookupholds, including claims from merged PRs (Advisory: ~34% of contributor tasks already resolved on upstream main before dispatch (wasted full task cycle)Β #6867) of every grade.Refs #Ngot the treatment meant for an open one (π bug(contribute): issues with a non-closing (Refs #N) PR are re-offered every cooldown window, foreverΒ #3980), under the reasonopen_pr_claim. It is not a settled claim, so it never reachesmerged_claim_staleeither. The 72-hour scan window is the only thing that ends it.contribute_select.go:879logs the skip on everyselectTaskpass.The ledger's own comments, and
src/docs/contributor-relay.md, already say a merged weak claim "is still released with context". The agent side does that (FilterClaimedIssues); the contribute side was never changed. v5 has the same code.Seen live on the projectbluefin hive (fcf0e78), 21:20β21:32Z. Each of these was logged 30 times in 11 minutes (about every 22s), every one a merged PR with only
Refs/Part of, and every issue still open:Three open-PR claims were logged at the same rate. Those are correct holds; only the log repetition was wrong for them.
Change
Contribute admission
issueClaimedByOpenPRskips a claim that is bothMergedPRandReference, for either repo spelling. The issue is admitted as it is for agents.Refsor closing, still holds the issue;merged_claim_staleafter 7 days (π bug(prclaims): a merged claim expires after 72h on a clock, not on the PR vanishing β chairlift#55 (fixed by merged #133 on 09-17) has been re-offered every ~3 days since, with two 'already fixed, please close' agent comments on itΒ #8003);Claim ledger rank
The ledger keeps one claim per issue.
claimRanklet a merged hive-authoredRefs #N(tier 1, merged) outrank a person's openRefs #NPR (tier 0, open). With the admission change alone, that issue would have been offered while the person was still working it. A merged reference claim now ranks below every open claim. Everything else keeps its order:Refs;Refsstill outranks a merged external one.On the agent side, this means an issue with both a merged hive
RefsPR and a person's openRefsPR is now held for that open PR (the #3768 external-PR rule) instead of being released for verification.Skip log
The
[contribute-ws] skip:admission lines are logged when an item is first withheld and again only when the reason or its detail changes (claiming PR, merged flag, claimant, label, blockers). An item that is admitted and later withheld again is logged again. The memory is per item key, cleared on admission, and capped at 4096 entries. The Withheld panel still shows every withheld item on each read.Docs
src/docs/contributor-relay.mdsaid merged claims "suppress identically", which matched the old code but not the rest of that page. It now says a mergedRefs #Ngoes back to contributors and never hides an open PR.Testing
From
src/under/var/tmp,-count=1:go test ./pkg/dashboard/... ./pkg/github/... ./pkg/scheduler/...: allok(dashboard, collect, theme, webstatic, github, automerge, requestwatch, scheduler).go build ./pkg/... ./cmd/...succeeds.go vet ./pkg/dashboard ./pkg/githubis clean;gofmtis clean on the changed files.src/scripts/check-changelog-fragment.sh v6passes.New tests:
TestMergedReferenceClaim_ContributeAdmission/open Refs PRβ¦RefsPR still holds the issue.β¦/merged closing PR still holds the issueβ¦/merged Refs PR returns the issue to the queueRefsPR no longer holds it.β¦/cross-repo merged Refs PR returns the issue to the queueRefsPR in another repo.TestMergedReferenceClaim_DoesNotHideAnOpenPRRefsPR plus a person's openRefsPR on one issue, in both scan orders: still held.TestAdmissionSkipLoggedOnceTestClaimRankMergedReferenceBelowEveryOpenClaim(pkg/github)Each
pkg/dashboardadmission test wires a realClaimLedgerasIssueClaimedand checks thatReadyQueueandselectTaskagree.Mutation checks
Each part was broken in turn and restored from a backup:
Refscounted again: both merged-Refscases fail.claimRank: the open-PR test and the rank test fail.Not verified
hive/partially-addressedlabel and the hive's "moving this issue back" comment, but no structured "implement the remainder" context like the agent kick has. The contribute task message has no field for it today.Fixes #Non an issue that stayed open) still hold for the scan window and never reachmerged_claim_stale, becauseSettled()excludes external authors. That is a separate question and is not changed here.Contributor checklist
changelog.d/fixed-contribute-merged-refs-claim.md.v6.src/docs/contributor-relay.md).π€ Generated with Claude Code
https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1