From 5e976dde220d859e838bc7de0f71756b87b9d9e8 Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Wed, 28 Oct 2026 21:06:01 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Approve=20toast:=20outcome=20in?= =?UTF-8?q?=20plain=20words,=20who=20merges=20next,=20sentinel=20only=20wh?= =?UTF-8?q?ile=20open?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dashboard Approve result on hivecommons/dibs#510 read "approved `/lgtm` + `/approve` posted (released: approval-recorded, sentinel-label)" followed by the ⚠ sentinel paragraph it had just cleared and "the merge gate takes it from here; the dashboard merges nothing". The owner: that's a confusing response. - Headline is the outcome: "Approved #510 as @user · sentinel alert cleared by your review". - Next names the concrete step and who merges: hive's merge sweep (with the soak figure), or "merge it on GitHub yourself" on the human-merge lane, or what a person must still do when the sentinel alert is still open. - The ⚠ sentinel paragraph shows only while the alert is still open after the action (sentinel_cleared / sentinel_open on the result). - What was posted (/lgtm, /approve, labels removed) is a muted Posted line. - On Prow-managed repos the relayed /approve from hive's account is not in OWNERS ("NOT APPROVED"): the toast says the approval is recorded in hive and Prow still needs an OWNERS reviewer. Signed-off-by: Andrew Anderson --- changelog.d/fixed-approve-toast-wording.md | 1 + .../api_repo_item_decide_delegated.go | 2 +- src/pkg/dashboard/api_repo_item_decision.go | 120 ++++++++++++++++-- .../dashboard/api_repo_item_decision_test.go | 80 ++++++++++++ src/pkg/dashboard/row_action_result.go | 14 +- .../stage_tracker_row_actions_test.go | 54 ++++++++ src/pkg/dashboard/static/index.html | 19 ++- src/pkg/github/pr_decision_operator.go | 27 +++- src/pkg/github/pr_decision_operator_test.go | 23 ++++ 9 files changed, 324 insertions(+), 16 deletions(-) create mode 100644 changelog.d/fixed-approve-toast-wording.md diff --git a/changelog.d/fixed-approve-toast-wording.md b/changelog.d/fixed-approve-toast-wording.md new file mode 100644 index 0000000000..921a74fd27 --- /dev/null +++ b/changelog.d/fixed-approve-toast-wording.md @@ -0,0 +1 @@ +- The dashboard's Approve result toast now leads with the outcome in plain words ("Approved #510 as @user · sentinel alert cleared by your review"), names the concrete next step and who merges (hive's merge sweep with the soak figure, or "merge it on GitHub yourself" on the human-merge lane), repeats the ⚠ sentinel paragraph only while the alert is still open after the action, lists what was posted on a muted secondary line, and on Prow-managed repos says the approval is recorded in hive while Prow approval still needs an OWNERS reviewer — instead of "`/lgtm` + `/approve` posted (released: …) … the merge gate takes it from here; the dashboard merges nothing" ([#15212](https://github.com/hivecommons/hive/issues/15212)). diff --git a/src/pkg/dashboard/api_repo_item_decide_delegated.go b/src/pkg/dashboard/api_repo_item_decide_delegated.go index fbc834b978..8e533da4d4 100644 --- a/src/pkg/dashboard/api_repo_item_decide_delegated.go +++ b/src/pkg/dashboard/api_repo_item_decide_delegated.go @@ -732,7 +732,7 @@ func (s *Server) executeDelegatedDecision(ctx context.Context, in delegatedecisi if _, err := client.RecordOperatorPullRequestApproval(ctx, repoFull, number, user); err != nil { return notes, fmt.Errorf("approving PR: %w", err) } - notes = append(notes, "`/lgtm` + `/approve` recorded; the merge gate takes it from here") + notes = append(notes, "`/lgtm` + `/approve` recorded; hive's merge sweep merges it once CI is green and the head has soaked") } case delegatedecision.ActionLabelsAdd: for _, l := range a.Labels { diff --git a/src/pkg/dashboard/api_repo_item_decision.go b/src/pkg/dashboard/api_repo_item_decision.go index 211fbf91ef..4d25aad032 100644 --- a/src/pkg/dashboard/api_repo_item_decision.go +++ b/src/pkg/dashboard/api_repo_item_decision.go @@ -189,20 +189,37 @@ func (s *Server) admitPullRequestHead(repoFull string, number int, head, user st return "" } +// repoItemApproveFacts is what a PR Approve has to say about itself: +// who approved, what the approval released (repoItemApprovalReleased plus +// "admitted"), the stage label the engine derived on the spot ("" when +// deferred to the sweep), whether the sentinel alert is still open, and +// how long the merge sweep soaks a head before merging. +type repoItemApproveFacts struct { + User string + Released []string + Stage string + SentinelOpen bool + SentinelLabel string + SoakMinutes int + // Prow says the PR is on a Prow-managed repo: the relayed `/approve` + // came from the hive's account, which is not in OWNERS, so Prow ignores + // it and the toast must not claim the PR is approved for Prow. + Prow bool +} + // repoItemDecisionResult reads a recorded human decision into the shared // row-action result. func repoItemDecisionResult(repoFull string, number int, itemType, decision, comment, supersededBy, label string, released []string) *RowActionResult { + return repoItemDecisionResultWith(repoFull, number, itemType, decision, comment, supersededBy, label, repoItemApproveFacts{Released: released}) +} + +func repoItemDecisionResultWith(repoFull string, number int, itemType, decision, comment, supersededBy, label string, facts repoItemApproveFacts) *RowActionResult { out := &RowActionResult{Target: rowActionTarget(repoFull, itemType, number)} switch decision { case repoItemDecisionApprove: out.Action = rowActionApproved if itemType == "pr" { - out.Outcome = "`/lgtm` + `/approve` posted" - if len(released) > 0 { - out.Outcome += " (released: " + strings.Join(released, ", ") + ")" - } - out.Outcome += "." - out.Next = "The merge gate takes it from here; the dashboard merges nothing." + repoItemApproveResult(out, number, facts) } else { out.Outcome = "`/hive approve` recorded; parking labels lifted, `" + label + "` applied." out.Next = "The hive un-parks the issue and carries on; nothing is merged." @@ -242,7 +259,7 @@ func repoItemDecisionResult(repoFull string, number int, itemType, decision, com case repoItemDecisionQueueMerge: out.Action = rowActionQueued out.Outcome = "Your approval is recorded; `" + label + "` applied." - out.Next = "Hive's merge sweep merges it on the next tick once CI is green, respecting soak; the dashboard merges nothing itself." + out.Next = "Hive's merge sweep merges it on its next pass once CI is green and the head has soaked." } return out } @@ -555,6 +572,7 @@ func (s *Server) handlePullRequestDecision(ctx context.Context, w http.ResponseW ) var released []string stage := "" + sentinelOpen, prow := false, false if decision == repoItemDecisionApprove { var event ghpkg.PullRequestApprovalEvent event, err = gh.RecordOperatorPullRequestApprovalWith(ctx, repoFull, number, user, ghpkg.PullRequestApprovalOptions{ @@ -563,7 +581,7 @@ func (s *Server) handlePullRequestDecision(ctx context.Context, w http.ResponseW SentinelLabel: s.sentinelLabel(), }) head = event.HeadSHA - released, stage = repoItemApprovalReleased(event), event.Stage + released, stage, sentinelOpen, prow = repoItemApprovalReleased(event), event.Stage, event.SentinelLabelOpen, event.ProwManaged } else { var event ghpkg.PullRequestDecisionEvent event, err = gh.RecordOperatorPullRequestDecision(ctx, repoFull, number, user, comment, repoItemDecisionChangeRequests()) @@ -603,7 +621,9 @@ func (s *Server) handlePullRequestDecision(ctx context.Context, w http.ResponseW // next tracker read instead of the cached 085. s.invalidateStageTrackerForWebhook() jsonResponse(w, repoItemDecisionResponse{OK: true, Repo: repoFull, Number: number, Decision: decision, Status: status, Released: released, Stage: stage, Head: head, AdmittedBy: admitted, - Result: repoItemDecisionResult(repoFull, number, "pr", decision, comment, "", "", released)}) + Result: repoItemDecisionResultWith(repoFull, number, "pr", decision, comment, "", "", repoItemApproveFacts{ + User: user, Released: released, Stage: stage, SentinelOpen: sentinelOpen, SentinelLabel: s.sentinelLabel(), SoakMinutes: s.approveSoakMinutes(admitted != ""), Prow: prow, + })}) } // repoItemApprovalReleased names what a PR approval released, for the @@ -624,3 +644,85 @@ func repoItemApprovalReleased(event ghpkg.PullRequestApprovalEvent) []string { } return out } + +// repoItemApproveResult words a PR Approve the way the owner reads it +// (hivecommons/dibs#510 read "`/lgtm` + `/approve` posted (released: +// approval-recorded, sentinel-label)" followed by the ⚠ sentinel warning +// it had just released and "the dashboard merges nothing"): the headline +// is the outcome in plain words, Next is the concrete next step and who +// merges, and what was posted rides Posted as the muted secondary line. +func repoItemApproveResult(out *RowActionResult, number int, facts repoItemApproveFacts) { + released := map[string]bool{} + for _, r := range facts.Released { + released[r] = true + } + headline := "Approved #" + strconv.Itoa(number) + if facts.Prow { + headline = "Approval of #" + strconv.Itoa(number) + " recorded in hive" + } + if u := strings.TrimSpace(facts.User); u != "" { + headline += " as @" + u + } + out.SentinelCleared = released["sentinel-label"] + out.SentinelOpen = facts.SentinelOpen && !out.SentinelCleared + switch { + case out.SentinelCleared: + headline += " · sentinel alert cleared by your review" + case out.SentinelOpen: + headline += " · sentinel alert still open" + } + out.Outcome = headline + "." + out.Posted = append(out.Posted, "`/lgtm` posted", "`/approve` posted") + if released["needs-human"] { + out.Posted = append(out.Posted, "`needs-human` label removed") + } + if out.SentinelCleared { + out.Posted = append(out.Posted, "`"+repoItemSentinelLabelOrDefault(facts.SentinelLabel)+"` label removed") + } + if released["admitted"] { + out.Posted = append(out.Posted, "head admitted as-is on your approval") + } + switch { + case out.SentinelOpen: + out.Next = "The `" + repoItemSentinelLabelOrDefault(facts.SentinelLabel) + "` label is still on the PR: a maintainer must review the flagged head and take the label off on GitHub before hive's merge sweep will touch it." + case facts.Stage == stagelabels.ReadyForHumanMerge.Label(): + out.Next = "Hive cannot merge this one (trust or role blocks it): merge it on GitHub yourself, or use Approve & merge as maintainer." + default: + out.Next = "Hive's merge sweep merges it on its next pass once CI is green and the head has soaked" + repoItemSoakNote(facts.SoakMinutes) + "." + } + if facts.Prow { + // Prow reads `/approve` only from OWNERS; the hive's account is not + // one, so the relayed command is "NOT APPROVED" there. + out.Next = "Prow approval still needs an OWNERS reviewer: the relayed `/approve` came from hive's account, which Prow does not count. " + out.Next + } +} + +func repoItemSentinelLabelOrDefault(label string) string { + if strings.TrimSpace(label) == "" { + return config.DefaultSentinelLabel + } + return strings.TrimSpace(label) +} + +func repoItemSoakNote(minutes int) string { + if minutes <= 0 { + return "" + } + return " (≈" + strconv.Itoa(minutes) + " min)" +} + +// approveSoakMinutes is how long the merge sweep soaks an approved head: +// the contributor lane's soak when the approval admitted a foreign head, +// otherwise the sweep's minimum head age. +func (s *Server) approveSoakMinutes(admitted bool) int { + if s == nil || s.deps == nil || s.deps.Config == nil { + if admitted { + return config.DefaultContributorLaneSoakMinutes + } + return int(config.DefaultAutoMergeMinHeadAge / time.Minute) + } + if admitted { + return s.deps.Config.AutoMerge.ContributorLane.EffectiveSoakMinutes() + } + return int(s.deps.Config.AutoMerge.EffectiveMinHeadAge() / time.Minute) +} diff --git a/src/pkg/dashboard/api_repo_item_decision_test.go b/src/pkg/dashboard/api_repo_item_decision_test.go index 8d4b3d8b2b..952e327907 100644 --- a/src/pkg/dashboard/api_repo_item_decision_test.go +++ b/src/pkg/dashboard/api_repo_item_decision_test.go @@ -559,3 +559,83 @@ func TestRepoItemDecisionMergeDecisionsGuard(t *testing.T) { t.Fatalf("guards must stop before GitHub writes: merges=%v reviews=%v", mock.merges, mock.reviews) } } + +// TestRepoItemApproveResultWording (owner, hivecommons/dibs#510 — "that's +// a confusing response"): the approve toast leads with the outcome in +// plain words, says who merges next, flags the sentinel alert only while +// it is still open, and lists what was posted as a secondary line. +func TestRepoItemApproveResultWording(t *testing.T) { + cases := []struct { + name string + facts repoItemApproveFacts + outcome string + next string + posted string + cleared bool + stillOpen bool + }{ + { + name: "sentinel released, lane merges", + facts: repoItemApproveFacts{User: "clubanderson", Released: []string{"approval-recorded", "sentinel-label", "sentinel-acknowledged", "admitted"}, SentinelLabel: "sentinel-alert", SoakMinutes: 30}, + outcome: "Approved #510 as @clubanderson · sentinel alert cleared by your review.", + next: "Hive's merge sweep merges it on its next pass once CI is green and the head has soaked (≈30 min).", + posted: "`/lgtm` posted|`/approve` posted|`sentinel-alert` label removed|head admitted as-is on your approval", + cleared: true, + }, + { + name: "sentinel still open", + facts: repoItemApproveFacts{User: "clubanderson", Released: []string{"approval-recorded"}, SentinelOpen: true, SentinelLabel: "sentinel-alert", SoakMinutes: 30}, + outcome: "Approved #510 as @clubanderson · sentinel alert still open.", + next: "The `sentinel-alert` label is still on the PR: a maintainer must review the flagged head and take the label off on GitHub before hive's merge sweep will touch it.", + posted: "`/lgtm` posted|`/approve` posted", + stillOpen: true, + }, + { + name: "human must merge", + facts: repoItemApproveFacts{User: "clubanderson", Released: []string{"approval-recorded", "needs-human"}, Stage: stagelabels.ReadyForHumanMerge.Label(), SoakMinutes: 3}, + outcome: "Approved #510 as @clubanderson.", + next: "Hive cannot merge this one (trust or role blocks it): merge it on GitHub yourself, or use Approve & merge as maintainer.", + posted: "`/lgtm` posted|`/approve` posted|`needs-human` label removed", + }, + { + name: "prow-managed repo: approval recorded, not claimed approved for prow", + facts: repoItemApproveFacts{User: "clubanderson", Released: []string{"approval-recorded"}, SoakMinutes: 3, Prow: true}, + outcome: "Approval of #510 recorded in hive as @clubanderson.", + next: "Prow approval still needs an OWNERS reviewer: the relayed `/approve` came from hive's account, which Prow does not count. Hive's merge sweep merges it on its next pass once CI is green and the head has soaked (≈3 min).", + posted: "`/lgtm` posted|`/approve` posted", + }, + { + name: "no user, no soak figure", + facts: repoItemApproveFacts{Released: []string{"approval-recorded"}}, + outcome: "Approved #510.", + next: "Hive's merge sweep merges it on its next pass once CI is green and the head has soaked.", + posted: "`/lgtm` posted|`/approve` posted", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := repoItemDecisionResultWith("hivecommons/dibs", 510, "pr", repoItemDecisionApprove, "", "", "", tc.facts) + if got.Action != rowActionApproved || got.Outcome != tc.outcome || got.Next != tc.next { + t.Fatalf("result = %+v", got) + } + if p := strings.Join(got.Posted, "|"); p != tc.posted { + t.Fatalf("posted = %q", p) + } + if got.SentinelCleared != tc.cleared || got.SentinelOpen != tc.stillOpen { + t.Fatalf("sentinel flags cleared=%v open=%v", got.SentinelCleared, got.SentinelOpen) + } + for _, bad := range []string{"released:", "merges nothing", "merge gate takes it"} { + if strings.Contains(got.Outcome+got.Next, bad) { + t.Fatalf("old wording %q survives: %+v", bad, got) + } + } + if !tc.facts.Prow && strings.Contains(got.Outcome+got.Next, "Prow") { + t.Fatalf("non-prow repo mentions Prow: %+v", got) + } + }) + } + // The legacy builder keeps the same shape for callers without facts. + if got := repoItemDecisionResult("o/r", 1, "pr", repoItemDecisionApprove, "", "", "", nil); got.Outcome != "Approved #1." { + t.Fatalf("legacy = %+v", got) + } +} diff --git a/src/pkg/dashboard/row_action_result.go b/src/pkg/dashboard/row_action_result.go index f01ad2d7ea..f110f61194 100644 --- a/src/pkg/dashboard/row_action_result.go +++ b/src/pkg/dashboard/row_action_result.go @@ -30,6 +30,15 @@ type RowActionResult struct { // the popover's shape (hivecommons/hive#14822). Both additive. Why string `json:"why,omitempty"` Options []StageTrackerDecisionOption `json:"options,omitempty"` + // Posted lists what the action wrote on GitHub (`/lgtm`, `/approve`, + // a label removed) for the toast's muted secondary line; Outcome stays + // the plain-words headline. SentinelCleared says the action took the + // sentinel alert off, so the toast drops its ⚠ paragraph; SentinelOpen + // says the PR still wears the alert after the action and a person must + // clear it. Both additive. + Posted []string `json:"posted,omitempty"` + SentinelCleared bool `json:"sentinel_cleared,omitempty"` + SentinelOpen bool `json:"sentinel_open,omitempty"` } // RowActionTarget names the issue or PR a row action acted on. @@ -162,9 +171,10 @@ func delegatedRowActionResult(in delegatedecision.Input, d delegatedecision.Deci case note("queued for auto-merge"): out.Next = "Queued: the auto-merge engine merges per repository settings." case note("/lgtm"): - out.Next = "`/lgtm` + `/approve` posted; the merge gate takes it from here." + out.Next = "Hive's merge sweep merges it on its next pass once CI is green and the head has soaked." + out.Posted = append(out.Posted, "`/lgtm` posted", "`/approve` posted") default: - out.Next = "The merge gate takes it from here; the dashboard merges nothing." + out.Next = "Hive's merge sweep merges it on its next pass once CI is green and the head has soaked." } } return out diff --git a/src/pkg/dashboard/stage_tracker_row_actions_test.go b/src/pkg/dashboard/stage_tracker_row_actions_test.go index 21a10f6444..c16588db3b 100644 --- a/src/pkg/dashboard/stage_tracker_row_actions_test.go +++ b/src/pkg/dashboard/stage_tracker_row_actions_test.go @@ -95,6 +95,7 @@ func stageTrackerRowActionsPreamble(t *testing.T, html string) string { jsFunc(t, html, "stageTrackerTargetKey"), jsFunc(t, html, "stageTrackerResultOf"), jsFunc(t, html, "stageTrackerErrorResult"), + jsFunc(t, html, "stageTrackerResultWarn"), jsFunc(t, html, "stageTrackerResultText"), jsFunc(t, html, "stageTrackerResultToast"), jsFunc(t, html, "stageTrackerResultToastStale"), @@ -860,6 +861,59 @@ const plain = { repo: 'o/r', kind: 'pr', number: 4, url: 'u4', stage: 85, modifi `) } +// TestStageTrackerApproveToastSentinelWording (owner, hivecommons/dibs#510: +// "approved `/lgtm` + `/approve` posted (released: approval-recorded, +// sentinel-label). ⚠ … a maintainer must review before merge. The merge +// gate takes it from here; the dashboard merges nothing" — "that's a +// confusing response"): the toast leads with the server's plain-words +// outcome, repeats the row's ⚠ sentinel paragraph only while the alert is +// still open after the action (sentinel_cleared drops it), renders Next +// as the concrete step, and lists what was posted on a muted line. +func TestStageTrackerApproveToastSentinelWording(t *testing.T) { + html := indexHTML(t) + if !strings.Contains(html, `.ham-result-posted {`) { + t.Fatalf("index.html missing the muted ham-result-posted rule") + } + runStageTrackerRowActionsScript(t, stageTrackerRowActionsPreamble(t, html)+stageTrackerConfirmOptOutJS(t, html)+` +const flagged = { repo: 'hivecommons/dibs', kind: 'pr', number: 510, url: 'u510', stage: 85, head: '30e94a9ffff', sentinel_alert: true, sentinel_reason: 'possible security override', modifiers: ['needs-decision'] }; +const target = { repo: 'hivecommons/dibs', kind: 'pr', number: 510, url: 'u510' }; +const posted = ['\u0060/lgtm\u0060 posted', '\u0060/approve\u0060 posted', '\u0060sentinel-alert\u0060 label removed']; +// Released: the alert the row carried is gone, so no ⚠ paragraph. +const cleared = { action: 'approved', target, outcome: 'Approved #510 as @clubanderson · sentinel alert cleared by your review.', next: "Hive's merge sweep merges it on its next pass once CI is green and the head has soaked (≈30 min).", posted, sentinel_cleared: true }; +let text = stageTrackerResultText(cleared, flagged, { id: 'decide:approve', rowRef: 'hivecommons/dibs#510' }); +assert.strictEqual(text.head, 'Approve on hivecommons/dibs#510: approved'); +assert.strictEqual(text.outcome, cleared.outcome); +assert.strictEqual(text.warn, '', 'a released alert is not re-warned: ' + text.warn); +assert.strictEqual(text.next, cleared.next); +assert.strictEqual(text.posted, 'Posted: /lgtm posted · /approve posted · sentinel-alert label removed'); +assert.ok(!text.next.includes('merges nothing') && !text.outcome.includes('released:'), 'no "released:" / "merges nothing"'); +let toast = stageTrackerResultToast(cleared, flagged, { id: 'decide:approve' }); +let classes = toast.el.children.map(c => c.className).filter(c => c !== 'toast-close'); +assert.deepStrictEqual(classes, ['ham-result-head', 'ham-result-outcome', 'ham-result-next', 'ham-result-posted', 'ham-result-link'], classes.join(',')); +assert.ok(classes.indexOf('ham-result-next') < classes.indexOf('ham-result-posted'), 'the posted list is the muted trailer, under Next'); +// Still open: the ⚠ paragraph stays and Next says what the person must do. +const open = { action: 'approved', target, outcome: 'Approved #510 as @clubanderson · sentinel alert still open.', next: 'The \u0060sentinel-alert\u0060 label is still on the PR: a maintainer must review the flagged head and take the label off on GitHub before hive\'s merge sweep will touch it.', posted: posted.slice(0, 2), sentinel_open: true }; +text = stageTrackerResultText(open, flagged, { id: 'decide:approve', rowRef: 'hivecommons/dibs#510' }); +assert.ok(text.warn.startsWith('⚠ hivecommons/dibs#510 — Sentinel flagged security-relevant changes at 30e94a9: possible security override'), text.warn); +assert.ok(text.next.includes('take the label off on GitHub'), text.next); +toast = stageTrackerResultToast(open, flagged, { id: 'decide:approve' }); +classes = toast.el.children.map(c => c.className).filter(c => c !== 'toast-close'); +assert.deepStrictEqual(classes, ['ham-result-head', 'ham-result-outcome', 'ham-result-warn', 'ham-result-next', 'ham-result-posted', 'ham-result-link'], classes.join(',')); +// An older hive's result (no flags) keeps the legacy behaviour: warn from the row. +text = stageTrackerResultText({ action: 'approved', target, outcome: 'x', next: '' }, flagged, { id: 'decide:approve' }); +assert.ok(text.warn.startsWith('⚠ hivecommons/dibs#510'), text.warn); +assert.strictEqual(text.posted, ''); +// A human-merge lane: Next names who merges. +text = stageTrackerResultText({ action: 'approved', target, outcome: 'Approved #510 as @clubanderson.', next: 'Hive cannot merge this one (trust or role blocks it): merge it on GitHub yourself, or use Approve & merge as maintainer.', posted: posted.slice(0, 2) }, { repo: 'hivecommons/dibs', kind: 'pr', number: 510, url: 'u510', stage: 90 }, { id: 'decide:approve' }); +assert.strictEqual(text.warn, ''); +assert.ok(text.next.startsWith('Hive cannot merge this one'), text.next); +// A Prow-managed repo: the headline says recorded-in-hive, Next says who still approves. +text = stageTrackerResultText({ action: 'approved', target, outcome: 'Approval of #510 recorded in hive as @clubanderson.', next: 'Prow approval still needs an OWNERS reviewer: the relayed \u0060/approve\u0060 came from hive\'s account, which Prow does not count. Hive\'s merge sweep merges it on its next pass once CI is green and the head has soaked (≈3 min).', posted: posted.slice(0, 2) }, { repo: 'hivecommons/dibs', kind: 'pr', number: 510, url: 'u510', stage: 85 }, { id: 'decide:approve' }); +assert.ok(text.outcome.startsWith('Approval of #510 recorded in hive'), text.outcome); +assert.ok(text.next.startsWith('Prow approval still needs an OWNERS reviewer'), text.next); +`) +} + // TestStageTrackerRowActionResults (owner request): every row action goes // through runRowAction → a persistent result toast built from the server's // shared result {action, target, outcome, next, rule} — headline naming the diff --git a/src/pkg/dashboard/static/index.html b/src/pkg/dashboard/static/index.html index 9e0c333fe5..e5f72d6857 100644 --- a/src/pkg/dashboard/static/index.html +++ b/src/pkg/dashboard/static/index.html @@ -3500,11 +3500,13 @@ .toast-action { margin-left: 10px; vertical-align: middle; } /* Row-action result toast (runRowAction): sticky until the target row changes state in the tracker, or ×. Headline, outcome, what happens - next, a ⚠ sentinel line when the head was flagged, and a link to the + next, a muted "Posted:" line, a ⚠ sentinel line while the head is + still flagged after the action, and a link to the item acted on (which may be a nested PR, not the clicked row). */ .ham-result-toast { display: grid; gap: var(--sp-2); } .ham-result-head { font-weight: var(--fw-medium); } .ham-result-next { color: var(--muted); } + .ham-result-posted { color: var(--text-faint); font-size: var(--fs-xs); } .ham-result-warn { color: var(--status-error); white-space: pre-line; } .ham-result-options { display: flex; flex-wrap: wrap; gap: var(--sp-2); } .ham-result-link { color: var(--accent); text-decoration: none; justify-self: start; } @@ -18208,9 +18210,19 @@

