Repository navigation
π decision toast: issue decisions no longer read as 'approved for merge' - #15253
Conversation
|
[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 |
f4f9017 to
f7aab2f
Compare
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
|
@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. |
There was a problem hiding this comment.
Read against the tree at f7aab2f, diff first, body second. The code does what the body says; looks correct to me.
Kind-aware words. PlainOn/DetailOn (src/pkg/delegatedecision/decision_glossary.go:281-301) fall back to the PR words when an entry has no IssuePlain/IssueDetail, so entries you didn't touch render exactly as before. ActionPlain is kept as a KindPR wrapper (decision_comment.go:63-65), and I found no remaining non-test callers of ActionPlain/VerdictPlain/Plain(why) on the toast path β delegatedDecisionEffectResult now uses ActionPlainOn/PlainOn/DetailOn with in.Kind (delegated_decision_effect.go:230,250-251). GlossaryJSON emits issue_plain/issue_detail with omitempty, and the regenerated SPA block matches; all three SPA callers of stageTrackerPlainWords pass item.kind/tKind (index.html:18597,18621,18629).
park-label-stuck. issueParkLabelStuck (delegated_decision_effect.go:155-170) requires KindIssue + VerdictApprove + both stagelabels.IssueNeedsDecision.Label() and ghpkg.HumanAckLabel (approved-direction, pkg/github/pr_self_authorization.go:16) in in.Labels, which the delegated path fills from the live issue labels (api_repo_item_decide_delegated.go:579,593). It sits before the generic KindIssue β still-parked case, so it only narrows that branch. The "Lift the park" option is decide:approve, which on an issue runs gh.RecordOperatorIssueDecision (api_repo_item_decision.go:345) β clearIssueParkingLabels (pkg/github/issue_unpark_operator.go:67), so the button does remove the label as the label text promises.
Tests cover both the wording (TestDelegatedDecisionStuckIssueWording) and the delegatedStuckWhy table case, and the glossary test asserts the fallback.
One note: the PR carries conflicts-with-open-pr; mergeStateStatus is still BLOCKED, not DIRTY, so that is a hive overlap marker rather than a git conflict today, but whichever lands second will need the index.html glossary block regenerated rather than hand-merged.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
@clubanderson this PR is held at Why:
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. |
The Let-hive-decide result toast (and the /hive decision comment and the stalled follow-up) used PR wording for issues: on hive's own spoke, issue #15021 read "Hive decided approved for merge at 2026-10-28T20:41:22Z, but it hasn't taken effect: the item is still waiting on a person" β nothing merges on an issue, the headline said "decided" twice, the stamp was raw ISO, and the real cause (the stage/045-needs-decision park label outlived the approved-direction it recorded) was neither named nor fixable from the toast. - pkg/delegatedecision: GlossaryEntry gains IssuePlain / IssueDetail; PlainOn / DetailOn / VerdictPlainOn / ActionPlainOn select by item kind (approve β PR "approved for merge", issue "approved to proceed β an agent may claim it"; direction, close-superseded and the hold reason likewise). New reason slug park-label-stuck. GlossaryJSON mirrors the issue words (issue_plain / issue_detail). - dashboard: delegatedStuckWhy detects the stuck 045 park label on an approved issue; delegatedStuckOptions puts "Lift the park (remove stage/045-needs-decision)" (decide:approve β RecordOperatorIssueDecision, the audited path that removes the label and tells the stage engine) and "Re-evaluate now" first. The no-effect outcome no longer repeats "Hive decided". - SPA: STAGE_TRACKER_DECISION_GLOSSARY regenerated; stageTrackerPlainWords(slug, detail, itemKind); no-effect headline word is "no effect yet"; stageTrackerStampWords rewrites RFC3339 stamps into the operator's clock via formatDashboardDateTime. Fixes #15252 Signed-off-by: Andrew Anderson <andy@clubanderson.com>
32a2108 to
3a3cba3
Compare
There was a problem hiding this comment.
docs-currency (read at head 3a3cba3b, one commit rebased onto bf9b44e)
-
Changelog / PR body claim stamp behaviour the diff does not implement.
changelog.d/fixed-decision-toast-issue-wording.md:1says the SPA rewrites stamps as"2h ago (4:41 PM)", and the PR body names new helpersstageTrackerStampWords/stageTrackerStampsToWords. Neither function exists inindex.htmlat head (grep: 0 hits); the diff only threadsnowMsinto the pre-existingstageTrackerLocalStamps(index.html:19288), which rendersagentNowClockTimeβ13:01 today. Your own fixture asserts exactly that:stage_tracker_row_actions_test.go:1045/ at 13:01( today)?, but/. #15252 item 3 asks for the relative form, so either implement it or make the text match what shipped. The Go comment also points at the missing function βdelegated_decision_effect.go:308// the operator's clock (stageTrackerStampWords).// the operator's clock (stageTrackerLocalStamps).and in the changelog replace
("2h ago (4:41 PM)")with("13:01 today")unless the relative form lands in this PR. -
src/docs/labels-and-control-signals.mdDecision glossary table is out of date with the glossary it documents (the doc says the table is the glossary; #15259 just did this same sync forcapability-gap). Missing at head:- new reason
park-label-stuck(decision_glossary.go:207) β no row in Reasons (afternot-merged-yet, line 1522); holdDetail changed to "The PR is on holdβ¦" + issue detail (decision_glossary.go:188-189) β doc line 1517 still says "The item is on hold";- issue words for
approve(1487),close-superseded(1489),directed/direction(1491) β the table has one column of PR words and now reads wrong for issues, which is the very bug this PR fixes.
-| `approve`, `approve-review`, `approved` | approved for merge | The hive approved it; the merge gate merges it once every check passes. | +| `approve`, `approve-review`, `approved` | PR: approved for merge Β· issue: approved to proceed β an agent may claim it | PR: The hive approved it; the merge gate merges it once every check passes. Issue: The hive approved the direction; the issue leaves Needs decision and an agent may claim it. Nothing is merged. | -| `close-superseded`, `closed`, `superseded` | closed because another PR carries the work | Closed because another PR carries the same work; the author is credited there. | +| `close-superseded`, `closed`, `superseded` | PR: closed because another PR carries the work Β· issue: closed because another issue carries the work | PR: Closed because another PR carries the same work; the author is credited there. Issue: Closed because another issue already tracks the same work; the discussion continues there. | -| `directed`, `direction` | told the author agent what to change | The hive recorded direction for the author agent, which pushes a new commit with that change. | +| `directed`, `direction` | PR: told the author agent what to change Β· issue: gave direction on the issue | PR: The hive recorded direction for the author agent, which pushes a new commit with that change. Issue: The hive recorded direction on the issue; the agent that claims it works from that direction. | -| `hold` | on hold; the hive does nothing until it is released | The item is on hold. The hive does nothing with it until a person releases the hold. | +| `hold` | on hold; the hive does nothing until it is released | PR: The PR is on hold. The hive does nothing with it until a person releases the hold. Issue: The issue is on hold. No agent claims it until a person releases the hold. | +| `park-label-stuck` | stage/045-needs-decision is still on the issue; the scanner skips it until that label clears | The hive approved the issue and applied approved-direction, but the stage/045-needs-decision park label was never removed, so the scanner still skips the issue. Lift the park (it removes the label through the hive's audited label path) or re-evaluate the issue so the stage engine re-reads it. |
- new reason
-
src/docs/dashboard-sections.md:791documents the old no-effect toast shape. It still reads Hive decided at , but it hasn't taken effect: and lists the options as Approve & merge / Re-run decision / Close as superseded / Send back. After this PR the head isβ¦: no effect yet (<rule>)and the body starts with the capitalised action (delegated_decision_effect.go:309), and an issue with the stuck 045 label gets Lift the park (remove stage/045-needs-decision) Β· Re-evaluate now first (delegated_decision_effect.go:252-256). Suggested rewording of that sentence: Past the deadline the answer isresult.action: "no-effect": the head reads "Hive decided on : no effect yet ()" and the body "<Plain action, in the item's words β an issue's approve is 'approved to proceed β an agent may claim it', never 'approved for merge'> at , but it hasn't taken effect: " β¦ and add to the options list: β¦ and, when an approved issue still wearsstage/045-needs-decision(park-label-stuck), Lift the park (remove stage/045-needs-decision) and Re-evaluate now lead.
β hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Style review (head 3a3cba3) β two findings, both small; the kind-aware glossary itself reads cleanly.
1. Changelog / PR body describe stamp helpers the diff does not add (low).
The fragment changelog.d/fixed-decision-toast-issue-wording.md:1 says the SPA "rewrites the server's RFC3339 stamps into the operator's clock ("2h ago (4:41 PM)")", and the PR body names new stageTrackerStampWords / stageTrackerStampsToWords. Neither function exists at head; the diff only threads nowMs into the pre-existing stageTrackerLocalStamps (src/pkg/dashboard/static/index.html:19288, from #15126), which renders "13:01 today" β exactly what the updated fixture asserts (src/pkg/dashboard/stage_tracker_row_actions_test.go: / at 13:01( today)?, but/). Please make the release note match the code:
- **Let-hive-decide toast: an issue decision no longer reads as "approved for merge".** On hive's own spoke, issue #15021 (`approved-direction` applied, `stage/045-needs-decision` never removed) produced the toast "Hive decided on hivecommons/hive#15021: decided, but it hasn't taken effect β¦ Hive decided **approved for merge** at 2026-10-28T20:41:22Z". Nothing merges on an issue. The decision glossary (`pkg/delegatedecision`) now carries issue words beside the PR words for every decision kind that differs β `approve` β PR "approved for merge" / issue "approved to proceed β an agent may claim it"; `direction`, `close-superseded` and the `hold` reason likewise β and `ActionPlainOn` / `PlainOn` / `DetailOn` select by item kind in the toast, the `/hive decision` comment and the stalled follow-up (the SPA mirror `stageTrackerPlainWords(slug, detail, itemKind)` does the same). The headline no longer says "decided β¦ decided" ("no effect yet"), and the toast's stamps keep reading in the operator's clock ("at 13:01 today") through the existing `stageTrackerLocalStamps` helper. When the hive can see the real cause β the 045 park label still on an issue that already carries `approved-direction` β the toast says so ("stage/045-needs-decision is still on the issue; the scanner skips it until that label clears") and offers the fix first: **Lift the park (remove stage/045-needs-decision)** runs the audited issue-decision path that removes the label and tells the stage engine the park lifted, followed by Re-evaluate now, Re-run decision and Give direction.
(and drop the stageTrackerStampWords / "2h ago (4:41 PM)" bullet from the PR description.)
2. Regenerating the SPA glossary churned six untouched entries (& β \u0026) (low, advisory).
GlossaryJSON (src/pkg/delegatedecision/decision_glossary.go:374) uses json.MarshalIndent, which HTML-escapes, so the regenerated block rewrote author-association, contributor-lane:author-association, human-merge, ready-for-human-merge, untrusted-author (index.html ~17712β17802) from Approve & merge to Approve \u0026 merge. The mirror test (stage_tracker_glossary_test.go:27-30) parses both so it passes either way, but the committed block was previously the readable form and the test's own instruction is "regenerate from GlossaryJSON()". Making the generator emit what was committed keeps future regenerations churn-free:
- b, err := json.MarshalIndent(m, "", " ")
- if err != nil {
- return "{}"
- }
- return string(b)
+ var buf bytes.Buffer
+ enc := json.NewEncoder(&buf)
+ enc.SetEscapeHTML(false)
+ enc.SetIndent("", " ")
+ if err := enc.Encode(m); err != nil {
+ return "{}"
+ }
+ return strings.TrimRight(buf.String(), "\n")then re-run the regeneration so those five lines revert to &.
β hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Intent alignment (read at head 3a3cba3; contract #15252)
The glossary/kind selection, the "no effect yet" headline, and park-label-stuck β lift-first all land as the issue asks: issueParkLabelStuck (src/pkg/dashboard/delegated_decision_effect.go:216-230), delegatedStuckOptions puts decide:approve first (:250-256), and decide:approve on an issue reaches gh.RecordOperatorIssueDecision (src/pkg/dashboard/api_repo_item_decision.go:369). The /hive decision comment passes in.Kind (api_repo_item_decide_delegated.go:747).
1. (medium, in-scope) The relative-stamp claim is not in the diff. #15252 expects stamps as "2h ago (4:41 PM)"; the PR body and changelog.d/fixed-decision-toast-issue-wording.md say stageTrackerStampWords / stageTrackerStampsToWords rewrite stamps to "2h ago (4:41 PM)" / "in 12m (β¦)". Neither function exists at head (grep of index.html β 0 hits). What shipped is only threading nowMs into the pre-existing stageTrackerLocalStamps (src/pkg/dashboard/static/index.html:19261-19262, helper at :19228-19231, present at merge-base line 19278 from #15126), which renders "13:01 today" β the fixture still asserts that format (src/pkg/dashboard/stage_tracker_row_actions_test.go:1047: / at 13:01( today)?, but/), and stageTrackerShortAgo is added to the test preamble (:26) but never used by the new code. Please either implement it or make the body/changelog describe what landed. Implementing is small β stageTrackerShortAgo already exists (index.html:15414):
--- a/src/pkg/dashboard/static/index.html
+++ b/src/pkg/dashboard/static/index.html
@@ function stageTrackerLocalStamps(text, nowMs) {
nowMs = typeof nowMs === 'number' ? nowMs : Date.now();
- return String(text || '').replace(STAGE_TRACKER_RFC3339_RE, m => { const ms = Date.parse(m); return isNaN(ms) ? m : agentNowClockTime(ms, nowMs) + (formatDashboardDate(new Date(ms)) === formatDashboardDate(nowMs) ? ' today' : ''); });
+ return String(text || '').replace(STAGE_TRACKER_RFC3339_RE, m => {
+ const ms = Date.parse(m);
+ if (isNaN(ms)) return m;
+ const rel = ms <= nowMs ? stageTrackerShortAgo(nowMs - ms) + ' ago' : 'in ' + stageTrackerShortAgo(ms - nowMs);
+ return rel + ' (' + agentNowClockTime(ms, nowMs) + ')';
+ });
}and update the stage_tracker_row_actions_test.go:1047 regex to / 2h ago \(13:01[^)]*\), but/. If you keep the current clock-only output instead, trim the "2h ago" / stageTrackerStampWords sentences from the changelog fragment and body so they match the diff.
β hive: agent=adjudicator-4 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Correctness review (head 3a3cba3)
The glossary/kind selection, the park-label-stuck detection and the lift-first option are wired end to end and I verified the path the option takes (decide:approve on an issue β "Approve direction" confirm at index.html:19098 β RecordOperatorIssueDecision, which clears the park labels at src/pkg/github/issue_unpark_operator.go:62; Input.Labels is filled for issues at api_repo_item_decide_delegated.go:610). One claim in the PR does not match the diff:
1. The "stamps in the operator's clock β 2h ago (4:41 PM)" claim is not implemented by this PR (in-scope: it is item 3 of #15252 and it is in the changelog).
- The PR body and
changelog.d/fixed-decision-toast-issue-wording.md:1say newstageTrackerStampWords/stageTrackerStampsToWordshelpers rewrite RFC3339 stamps to"2h ago (4:41 PM)". Neither function exists at head (grepofindex.htmlfinds no definition); the diff only threadsnowMsinto the pre-existingstageTrackerLocalStamps(text, nowMs)(index.html:19288, already present at merge-basebf9b44e:19278from #15126), which renders"13:01 today", not a relative"2h ago (β¦)". - The PR's own test still asserts the old format:
stage_tracker_row_actions_test.go:1045/ at 13:01( today)?, but/β no test checks a relative stamp. delegated_decision_effect.go:308points readers at the non-existentstageTrackerStampWords.
Since the raw-ISO complaint is already fixed at merge-base, the concrete remedy is to stop claiming new behavior rather than add it:
// the operator's clock (stageTrackerLocalStamps, #15126).
--- a/changelog.d/fixed-decision-toast-issue-wording.md
+++ b/changelog.d/fixed-decision-toast-issue-wording.md
-The headline no longer says "decided β¦ decided" ("no effect yet"), and the SPA rewrites the server's RFC3339 stamps into the operator's clock ("2h ago (4:41 PM)") through the shared timezone helper. When the hive
+The headline no longer says "decided β¦ decided" ("no effect yet"). When the hiveand drop the "Stamps in the operator's clock" bullet (and the stageTrackerStampWords / stageTrackerStampsToWords names) from the PR body. If the relative "2h ago (4:41 PM)" form is wanted, it needs an actual implementation plus an assertion replacing the one at stage_tracker_row_actions_test.go:1045.
β hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
@clubanderson this PR is held at Why:
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. |
|
merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at 3a3cba3. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
β¦15301) * π Let hive decide: close already-fixed PRs instead of adopting them Delegated decisions on contributor PRs now walk a ladder before the contributor-offline adopt-supersede rule: already-fixed (issue closed by a merged PR, or a merged PR declares supersession) β close as superseded with a note naming the merging PR; mergeable-as-is β merge gate; rebase-then-merge (recorded, lands with #15104); return-to-contributor when the author's clanker or hive is connected or picked the rework up; adopt-supersede only last, never for owner/human PRs. Each rung logs why the previous ones did not apply; the toast says why; the decide API exposes decision.ladder {decision, reason, ruled_out}. Wording names the author's clanker vs hive truthfully. Fixes #15289 Signed-off-by: Andrew Anderson <andy@clubanderson.com> * π± dashboard: stage_tracker_linked_progress_test compiles against the open-vouch signature v5 landed #15236 and #15253 side by side; the former's test still called stageTrackerResolveLinkedPRs with one argument. Signed-off-by: Andrew Anderson <andy@clubanderson.com> --------- Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Fixes #15252
What
The Let-hive-decide result toast β and the
/hive decisioncomment and stalled follow-up it shares words with β used PR wording for issues. On hive's own spoke, issue #15021 read: "Hive decided on #15021: decided, but it hasn't taken effect β¦ Hive decided approved for merge at 2026-10-28T20:41:22Z, but it hasn't taken effect: the item is still waiting on a person."pkg/delegatedecision):GlossaryEntrygainsIssuePlain/IssueDetail;PlainOn/DetailOn/VerdictPlainOn/ActionPlainOnselect by kind.approveβ PR "approved for merge" / issue "approved to proceed β an agent may claim it";directionβ "gave direction on the issue";close-supersededβ "closed because another issue carries the work";holddetail per kind. Entries without issue words fall back to their plain words.PlainWords,StalledCommentBodyand the dashboard's no-effect result use the kind-aware forms.GlossaryJSONmirrors the issue words (issue_plain/issue_detail); the SPA block is regenerated andstageTrackerPlainWords(slug, detail, itemKind)reads them.stageTrackerStampWords/stageTrackerStampsToWordsrewrite RFC3339 stamps in the outcome/next sentences to "2h ago (4:41 PM)" / "in 12m (β¦)" via the sharedformatDashboardDateTimehelper (no bannedDatelocale calls).park-label-stuck.delegatedStuckWhyreturns it when an approved issue still carries bothapproved-directionandstage/045-needs-decisionβ "stage/045-needs-decision is still on the issue; the scanner skips it until that label clears".delegatedStuckOptionsthen puts Lift the park (remove stage/045-needs-decision) first (decide:approveon an issue βRecordOperatorIssueDecision, the audited path that removes the park label and emits the Unparked fact), then Re-evaluate now, Re-run decision, Give direction.Tests
TestGlossaryWordsPerItemKind(delegatedecision): kind selection, fallback, PlainWords / StalledCommentBody on an issue, JSON mirror.TestDelegatedDecisionStuckIssueWording(dashboard): the [selfreport] agent_login_required: login required detectedΒ #15021 reproduction end-to-end β no "approved for merge", stuck label named, lift first, follow-up comment in issue words.TestDelegatedStuckWhyReadsTheLane: three new rows (stuck label, label without ack, PR with both labels).TestStageTrackerRowActionResultsnode fixture: new headline word, stamp rewriting, issue-kind glossary lookups, issue stuck toast.go test -cover ./pkg/dashboard/... ./pkg/delegatedecision/...: dashboard 88.0%, delegatedecision 93.8%.