diff --git a/changelog.d/fixed-14814-supersession-stage.md b/changelog.d/fixed-14814-supersession-stage.md new file mode 100644 index 0000000000..6d967b525e --- /dev/null +++ b/changelog.d/fixed-14814-supersession-stage.md @@ -0,0 +1 @@ +- Keep superseded contributor PRs at the decision stage despite CI or rebase failures, show the closing PR and keep-open reason in the stage tracker, and let trusted maintainers close them with `/hive decision reject` or `/hive decision close superseded by #N`. Supplemental tests, docs and changelog files no longer block duplicate recommendations when implementation files overlap; held PRs remain visible without sweep writes, and closed issue snapshots cannot be offered as contributor work. diff --git a/src/cmd/hive/main.go b/src/cmd/hive/main.go index d858649305..b3f7debeb5 100644 --- a/src/cmd/hive/main.go +++ b/src/cmd/hive/main.go @@ -9530,8 +9530,8 @@ func runSupersessionSweepIfDue(ctx context.Context, ghClient *github.Client, cfg if dashSrv == nil { return } - detail := fmt.Sprintf("repo=%s, pr=%d, issue=%s#%d, closer_pr=%d, action=%s", - event.Repo, event.Number, event.IssueRepo, event.Issue, event.CloserPR, event.Action) + detail := fmt.Sprintf("repo=%s, pr=%d, issue=%s#%d, closer_pr=%d, action=%s, reason=%s", + event.Repo, event.Number, event.IssueRepo, event.Issue, event.CloserPR, event.Action, event.Reason) dashSrv.AuditLogRecord("system", github.SupersessionSweepAuditPrefix+event.Action, detail, "", event.Repo, event.Number) }, }) diff --git a/src/docs/maintainer-commands.md b/src/docs/maintainer-commands.md index aa05348fe6..3e0769d74d 100644 --- a/src/docs/maintainer-commands.md +++ b/src/docs/maintainer-commands.md @@ -59,6 +59,20 @@ specific to that queue flow. A swarm `changes_requested` or `reject` verdict is not overridden by a human approval; the head must be fixed or merged manually. +For a PR whose closing claims were superseded by another merged PR, a trusted +maintainer with repository write access can reply `/hive decision reject` or +`/hive decision close superseded by #N`. Hive rechecks that the claimed issues +are closed and a different merged PR closed them; an explicit `#N` must match +that closer. It comments and closes the PR with `hive/superseded`, including +PRs opened by contributors, without pushing to their branches. Hold and +`do-not-merge` gates still prevent the close. When nothing supersedes the PR, +a bare `reject` keeps its ordinary meaning (changes requested, nudging the +author), while an explicit `close superseded by #N` is answered with why the +PR stays open. The sweep's +`hive/supersession-pending` cause label keeps the unresolved decision at 085 +through CI or rebase failures; reopening the issue, rewriting the claim, or +choosing `hive/keep-open` clears the sweep's park on its next pass. + On a **hive-authored** PR (`author/hive`) parked at `stage/085-needs-decision` the same trusted human has a second lever ([#13041](https://github.com/hivecommons/hive/issues/13041)): a GitHub diff --git a/src/pkg/dashboard/api_review_queue_superseded.go b/src/pkg/dashboard/api_review_queue_superseded.go index 97d68ea39d..e81d552255 100644 --- a/src/pkg/dashboard/api_review_queue_superseded.go +++ b/src/pkg/dashboard/api_review_queue_superseded.go @@ -79,7 +79,7 @@ func supersededGraceStates(entries []AuditEntry, org string, grace time.Duration } key := ghpkg.ReviewQueueKey(repo, e.Target) switch e.Action { - case supersessionDetectedAuditAction, supersessionGraceAuditAction: + case supersessionDetectedAuditAction, supersessionGraceAuditAction, ghpkg.SupersessionSweepAuditPrefix + ghpkg.SupersessionActionEscalated: case supersessionCommentedAuditAction: if detected[key] { info := out[key] @@ -104,6 +104,7 @@ func supersededGraceStates(entries []AuditEntry, org string, grace time.Duration } info := ghpkg.ReviewQueueSuperseded{ Reason: ghpkg.ReviewQueueSupersededReason, + KeepReason: fields["reason"], Issue: fields["issue"], CloserRepo: repo, CloserPR: closerPR, @@ -117,7 +118,10 @@ func supersededGraceStates(entries []AuditEntry, org string, grace time.Duration if previous, ok := out[key]; ok && previous.CloserRepo == info.CloserRepo && previous.CloserPR == info.CloserPR && previous.Issue == info.Issue { info.GraceStartedAt, info.GraceEndsAt = previous.GraceStartedAt, previous.GraceEndsAt } - if e.Action == supersessionDetectedAuditAction { + if e.Action != supersessionGraceAuditAction { + if e.Action == ghpkg.SupersessionSweepAuditPrefix+ghpkg.SupersessionActionEscalated { + info.GraceStartedAt, info.GraceEndsAt = time.Time{}, time.Time{} + } detected[key] = true } else if info.GraceStartedAt.IsZero() { info.GraceStartedAt, info.GraceEndsAt = at.UTC(), at.Add(grace).UTC() diff --git a/src/pkg/dashboard/api_review_queue_superseded_test.go b/src/pkg/dashboard/api_review_queue_superseded_test.go index 45adeb9af7..a347d79eb3 100644 --- a/src/pkg/dashboard/api_review_queue_superseded_test.go +++ b/src/pkg/dashboard/api_review_queue_superseded_test.go @@ -15,6 +15,8 @@ import ( "github.com/hivecommons/hive/pkg/config" ghpkg "github.com/hivecommons/hive/pkg/github" + "github.com/hivecommons/hive/pkg/stageengine/prstage" + "github.com/hivecommons/hive/pkg/stageengine/stageenginetest" ) // useRingSupersessionAudit points the queue's on-disk audit scan at an empty @@ -325,6 +327,8 @@ func TestSupersededDetectionReplay(t *testing.T) { start time.Time }{ {"detection only", []AuditEntry{detection}, true, time.Time{}}, + {"escalation retains decision", []AuditEntry{detection, entry("supersession-sweep-commented-needs-human", now)}, true, time.Time{}}, + {"escalation cancels grace", []AuditEntry{detection, grace, entry("supersession-sweep-commented-needs-human", now)}, true, time.Time{}}, {"plain notice", []AuditEntry{detection, notice, repeat}, true, time.Time{}}, {"grace starts at notice", []AuditEntry{detection, grace}, true, now.Add(time.Hour)}, {"repeated detection preserves grace", []AuditEntry{detection, grace, repeat, entry(supersessionGraceAuditAction, now.Add(4*time.Hour))}, true, now.Add(time.Hour)}, @@ -368,11 +372,17 @@ func TestReviewQueue_SweepDetectionWithoutAutoClose(t *testing.T) { useRingSupersessionAudit(t) s := reviewQueueTestServer(t, filepath.Join(t.TempDir(), "missing.json")) var notice string + var labels []string + var labelWrites int var writes int api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.Method + " " + r.URL.Path { case "GET /repos/myorg/repo1/pulls": - fmt.Fprint(w, `[{"number":2,"body":"Fixes #3","user":{"login":"alice"}}]`) + wireLabels := make([]map[string]string, 0, len(labels)) + for _, label := range labels { + wireLabels = append(wireLabels, map[string]string{"name": label}) + } + json.NewEncoder(w).Encode([]map[string]any{{"number": 2, "body": "Fixes #3", "user": map[string]string{"login": "alice"}, "labels": wireLabels}}) case "GET /repos/myorg/repo1/issues/3": fmt.Fprint(w, `{"number":3,"state":"closed"}`) case "POST /graphql": @@ -385,6 +395,12 @@ func TestReviewQueue_SweepDetectionWithoutAutoClose(t *testing.T) { } else { json.NewEncoder(w).Encode([]map[string]any{{"id": 1, "body": notice}}) } + case "POST /repos/myorg/repo1/issues/2/labels": + if err := json.NewDecoder(r.Body).Decode(&labels); err != nil { + t.Error(err) + } + labelWrites++ + fmt.Fprint(w, `[]`) case "POST /repos/myorg/repo1/issues/2/comments": var in struct { Body string `json:"body"` @@ -403,6 +419,10 @@ func TestReviewQueue_SweepDetectionWithoutAutoClose(t *testing.T) { defer api.Close() client := ghpkg.NewClient("token", "myorg", []string{"repo1"}, slog.Default(), api.URL) client.SetAppBotLogin("hive-app[bot]") + // The park goes through the stage engine; record it instead of letting the + // default sink re-read the PR and reconcile against the shared label store. + facts := &stageenginetest.Recorder{} + client.SetStageFactSink(facts) for pass := 0; pass < 2; pass++ { res, err := client.SweepSupersededOpenPRs(context.Background(), ghpkg.SupersessionSweepOptions{ ACMMLevelForRepo: func(string) int { return 6 }, @@ -440,4 +460,16 @@ func TestReviewQueue_SweepDetectionWithoutAutoClose(t *testing.T) { if writes != 1 { t.Fatalf("notice writes = %d, want 1", writes) } + if labelWrites != 1 || strings.Join(labels, ",") != "needs-human,"+prstage.SupersessionPendingLabel { + t.Fatalf("park labels = %v, writes = %d; want one supersession park", labels, labelWrites) + } + var parked int + for _, f := range facts.All() { + if f.Method == "Parked" && f.Ref.Number == 2 && f.Label == "needs-human" { + parked++ + } + } + if parked != 1 { + t.Fatalf("Parked facts for PR 2 = %d, want 1: %+v", parked, facts.All()) + } } diff --git a/src/pkg/dashboard/stage_tracker.go b/src/pkg/dashboard/stage_tracker.go index 6e52fc82e7..3a3309a1fc 100644 --- a/src/pkg/dashboard/stage_tracker.go +++ b/src/pkg/dashboard/stage_tracker.go @@ -213,12 +213,13 @@ type StageTrackerHop struct { // StageTrackerItem is one open issue or PR on the tracker. type StageTrackerItem struct { - Repo string `json:"repo"` - Number int `json:"number"` - Kind string `json:"kind"` - Title string `json:"title"` - URL string `json:"url,omitempty"` - Author string `json:"author,omitempty"` + Superseded *github.ReviewQueueSuperseded `json:"superseded,omitempty"` + Repo string `json:"repo"` + Number int `json:"number"` + Kind string `json:"kind"` + Title string `json:"title"` + URL string `json:"url,omitempty"` + Author string `json:"author,omitempty"` // Head is the PR head SHA the lane last evaluated ("" for issues), so // a "waiting for a verdict" caption can name the revision it waits on. Head string `json:"head,omitempty"` @@ -1304,6 +1305,15 @@ func (s *Server) buildStageTracker(status *StatusPayload, store *stagelabels.Sto return resp } resp.HiveID = status.HiveID + qualifyRepo := stageTrackerRepoQualifier(status) + superseded := map[string]github.ReviewQueueSuperseded{} + entries := append([]AuditEntry(nil), s.supersessionAuditEntries(now)...) + for i := range entries { + entries[i].Repo = qualifyRepo(entries[i].Repo) + } + for key, info := range supersededGraceStates(entries, "", s.supersessionGracePeriod()) { + superseded[key] = info + } var cfg *config.Config if s.deps != nil { cfg = s.deps.Config @@ -1496,6 +1506,18 @@ func (s *Server) buildStageTracker(status *StatusPayload, store *stagelabels.Sto it.DeferredModifiers = append([]string{modSentinel}, it.DeferredModifiers...) } it.LinkedIssues = stageTrackerPRLinkedIssues(pr, repoName, status.GitHubBaseURL) + info, found := superseded[github.ReviewQueueKey(qualifyRepo(repoName), pr.Number)] + if !found { + info, found = superseded[github.ReviewQueueKey(repoName, pr.Number)] + } + if found && !labelSet(pr.Labels)[github.SupersessionKeepOpenLabel] { + it.Superseded = &info + it.Caption = fmt.Sprintf("Superseded by #%d — decide", info.CloserPR) + if info.KeepReason != "" { + it.Caption += ": " + info.KeepReason + } + it.Actor = "maintainer" + } add(it) } for _, repo := range status.Repos { diff --git a/src/pkg/dashboard/stage_tracker_supersession_test.go b/src/pkg/dashboard/stage_tracker_supersession_test.go new file mode 100644 index 0000000000..b84f4f485b --- /dev/null +++ b/src/pkg/dashboard/stage_tracker_supersession_test.go @@ -0,0 +1,62 @@ +package dashboard + +import ( + "strings" + "testing" + "time" + + "github.com/hivecommons/hive/pkg/github" + "github.com/hivecommons/hive/pkg/stagelabels" + "github.com/hivecommons/hive/pkg/worksource" +) + +func TestTrackerSupersessionSurvivesEscalationAndStaleRepairStage(t *testing.T) { + useRingSupersessionAudit(t) + s := newTestServer() + now := time.Now().UTC() + status := minimalPayload() + status.Repos = []FrontendRepo{{Name: "hive", Full: "hivecommons/hive", OpenPrs: []any{ + FrontendPR{PullRequest: github.PullRequest{Number: 14795, Title: "duplicate", Author: "contributor", Labels: []string{"stage/072-returned-to-contributor", "sentinel-alert"}, LinkedIssues: []github.PRLinkedIssue{{Number: 14789, State: "closed"}}}}, + }}} + detail := "repo=hive, pr=14795, issue=hivecommons/hive#14789, closer_pr=14791, reason=extra implementation files" + s.audit.LogRecordAt(now.Add(-time.Minute).Format(time.RFC3339), "system", supersessionDetectedAuditAction, detail, "", "hive", 14795) + s.audit.LogRecordAt(now.Format(time.RFC3339), "system", github.SupersessionSweepAuditPrefix+github.SupersessionActionEscalated, detail, "", "hive", 14795) + store := stagelabels.NewStore("", nil) + store.Observe(stagelabels.Key("hivecommons/hive", 14795), stagelabels.ReturnedToContributor, "check-failing:unit") + resp := s.buildStageTracker(status, store, now) + if len(resp.Items) != 1 { + t.Fatalf("items=%+v", resp.Items) + } + it := resp.Items[0] + if it.Superseded == nil || it.Superseded.CloserPR != 14791 || !strings.Contains(it.Caption, "Superseded by #14791 — decide") || !strings.Contains(it.Caption, "extra implementation files") { + t.Fatalf("supersession lost to repair stage: %+v", it) + } + if it.Actor != "maintainer" { + t.Fatal(it.Actor) + } + s.audit.LogRecord("owner", supersessionKeepOpenAction, "", "", "hive", 14795) + if it := s.buildStageTracker(status, store, now).Items[0]; it.Superseded != nil { + t.Fatalf("kept-open still needs decision: %+v", it) + } +} + +func TestClosedSnapshotIsNeverActionable(t *testing.T) { + raw := []any{map[string]any{"number": 1, "state": "closed"}, map[string]any{"number": 2, "state": "open"}} + var offered []string + forEachActionableIssue(nil, "test", "o/r", raw, func(_ map[string]any, ref worksource.Ref) { offered = append(offered, ref.Key()) }) + if len(offered) != 1 || offered[0] != "o/r#2" { + t.Fatalf("offered=%v", offered) + } +} + +func TestSupersededRepairRowHasDecisionActionAndHeadline(t *testing.T) { + html := indexHTML(t) + script := stageTrackerRowActionsPreamble(t, html) + "\n" + jsFunc(t, html, "stageTrackerHeadline") + "\n" + jsFunc(t, html, "stageTrackerCurrentStageHtml") + ` + const item = {kind:'pr',repo:'o/r',number:12,stage:72,superseded:{closer_pr:22}}; + assert.strictEqual(stageTrackerHeadline(item), 'Superseded by #22 — decide'); + assert.ok(stageTrackerCurrentStageHtml(item, [], null, 0).includes('Superseded by #22')); + assert.strictEqual(stageTrackerDecisionTarget(item), item); + assert.ok(stageTrackerRowActions(item, {canHold:true}).some(a => a.id === 'decide')); + ` + runStageTrackerRowActionsScript(t, script) +} diff --git a/src/pkg/dashboard/static/index.html b/src/pkg/dashboard/static/index.html index c81e5bd0c2..5cddbd008d 100644 --- a/src/pkg/dashboard/static/index.html +++ b/src/pkg/dashboard/static/index.html @@ -13927,6 +13927,7 @@

Where to get help

var _stageTrackerLabelQuery = ''; function stageTrackerHeadline(item) { + if (item.superseded && item.superseded.closer_pr) return 'Superseded by #' + item.superseded.closer_pr + ' — decide'; const n = Number(item.stage) || 0; const mods = Array.isArray(item.modifiers) ? item.modifiers : []; const holdLabels = stageTrackerHoldLabels(item); @@ -14190,6 +14191,7 @@

Where to get help

// Compact strips retain their column alignment; the current step gets a // full-width caption so detours and the next actor never need a hover. function stageTrackerCurrentStageHtml(item, stages, range, nowMs) { + if (item.superseded && item.superseded.closer_pr) return `

${esc(stageTrackerHeadline(item) + (item.superseded.keep_reason ? " · " + item.superseded.keep_reason : ""))}

`; const active = stageTrackerSegments(item, stages, nowMs).find(sg => sg.state === 'active' && (!range || range.from == null || sg.number >= range.from) && (!range || range.to == null || sg.number <= range.to)); @@ -15828,7 +15830,7 @@

All labels

function stageTrackerDecisionTarget(item) { if (!item) return null; const mods = Array.isArray(item.modifiers) ? item.modifiers : []; - const parked = it => (Array.isArray(it.modifiers) ? it.modifiers : []).indexOf('needs-decision') >= 0 || STAGE_TRACKER_DECISION_STAGES.indexOf(Number(it.stage) || 0) >= 0; + const parked = it => !!it.superseded || (Array.isArray(it.modifiers) ? it.modifiers : []).indexOf('needs-decision') >= 0 || STAGE_TRACKER_DECISION_STAGES.indexOf(Number(it.stage) || 0) >= 0; if (item.kind === 'pr') return parked(item) ? item : null; for (const lp of (Array.isArray(item.linked_prs) ? item.linked_prs : [])) { const pr = { repo: item.repo, number: lp.number, kind: 'pr', title: lp.title || '', url: lp.url || '', stage: lp.stage || 0, modifiers: lp.modifiers || [], deferred_modifiers: lp.deferred_modifiers || [], author: lp.author || '', diff --git a/src/pkg/dashboard/worksource_identity.go b/src/pkg/dashboard/worksource_identity.go index 6b262248e8..33a9fb57c7 100644 --- a/src/pkg/dashboard/worksource_identity.go +++ b/src/pkg/dashboard/worksource_identity.go @@ -3,6 +3,7 @@ package dashboard import ( "encoding/json" "log/slog" + "strings" ghpkg "github.com/hivecommons/hive/pkg/github" "github.com/hivecommons/hive/pkg/worksource" @@ -104,6 +105,9 @@ func forEachActionableIssue(logger *slog.Logger, logTag string, repoFull string, } continue } + if state, _ := issue["state"].(string); strings.EqualFold(state, "closed") { + continue + } ref := refFromIssueMap(repoFull, issue) if ref.Key() == "" { if logger != nil { diff --git a/src/pkg/github/automerge/automerge_stage_labels.go b/src/pkg/github/automerge/automerge_stage_labels.go index 6e687c20b3..bf6a602bae 100644 --- a/src/pkg/github/automerge/automerge_stage_labels.go +++ b/src/pkg/github/automerge/automerge_stage_labels.go @@ -807,7 +807,7 @@ func (c *Engine) ApplyStageLabels(ctx context.Context) (StageLabelResult, error) obs.Reason = replacement.Reason } var route stagelabels.Route - if verdict.Human && !verdict.Terminal { + if verdict.Human && !verdict.Terminal && verdict.Reason != prstage.ReasonSupersessionPending { var evidence stagelabels.Evidence route, evidence = c.routeHuman(ctx, tracker, key, obs, verdict.Stage, now) pass.Route = &evidence @@ -824,7 +824,7 @@ func (c *Engine) ApplyStageLabels(ctx context.Context) (StageLabelResult, error) // the label state, which is reset below for informational no-dco and // is unknown for heads with nothing to sign. dcoRead := false - if !verdict.Terminal && verdict.Stage != stagelabels.None { + if !verdict.Terminal && verdict.Stage != stagelabels.None && verdict.Reason != prstage.ReasonSupersessionPending { // The commit check is the one author-side gate decided here, // before CI or a verdict: an unsigned head goes straight back // to its author (hivecommons/hive#12542). diff --git a/src/pkg/github/automerge/automerge_stage_supersession_test.go b/src/pkg/github/automerge/automerge_stage_supersession_test.go new file mode 100644 index 0000000000..7c6e1116fb --- /dev/null +++ b/src/pkg/github/automerge/automerge_stage_supersession_test.go @@ -0,0 +1,37 @@ +package automerge + +import ( + "context" + "slices" + "testing" + "time" + + "github.com/hivecommons/hive/pkg/stageengine/prstage" + "github.com/hivecommons/hive/pkg/stagelabels" +) + +func TestApplyStageLabelsKeepsSupersessionParkOverConflictAndUnsignedHead(t *testing.T) { + api, closeAPI := seededStageAPI(t) + defer closeAPI() + clock := &contributorLaneClock{at: time.Date(2026, 10, 28, 0, 0, 0, 0, time.UTC)} + e, _, store := newStageEngine(t, apiURLOf(t, api), clock) + api.commits = map[int][]string{14795: {"unsigned commit"}} + pr := nudgePR(14795, "head", "needs-human", prstage.SupersessionPendingLabel, "needs-rebase") + e.recordStage(stageLaneTrusted, "acme/widget", "acme", "widget", pr, "check-failing:unit", false, nil) + e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", pr, "conflicting", false, nil) + if _, err := e.ApplyStageLabels(context.Background()); err != nil { + t.Fatal(err) + } + rec, ok := store.Get(stageKey("acme", "widget", 14795)) + if !ok || rec.Stage != stagelabels.NeedsDecision || rec.Reason != prstage.ReasonSupersessionPending { + t.Fatalf("record=%+v", rec) + } + for _, added := range api.added { + if slices.Contains(added, stagelabels.ReturnedToContributor.Label()) || slices.Contains(added, stagelabels.NeedsAuthor.Label()) { + t.Fatalf("sent to author: %v", api.added) + } + } + if api.commitReads != 0 { + t.Fatalf("superseded head read for repair: %d", api.commitReads) + } +} diff --git a/src/pkg/github/pr_supersession_autoclose.go b/src/pkg/github/pr_supersession_autoclose.go index f143bc53a9..eeec4eb23d 100644 --- a/src/pkg/github/pr_supersession_autoclose.go +++ b/src/pkg/github/pr_supersession_autoclose.go @@ -184,7 +184,7 @@ func renderSupersessionGraceNotice(grace time.Duration) string { if grace <= 0 { grace = DefaultSupersessionGracePeriod } - return fmt.Sprintf("A different merged PR closed the claimed issue and every file this PR touches was also changed by it. "+ + return fmt.Sprintf("A different merged PR closed the claimed issue and its implementation files were also changed by it (additional tests, docs and changelog fragments are allowed). "+ "Unless someone comments here, the sweep will close this PR as superseded once %s have passed since this notice. "+ "Reply on this PR to keep it open.", formatSupersessionGrace(grace)) } @@ -207,7 +207,7 @@ func renderSupersessionCloseComment(closer supersessionCloser, grace time.Durati if !closer.MergedAt.IsZero() { merged = "which merged at " + closer.MergedAt.UTC().Format("2006-01-02 15:04Z") } - fmt.Fprintf(&b, "Closing as superseded by %s, %s. Every file this PR touches was also changed there and nobody replied within the %s grace window, so nothing from this branch looks still needed.\n", + fmt.Fprintf(&b, "Closing as superseded by %s, %s. Its implementation files were also changed there (additional tests, docs and changelog fragments are allowed) and nobody replied within the %s grace window, so nothing from this branch looks still needed.\n", ref, merged, formatSupersessionGrace(grace)) fmt.Fprintln(&b) fmt.Fprintf(&b, "Reopen if that's wrong. The `%s` label marks PRs the supersession sweep closed.\n", SupersededLabel) @@ -317,7 +317,7 @@ func (c *Client) supersessionReviewThreads(ctx context.Context, owner, repo stri // are a subset of closer's. The grace window runs from the notice's last // edit (so a reworded notice restarts it); a human reply is detected from the // notice's creation, so a later re-edit can never hide an earlier reply. -func (c *Client) trySupersessionAutoCloseContributorPR(ctx context.Context, owner, repo string, pr *gh.PullRequest, event SupersessionSweepEvent, closer supersessionCloser, identity HiveIdentity, now time.Time, grace time.Duration) (SupersessionSweepEvent, string, error) { +func (c *Client) trySupersessionAutoCloseContributorPR(ctx context.Context, owner, repo string, pr *gh.PullRequest, event SupersessionSweepEvent, closer supersessionCloser, identity HiveIdentity, now time.Time, grace time.Duration, closingClaim bool) (SupersessionSweepEvent, string, error) { if grace <= 0 { grace = DefaultSupersessionGracePeriod } @@ -388,9 +388,13 @@ func (c *Client) trySupersessionAutoCloseContributorPR(ctx context.Context, owne } return event, "commented", nil default: + event.Reason = decision.Reason + notice := supersessionDecisionNotice(owner, closer, decision.Reason) + if closingClaim { + return c.escalateSupersededPR(ctx, owner+"/"+repo, owner, repo, pr, event, closer, notice) + } event.Action = SupersessionActionCommentedContributor - body := renderSupersessionComment(event, closer, supersessionDecisionNotice(owner, closer, decision.Reason)) - if _, err := c.ensureSupersessionComment(ctx, owner, repo, number, body); err != nil { + if _, err := c.ensureSupersessionComment(ctx, owner, repo, number, renderSupersessionComment(event, closer, notice)); err != nil { return event, "", err } return event, "commented", nil diff --git a/src/pkg/github/pr_supersession_autoclose_test.go b/src/pkg/github/pr_supersession_autoclose_test.go index cc0d229f6c..1d2c6fdd3b 100644 --- a/src/pkg/github/pr_supersession_autoclose_test.go +++ b/src/pkg/github/pr_supersession_autoclose_test.go @@ -300,7 +300,7 @@ func TestSupersessionAutoClose_Guards(t *testing.T) { if len(res.Commented) != 1 || res.Commented[0].Action != SupersessionActionEscalated { t.Fatalf("guard %q result = %+v, want escalation", tc.name, res) } - if got := obs.labels[12]; len(got) != 1 || got[0] != issueNeedsHumanLabel { + if got := obs.labels[12]; len(got) != 2 || got[0] != issueNeedsHumanLabel { t.Fatalf("guard %q labels = %v, want needs-human", tc.name, got) } if got := obs.edits[900]; !strings.Contains(got, "Files the merged PR did not touch: `pkg/c.go`") { diff --git a/src/pkg/github/pr_supersession_decision.go b/src/pkg/github/pr_supersession_decision.go new file mode 100644 index 0000000000..fa0a63f610 --- /dev/null +++ b/src/pkg/github/pr_supersession_decision.go @@ -0,0 +1,69 @@ +package github + +import ( + "context" + "fmt" + "strconv" + "strings" + "time" + + gh "github.com/google/go-github/v72/github" +) + +// supersessionCloseDecision accepts only an explicit close instruction. A bare +// reject is safe only after detecting a live supersession below; other decision +// text continues to the existing changes-requested path. +func supersessionCloseDecision(command unparkCommand) (closer int, ok bool) { + if command.Kind != unparkCommandDecision { + return 0, false + } + text := strings.ToLower(strings.TrimSpace(command.DecisionText())) + if text == "reject" { + return 0, true + } + const prefix = "close superseded by #" + if !strings.HasPrefix(text, prefix) { + return 0, false + } + n, err := strconv.Atoi(strings.TrimPrefix(text, prefix)) + return n, err == nil && n > 0 +} + +// Called only after eligibleUnparkCommand and TrustedMerger authorize the actor. +// No contributor branch is pushed or rewritten. +// +// handled is false when a bare `reject` finds no live supersession (or the PR +// is held): the decision is not about supersession, so the caller continues +// to the existing changes-requested path. An explicit `close superseded by +// #N` is always answered here, since it can mean nothing else. +func (c *Client) closeSupersededPRDecision(ctx context.Context, displayRepo, owner, repo string, pr *gh.PullRequest, actor string, requestedCloser int) (reply string, handled bool, err error) { + explicit := requestedCloser != 0 + if labels := labelNames(pr.Labels); c.isHeld(labels) || c.isExempt(labels) { + if !explicit { + return "", false, nil + } + return "The PR is held or exempt from hive writes; clear that gate before closing it as superseded.", true, nil + } + identity := c.getHiveIdentity() + candidate, closer, detected, err := c.detectSupersession(ctx, displayRepo, pr, identity, time.Now()) + if err != nil { + return "", false, err + } + if !detected || !candidate.Closing { + if !explicit { + return "", false, nil + } + return "No merged PR currently supersedes this PR's closing claims; the PR stays open.", true, nil + } + if explicit && (requestedCloser != closer.Number || !strings.EqualFold(closer.Repo, owner+"/"+repo)) { + return fmt.Sprintf("The detected closer is %s#%d, which does not match the requested PR; the PR stays open.", closer.Repo, closer.Number), true, nil + } + note := fmt.Sprintf("Closing as superseded by %s#%d, following @%s's `/hive decision`. No branch changes are required.", closer.Repo, closer.Number, actor) + if err := c.CreateIssueComment(ctx, displayRepo, pr.GetNumber(), note); err != nil { + return "", false, err + } + if err := c.CloseSupersededPR(ctx, displayRepo, pr.GetNumber()); err != nil { + return "", false, err + } + return fmt.Sprintf("Closed as superseded by %s#%d, following @%s's decision.", closer.Repo, closer.Number, actor), true, nil +} diff --git a/src/pkg/github/pr_supersession_decision_test.go b/src/pkg/github/pr_supersession_decision_test.go new file mode 100644 index 0000000000..26ba399325 --- /dev/null +++ b/src/pkg/github/pr_supersession_decision_test.go @@ -0,0 +1,191 @@ +package github + +import ( + "context" + "strings" + "testing" + "time" + + gh "github.com/google/go-github/v72/github" + "github.com/hivecommons/hive/pkg/review" + "github.com/hivecommons/hive/pkg/stageengine/prstage" +) + +func TestSupersessionCloseDecisionIsExplicit(t *testing.T) { + for _, text := range []string{"reject", "close superseded by #22"} { + cmd, ok := parseUnparkCommand("/hive decision " + text) + if !ok { + t.Fatal(text) + } + if _, ok := supersessionCloseDecision(cmd); !ok { + t.Fatal(text) + } + } + for _, text := range []string{"reject the old design and fix it", "close superseded by #0", "close superseded by #22\nkeep tests", "approve"} { + cmd, _ := parseUnparkCommand("/hive decision " + text) + if _, ok := supersessionCloseDecision(cmd); ok { + t.Fatalf("treated instruction as close: %q", text) + } + } +} + +func TestContributorSupersessionDecisionClosesWithoutPush(t *testing.T) { + for _, tc := range []struct { + name, state string + requested int + labels []string + closes bool + // handled is false only for a bare reject that found no live + // supersession: the decision then continues to the + // changes-requested path instead of being answered here. + handled bool + }{ + {name: "reject", state: "closed", closes: true, handled: true}, + {name: "explicit closer", state: "closed", requested: 22, closes: true, handled: true}, + {name: "wrong closer", state: "closed", requested: 23, handled: true}, + {name: "reopened issue", state: "open"}, + {name: "reopened issue explicit", state: "open", requested: 22, handled: true}, + {name: "held", state: "closed", labels: []string{"hold"}}, + {name: "held explicit", state: "closed", requested: 22, labels: []string{"hold"}, handled: true}, + } { + t.Run(tc.name, func(t *testing.T) { + prs := autoClosePRs(supersessionPRFixture{Body: "Fixes #3", Labels: tc.labels}) + server, obs := supersessionSweepServer(t, autoCloseOrg, autoCloseRepo, prs, []supersessionIssueFixture{{Number: 3, State: tc.state, GraphQLCloserPR: 22}}) + defer server.Close() + c := newTestClient(t, server, autoCloseOrg, []string{autoCloseRepo}) + c.SetAppBotLogin(autoCloseBot) + pr := &gh.PullRequest{Number: gh.Ptr(12), Body: gh.Ptr("Fixes #3"), User: &gh.User{Login: gh.Ptr("contributor")}} + for _, l := range tc.labels { + pr.Labels = append(pr.Labels, &gh.Label{Name: gh.Ptr(l)}) + } + reply, handled, err := c.closeSupersededPRDecision(context.Background(), autoCloseOrg+"/"+autoCloseRepo, autoCloseOrg, autoCloseRepo, pr, "maintainer", tc.requested) + if err != nil { + t.Fatal(err) + } + if handled != tc.handled || (!handled && reply != "") { + t.Fatalf("handled=%t reply=%q, want handled=%t", handled, reply, tc.handled) + } + if (len(obs.closed) == 1) != tc.closes { + t.Fatalf("closed=%v reply=%s", obs.closed, reply) + } + if tc.closes && !strings.Contains(reply, "#22") { + t.Fatal(reply) + } + if len(obs.updated) != 0 || len(obs.bodies) != 0 { + t.Fatalf("contributor branch mutated: %+v", obs) + } + }) + } +} + +func TestSupersessionSupplementalFilesNeedImplementationOverlap(t *testing.T) { + closer := map[string]bool{"pkg/fix.go": true} + for _, tc := range []struct { + name string + open map[string]bool + want int + }{ + {"ancillary only extras", map[string]bool{"pkg/fix.go": true, "pkg/fix_test.go": true, "src/docs/fix.md": true, "changelog.d/fixed-fix.md": true}, 0}, + {"unrelated implementation", map[string]bool{"pkg/other.go": true, "pkg/other_test.go": true}, 2}, + {"extra implementation", map[string]bool{"pkg/fix.go": true, "pkg/other.go": true, "pkg/fix_test.go": true}, 1}, + {"docs only", map[string]bool{"docs/fix.md": true}, 1}, + } { + if got := supersessionBlockingExtraFiles(tc.open, closer); len(got) != tc.want { + t.Fatalf("%s: %v", tc.name, got) + } + } +} + +func TestUntrustedSupersessionRejectCannotCloseContributorPR(t *testing.T) { + issue := parkedPR(12, "contributor", []string{PRNeedsDecisionStageLabel}, humanComment(4242, "reader", "/hive decision reject")) + c, rec := newUnparkPRClient(t, []unparkWireIssue{issue}, map[string]string{"reader": "read"}) + result, err := c.SweepIssueUnparkCommands(context.Background(), IssueUnparkSweepOptions{TrustedMerger: trustedMergerSet("reader")}) + if err != nil { + t.Fatal(err) + } + if result.Replies != 0 || len(rec.posted[12]) != 0 || len(rec.patched[12]) != 0 || len(rec.added[12]) != 0 { + t.Fatalf("unauthorized command acted: %+v %+v", result, rec) + } +} + +func TestHeldSupersessionRemainsObservableWithoutWrites(t *testing.T) { + pr := supersessionPRFixture{Body: "Fixes #3", Labels: []string{"sentinel-alert"}} + server, obs := supersessionSweepServer(t, autoCloseOrg, autoCloseRepo, autoClosePRs(pr), []supersessionIssueFixture{{Number: 3, State: "closed", GraphQLCloserPR: 22}}) + defer server.Close() + c := newTestClient(t, server, autoCloseOrg, []string{autoCloseRepo}) + c.SetAppBotLogin(autoCloseBot) + c.SetHoldLabels([]string{"sentinel-alert"}) + var events []SupersessionSweepEvent + opts := autoCloseEnabled + opts.Audit = func(e SupersessionSweepEvent) { events = append(events, e) } + if _, err := c.SweepSupersededOpenPRs(context.Background(), opts); err != nil { + t.Fatal(err) + } + if len(events) != 1 || events[0].CloserPR != 22 || events[0].Action != SupersessionActionDetectedContributor { + t.Fatalf("events=%+v", events) + } + if len(obs.closed) != 0 || len(obs.comments[12]) != 0 || len(obs.labels[12]) != 0 { + t.Fatalf("hold bypassed: %+v", obs) + } +} + +func TestSupplementalFilesPermitGraceCloseButNotLargePRClose(t *testing.T) { + body := graceNoticeBody(t) + old := time.Now().Add(-25 * time.Hour) + for _, size := range []string{"size/S", "size/L"} { + res, obs := runAutoCloseSweep(t, supersessionPRFixture{Body: "Fixes #3", Labels: []string{size}, Files: []string{"pkg/a.go", "pkg/a_test.go", "changelog.d/fixed-a.md"}, Comments: []supersessionCommentFixture{noticeComment(body, old)}}, autoCloseEnabled) + if size == "size/S" { + if len(obs.closed) != 1 { + t.Fatalf("ancillary files blocked closure: %+v", res) + } + } else if len(obs.closed) != 0 || len(res.Commented) != 1 || res.Commented[0].Action != SupersessionActionEscalated { + t.Fatalf("large PR must need a decision: %+v closed=%v", res, obs.closed) + } + } +} + +func TestReopenedIssueClearsSupersessionCauseWithoutClearingHumanPark(t *testing.T) { + pr := supersessionPRFixture{Body: "Fixes #3", Labels: []string{issueNeedsHumanLabel, prstage.SupersessionPendingLabel}} + server, obs := supersessionSweepServer(t, autoCloseOrg, autoCloseRepo, autoClosePRs(pr), []supersessionIssueFixture{{Number: 3, State: "open"}}) + defer server.Close() + c := newTestClient(t, server, autoCloseOrg, []string{autoCloseRepo}) + c.SetAppBotLogin(autoCloseBot) + result, err := c.SweepSupersededOpenPRs(context.Background(), autoCloseEnabled) + if err != nil { + t.Fatal(err) + } + removed := obs.removedLabels[12] + if len(removed) != 1 || removed[0] != prstage.SupersessionPendingLabel || len(result.Resolved) != 1 { + t.Fatalf("removed=%v result=%+v", removed, result) + } +} + +// A bare `reject` on a hive PR nothing supersedes is an instruction, not a +// close: it still reaches the changes-requested path (the author is kicked) +// instead of being answered with "the PR stays open". +func TestBareRejectWithoutSupersessionStillRequestsChanges(t *testing.T) { + claimed := unparkWireIssue{Number: 1, Title: "tunnel drain", State: "open", User: wireUser{Login: "reporter"}} + issues := []unparkWireIssue{claimed, hiveParkedPR(humanComment(4242, "clubanderson", "/hive decision reject"))} + c, rec := newUnparkPRClient(t, issues, map[string]string{"clubanderson": "admin"}) + store := review.NewHumanChangeRequests("") + result, err := c.SweepIssueUnparkCommands(context.Background(), IssueUnparkSweepOptions{HumanChangeRequests: store, TrustedMerger: trustedMergerSet("clubanderson")}) + if err != nil { + t.Fatalf("sweep: %v", err) + } + if len(result.PRDecisions) != 1 || result.Replies != 0 || result.Skipped != 0 { + t.Fatalf("result = %+v, want one PR decision routed as changes requested", result) + } + if req, ok := store.For("org/repo", liveDecisionPR, liveDecisionHead); !ok || req.Body != "reject" { + t.Fatalf("recorded request = %+v, %v", req, ok) + } + if len(rec.patched[liveDecisionPR]) != 0 { + t.Fatalf("PR closed: %v", rec.patched[liveDecisionPR]) + } + if len(rec.posted[liveDecisionPR]) != 1 || strings.Contains(rec.posted[liveDecisionPR][0], "stays open") { + t.Fatalf("reply = %v, want the changes-requested reply", rec.posted[liveDecisionPR]) + } + facts := prDecisionFacts(t, c) + if len(facts) != 1 || !facts[0].Instruction { + t.Fatalf("stage facts = %+v, want one PRDecided instruction", facts) + } +} diff --git a/src/pkg/github/pr_supersession_resolve.go b/src/pkg/github/pr_supersession_resolve.go index 227e432d67..c243d24339 100644 --- a/src/pkg/github/pr_supersession_resolve.go +++ b/src/pkg/github/pr_supersession_resolve.go @@ -10,6 +10,7 @@ import ( gh "github.com/google/go-github/v72/github" "github.com/hivecommons/hive/pkg/stageengine" + "github.com/hivecommons/hive/pkg/stageengine/prstage" "github.com/hivecommons/hive/pkg/stagelabels" ) @@ -142,7 +143,7 @@ func supersessionExtraFiles(open, closer map[string]bool) []string { // beside the label until the automerge sweep next looks (#13782 wore // stage/050-review-pending + needs-human for fifteen minutes). func (c *Client) parkSupersededPR(ctx context.Context, prRepo string, number int) error { - if err := c.AddLabels(ctx, prRepo, number, []string{issueNeedsHumanLabel}); err != nil { + if err := c.AddLabels(ctx, prRepo, number, []string{issueNeedsHumanLabel, prstage.SupersessionPendingLabel}); err != nil { return fmt.Errorf("labeling superseded PR %s#%d: %w", prRepo, number, err) } sink := c.StageFacts() @@ -170,6 +171,11 @@ func (c *Client) supersessionNeedsHumanByPerson(ctx context.Context, owner, repo // hive has taken the resolution over. The stage engine re-derives the lane // stage from the next automerge pass (leaving 085 is never a regression). func (c *Client) releaseSupersessionPark(ctx context.Context, prRepo string, pr *gh.PullRequest) error { + if supersessionHasLabel(pr, prstage.SupersessionPendingLabel) { + if err := c.RemoveLabel(ctx, prRepo, pr.GetNumber(), prstage.SupersessionPendingLabel); err != nil { + return err + } + } if !supersessionHasLabel(pr, issueNeedsHumanLabel) { return nil } @@ -199,25 +205,28 @@ const supersessionEscalationMark = "**Decision needed" // one the hive cannot attribute, or one next to no sweep notice is left // alone. The lane re-derives the stage from the labels on its next pass. func (c *Client) releaseStaleSupersessionPark(ctx context.Context, prRepo, owner, repo string, pr *gh.PullRequest) (SupersessionSweepEvent, string, error) { - if !supersessionHasLabel(pr, issueNeedsHumanLabel) { - return SupersessionSweepEvent{}, "", nil - } - notice, err := c.findSupersessionComment(ctx, owner, repo, pr.GetNumber()) - if err != nil { - return SupersessionSweepEvent{}, "", err + changed := false + if supersessionHasLabel(pr, prstage.SupersessionPendingLabel) { + if err := c.RemoveLabel(ctx, prRepo, pr.GetNumber(), prstage.SupersessionPendingLabel); err != nil { + return SupersessionSweepEvent{}, "", err + } + changed = true } - if notice == nil || !strings.Contains(notice.GetBody(), supersessionEscalationMark) { - return SupersessionSweepEvent{}, "", nil + if supersessionHasLabel(pr, issueNeedsHumanLabel) { + notice, err := c.findSupersessionComment(ctx, owner, repo, pr.GetNumber()) + if err != nil { + return SupersessionSweepEvent{}, "", err + } + if notice != nil && strings.Contains(notice.GetBody(), supersessionEscalationMark) && !c.supersessionNeedsHumanByPerson(ctx, owner, repo, pr) { + if err := c.RemoveLabel(ctx, prRepo, pr.GetNumber(), issueNeedsHumanLabel); err != nil { + return SupersessionSweepEvent{}, "", err + } + changed = true + } } - if c.supersessionNeedsHumanByPerson(ctx, owner, repo, pr) { + if !changed { return SupersessionSweepEvent{}, "", nil } - if err := c.releaseSupersessionPark(ctx, prRepo, pr); err != nil { - return SupersessionSweepEvent{}, "", err - } - if c.logger != nil { - c.logger.Info("supersession sweep: released its needs-human, the cause is gone", "repo", prRepo, "pr", pr.GetNumber()) - } event := SupersessionSweepEvent{Repo: prRepo, Number: pr.GetNumber(), Author: safeGetLogin(pr.GetUser()), Action: SupersessionActionReleased} return event, "resolved", nil } @@ -375,6 +384,12 @@ func (c *Client) escalateSupersededPR(ctx context.Context, prRepo, owner, repo s return event, "", err } if !changed { + if supersessionHasLabel(pr, issueNeedsHumanLabel) && !supersessionHasLabel(pr, prstage.SupersessionPendingLabel) { + if err := c.parkSupersededPR(ctx, prRepo, pr.GetNumber()); err != nil { + return event, "", err + } + return event, "commented", nil + } return event, "", nil } if err := c.parkSupersededPR(ctx, prRepo, pr.GetNumber()); err != nil { @@ -391,3 +406,29 @@ func supersessionSortedFiles(files map[string]bool) []string { sort.Strings(out) return out } + +// Ignore ancillary additions only when at least one implementation file also +// overlaps. A docs-only PR or unrelated implementation is never a duplicate +// merely because it mentions the same closed issue. +func supersessionBlockingExtraFiles(open, closer map[string]bool) []string { + ancillary := func(f string) bool { + return strings.HasSuffix(f, "_test.go") || strings.HasSuffix(f, ".md") || strings.HasPrefix(f, "docs/") || strings.Contains(f, "/docs/") || strings.HasPrefix(f, "changelog.d/") + } + overlap := false + for f := range open { + if closer[f] && !ancillary(f) { + overlap = true + } + } + extra := supersessionExtraFiles(open, closer) + if !overlap { + return extra + } + var blocking []string + for _, f := range extra { + if !ancillary(f) { + blocking = append(blocking, f) + } + } + return blocking +} diff --git a/src/pkg/github/pr_supersession_resolve_test.go b/src/pkg/github/pr_supersession_resolve_test.go index cb4b956944..9b19347e1a 100644 --- a/src/pkg/github/pr_supersession_resolve_test.go +++ b/src/pkg/github/pr_supersession_resolve_test.go @@ -232,7 +232,7 @@ func TestSupersessionResolve_BelowL6EscalatesOwnPRThroughStageEngine(t *testing. if len(parked) != 1 || parked[0] != 13782 { t.Fatalf("parked = %v, want #13782 at 085", parked) } - if got := obs.labels[13782]; len(got) != 1 || got[0] != issueNeedsHumanLabel { + if got := obs.labels[13782]; len(got) != 2 || got[0] != issueNeedsHumanLabel { t.Fatalf("labels = %v", got) } if obs.updated[13782] != "" || len(obs.closed) != 0 { @@ -248,7 +248,7 @@ func TestSupersessionResolve_ContributorExtraFilesEscalatesWithFiles(t *testing. if len(parked) != 1 || parked[0] != 13782 { t.Fatalf("parked = %v, want #13782 at 085", parked) } - if got := obs.labels[13782]; len(got) != 1 || got[0] != issueNeedsHumanLabel { + if got := obs.labels[13782]; len(got) != 2 || got[0] != issueNeedsHumanLabel { t.Fatalf("labels = %v", got) } comment := obs.comments[13782][0] diff --git a/src/pkg/github/pr_supersession_sweep.go b/src/pkg/github/pr_supersession_sweep.go index a33fc286b4..09a3dbbdf2 100644 --- a/src/pkg/github/pr_supersession_sweep.go +++ b/src/pkg/github/pr_supersession_sweep.go @@ -48,6 +48,7 @@ type SupersessionSweepEvent struct { Issue int CloserPR int Action string + Reason string } type SupersessionSweepResult struct { @@ -153,17 +154,18 @@ func (c *Client) listOpenPRsForSupersessionSweep(ctx context.Context, owner, rep } func (c *Client) trySweepSupersededPR(ctx context.Context, prRepo, prOwner, prRepoName string, pr *gh.PullRequest, identity HiveIdentity, now time.Time, opts SupersessionSweepOptions) (SupersessionSweepEvent, string, error) { - // A hold or do-not-merge label is an operator's explicit instruction to - // leave the PR alone. Honor it the same way the automerge sweep does: - // no comment, no label, no close — before any GitHub call is made. - if labels := labelNames(pr.Labels); c.isHeld(labels) || c.isExempt(labels) { - return SupersessionSweepEvent{}, "", nil - } + // Holds gate writes, not detection: a sentinel must not hide supersession + // from the tracker. No comments, labels or closes happen while held. + labels := labelNames(pr.Labels) + held := c.isHeld(labels) || c.isExempt(labels) candidate, closer, detected, err := c.detectSupersession(ctx, prRepo, pr, identity, now) if err != nil { return SupersessionSweepEvent{}, "", err } if !detected { + if held { + return SupersessionSweepEvent{}, "", nil + } // Nothing supersedes this PR (any more). A park an earlier pass // put on it for a cause that has since gone — the claimed issue was // reopened, the claim was rewritten — clears with that cause @@ -180,6 +182,14 @@ func (c *Client) trySweepSupersededPR(ctx context.Context, prRepo, prOwner, prRe CloserPR: closer.Number, } + if held { + if opts.Audit != nil && !identity.Matches(event.Author) { + event.Action = SupersessionActionDetectedContributor + event.Reason = "held; awaiting an operator decision" + opts.Audit(event) + } + return event, "", nil + } reason := "" openFiles, closerFiles, err := c.prFileSets(ctx, prOwner, prRepoName, pr.GetNumber(), closer) if err != nil { @@ -200,6 +210,14 @@ func (c *Client) trySweepSupersededPR(ctx context.Context, prRepo, prOwner, prRe return c.resolveOwnSupersededPR(ctx, prRepo, prOwner, prRepoName, pr, event, closer, closerFiles, subset, level) } + blockingExtra := supersessionBlockingExtraFiles(openFiles, closerFiles) + if len(blockingExtra) == 0 { + reason = "" + } + event.Reason = reason + if reason == "" { + _, event.Reason = supersessionSizeAllowsAutoClose(supersessionSizeLabel(pr), -1) + } // Record every detection, even when the notice is already present or // automatic closure is disabled. This does not consume a write action. if opts.Audit != nil { @@ -212,12 +230,16 @@ func (c *Client) trySweepSupersededPR(ctx context.Context, prRepo, prOwner, prRe event.Action = SupersessionActionKeptOpen opts.Audit(event) } + if supersessionHasLabel(pr, issueNeedsHumanLabel) { + return c.releaseStaleSupersessionPark(ctx, prRepo, prOwner, prRepoName, pr) + } + reason = supersessionReasonKeptOpen } if reason == "" && opts.CloseContributorPRs && level >= acmmLevelAutonomous { - return c.trySupersessionAutoCloseContributorPR(ctx, prOwner, prRepoName, pr, event, closer, identity, now, opts.GracePeriod) + return c.trySupersessionAutoCloseContributorPR(ctx, prOwner, prRepoName, pr, event, closer, identity, now, opts.GracePeriod, candidate.Closing) } notice := supersessionDecisionNotice(prOwner, closer, reason) - if !subset { + if reason == supersessionReasonExtraFiles { notice += "\n\nFiles the merged PR did not touch: " + supersessionFilesSummary(extra) + "." if candidate.Closing { // A contributor's PR that adds files the merged PR did not: the @@ -231,6 +253,9 @@ func (c *Client) trySweepSupersededPR(ctx context.Context, prRepo, prOwner, prRe // and #14090 (Part of / Refs a closed tracker) sat at 085 for // this until a maintainer removed the label (hivecommons/hive#14155). } + if candidate.Closing && reason != supersessionReasonKeptOpen { + return c.escalateSupersededPR(ctx, prRepo, prOwner, prRepoName, pr, event, closer, notice) + } event.Action = SupersessionActionCommentedContributor body := renderSupersessionComment(event, closer, notice) changed, err := c.ensureSupersessionComment(ctx, prOwner, prRepoName, pr.GetNumber(), body) @@ -448,7 +473,7 @@ func supersessionDecisionNotice(operator string, closer supersessionCloser, note return fmt.Sprintf("**Decision needed (%s):** #%d already merged and closed the claimed issue, but %s. Recommended: **keep open** until the extra changes are reviewed, or close as superseded if they are not wanted. Another option: merge anyway.", addressee, closer.Number, note) } var b strings.Builder - fmt.Fprintf(&b, "**Decision needed (%s):** this PR appears to duplicate #%d, which already merged: every file it touches was also changed there (the sweep compares file names, not contents). Recommended: **close as superseded**. Other options: keep open, or merge anyway.", addressee, closer.Number) + fmt.Fprintf(&b, "**Decision needed (%s):** this PR appears to duplicate #%d, which already merged: its implementation files overlap the merged PR (additional tests, docs and changelog fragments do not block this recommendation; the sweep compares file names, not contents). Recommended: **close as superseded**. Other options: keep open, or merge anyway.", addressee, closer.Number) if note != "" { fmt.Fprintf(&b, " The sweep did not close it automatically: %s.", note) } diff --git a/src/pkg/github/pr_supersession_sweep_test.go b/src/pkg/github/pr_supersession_sweep_test.go index da12e25c10..8fed077c94 100644 --- a/src/pkg/github/pr_supersession_sweep_test.go +++ b/src/pkg/github/pr_supersession_sweep_test.go @@ -280,7 +280,8 @@ func supersessionSweepServer(t *testing.T, org, repo string, prs []supersessionP _ = json.NewEncoder(w).Encode(map[string]any{"id": id, "body": payload.Body}) return } - parts := strings.Split(rest, "/") + // A label name may contain slashes; keep the label suffix intact. + parts := strings.SplitN(rest, "/", 3) n, _ := strconv.Atoi(parts[0]) switch { case len(parts) == 1 && r.Method == "GET": @@ -473,7 +474,7 @@ func TestSweepSupersededOpenPRs_BelowL6LabelsOwnPR(t *testing.T) { if len(res.Commented) != 1 || len(obs.closed) != 0 { t.Fatalf("below L6 should comment without closing: result=%+v closed=%v", res, obs.closed) } - if got := obs.labels[13]; len(got) != 1 || got[0] != issueNeedsHumanLabel { + if got := obs.labels[13]; len(got) != 2 || got[0] != issueNeedsHumanLabel { t.Fatalf("labels on #13 = %v, want needs-human", got) } if got := parkedPRs(rec); len(got) != 1 || got[0] != 13 { @@ -649,7 +650,7 @@ func TestSweepSupersededOpenPRs_AllClosingClaimsClosedEscalates(t *testing.T) { if len(res.Commented) != 1 || res.Commented[0].Issue != 30 || res.Commented[0].CloserPR != 40 || res.Commented[0].Action != SupersessionActionEscalated { t.Fatalf("expected needs-human escalation on #30 via #40: %+v", res) } - if got := obs.labels[21]; len(got) != 1 || got[0] != issueNeedsHumanLabel { + if got := obs.labels[21]; len(got) != 2 || got[0] != issueNeedsHumanLabel { t.Fatalf("labels on #21 = %v, want needs-human", got) } if got := parkedPRs(rec); len(got) != 1 || got[0] != 21 { @@ -708,7 +709,7 @@ func TestSweepSupersededOpenPRs_DoesNotReapplyNeedsHumanAfterClear(t *testing.T) t.Fatalf("edited = %v, want %v (edit body: %q)", edited, tc.wantEdited, obs.edits[501]) } if tc.wantLabel { - if len(res.Commented) != 1 || len(obs.labels[22]) != 1 || obs.labels[22][0] != issueNeedsHumanLabel { + if len(res.Commented) != 1 || len(obs.labels[22]) != 2 || obs.labels[22][0] != issueNeedsHumanLabel { t.Fatalf("new escalation must label: result=%+v labels=%v", res, obs.labels[22]) } return @@ -876,6 +877,7 @@ func TestSweepSupersededOpenPRs_ReleasesOwnParkWhenCauseIsGone(t *testing.T) { name: "closing claim still closed: park stands", body: "Fixes #3", issueState: "closed", labels: []string{issueNeedsHumanLabel}, labelEvents: hiveApplied, comments: notice(escalation()), + wantPark: true, }, { name: "closing claim still closed, not yet parked: escalates", diff --git a/src/pkg/github/pr_unpark_decision.go b/src/pkg/github/pr_unpark_decision.go index 81b55ba931..7cc9afdd3e 100644 --- a/src/pkg/github/pr_unpark_decision.go +++ b/src/pkg/github/pr_unpark_decision.go @@ -67,6 +67,19 @@ func (c *Client) tryUnparkPullRequest(ctx context.Context, displayRepo, owner, r selfAuthored := c.isHiveAccount(pr.GetUser()) || labelPresent(issueLabelNames(pr.Labels), authorkind.Hive.Label()) reply := "" + if closer, closeRequested := supersessionCloseDecision(command); closeRequested { + reply, handled, err := c.closeSupersededPRDecision(ctx, displayRepo, owner, repo, pr, actor, closer) + if err != nil { + return false, err + } + if handled { + if err := c.postUnparkReply(ctx, owner, repo, number, candidate.GetID(), reply); err != nil { + return false, err + } + result.Replies++ + return true, nil + } + } switch { case command.Kind == unparkCommandDecision && selfAuthored && head != "": mode := prDecisionModeFor(command) @@ -414,6 +427,7 @@ func renderPRHelpReply(selfAuthored bool, head string) string { } else { b.WriteString("- Leave a **changes requested** review and the author is nudged with your text; the hive does not push to a PR it did not open.\n") } + b.WriteString("- For a superseded PR, reply `/hive decision reject` or `/hive decision close superseded by #N` to close it.\n") fmt.Fprintf(&b, "\nSee <%s>.", maintainerCommandsDocURL) return b.String() } diff --git a/src/pkg/github/review_queue.go b/src/pkg/github/review_queue.go index b3f99b4389..a71808531d 100644 --- a/src/pkg/github/review_queue.go +++ b/src/pkg/github/review_queue.go @@ -269,6 +269,7 @@ const ReviewQueueSupersededReason = "superseded" // ReviewQueueSuperseded describes a PR whose claimed issue was closed by a // different merged PR. Grace timestamps are present only for automatic closure. type ReviewQueueSuperseded struct { + KeepReason string `json:"keep_reason,omitempty"` Reason string `json:"reason"` Issue string `json:"issue,omitempty"` CloserRepo string `json:"closer_repo,omitempty"` diff --git a/src/pkg/stageengine/derive_pr.go b/src/pkg/stageengine/derive_pr.go index 72af92e547..26e3ca9054 100644 --- a/src/pkg/stageengine/derive_pr.go +++ b/src/pkg/stageengine/derive_pr.go @@ -246,7 +246,7 @@ func passDecision(verdict stageVerdict, role stagelabels.Role, obs stageObservat } } var human *stagelabels.Route - if verdict.Human && !verdict.Terminal { + if verdict.Human && !verdict.Terminal && verdict.Reason != prstage.ReasonSupersessionPending { if pass.Route != nil { route := stagelabels.RouteHuman(*pass.Route) verdict.Stage = route.Stage @@ -259,7 +259,7 @@ func passDecision(verdict stageVerdict, role stagelabels.Role, obs stageObservat } } dco := f.PR.DCO - if !verdict.Terminal && verdict.Stage != stagelabels.None && pass.DCOKnown { + if !verdict.Terminal && verdict.Stage != stagelabels.None && pass.DCOKnown && verdict.Reason != prstage.ReasonSupersessionPending { dco = pass.DCO if pass.DCO == stagelabels.DCONo && pass.DCORequired { verdict.Stage = FixingStage(obs) diff --git a/src/pkg/stageengine/derive_pr_classify.go b/src/pkg/stageengine/derive_pr_classify.go index bd7d1eaf47..4aa049e61b 100644 --- a/src/pkg/stageengine/derive_pr_classify.go +++ b/src/pkg/stageengine/derive_pr_classify.go @@ -326,6 +326,9 @@ func ClassifyStage(o stageObservation) stageVerdict { if o.Merged && reason == "" { return stageVerdict{Terminal: true, Rank: stageRankOwned} } + if reason != "closed" && reason != "gone" && reason != "label-removed" && HasLabelFold(o.Labels, prstage.SupersessionPendingLabel) && HasLabelFold(o.Labels, NeedsHumanLabel) && !HasLabelFold(o.Labels, "hive/keep-open") { + return stageVerdict{Stage: stagelabels.NeedsDecision, Rank: stageRankOwned, Human: true, Reason: prstage.ReasonSupersessionPending} + } if o.Err { // The lane tripped before deciding. If every author-side gate is // already satisfied the PR is waiting on the hive, not the author. diff --git a/src/pkg/stageengine/derive_pr_supersession_test.go b/src/pkg/stageengine/derive_pr_supersession_test.go new file mode 100644 index 0000000000..6dd692662a --- /dev/null +++ b/src/pkg/stageengine/derive_pr_supersession_test.go @@ -0,0 +1,38 @@ +package stageengine + +import ( + "testing" + + "github.com/hivecommons/hive/pkg/authorkind" + "github.com/hivecommons/hive/pkg/stageengine/prstage" + "github.com/hivecommons/hive/pkg/stagelabels" +) + +func TestSupersessionParkSurvivesRepairAndPassOverrides(t *testing.T) { + for _, kind := range []authorkind.Kind{authorkind.Human, authorkind.Clanker} { + for _, reason := range []string{"needs-rebase", "check-failing:unit", "label:needs-human", "no-claim"} { + t.Run(string(kind)+"/"+reason, func(t *testing.T) { + o := stageObservation{Lane: stageLaneContributor, Reason: reason, AuthorKind: kind, Labels: []string{NeedsHumanLabel, prstage.SupersessionPendingLabel, "needs-rebase"}} + f := FactsFromPRObservations([]stageObservation{o}) + f.PR.Pass = &PRPassFacts{AuthorKind: kind, DCOKnown: true, DCO: stagelabels.DCONo, DCORequired: true, + Route: &stagelabels.Evidence{}} + d := DerivePR(f) + if d.Stage != stagelabels.NeedsDecision || d.Reason != prstage.ReasonSupersessionPending { + t.Fatalf("superseded PR sent to repair: %+v", d) + } + // Once the cause or the park is cleared the ordinary lane owns it. + for _, labels := range [][]string{{NeedsHumanLabel}, {prstage.SupersessionPendingLabel}, {NeedsHumanLabel, prstage.SupersessionPendingLabel, "hive/keep-open"}} { + o.Labels = labels + if v := ClassifyStage(o); v.Reason == prstage.ReasonSupersessionPending { + t.Fatalf("stale park retained: %+v", v) + } + } + o.Labels = []string{NeedsHumanLabel, prstage.SupersessionPendingLabel} + o.Reason = "closed" + if v := ClassifyStage(o); !v.Terminal { + t.Fatalf("closed PR still parked: %+v", v) + } + }) + } + } +} diff --git a/src/pkg/stageengine/prstage/reasons.go b/src/pkg/stageengine/prstage/reasons.go index 8e1c49a6ae..74b8a37a78 100644 --- a/src/pkg/stageengine/prstage/reasons.go +++ b/src/pkg/stageengine/prstage/reasons.go @@ -6,6 +6,11 @@ import ( "github.com/hivecommons/hive/pkg/stagelabels" ) +// SupersessionPendingLabel records the sweep's unresolved supersession park. +// It is independent of CI/head state and is removed when the cause clears. +const SupersessionPendingLabel = "hive/supersession-pending" +const ReasonSupersessionPending = "supersession-decision-pending" + // Lane reasons are shared by the producers and the stage classifier. const ( ReviewGateOutcomeAwaiting = "awaiting-review-approval"