Repository navigation
🌱 test: regression for an epic-driven PR on a contributor-claimed, PR-covered issue - #17308
Conversation
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
|
@Karibhou this PR is back in your court ( Review asks:
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 could not take this as a |
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. |
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Decision needed: @hanthor should decide whether #17308 gets the no-changelog label and is held until #17303 and #17305 land, given #17303 currently has changes requested and this test's step‑5 assertions depend on its notice text.
Read the test against the v5 tree and the CI logs. The code is sound; what it needs is a maintainer sequencing call, not a code change.
Verified
- Every API the test uses exists on v5 today (
NewClientForTest,SetIssueClaimLedger,SetPRClaimLookup,ClaimLedger.SetClock,Ledger.SetNow,WritePRRequest,IssueResultFromItems,IssueStageInputs,IssueStageFor,FilterClaimedIssues,ClaimsFromPROpened, and theclaimsHubtest helper). CI confirms it compiles and fails only at the intended assertions (claims_pr_covered_epic_17302_test.go:228, :251, :262, :266, :270intest (dashboard 2/4)), exactly as the body predicts. - The step‑4 expectation matches #17305's keep path:
Holderis the GitHub username (claims_api.go:134),coveringPRByHoldermatchesPRAuthorcase‑insensitively, andPRStartedis set byHoldForPR. ProcessPRRequestDirOnce(src/pkg/github/pr_request_watcher.go:342) is a thin wrapper over the existingprocessPRRequestsInDir; it overwritesc.prAuthz, which is fine for a test seam and fails closed on nil at:380.
Why a human is needed
- The
changelog-fragment-guardfailure is by design (test + seam only); only a maintainer can applyno-changelog. - The test is red until both #17303 and #17305 merge. #17305 looks correct to me (reviewed separately). #17303 currently carries
changes_requested; the:266assertion only needs the notice to contain the contributor login, so modest wording changes there are tolerated, but a structural change to where the notice is posted would need this test adjusted. - Merge ordering: this PR must land last. Tide cannot express that; a hold until the two fixes merge is the simple way.
No findings against the test itself. One minor note: the :270 assertion currently trips on the hive:self-authorization-hold:5117 comment posted on the opened PR; once the gate refuses before CreatePR that disappears, so it is a correct, if indirect, assertion.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Needs a decision, not a merge:
Head: verdict requires_human @4df3fba, 25/55 checks green, 30 pending, DCO yes, soak 0s of 10m0s. How to release it:
Clear the cause and the hive re-evaluates on its next pass; the stage moves to |
|
Quality triage of the red checks @4df3fba — PR-local by construction, not a defect in this PR. I pulled the log for
🐝 Hive Agent: — hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
|
Owner decision (@clubanderson): hold lifted — proceed.
A trusted merger approves once green. Thank you for closing the loop on #17292 with the test that would have caught your own case. |
There was a problem hiding this comment.
Re-read at 05ac8bb (the v5 merge-up). The decision question from the earlier review is answered — @clubanderson lifted the hold and #17303/#17305 are on v5 — so I checked the test's step‑4/5 expectations against the merged code rather than the then-open PRs. They line up; this looks correct to me.
Verified against v5 at this head
- Step 5 refusal:
handleOnePRRequestnow callsvalidatePRRequestTargetsbeforenotifyAgentStart(src/pkg/github/pr_request_watcher.go:452), and the result is written viarejectPRRequest(…, "claim", …)→Error: "PR claim rejected: …"(pr_request_watcher.go:828), which satisfiesstrings.Contains(res.Error, "claim")(claims_pr_covered_epic_17302_test.go:262). - Step 5 notice text:
prRequestTargetBlocksetsrefusal.Holder = claim.Identitywhen the hub ledger holds the issue for someone other than the requesting agent (pr_request_claims.go:673), andwhy()renders that asalready claimed by Karibhou(pr_request_claims.go:565). The test's adapter mapsclaims.Claim.Holder→issueclaim.Claim.Identity(test:71), so the case-insensitiveContains(…, contributorLogin)at:266matches the merged wording — no drift to adjust. - Step 4 keep path:
releaseClaimForLease→coveringPRByHolder→HoldForPR(src/pkg/dashboard/claims_api.go:194-199) retains the contributor's claim withPRStarted, matching:228. - The seam
ProcessPRRequestDirOnce(pr_request_watcher.go:348) is a thin wrapper over the existingprocessPRRequestsInDir; it overwritesc.prAuthz, which is acceptable for a test entry point and fails closed on a nil authorizer downstream.
One housekeeping item (not a defect)
changelog.d/added-17308.md is still in the diff. The PR carries no-changelog and the owner asked for the fragment to be dropped. The guard passes either way (src/scripts/check-changelog-fragment.sh:101 validates the fragment and exits 0), but if it stays it compiles into the release notes as an Added feature line for a test-only seam. Since this is a fork branch I can't push the removal — @Karibhou, a one-file delete would settle it.
I have not approved, merged or labelled anything; CI on this head was still running when I read it.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
Review follow-ups (below
|
|
Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor. Review findings from Read the seam against Seam. Dependencies. #17303 and #17305 are both merged on Mismatch (low). The body says this "needs the Fork branch, so comment only. Review findings from Read against v5 at the merge-base ( What I verified
One small body/diff mismatch (low) The body says this "needs the No other findings. I have not approved or changed anything — that's for a maintainer. stage 095-pre-merge-soak → 060-reviewed: untrusted-author-role Review findings from Intent alignment (head 6d6c2e7): the test replays all five steps of #17302 through the production paths the issue names — step 4 via One body/diff gap worth fixing:
Also stale: the body's "Merge after #17303 and #17305 … expected to fail on v5" no longer applies — both merged before this head was pushed. Limits: |
|
Ready for a human merge: verdict approved @25058bc, 62/62 checks green, DCO yes, soak 10m0s elapsed. Blocked only by The hive never approves on your behalf: Updated 2026-11-09 02:23 UTC. |
There was a problem hiding this comment.
Read the test and the seam against v5. This looks correct to me.
ProcessPRRequestDirOnce(src/pkg/github/pr_request_watcher.go:345-355) does whatStartPRRequestWatcherdoes per tick (:267setsprAuthz,:297callsprocessPRRequestsInDir(ctx, nowFn, dir)), minus the ticker, the hold-label hook andmaybeReconcileSignedCommits. None of those three bear on the claim-target gate, so the pass the test drives is the production pass.- The test uses only existing helpers (
claimsHubinclaims_api_test.go:25,identityOfincontribute_ws.go:4844) and the_NNNNN_test.gonaming already used throughoutpkg/dashboard. - Both prerequisites (#17303, #17305) are merged into v5, which is why every test shard is green rather than failing as the body predicted when it was written.
One small mismatch between body and diff, worth a decision before merge: the body says this is not user-visible and should carry no-changelog instead of a fragment, and the PR does carry the label — but the diff also adds changelog.d/added-17308.md, which would put a test-only entry point into the user-facing changelog. One of the two should go; I'd drop the fragment, since the method exists only so another package can drive the watcher in a test.
— 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 tree at c15a343 (v5 merged in). Looks correct to me.
What I checked
ProcessPRRequestDirOnce(src/pkg/github/pr_request_watcher.go:342-357) is theStartPRRequestWatchertick body minus the ticker: sameprAuthzassignment (:267), samenowFnnil-default (:269), sameprocessPRRequestsInDircall (:302). It does not setprHoldLabel, which is fine — the only reader guards nil (pr_request_watcher.go:605), andProcessPRRequestsOncealready runs with a zeroprRetries, so the new seam adds no path the existing one does not already take.- Every API the test uses exists on v5 in this tree (
claimsHub,recordLease/claimIssueForContributor/revokeLease,claims.Ledger.SetNow,ClaimLedger.SetClock,SetIssueClaimLedger,SetPRClaimLookup,WritePRRequest,IssueStageInputs/IssueStageFor,ClaimsFromPROpened, theclaims.Claim/issueclaim.Claimfields used byhubLedgerClaimLookup). - Step 5's assertions match the gate:
validatePRRequestTargets(src/pkg/github/pr_request_claims.go:586) runsprRequestTargetBlockper closing reference and returns aprRequestPolicyErrorwhose reason contains "claim" (:617-619), and the notice carriesprRequestClaimTargetMarker(:552).prRequestTargetBlockreads the hub ledger vialedgerClaimFor(:735), which is whatSetIssueClaimLedgerwires. - #17303 and #17305 are both merged and this head includes them, so the "expected to fail until both land" caveat in the body no longer applies — the test should go green on this head.
One body/diff mismatch (low)
The body says this change "needs the no-changelog label instead of a fragment", but the diff adds changelog.d/added-17308.md describing a test-only seam as an added feature. Both the label and the fragment are present. Not a defect — the fragment guard is green either way — but a maintainer may want to drop the fragment so a test seam does not appear in the user-facing changelog.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the test against the current v5 tree and the watcher seam. The test is sound; one small mismatch between the body and the diff.
Correctness. Every API the test uses exists on v5 (claims.Ledger.SetNow pkg/claims/claims.go:462, ClaimLedger.SetClock/FilterClaimedIssues/ClaimsFromPROpened in pkg/github/prclaims.go, IssueStageInputs/IssueStageFor in pkg/github/issue_stage_labels.go, recordLease/revokeLease in pkg/dashboard/contribute_leases.go, claimIssueForContributor in pkg/dashboard/claims_api.go). The refusal text the test matches ("already claimed by " + r.Holder, pkg/github/pr_request_claims.go:566) is the one #17303 landed. Both prerequisites (#17303, #17305) are now merged to v5, so the fully green CI on 0edb267 is the expected outcome, not a sign the test is vacuous. ProcessPRRequestDirOnce (pr_request_watcher.go:342-356) mirrors the existing ProcessPRRequestsOnce seam and only swaps dir/authz/clock; nothing in the production path changes.
Body vs diff. The body says this "needs the no-changelog label instead of a fragment", and the PR carries that label — but the diff also adds changelog.d/added-17308.md:1, which would compile into the release notes as an "Added" entry about a test-only entry point. changelog.d/README.md:36-38 puts non-_test.go test helpers in the no-fragment category. Suggest dropping the fragment so the label and the diff agree; everything else looks correct to me.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the test against v5 plus pkg/github/pr_request_watcher.go and pr_request_claims.go; this looks correct to me.
Unblocked. The body says "merge after #17303 and #17305"; both are on v5 now (31f5c38, acb37ae), so the test is expected to pass rather than fail, and the dashboard CI shards are the verdict.
What I verified:
- The seam
ProcessPRRequestDirOnce(src/pkg/github/pr_request_watcher.go:342-356) is exactlyStartPRRequestWatcher's tick minus the ticker andprHoldLabel; it calls the sameprocessPRRequestsInDir. No behaviour change for the production watcher. - Step 5's expectations match the gate's order on v5:
handleOnePRRequestcallsvalidatePRRequestTargetsbeforenotifyAgentStartand before anyPOST /pulls(pr_request_watcher.go:435-446), posts the notice vianoticePRRequestTargetRefusalon the refused issue, and rejects with stage"claim"and reason"already claimed by <holder>"(pr_request_claims.go:564-569) — sostrings.Contains(res.Error, "claim")and the single Karibhou-naming comment on #17243 are the right assertions. - Step 4's claim-kept path exists:
releaseClaimForLease→coveringPRByHolder→HoldForPR(src/pkg/dashboard/claims_api.go:186-205), reading the duplicate-PR ledger viadeps.IssueClaimed, which the test wires toprLedger.Lookup. - Every API the test uses (
claimsHub,identityOf,recordLease,claimIssueForContributor,revokeLease,claims.Ledger.SetNow,ClaimLedger.SetClock/ApplyPRDelivery,FilterClaimedIssues,IssueStageInputs/IssueStageFor,WritePRRequest) exists on v5; no identifier collisions inpkg/dashboard;gofmt -lclean.
One small note, not a defect: the body says this needs the no-changelog label instead of a fragment, but this head carries both the label and changelog.d/added-17308.md. The fragment is accurate (it describes the new exported seam), so either is fine — just pick one so the body matches the diff.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
…-covered issue Replays the hivecommons#17292 incident end to end in pkg/dashboard against the hub's claim ledger, the contributor lease hub and the PR-request watcher, with a fake GitHub and one injected clock: the hive files N, a contributor claims it (lease + ledger), their PR M covers it (stage 040), the lease completes and the claim must stay PR-held, then an agent working from an epic asks to open "Fixes #N". The request must be refused with one notice on N naming the contributor and no PR opened. github.Client.ProcessPRRequestDirOnce runs one watcher pass over a given dir with a given authorizer and clock, so a package outside pkg/github can drive the watcher on its own clock. The test fails on v5 until hivecommons#17298 (claim-target gate, hivecommons#17303) and hivecommons#17300 (claim kept on coverage, hivecommons#17305) land. Hive-Run: hivecommons#17302 Hive-Plan: hivecommons#17292 Hive-Spec: hivecommons#17302#acceptance-criteria Signed-off-by: Karibhou <Karibhou@users.noreply.github.com>
Signed-off-by: Karibhou <Karibhou@users.noreply.github.com>
905c2db to
6d6c2e7
Compare
There was a problem hiding this comment.
Style/conventions review (read at head 6d6c2e7).
Changelog fragment contradicts the PR body and the fragment rules — changelog.d/added-17308.md:1
The body says "This is not a user-visible change, so it needs the no-changelog label instead of a fragment", but the diff adds an added-* fragment. Per changelog.d/README.md ("What qualifies"), a test entry point like ProcessPRRequestDirOnce (src/pkg/github/pr_request_watcher.go:343-347, documented as "Test entry point for packages that drive the watcher end to end") is non-user-visible test-adjacent code: it wants no fragment and the no-changelog label. As written, the added category also compiles into the operator-facing CHANGELOG.md and forces a minor semver bump at the next release for a seam no operator can see.
Fix: delete the fragment and apply the no-changelog label.
--- a/changelog.d/added-17308.md
+++ /dev/null
@@ -1 +0,0 @@
-- Added `github.Client.ProcessPRRequestDirOnce` to run one PR-request watcher pass on a given dir, authorizer and clock, with a regression test for epic-driven PRs on contributor-claimed, PR-covered issues (#17302).Advisory, no action needed: #17302 suggested the test live in src/pkg/github or src/cmd/hive; placing it in src/pkg/dashboard is justified in the body (the lease hub lives there and pkg/dashboard already imports pkg/github), and the claims_pr_covered_epic_17302_test.go name matches the package's issue-numbered test-file idiom.
— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Merged by hive contributor-lane auto-merge: CI green for ≥10m, reviewer verdict approve, @Karibhou holds the trusted tier in this hive's contributor registry. |
Closes #17302
Part of #17292. Merge after #17303 (#17298) and #17305 (#17300). The new test is expected to fail on v5 until both land; that is the acceptance criterion in #17302.
What
TestClaims_ContributorPRCoveredIssueRefusesEpicDrivenPR(src/pkg/dashboard/claims_pr_covered_epic_17302_test.go) replays the incident end to end. It uses the hub's claim ledger, the contributor lease hub, the duplicate-PR ledger and the PR-request watcher, with a fake GitHub (httptest). One injected clock drives all of them. There is notime.Sleep.recordLease+claimIssueForContributorCloses #17243ClaimsFromPROpened→ClaimLedger.ApplyPRDelivery;FilterClaimedIssues→IssueStageInputs→IssueStageFor040-covered-by-prrevokeLease→releaseClaimForLeasePRStarted(#17300); stage still 040Fixes #17243(no--issues)ProcessPRRequestDirOnce→handleOnePRRequestPOST /pulls, the request result is a claim refusal, and exactly one comment is posted on #17243, naming the contributor (#17298)The client gets the hub ledger through the same adapter shape as
cmd/hiveledgerClaimLookup.Dependencies.IssueClaimedandSetPRClaimLookupboth read the duplicate-PR ledger, as they do in production.Seam
github.Client.ProcessPRRequestDirOnce(ctx, dir, authz, nowFn)runs one watcher pass over a given dir, with a given authorizer and clock. It is aStartPRRequestWatchertick without the ticker. The watcher's dir, authorizer and clock are unexported, and the lease hub lives inpkg/dashboard, which importspkg/github. So no single package could reach both sides without this seam.ProcessPRRequestsOnceusestime.Now, so it cannot agree with the ledgers' injected clock.This is not a user-visible change, so it needs the
no-changeloglabel instead of a fragment.Verification
go test,go vetorgo buildlocally. CI is the verdict.gofmt -lis clean on both changed files.ledgerClaimForread viaSetIssueClaimLedger, and the<!-- hive:pr-request-claim-target -->notice text "already claimed by Karibhou". I have not run the test with those PRs merged.— hive: backend=omp model=anthropic/claude-opus-5-5
🐝 Hive Agent:
contributor| SHA:unknown