All labels

// The rule in plain words from the glossary, never the bare slug. const rule = result.rule ? ' (' + stageTrackerPlainWords(result.rule) + ')' : ''; if (word) head += ': ' + word + rule; - const warn = typeof stageTrackerSentinelPreface === 'function' ? stageTrackerSentinelPreface([item]) : ''; + const warn = stageTrackerResultWarn(result, item); + const posted = Array.isArray(result.posted) ? result.posted.filter(p => typeof p === 'string' && p.trim()).map(p => p.replace(/`/g, '')) : []; const options = noEffect && Array.isArray(result.options) ? result.options.filter(o => o && o.action && o.label) : []; - return { head, outcome: result.outcome || '', next: result.next || '', warn, options, why: noEffect ? stageTrackerPlainWords(result.why || 'no-effect', true) : '', type: error ? 'error' : (noEffect ? 'warning' : (noop ? 'info' : 'success')), url: t.url || item.url || '', linkText: 'Open ' + (t.kind === 'pr' ? 'PR' : 'issue') + ' #' + (t.number || item.number) }; + return { head, outcome: result.outcome || '', next: result.next || '', warn, posted: posted.length ? 'Posted: ' + posted.join(' · ') : '', options, why: noEffect ? stageTrackerPlainWords(result.why || 'no-effect', true) : '', type: error ? 'error' : (noEffect ? 'warning' : (noop ? 'info' : 'success')), url: t.url || item.url || '', linkText: 'Open ' + (t.kind === 'pr' ? 'PR' : 'issue') + ' #' + (t.number || item.number) }; + } + // Pure: the ⚠ sentinel paragraph a result toast carries. The row's + // alert is what the action started from, so it is repeated only while + // the alert is still open: a result that says it cleared the alert + // (sentinel_cleared) drops the warning; one that says it is still on + // (sentinel_open) keeps it, with Next saying what the person must do. + function stageTrackerResultWarn(result, item) { + if (result && result.sentinel_cleared) return ''; + return typeof stageTrackerSentinelPreface === 'function' ? stageTrackerSentinelPreface([item]) : ''; } let _stageTrackerResultToasts = []; // Build and show the persistent result toast; it leaves on × or when @@ -18225,6 +18237,7 @@

