Repository navigation
π fix: preserve supersession decisions across stage reconciliation - #14862
hivecommons-hive[bot] wants to merge 8 commits into
Conversation
Keep a supersession cause label separate from CI and author repair state, retain escalation evidence in tracker audit replay, and expose the closer and keep-open reason even when a stale record still says 071/072. Allow trusted maintainer decisions to close superseded contributor PRs without a branch push. Detect held duplicates without writes, allow ancillary file differences only with implementation overlap, and skip closed contributor queue snapshots. Hive-Run: #14814 Hive-Plan: issue-14814-supersession-sweep-and-stage-tracker Hive-Spec: issue-14814#fleet-generic-fix Co-authored-by: clubanderson <407614+clubanderson@users.noreply.github.com> Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
Persist supersession park labels in the dashboard sweep fixture and assert the second pass makes no additional writes. Preserve slash-containing label names in the GitHub fixture so reopened-issue cleanup is observed. Signed-off-by: thatguywob <thatguywob@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 |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
|
Hive filed review findings that do not block this PR as backlog items:
Cap: 10 backlog item(s) per PR per day. |
There was a problem hiding this comment.
This head (30a71c9) is byte-identical to #14821's, which already carries two hive reviews at the same commit. Pointing the fix agent at them rather than restating:
-
CI red is the diff's own test fixture, one
caseline to fix.TestReviewQueue_SweepDetectionWithoutAutoClose(src/pkg/dashboard/api_review_queue_superseded_test.go:414,430) now expects the sweep to park, which drivesparkSupersededPRβsink.Parkedβstage_facts_sink.go:111 Issues.Get(repo1, 2); the fake server'sdefault:branch 404s it. Add acase "GET /repos/myorg/repo1/issues/2":returning{"number":2,"labels":[β¦]}. -
Bare
/hive decision rejecton a hive-authored PR is swallowed. Re-read at this SHA:src/pkg/github/pr_supersession_decision.go:22returnsok=trueforreject, sopr_unpark_decision.go:70-80runscloseSupersededPRDecision, replies "No merged PR currently supersedes β¦ the PR stays open" and returnstrueβ before theselfAuthoredswitch that previously turnedrejectinto a changes-requested kick. The doc comment onsupersessionCloseDecision("other text continues to the existing changes-requested path") does not describe this case. Either restrict the close path toclose superseded by #N, or fall through to the existing path when no closer is detected.
Everything else verified in the #14821 reviews (sticky-085 gating in derive_pr_classify.go:329, automerge_stage_labels.go:810,827; TrustedMerger gate; no flap between releaseStaleSupersessionPark and escalateSupersededPR) applies unchanged here. Once this branch is rebased and the two items above are addressed, #14821 can be closed as superseded by this one β I have not closed anything.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
This is #14821's head 30a71c9 byte-for-byte (same headRefOid), adopted onto hive/adopt-14821. The two reviews already on #14821 at this sha apply unchanged; summarising here so the fix agent working this branch has them in one place, and I re-read the tree at 30a71c9 to confirm.
1. CI red is this diff's own test fixture β one case line. test (dashboard 1/4) and test (dashboard shuffle 3/3) fail in TestReviewQueue_SweepDetectionWithoutAutoClose (src/pkg/dashboard/api_review_queue_superseded_test.go:370-460). The test now expects the sweep to park the contributor PR (labelWrites == 1, labels needs-human,hive/supersession-pending, line 458). Parking goes parkSupersededPR β AddLabels β sink.Parked(...) β src/pkg/github/stage_facts_sink.go:111 Issues.Get(repo1, 2). The fake server's switch (lines 379β413) has cases for issues/3, issues/2/comments, issues/2/labels, but no GET /repos/myorg/repo1/issues/2, so the default: branch at line 413 reports unexpected GitHub request and 404s. Adding a case "GET /repos/myorg/repo1/issues/2": returning {"number":2,"labels":[β¦]} should turn both shards green; coverage fails only downstream of them.
2. Low: a bare /hive decision reject on a non-superseded hive-authored PR is swallowed (src/pkg/github/pr_unpark_decision.go:70-80, pr_supersession_decision.go:19-22). The close-decision branch runs before the selfAuthored switch; when detectSupersession finds no live closer it replies "stays open" and returns true, so the comment is marked answered and never reaches the existing changes-requested kick. Fix agent: either fall through to the existing path when no supersession is detected, or make the reply say what will act.
Design otherwise read as matching #14814 Β§1β3, Β§5 (sticky 085 via derive_pr_classify.go:329, automerge_stage_labels.go:810,827; stale-park release only under hive/keep-open at pr_supersession_sweep.go:235-237; trusted-merger gate at pr_unpark_decision.go:50-52). Scope note carried from #14821: the worksource_identity.go closed-snapshot skip is not asked for by #14814.
I have not approved, merged, or changed anything.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Correctness review (head 30a71c9)
-
The PR's own modified test fails at head.
TestReviewQueue_SweepDetectionWithoutAutoClose(src/pkg/dashboard/api_review_queue_superseded_test.go:414,430) now hits the escalation path βpr_supersession_sweep.go:256βescalateSupersededPRβparkSupersededPRβsink.Parked(pr_supersession_resolve.go:153), which issuesGET /repos/myorg/repo1/issues/2. The fixture has no handler for that route, so the sweep errors withparking superseded PR repo1#2 ... 404and the test fails (CI jobstest (dashboard 4/4),test (dashboard shuffle 3/3),test). Add theissues/2route to the fixture. -
Tracker marks Refs-only PRs as "Superseded β decide".
stage_tracker.go:1509-1520overrides the caption/actor andindex.htmloffers thedecideaction for any PR with adetected-contributoraudit record. The sweep records that detection for every contributor PR (pr_supersession_sweep.go:221-226), includingcandidate.Closing == false(bareRefs/Part of/branch-name claims, seesupersessionCandidateClaims), which the sweep itself deliberately refuses to park (pr_supersession_sweep.go:247-253, #14155). Those rows will now read "Superseded by #N β decide" with a close-superseded action even though nothing supersedes them. Recordclosing=in the audit detail (cmd/hive/main.go:9533) and only surface the row when the claim is closing (or when anescalatedrecord exists). -
Advisory: the grace notice still says "Reply on this PR to keep it open" (
pr_supersession_autoclose.go:189), but a human reply (or a transientThreadsUnknown,:365) now escalates and parks at 085 with the sticky label (:393-395); theWaitGracebranch never releases that label afterwards. Worth either rewording the notice or releasing the park when the decision returns toWaitGrace. -
Advisory: bare
/hive decision rejecton a hive-authored PR is now intercepted before the changes-requested path (pr_unpark_decision.go:70-80); when no supersession is detected the maintainer gets "the PR stays open" and no change request is recorded, where previously the text was routed to the author agent.
β hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
β¦reject on hive PRs Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
hive/awaiting-ci: CI pending β sweep will merge when green. π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
β¦dable thread query A failed reviewThreads read is a transient API error, not a keep verdict. Escalating it sticky-parked the PR at stage/085 and asked a maintainer to decide something the next sweep would have answered itself. Comment only, as merge-base did, and let the next cycle re-read the threads. Hive-Fix-Attempt: 1/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: ci-maintainer <280983584+hivecommons-hive[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Delta re-review c47eff3β¦9d707b7 β every open finding from the previous rounds is now addressed; nothing new. Looks correct to me.
Checked each against the tree at 9d707b7:
- Refs-only PRs captioned "Superseded by #N β decide" β fixed.
src/pkg/github/pr_supersession_sweep.go:186and:226now gate bothSupersessionActionDetectedContributoraudits oncandidate.Closing, so aRefs #NPR is commented on but never reaches the tracker as a decision. Pinned byTestSweepSupersededOpenPRs_RefsOnlyContributorPRIsNotAuditedAsDetected(pr_supersession_sweep_test.go:458-490), which asserts one comment, zero labels, zero detected events. - Bare
/hive decision rejecton hive PRs β thecloser == 0sentinel is now commented atsrc/pkg/github/pr_unpark_decision.go:70-71,renderPRHelpReply(:430-436) no longer promises a close that barerejectwon't perform on a self-authored PR,src/docs/maintainer-commands.md:66-68says the same, andTestSweepIssueUnparkCommandsBareRejectOnHivePRIsChangeRequest(pr_unpark_decision_test.go:80-108) asserts the change-request path (no PATCH, store entry recorded, reply without "superseded"). - Grace-notice wording (
pr_supersession_autoclose.go:189) β now says a reply stops the countdown and a maintainer decides, which matches whatHumanActivityAfterNoticeactually does. - Sweep park fixture (
api_review_queue_superseded_test.go:367-477) β now accumulates labels and distinguishes the park write from the 085 stage write; thestageWrites > 1tolerance is explained in-line (shared stage store across tests).
New in 9d707b7: pr_supersession_autoclose.go:396 adds && !facts.ThreadsUnknown to the 085 escalation. Traced it: ThreadsUnknown is set only on the warn path at :365, DecideSupersessionAutoClose already returns keep for it (:116), and the fall-through uses ensureSupersessionComment, so a transient GraphQL failure now produces at most one idempotent comment instead of a sticky 085 park. The new Guards row (pr_supersession_autoclose_test.go:281-283) covers the closing-claim case specifically. That is the right call β a park asks a human to decide something the next sweep answers itself.
go build of pkg/github, pkg/dashboard, pkg/stageengine/..., cmd/hive passes at this head; CI test shards completed so far (scheduler, rest, hub) are green, the rest were still running when I looked.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Conclusion: the red test (rest 3/4) (and the coverage/test jobs that gate on it) is a self-inflicted test drift inside the adoption commits, not flakiness and not a defect in @thatguywob's original change. One targeted test edit clears it.
What is failing and why
be191cd deliberately changed trySweepSupersededPR so a non-closing claim is no longer audited as a detection:
src/pkg/github/pr_supersession_sweep.go:186ββ¦ && candidate.Closingon the held pathsrc/pkg/github/pr_supersession_sweep.go:226βif opts.Audit != nil && candidate.Closing {before thedetected-contributoraudit
That is consistent with the consumer: src/pkg/dashboard/api_review_queue_superseded.go:82 builds the "Superseded by #N β decide" row only from detected-contributor / grace / escalated audit actions, so a Refs #N-only PR no longer gets a close caption. Good.
But the two tests that fail both run on the shared fixture whose default body is Refs #3 (src/pkg/github/pr_supersession_autoclose_test.go:118-120), i.e. a non-closing claim, and still assert events[0].Action == "detected-contributor":
src/pkg/github/pr_supersession_sweep_test.go:774-799TestSweepSupersededOpenPRs_AuditsDetectionWithoutAutoCloseβ CI:missing detection: []/missing detection: [{β¦ Action:commented-contributor}]src/pkg/github/pr_supersession_sweep_test.go:801-812TestSweepSupersededOpenPRs_KeptOpenDetectionβ CI:events=[{Action:kept-open} {Action:commented-contributor}]
Under the new rule those PRs are correctly not detected; the tests encode the pre-be191cd expectation and were not updated with it. The second commit's own message says "Refs-only contributor PRs are never superseded, so they no longer get the detected-contributor audit" β the tests just need to agree.
Suggested fix (for the hive fix agent on this branch)
Note that simply switching the fixture to Fixes #3 does not work for the first test: a closing claim with no blocking reason takes the escalateSupersededPR path at pr_supersession_sweep.go:259, which parks/labels, and the test asserts len(obs.labels[12]) == 0. So:
AuditsDetectionWithoutAutoClose: invert the assertion for theRefs #3fixture β expect nodetected-contributorevent and no label, keep the "existing notice β no new comment/edit" check. That turns it into the regression test for the behaviourbe191cdintroduced, which currently has none.KeptOpenDetection: either drop theevents[0].Action == "detected-contributor"expectation (keepkept-open), or setBody: "Fixes #3"β withSupersessionKeepOpenLabelpresentreasonbecomessupersessionReasonKeptOpen, so the:259escalation is skipped and[detected-contributor, kept-open]is what the sweep emits.
Also checked
#14823(merged, also "Fixes #14814") touches a fully disjoint file set (webhook PRβissue claims); this PR is not redundant against it, and the body'sRefs #14814 (closed by #14823 β¦)+hive:supersession-resolvedmarker already handles the issue-link.9d707b7(unreadable reviewThreads β comment only, no 085 park) reads correctly againstpr_supersession_autoclose.go; it restores merge-base behaviour.- The
pr_unpark_decision.go:70guard (bare/hive decision rejecton a hive-authored PR stays a change request) matches the updated help text at:430-435andsrc/docs/maintainer-commands.md.
I have not approved or changed anything; this is advisory.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
β¦on candidates Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
hive/awaiting-ci: pushed 00ab642 β merged v5 and restored v5's detected-contributor audit for every supersession candidate (dropped the Closing-only gating and its Refs-only test that contradicted v5's sweep tests). CI pending β sweep will merge when green. π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Reviewed (correctness, delta 9d707b7β00ab642) β previous finding resolved; no in-scope blockers.
- Fixed: "Sweep tests not updated for Closing-gated detection audit". Commit
00ab642resolves it by removing thecandidate.Closinggate on bothdetected-contributoraudits (src/pkg/github/pr_supersession_sweep.go:186,:224) rather than rewriting the tests, and deletes the test that pinned the gate (TestSweepSupersededOpenPRs_RefsOnlyContributorPRIsNotAuditedAsDetected).AuditsDetectionWithoutAutoClose(pr_supersession_sweep_test.go:740) andKeptOpenDetection(:767) now agree with the code again, and alltest (β¦)shards pass at this head. This matches base v5 (pr_supersession_sweep.go:207atdec61e4faudits every detection), so it is a restoration, not a regression. - The remaining 12 delta files are the
origin/v5merge (#14864β#14866), already on base; not re-reviewed.
Out-of-scope note (pre-existing, not introduced here): with ungated detection, a Refs #N-only contributor PR is still captioned "close as superseded or keep open" in the review queue (src/pkg/dashboard/api_review_queue_superseded.go:174, same text on base at :170), while the new /hive decision close superseded by #N correctly refuses it (src/pkg/github/pr_supersession_decision.go:43, !candidate.Closing). Worth a follow-up to caption Refs-only detections differently.
β hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Decision needed: @clubanderson should decide whether the supersession-stage work for #14814 (already closed by #14823 via a different mechanism) should still land, and if so whether this adopted branch or the contributor's #14821 (which has since moved to beb4233) is the one to keep.
Code looks correct to me; the open question is scope and which branch survives, not the diff.
Read against the tree at the merge-base (dec61e4f0) plus the callers:
- The new close path is behind the existing trust gate:
tryUnparkPullRequestreturns before it unlessTrustedMerger(actor)holds (src/pkg/github/pr_unpark_decision.go:49), andcloseSupersededPRDecisiononly closes afterdetectSupersessionre-confirms a live closing claim (pr_supersession_decision.go:43) and the explicit#Nmatches that closer (:46). Held/exempt labels still block it. No contributor branch is pushed. - Behaviour change worth a maintainer's eye: every contributor PR with a closing claim that the sweep cannot auto-close now parks at 085 with
needs-human+hive/supersession-pending(pr_supersession_sweep.go:256,pr_supersession_autoclose.go:396), where before it only got a comment. That matches the stated goal; it will also raise the 085 count on repos that haveCloseContributorPRsoff. ClassifyStageshort-circuits to 085 on the new label (derive_pr_classify.go:329), and both the engine andApplyStageLabelsskiprouteHuman/DCO for that reason, so CI red or a stale head no longer flips the row to 071/072. The park is cleared when detection stops (pr_supersession_resolve.go,releaseStaleSupersessionPark).- Audit
reason=is the last field and none of the keep reasons contain", ", soparseSupersessionAuditDetailreads it intact. src/docs/maintainer-commands.mdand the changelog fragment describe exactly what the code does.
Two things a human needs to settle:
- #14814 is already closed by #14823 (claims from
pull_requestwebhooks). This PR fixes the downstream presentation of superseded PRs β related, but a different mechanism and 620 lines. The body correctly saysRefs, notCloses; a maintainer should confirm this scope is still wanted. - This is no longer a faithful adoption of #14821. The body says it is #14821's head
30a71c9funchanged; it now carries five hive fix-up commits (c47eff346β¦00ab642ab), and meanwhileci-maintainerpushedbeb4233to the fork so #14821 diverged too.gh api compareshows the two heads differ inpr_supersession_decision.go,pr_unpark_decision.go, their tests and the docs β the same "bare reject" follow-up fixed two ways. They conflict rather than duplicate: only one can merge, and the other should be closed by a person. I have not closed anything.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Needs a decision, not a merge:
Head: verdict requires_human @00ab642, 64/65 checks green, 1 pending, DCO yes, soak 0s of 30m0s. What to do: approve this head ( How to release it:
Clear the cause and the hive re-evaluates on its next pass; the stage moves to |
|
Closed as superseded by #14821 by @clubanderson from the Hive dashboard. |
Supersedes #14821 (adopted after CI went red and the contributor was unavailable; original commits by @thatguywob preserved).
Refs #14814 (closed by #14823; this PR adds
changelog.d/fixed-14814-supersession-stage.md,src/cmd/hive/main.go,src/docs/maintainer-commands.md,src/pkg/dashboard/api_review_queue_superseded.go,src/pkg/dashboard/api_review_queue_superseded_test.goand 20 more)This branch is #14821's head
30a71c9f7d4eas pushed by @thatguywob (forkthatguywob/hive), unchanged. Last lane reason on the original:reviewer-verdict:changes_requested.Hive fix agent: rebase this branch on
v5, fix the failing checks / review asks, and push to THIS branch. Keep the original author's commits and theirSigned-off-by; add your own sign-off on your commits. Do not open another PR.