Repository navigation
π fix(dashboard): merge gate follows live sentinel label and honors human decisions - #14579
Conversation
β¦uman decisions The tracker's sentinel pill was derived from the head's append-only review evidence bundle, so it stayed red after a maintainer removed sentinel-alert. It now follows the live label (a notice-only finding still raises it). Linked issues match repo names regardless of owner qualification and link to the fully-qualified repo. humanEvidence now carries the trusted approver and lane reason, and RouteHuman routes an approved manual-merge rule to 090 with an explicit 'click Merge' caption/comment instead of re-posting 085. Fixes #14578 Signed-off-by: Andy Anderson <andy@clubanderson.com>
|
@clubanderson is auto-promoted for contributor-lane merging on this PR: the last 5 merged PRs by this author in this repository have no revert or sentinel-alert evidence. CI, reviewer verdict, diff fences, soak, holds and branch protection still apply. |
|
[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
|
Signed-off-by: Andy Anderson <andy@clubanderson.com>
β¦name only Signed-off-by: Andy Anderson <andy@clubanderson.com>
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
Signed-off-by: Andy Anderson <andy@clubanderson.com>
There was a problem hiding this comment.
Read the three fixes against the tree at 17dd6f7 and the callers (automerge_stage_labels.go, sentinel_sweep.go, review_evidence.go). The routing and sentinel logic look correct to me; one low-severity, one-time migration wrinkle below that the body's "old bundles self-heal on deploy" line doesn't quite cover.
Verified
- A (sentinel pill) β
stage_tracker.go:600-629: alert now comes from the live label, the bundle only promotes it when a finding hasNotice. ThesentinelClearedByMaintainerbranch atsentinel_sweep.go:427-436returnsacted=false, so a cleared blocking finding is never re-recorded as notice-only β the case I was worried about doesn't exist. - A2 β
stageTrackerSameRepo(stage_tracker.go:1519-1539):hivevshivecommons/hiveβ same;a/hivevsb/hiveβ different.delegated_pr_context.go:29updated for the new signature. - B (human approval) β
humanEvidence(automerge_stage_human.go:106-108) only takesDetail.HumanApproverwhenHumanApprovalHead == obs.HeadSHA, consistent with howautomerge_stage_labels.go:285populates it.RouteHuman'smanualMergeApprovedrequireslen(decide)==0, so holds / fork-guard / unavailable-verdict still park at 085.automerge_stage_labels.go:769-770writesroute.LaneReason(nowβ¦:human-approved@login) intoobs.Reason;ReadyCommentreturns beforetrustCause's result is used, andHumanApprovedReasonsplits it back for the tracker caption.ReadyComment'sreason[:1]is guarded by the non-empty trim. - Body matches the diff throughout.
Low β pre-existing notice-only bundles get a duplicated finding on first sweep after deploy
review_evidence.go:475setsef.Notice = !blockedon every record, andcontainsSentinelFinding(review_evidence.go:509-516) dedupes withreflect.DeepEqual, which now includesNotice.- For a trusted author's PR whose notice comment is already posted and has no label,
trySentinelPRstill returnsevent, true(sentinel_sweep.go:460-474, thepostedcase skips the create but not the return), sosentinelEvaluatePRrecords again. Such PRs are normally skipped bysentinelHeads.isSeen(sentinel_sweep.go:294), but that map is in-memory and empty right after the restart this deploy causes. - Result: an old bundle
{Rule, Summary, Paths}plus a new{β¦, Notice:true}βSentinelFindingsdoubles and the tracker reason gains(+1 more)for the same finding. Alert does self-heal totrue, so only the count/tooltip is off, and only once per pre-existing bundle. - Cheap fix if you want it: have
containsSentinelFindingmatch onRule/Summary/Pathsand upgradeNoticein place, so the existing "same finding flagged again is not duplicated" test still holds across the flag flip.
Not blocking from my side; flagging so the doubled counts right after rollout aren't mistaken for new findings.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the routing change end-to-end (stagelabels/human.go, automerge_stage_human.go, sentinel_sweep.go, review_evidence.go, stage_tracker.go) and traced where the new ManualMerge / HumanApprover values go. The design holds: Route.ManualMerge is consumed only by stage labels, ReadyComment, and the dashboard (stage_tracker.go:1130, human.go:470/494); the automerge lane still decides on obs.Reason, so a trusted approval moves a workflow-path / tests-removed / human-merge-path PR from 085 to a manual-merge 090 without ever letting the hive merge it. That matches the body.
Also verified: the approver is only honoured when HumanApprovalHead == obs.HeadSHA (automerge_stage_human.go:107), so an approval on an older head does not settle a new one.
One low-severity, transitional finding β duplicated sentinel entries for pre-existing notice-only PRs.
review_evidence.go:475now stampsef.Notice = !blockedon every recorded finding.- For a trusted-author PR whose notice comment is already posted and has no label,
trySentinelPRstill returnsacted=trueeach sweep (sentinel_sweep.go:467-474), sorecordReviewEvidenceSentinelre-runs for heads that already have a bundle on disk. - The bundle merge de-dupes with
reflect.DeepEqual(review_evidence.go:332-336,:505-512). An existing entry written before this PR hasNotice=false; the new one hasNotice=true, so they are not equal and the bundle grows a second copy of the same finding. - Consequence:
stageTrackerSentinelcountslen(b.Sentinel)(stage_tracker.go:~607), so such PRs read(+1 more)in the tooltip until the head changes. Alert state itself is correct (theNotice=truecopy promotes it). Cheap fix if you want it: compareRule/Summary/PathsincontainsSentinelFindingand let a laterNoticewin, or treatNoticeas a per-bundle upgrade.
Everything else I checked looked correct to me. The schema addition (v1.json:191) is additive and optional, so older bundles still validate.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at 17dd6f7. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Second-round catch-up for #14340: the 7 v5 commits that landed after the main forward-merge (#14585 credit issue authors, #14583, #14579 merge gate follows live sentinel label / HumanApprover, #14580, #14571, #14427, #14428). Hand-resolved (16 files): - stagelabels/human.go: union of v6 Route.ManualMerge and v5 HumanApprover, ManualMergeRule, LaneReason. - config: v5 auto_merge.credit_issue_authors applied in review_config.go (v6 moved AutoMergeConfig there). - github/review_evidence*: v5 (blocked param, Notice = !blocked); sentinel_sweep.go keeps v6 adoption handling with the v5 one-line change. - automerge: linked-issue gate (v6 #14184 fallout) now seeds the v5 MergeCreditor's author cache so a merge-ready PR costs one issue GET; v5's lane-side decisionActions test dropped (v6 renders actions in stagelabels.DecisionActions). - dashboard/stage_tracker.go: v5 #14579 stageTrackerSentinel supersedes v6 #14053 (cleared label: alert off, findings still fill the tooltip). - dashboard/server.go: v5 FleetRun + v6 AdmissionLabels; index.html: v5 attention title + v6 "Hive end to end first" sort. - docs: v5 text with v6-only sentences re-applied; api-reference citations regenerated. Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Fixes #14578
Root causes
A β stale "Sentinel alert Β· needs human" pill (
src/pkg/dashboard/stage_tracker.go,stageTrackerSentinel): anysentinel_findingsin the head's verified review-evidence bundle returnedalert=trueunconditionally. The bundle never clears, so removingsentinel-alertcould not clear the pill. Webhooks/polling were healthy β labels in/api/stage-trackerwere live; only the derivation was wrong.A2 β "(title unavailable)" /
github.com/hive/issues/14567(stageTrackerPRLinkedIssues): GraphQL closing refs carryhivecommons/hive, the configured repo ishive; the mismatch dropped the ref and the fallback URL used the short name.B β needs-decision ignored the human decision:
automerge_stage_human.gohumanEvidencenever passedobs.Detail.HumanApprover(already noted for an APPROVED review on the head) tostagelabels.Evidence; andstagelabels/human.goRouteHumanalways produced 085 forCauseDecision && CauseReason != "", with no 090 route for a hard manual-merge rule (contributor-lane:workflow-pathetc.) a trusted merger had approved.Changes
stageTrackerSentinel: alert follows the live label; bundle supplies findings/reason; only a notice-only finding (SentinelFinding.Notice, new optional schema field set byrecordReviewEvidenceSentinel) raises the pill alone. Old bundles lacknoticeβ treated as blocking β rows self-heal on deploy.stageTrackerPRLinkedIssues(pr, repoName, baseURL)+stageTrackerSameRepo: name match independent of owner qualification; URL from the fully-qualified repo.stagelabels.Evidence{HumanApprover, ManualMergeRule, LaneReason};RouteHumanroutes an approved manual-merge rule toReadyForHumanMergewith reason<lane>:human-approved@login(HumanApprovedReasonparses it back);ReadyComment/decisionActionssay the only step left is Merge (or/lgtm+/approve). The workflow-path fence stays a hard rule β the hive still never merges.github/workflows/**on the contributor lane.human_approver,manual_merge; caption "Awaiting human merge (workflow path) β approved by @login; the hive never merges this itself: click Merge (or /lgtm + /approve for tide)"; JS 090 detour word "Awaiting human merge Β· approved by @login".Tests
Table-driven:
TestStageTrackerSentinelLabelClearsBlockingBundle,TestStageTrackerPRLinkedIssues(hive#14568 case),TestStageTrackerSameRepo,TestStageTrackerManualMergeApproved,TestRouteHumanApproved,TestHumanApprovedReason, extendedTestHumanizeStageReason,TestDecisionCommentNamesLaneAction, review-evidence notice case, node assertions for the 090 lane word. Notime.Sleep.