Repository navigation
π dashboard: Let hive decide preview is one clear decision, not a wall of text - #15399
Conversation
β¦l of text The delegated-decision preview showed the repo#number twice, composed 'Hive would:' twice (the plan API's summary already carried the prefix and stageTrackerDelegatePlanText prepended it again), inlined the consequences into a paragraph, drowned the decision under a rails footer, and offered 'Approve & merge as-is' for a head the preview itself called not mergeable. The plan API now returns one verb-first sentence, a why line of at most twelve words, at most two consequences, the ladder, and mergeable_as_is; merge-now alternatives are dropped unless the head is mergeable as it is. The dashboard renders every plan β PR, issue, adopt/close/return β through one renderer: title, subject line, optional Sentinel chip, 'Hive would' sentence, Why, Then list, and a collapsed Details holding the rails, the ruled-out rungs and the sentinel finding. Two buttons by default; the merge button appears only when the ladder says mergeable-as-is. Fixes #15397 Signed-off-by: Andrew Anderson <andy@clubanderson.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 |
|
@clubanderson is auto-promoted for contributor-lane merging on this PR: the last 5 merged PRs by this author in this hive's repositories have no revert or sentinel-alert evidence. CI, reviewer verdict, diff fences, soak, holds and branch protection still apply. |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
|
stage 080-awaiting-merge-admission β 050-review-pending: required-check-pending:build-and-test |
There was a problem hiding this comment.
Correctness review (head 499bb51)
1. Direction sentence garbles / drops the reviewer's question (medium, in-scope) β src/pkg/dashboard/api_repo_item_decide_delegated_plan.go:233-252 (delegatedPlanInstruction). When the PR has a fix list and in.Question is set, the function falls back to delegatedPlanPlain(d.Text). For a person-authored PR decideReturnToAuthor builds d.Text as "@author: make CI green without widening scope on #N; then it merges β @author, the reviewer asked: <question>" (src/pkg/delegatedecision/policy.go:613-626), and delegatedPlanPlain cuts at the first "; " (plan.go:297-300). Result: "Send #N back to its author with one instruction: @author: make CI green without widening scope on #N." β the reviewer's question (the only reason this branch exists) is lost, the @author: prefix is duplicated into the sentence, and the "one instruction" wording is wrong. No test covers Question != "". Suggested fix:
func delegatedPlanInstruction(in delegatedecision.Input, d delegatedecision.Decision) string {
var fix []string
if in.Conflicting {
fix = append(fix, "rebase onto the base branch")
}
if in.DCOMissing {
fix = append(fix, "add the sign-off")
}
if !in.CIGreen {
fix = append(fix, "make CI green")
}
- if len(fix) == 0 || strings.TrimSpace(in.Question) != "" {
+ question := strings.TrimSuffix(strings.TrimSpace(in.Question), ".")
+ if len(fix) == 0 && question == "" {
if text := delegatedPlanPlain(d.Text); text != "" {
return text
}
+ return "address the review"
}
if len(fix) == 0 {
- return "address the review"
+ return "answer the reviewer: " + question
}
+ if question != "" {
+ return strings.Join(fix, ", ") + ", and answer the reviewer: " + question
+ }
return strings.Join(fix, ", ") + ", nothing else"
}plus a case in TestDelegatedPlanWordingDirectionIsOneSentence with OwnerAuthor: false, TrustedAuthor: true, Question: "β¦".
2. mergeable_as_is fallback ignores policy holds the ladder would have ruled out (low, in-scope, advisory) β plan.go:306-315: when the ladder does not land (decideLadder returns ok=false for ReporterTrustHold/WorkflowPath, ladder.go:198-200), d.Ladder is nil and the fallback is head facts only, so a green head under a reporter-trust hold still gets mergeable_as_is: true and the merge button, even though rung 2 had skipped mergeable-as-is with "reporter-trust hold" (ladder.go:183, mergeBlockers ladder.go:288-293). If an as-is merge is meant as a deliberate maintainer override there, fine β but then the Details list won't show the ruled-out rung either. Consider && !in.ReporterTrustHold && !in.AdoptionStalled in the fallback to match rung 2.
Otherwise verified: in-place filter of plan.Alternatives is safe (options are freshly built per call, delegated_decision_human_merge.go:88); merge-now disabled reason merger role required is always present (:48,58); JS OffersMerge/Mergeable gating and the #15332 duplicate-prefix normalisation match the Go output.
β hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the Go and JS against the tree; this looks correct to me.
- The doubled prefix is fixed at both ends:
delegatedPlanWordingno longer emitsHive would:(api_repo_item_decide_delegated_plan.go:185-230) andstageTrackerPlanSentencestrips any stale(hive would:\s*)+from an older server (tracker-actions.js:1216), so a mixed-version spoke still renders one eyebrow. - The API and the dialog cannot disagree on the merge button: the server drops
decide:merge-nowunlessMergeableAsIs(:156-166), andstageTrackerDelegatePlanOffersMergeadditionally requiresmergeable_as_is === true(tracker-actions.js:1300). A non-merger still gets the option withenabled=falsefromdelegatedHumanMergeOptions(delegated_decision_human_merge.go:48), so the greyed-with-reason path in the body is real. - The 090 shortcut (
delegatedHumanMergeOutcome) builds a Decision with no Ladder, soMergeableAsIsfalls back to head facts there β a green 090 head keeps the as-is button, a red one loses it. Sensible. stageTrackerDelegatePlanTexthas no remaining callers outside the regeneratedindex.html;static-srcandstatic/index.htmlare in sync (CItest (rest)green).
One open question, not a defect: with a Ladder present, mergeable_as_is is d.Ladder.Decision == RuleMergeableAsIs (:313). Rung 2 is skipped for a green, conflict-free, signed head when ReporterTrustHold or AdoptionStalled is set (delegatedecision/ladder.go:183), and the ladder then lands on rung 4/5 with a Ladder attached β so a merger loses Approve & merge as-is on a head that is technically mergeable, where the old dialog offered it. If that narrowing is intended (the ladder's judgement outranks the head facts), fine; if not, the fallback at :315 could also apply when the ladder's RuledOut entry for mergeable-as-is names only a trust/stall hold.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the Go change against v5 and ran the generator check on 499bb51; looks correct to me.
- Alternatives filter (
api_repo_item_decide_delegated_plan.go, thekept := plan.Alternatives[:0]loop):decide:merge-nowis dropped only whenMergeableAsIsis false, anddelegatedPlanMergeableAsIsdefers to the ladder rung when a ladder ran and to!Conflicting && CIGreen && !DCOMissingotherwise. Both branches are asserted (TestDelegatedDecisionPlanMergeableAsIsOffersMergeoffers it; the contributor-offline case asserts it is absent). The 090 shortcut path still gets its own alternatives first and then goes through the same filter, so the two paths cannot disagree β which is the point of the change. - Why/Direction wording:
delegatedPlanWhynow suppresses the "untrusted / clanker offline" clauses forVerdictDirectionand usesdelegatedPlanBlockersinstead, so a send-back no longer explains itself with trust facts that did not decide it. Consistent with thedelegatedPlanInstructionfix list. - Source layout:
static-src/js/tracker-actions.js,static-src/css/tracker.cssand the regeneratedstatic/index.htmlall move together;go run ./pkg/dashboard/webstatic/gen β¦ -checkexits 0 on this head, soTestIndexHTMLIsGeneratedwill pass.
One open question, not a defect: delegatedPlanPlain truncates at the first "; " and strips any (β¦#N) parenthetical. A rule rationale whose first clause is the less informative half would lose the useful part in the preview; the full text still rides ladder under Details, so this only matters if a rationale is written clause-second. Worth keeping in mind when adding rules.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Fixes #15397
Operator feedback on the "Let hive decide β preview" dialog (seen on #15332): "yikes - this is a mess. different fonts, repeated wording, placement of text and buttons, explanation is confusing" β "this needs to be exceedingly simple to understand and act on."
What changed
Plan API (
GET β¦/decide/delegated/plan,api_repo_item_decide_delegated_plan.go)summaryis one verb-first sentence with noHive would:prefix (the prefix was composed on both sides β Go put it insummary, JS prepended it again β "Hive would: Hive would:").whyis one plain line (β€ 12 words);consequencesβ€ 2.title,ladder(π Let hive decide: close already-fixed PRs instead of adopting themΒ #15301),mergeable_as_is. Thedecide:merge-nowalternative is dropped unless the head is mergeable as it is (ladder rungmergeable-as-is, or head facts when the rule had no ladder), so the API and the UI can't disagree.Dashboard (
static-src/js/tracker-actions.js,static-src/css/tracker.css, regeneratedstatic/index.html)stageTrackerDelegatePlanViewβstageTrackerDelegatePlanHtml) for PR, issue, batch and the adopt / close / return variants: title Β· subject line (repo#N Β· titleβ¦) Β· optionalβ Sentinelchip Β· Hive would sentence Β· Why Β· Then list Β· collapsed Details (rails, "Why not the others" ruled-out rungs, sentinel finding).CancelΒ·Let hive do thatβ right-aligned with the.hive-dialog-btnfamily;Approve & merge as-isonly appears whenmergeable_as_is(greyed with a reason if the caller lacks the merger role).Tests
TestDelegatedPlanWordingDirectionIsOneSentence(π± ci: run crux's Go file-size ratchet against src/crux on hive PRsΒ #15332 input β "Send π± ci: run crux's Go file-size ratchet against src/crux on hive PRsΒ #15332 back to its author with one instruction: make CI green, nothing else.", why "CI is red.", no jargon),TestDelegatedDecisionPlanMergeableAsIsOffersMerge, updated adopt/090 cases.TestStageTrackerDelegatePlanDialogIsOneDecisionβ π± ci: run crux's Go file-size ratchet against src/crux on hive PRsΒ #15332 case has no duplicate "Hive would", no merge button, visible text β€ 60 words, no jargon; mergeable-as-is case shows the merge button (and greys it without the role); issue case; batch case; stale double-prefixed summaries are normalised.Docs / changelog:
src/docs/dashboard-sections.md(Preview before acting),src/docs/api-reference/packs-and-acmm.md,changelog.d/fixed-decide-preview-one-decision.md.Before / after (#15332, headless)
After β Details expanded, and a mergeable-as-is PR