All labels

if (text.outcome) { const o = document.createElement('div'); o.className = 'ham-result-outcome'; o.textContent = text.outcome; el.appendChild(o); } if (text.warn) { const w = document.createElement('div'); w.className = 'ham-result-warn'; w.textContent = text.warn; el.appendChild(w); } if (text.next) { const n = document.createElement('div'); n.className = 'ham-result-next'; n.textContent = text.next; el.appendChild(n); } + if (text.posted) { const p = document.createElement('div'); p.className = 'ham-result-posted'; p.textContent = text.posted; el.appendChild(p); } if (text.options && text.options.length) { // The stuck decision's next actions (#14822): each runs the row // action on the toast's target; "open:github" is a link. diff --git a/src/pkg/github/pr_decision_operator.go b/src/pkg/github/pr_decision_operator.go index 46b52ab6f1..bdce03cbb5 100644 --- a/src/pkg/github/pr_decision_operator.go +++ b/src/pkg/github/pr_decision_operator.go @@ -67,6 +67,15 @@ type PullRequestApprovalEvent struct { // sentinel findings. SentinelLabelCleared bool SentinelAcknowledged bool + // SentinelLabelOpen is true when the PR wore the sentinel alert label + // and this approval could not take it off: the alert is still open and + // a person must clear it on GitHub before the merge lanes touch the PR. + SentinelLabelOpen bool + // ProwManaged is true when the PR wears Prow's plugin labels (`approved`, + // `size/*`, `do-not-merge/*`, `ok-to-test`), so the relayed `/approve` + // came from the hive's account, which is not in OWNERS: Prow ignores it + // ("NOT APPROVED"), and an OWNERS reviewer still has to approve there. + ProwManaged bool // Stage is the stage label the engine derived when the approval was // submitted to it, or "" when the stage write was deferred to the sweep. Stage string @@ -246,7 +255,7 @@ func (c *Client) RecordOperatorPullRequestApprovalWith(ctx context.Context, repo return PullRequestApprovalEvent{}, fmt.Errorf("posting %s on %s/%s#%d: %w", command, owner, name, number, err) } } - event := PullRequestApprovalEvent{Repo: full, Number: number, HeadSHA: head, Actor: actor} + event := PullRequestApprovalEvent{Repo: full, Number: number, HeadSHA: head, Actor: actor, ProwManaged: prowManagedLabels(labels)} if c.logger != nil { c.logger.Info("pull request approval commands posted by dashboard decision", "repo", event.Repo, "pr", number, "actor", actor, "head", head) } @@ -277,6 +286,7 @@ func (c *Client) RecordOperatorPullRequestApprovalWith(ctx context.Context, repo if labelPresent(labels, label) { if err := c.RemoveLabel(ctx, full, number, label); err != nil { c.warnOperatorApproval("sentinel label not cleared", full, number, head, err) + event.SentinelLabelOpen = true } else { event.SentinelLabelCleared = true } @@ -294,6 +304,21 @@ func (c *Client) RecordOperatorPullRequestApprovalWith(ctx context.Context, repo return event, nil } +// prowManagedLabels reports whether labels carry Prow's plugin markers: +// the approve plugin's `approved`, size's `size/*`, the `do-not-merge/*` +// family and trigger's `ok-to-test` / `needs-ok-to-test`. +func prowManagedLabels(labels []string) bool { + for _, l := range labels { + l = strings.ToLower(strings.TrimSpace(l)) + switch { + case l == "approved", l == "ok-to-test", l == "needs-ok-to-test", + strings.HasPrefix(l, "size/"), strings.HasPrefix(l, "do-not-merge/"), strings.HasPrefix(l, "tide/"): + return true + } + } + return false +} + // prDecisionApproveBody is the decision text a dashboard approval records, // the bare form `/hive decision approve` carries. const prDecisionApproveBody = "approve" diff --git a/src/pkg/github/pr_decision_operator_test.go b/src/pkg/github/pr_decision_operator_test.go index d1aa13a8f6..8dc594d0b1 100644 --- a/src/pkg/github/pr_decision_operator_test.go +++ b/src/pkg/github/pr_decision_operator_test.go @@ -155,6 +155,9 @@ func TestRecordOperatorPullRequestApprovalWithReleasesThePark(t *testing.T) { if !event.Recorded || !event.NeedsHumanLifted || !event.SentinelLabelCleared || !event.SentinelAcknowledged || event.HeadSHA != liveDecisionHead { t.Fatalf("event = %+v", event) } + if event.SentinelLabelOpen || event.ProwManaged { + t.Fatalf("a cleared alert on a non-Prow repo: event = %+v", event) + } if posted := rec.posted[liveDecisionPR]; len(posted) != len(PRApprovalCommands) { t.Fatalf("the command comments are still posted, got %v", posted) } @@ -200,6 +203,26 @@ func TestRecordOperatorPullRequestApprovalWithReleasesThePark(t *testing.T) { } } +// TestProwManagedLabels pins the Prow markers an approval reports so the +// dashboard does not claim the relayed `/approve` counted for Prow. +func TestProwManagedLabels(t *testing.T) { + for _, tc := range []struct { + labels []string + want bool + }{ + {[]string{"size/M", "role/trusted"}, true}, + {[]string{"approved", "lgtm"}, true}, + {[]string{"do-not-merge/hold"}, true}, + {[]string{"needs-ok-to-test"}, true}, + {[]string{"lgtm", "role/trusted", PRNeedsDecisionStageLabel}, false}, + {nil, false}, + } { + if got := prowManagedLabels(tc.labels); got != tc.want { + t.Errorf("prowManagedLabels(%v) = %v, want %v", tc.labels, got, tc.want) + } + } +} + // TestRecordOperatorPullRequestApprovalWithoutOptionsStaysCommentsOnly pins // the delegated "Let hive decide" path: it recommends, it does not clear a // sentinel alert or record a human decision.