From 7971312847f422b5164dded286da5387916342b9 Mon Sep 17 00:00:00 2001 From: Danathar Date: Mon, 26 Oct 2026 20:54:32 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix(automerge):=20sign-off=20sta?= =?UTF-8?q?ll=20notes=20name=20the=20squash=20gate=20only=20where=20it=20r?= =?UTF-8?q?uns;=20log=20the=20stall=20at=20INFO?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adopted-PR stall escalation ("adopted PR sits with nothing the hive can push") is designed, but it was logged at WARN, so the slog feed counted three projectbluefin parks toward an unclassified_error_repeated fleet self-report (#14466). It is now logged at INFO, the same line as #14480 on v5. The adoption stall note and the maintainer sign-off note said the `Sign-off survives the squash` gate stays red. That check is dco-squash-attribution.yml, which only some repositories carry; elsewhere the hive's own commit check holds the head. noteStageDCOGate now records whether the head's required or reported checks include it, and the notes say the hive will not squash-merge the head, naming the check only when it is there. Refs #14466 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1 Signed-off-by: Danathar --- .../fixed-signoff-stall-note-and-log-level.md | 1 + src/docs/labels-and-control-signals.md | 6 +- .../automerge_adoption_escape_test.go | 2 + .../automerge/automerge_contributor_adopt.go | 6 +- .../automerge/automerge_merge_commit.go | 25 ++- .../automerge_signoff_attribution_085_test.go | 5 + .../automerge_signoff_gate_note_test.go | 144 ++++++++++++++++++ .../automerge/automerge_stage_labels.go | 12 +- src/pkg/stageengine/prstage/types.go | 6 + 9 files changed, 198 insertions(+), 9 deletions(-) create mode 100644 changelog.d/fixed-signoff-stall-note-and-log-level.md create mode 100644 src/pkg/github/automerge/automerge_signoff_gate_note_test.go diff --git a/changelog.d/fixed-signoff-stall-note-and-log-level.md b/changelog.d/fixed-signoff-stall-note-and-log-level.md new file mode 100644 index 0000000000..013b26bd27 --- /dev/null +++ b/changelog.d/fixed-signoff-stall-note-and-log-level.md @@ -0,0 +1 @@ +- The sign-off attribution notes on an adopted or hive-caused `stage/085` PR (`adoption-stalled:hive-signoff-attribution` and the maintainer sign-off note) no longer say the `Sign-off survives the squash` gate "stays red" on repositories that do not run `.github/workflows/dco-squash-attribution.yml`. They now say the hive will not squash-merge the head, and name that check only when the head's required or reported checks include it. The adopted-PR stall escalation ("adopted PR sits with nothing the hive can push") is logged at INFO instead of WARN, so designed escalations no longer add up to an `unclassified_error_repeated` fleet self-report ([#14466](https://github.com/hivecommons/hive/issues/14466)). diff --git a/src/docs/labels-and-control-signals.md b/src/docs/labels-and-control-signals.md index 492c63de9d..9cdaa9c533 100644 --- a/src/docs/labels-and-control-signals.md +++ b/src/docs/labels-and-control-signals.md @@ -620,7 +620,11 @@ The split also covers a head whose commits GitHub links to the hive App while the PR author is someone else — a hive follow-up commit on the branch, or a contributor who committed from inside a hive pod with the App's noreply address (hivecommons/pluk#315 and the other OWNERS PRs of 2026-10-20). The -required "Sign-off survives the squash" check is red there, and no hive push +hive will not squash-merge such a head. On a repository that runs +`.github/workflows/dco-squash-attribution.yml`, the "Sign-off survives the +squash" check is red there too. The hive's notes name that check only when +the head's required or reported checks include it, because most managed +repositories do not carry the workflow. No hive push lands on a non-App-authored branch (`review.fix_human_prs` does not authorize one and the hive must not forge the author's `Signed-off-by`). Who put those commits there decides the route. When the hive did — the diff --git a/src/pkg/github/automerge/automerge_adoption_escape_test.go b/src/pkg/github/automerge/automerge_adoption_escape_test.go index 77e81c3ba1..82df443e18 100644 --- a/src/pkg/github/automerge/automerge_adoption_escape_test.go +++ b/src/pkg/github/automerge/automerge_adoption_escape_test.go @@ -154,6 +154,8 @@ func TestAdoptedInPlaceSquashUnsafeHiveCommitStallsWithoutSupersede(t *testing.T h.adopt.setCommitAuthor("f72fa44", testHiveAppBotLogin) h.clock.advance(time.Minute) + // This repo runs the gate (dco-squash-attribution.yml), so the note names it. + h.e.noteStageDCOGate("acme", "widget", 7, "f72fa44", map[string]bool{squashSignoffCheck: true}, nil) h.observe(adoptPR(7, "f72fa44", adoptedLabels...), contributorLaneReasonNoVerdict) if h.adopt.refAttempts != 0 || len(h.adopt.refs) != 0 || len(h.adopt.pulls) != 0 { t.Fatalf("signoff supersede opened: attempts=%d refs=%v pulls=%v", h.adopt.refAttempts, h.adopt.refs, h.adopt.pulls) diff --git a/src/pkg/github/automerge/automerge_contributor_adopt.go b/src/pkg/github/automerge/automerge_contributor_adopt.go index 5ff1d9f2e4..ae5f183a57 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt.go @@ -452,9 +452,9 @@ func adoptionBlockerText(obs stageObservation, rec stagelabels.Record) string { switch { case adoptedHeadSignoffUnfixable(obs, rec): author := nonEmpty(obs.Author, "the contributor") - msg := fmt.Sprintf("%d commit(s) on `%s` carry the App's `Signed-off-by` while a squash merge would land them as @%s, so the `Sign-off survives the squash` gate stays red. "+ + msg := fmt.Sprintf("%d commit(s) on `%s` carry the App's `Signed-off-by` while a squash merge would land them as @%s, so the hive will not squash-merge this head%s. "+ "The hive cannot rewrite that authorship without the contributor, and a superseding PR would carry the same head, so it opened none. ", - obs.Detail.DCOSquashUnsafeHive, shortSHA(obs.HeadSHA), author) + obs.Detail.DCOSquashUnsafeHive, shortSHA(obs.HeadSHA), author, squashSignoffGateNote(obs)) msg += mergeCommitRefusalText(obs) return msg case verdictBlocks(obs.Detail): @@ -1176,7 +1176,7 @@ func (c *Engine) escapeStalledAdoption(ctx context.Context, store *stagelabels.S } noted := rec.AdoptionEscalationNoted(obs.HeadSHA) store.MarkAdoptionEscalated(key, obs.HeadSHA, reason) - c.warn("stage labels: adopted PR sits with nothing the hive can push; escalating to needs-decision", "repo", obs.Display, "pr", obs.Number, "head", obs.HeadSHA, "reason", reason, "head_since", rec.AdoptedHeadSince(), "noted", noted) + c.info("stage labels: adopted PR sits with nothing the hive can push; escalating to needs-decision", "repo", obs.Display, "pr", obs.Number, "head", obs.HeadSHA, "reason", reason, "head_since", rec.AdoptedHeadSince(), "noted", noted) if noted { // The blocker flapped on a head already noted: re-escalate silently. return adoptionEscape{Escalated: true, Reason: reason}, comments, firstErr diff --git a/src/pkg/github/automerge/automerge_merge_commit.go b/src/pkg/github/automerge/automerge_merge_commit.go index d1ad05a388..c1b940f9f5 100644 --- a/src/pkg/github/automerge/automerge_merge_commit.go +++ b/src/pkg/github/automerge/automerge_merge_commit.go @@ -142,6 +142,27 @@ func (c *Engine) noteMergeCommit(ctx context.Context, store *stagelabels.Store, return 1, nil } +// squashSignoffCheck is the check-run name of +// .github/workflows/dco-squash-attribution.yml. +const squashSignoffCheck = "Sign-off survives the squash" + +func isSquashSignoffCheck(name string) bool { + return strings.EqualFold(strings.TrimSpace(name), squashSignoffCheck) +} + +// squashSignoffGateNote is the sign-off attribution notes' mention of that +// check, or "" when the head's required and reported checks do not include +// it (noteStageDCOGate): the workflow lives in the repositories that carry +// it, not in every repository the hive manages, and the hive's own commit +// check is what holds the head everywhere else. +func squashSignoffGateNote(obs stageObservation) string { + d := obs.Detail + if !d.SquashSignoffGate || (d.DCOGateHead != "" && obs.HeadSHA != "" && d.DCOGateHead != obs.HeadSHA) { + return "" + } + return " (the `" + squashSignoffCheck + "` check stays red)" +} + // SignoffAttributionDecisionMarkerPrefix opens the one stage/085 note per // head for a hive-caused sign-off attribution the lane could not resolve // with a merge commit by itself. @@ -158,8 +179,8 @@ func SignoffAttributionDecisionComment(obs stageObservation) string { author := nonEmpty(obs.Author, "the contributor") var b strings.Builder fmt.Fprintf(&b, "%s head=%s -->\n", SignoffAttributionDecisionMarkerPrefix, obs.HeadSHA) - fmt.Fprintf(&b, "Held at `%s` for a maintainer. %d of %d commits on `%s` are the hive's own, signed off as the hive App, so a squash merge would land them as @%s with the App's sign-off and the `Sign-off survives the squash` gate stays red. ", - stagelabels.NeedsDecision.Label(), n, total, shortSHA(obs.HeadSHA), author) + fmt.Fprintf(&b, "Held at `%s` for a maintainer. %d of %d commits on `%s` are the hive's own, signed off as the hive App, so a squash merge would land them as @%s with the App's sign-off, and the hive will not squash-merge this head%s. ", + stagelabels.NeedsDecision.Label(), n, total, shortSHA(obs.HeadSHA), author, squashSignoffGateNote(obs)) fmt.Fprintf(&b, "@%s did not make those commits and has nothing to fix here.\n\n", author) b.WriteString(mergeCommitRefusalText(obs)) return b.String() diff --git a/src/pkg/github/automerge/automerge_signoff_attribution_085_test.go b/src/pkg/github/automerge/automerge_signoff_attribution_085_test.go index fbb14943e1..0211276884 100644 --- a/src/pkg/github/automerge/automerge_signoff_attribution_085_test.go +++ b/src/pkg/github/automerge/automerge_signoff_attribution_085_test.go @@ -31,6 +31,8 @@ type signoffCase struct { squashOnly bool // approvedHead, when set, is the head a trusted maintainer approved. approvedHead string + // squashGate: the head's checks include `Sign-off survives the squash`. + squashGate bool } func runSignoffCase(t *testing.T, tc signoffCase) (*stageAPI, stagelabels.Record, *Engine, func()) { @@ -66,6 +68,9 @@ func runSignoffCase(t *testing.T, tc signoffCase) (*stageAPI, stagelabels.Record pr.User = &gh.User{Login: gh.Ptr("j1mc")} e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", pr, tc.reason, false, nil) e.noteStageRole("acme", "widget", n, stagelabels.RoleContributor) + if tc.squashGate { + e.noteStageDCOGate("acme", "widget", n, pr.GetHead().GetSHA(), nil, map[string]bool{squashSignoffCheck: true}) + } if _, err := e.ApplyStageLabels(context.Background()); err != nil { t.Fatal(err) } diff --git a/src/pkg/github/automerge/automerge_signoff_gate_note_test.go b/src/pkg/github/automerge/automerge_signoff_gate_note_test.go new file mode 100644 index 0000000000..e5314a24e7 --- /dev/null +++ b/src/pkg/github/automerge/automerge_signoff_gate_note_test.go @@ -0,0 +1,144 @@ +package automerge + +import ( + "bytes" + "log/slog" + "strings" + "sync" + "testing" + "time" + + "github.com/hivecommons/hive/pkg/fleetreport/slogfeed" + "github.com/hivecommons/hive/pkg/stagelabels" +) + +// hivecommons/hive#14466: the projectbluefin hive parked utah-packages#660, +// chairlift#638 and testsuite#1028 at 085 on hive-signoff-attribution after +// its own fix commit. The park is designed; two things around it were not. +// Its notes said the `Sign-off survives the squash` gate stays red, but that +// check is a workflow only some repositories carry and none of those three +// ran it. And the escalation was logged at WARN, so the slog feed counted +// three designed parks toward an unclassified_error_repeated self-report. + +const escalationLogMsg = "stage labels: adopted PR sits with nothing the hive can push" + +// squashUnsafeAdoption adopts PR 7 in place and gives head 021bb6f the shape +// of all three PRs: the contributor's own signed commit, then the hive's fix +// signed off as the App. The caller observes it once to trigger the stall. +func squashUnsafeAdoption(t *testing.T) (*adoptHarness, func()) { + t.Helper() + p := adoptPolicy() + p.AdoptInPlace = true + h, done := newAdoptHarness(t, ContributorNudgeOutcome{Mode: "comment"}, p) + h.api.commitDetails = map[int][]stageCommitFixture{7: { + {message: "fix(smoke): restore default app checks\n\nSigned-off-by: thatguywob ", authorLogin: "thatguywob", authorName: "thatguywob", authorEmail: "thatguywob@users.noreply.github.com"}, + {message: "fix(ci): preload apps for MIME checks\n\nSigned-off-by: hive[bot] <12345+hive[bot]@users.noreply.github.com>", authorLogin: testHiveAppBotLogin, authorName: "hive[bot]", authorEmail: "12345+hive[bot]@users.noreply.github.com"}, + }} + adoptInPlaceAt(h, "021bb6f", contributorLaneReasonNoVerdict) + h.adopt.setCommitAuthor("021bb6f", testHiveAppBotLogin) + h.clock.advance(time.Minute) + return h, done +} + +func TestSignoffStallNoteNamesSquashGateOnlyWhereItRuns(t *testing.T) { + for _, tc := range []struct { + name string + gate bool + }{ + {"repository without dco-squash-attribution.yml", false}, + {"repository running it", true}, + } { + t.Run(tc.name, func(t *testing.T) { + h, done := squashUnsafeAdoption(t) + defer done() + if tc.gate { + h.e.noteStageDCOGate("acme", "widget", 7, "021bb6f", map[string]bool{squashSignoffCheck: true}, nil) + } + h.observe(adoptPR(7, "021bb6f", adoptedLabels...), contributorLaneReasonNoVerdict) + notes := stalledComments(h.api) + if len(notes) != 1 { + t.Fatalf("stalled note: %v", h.api.comments) + } + for _, want := range []string{"so the hive will not squash-merge this head", "`gh pr merge 7 --merge`, not squash"} { + if !strings.Contains(notes[0], want) { + t.Fatalf("note missing %q:\n%s", want, notes[0]) + } + } + if named := strings.Contains(notes[0], squashSignoffCheck); named != tc.gate { + t.Fatalf("note names the %q check = %v, want %v:\n%s", squashSignoffCheck, named, tc.gate, notes[0]) + } + if strings.Contains(notes[0], "gate stays red") { + t.Fatalf("note still uses the old wording:\n%s", notes[0]) + } + }) + } +} + +func TestSignoffAttributionDecisionNoteNamesSquashGateOnlyWhereItRuns(t *testing.T) { + for _, gate := range []bool{false, true} { + api, rec, _, _ := runSignoffCase(t, signoffCase{commits: testsuite991Commits, reason: contributorLaneReasonNoVerdict, squashGate: gate}) + if rec.Stage != stagelabels.NeedsDecision || rec.Reason != stagelabels.LaneReasonHiveSignoffAttribution { + t.Fatalf("gate=%v: record %+v, want 085 on hive-signoff-attribution", gate, rec) + } + notes := commentsWith(api, SignoffAttributionDecisionMarkerPrefix) + if len(notes) != 1 { + t.Fatalf("gate=%v: maintainer note: %q", gate, api.comments) + } + if !strings.Contains(notes[0], "the hive will not squash-merge this head") || strings.Contains(notes[0], "gate stays red") { + t.Fatalf("gate=%v: note wording:\n%s", gate, notes[0]) + } + if named := strings.Contains(notes[0], squashSignoffCheck); named != gate { + t.Fatalf("note names the %q check = %v, want %v:\n%s", squashSignoffCheck, named, gate, notes[0]) + } + } +} + +// The check seen on an earlier head says nothing about this one. +func TestSquashSignoffGateNoteIsPerHead(t *testing.T) { + obs := stageObservation{HeadSHA: "new"} + obs.Detail.SquashSignoffGate, obs.Detail.DCOGateHead = true, "old" + if got := squashSignoffGateNote(obs); got != "" { + t.Fatalf("gate seen on another head: got %q, want none", got) + } + obs.Detail.DCOGateHead = "new" + if got := squashSignoffGateNote(obs); !strings.Contains(got, squashSignoffCheck) { + t.Fatalf("gate seen on this head: got %q", got) + } +} + +// The escalation is logged at INFO, so the slog feed (WARN and above) never +// counts it toward a self-report. The feed here is the real handler. +func TestAdoptionStallEscalationLogsAtInfo(t *testing.T) { + h, done := squashUnsafeAdoption(t) + defer done() + var buf bytes.Buffer + var mu sync.Mutex + var fed []string + h.e.logger = slog.New(slogfeed.NewWithObserver(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}), func(_, _, message, _ string) { + mu.Lock() + defer mu.Unlock() + fed = append(fed, message) + })) + + h.observe(adoptPR(7, "021bb6f", adoptedLabels...), contributorLaneReasonNoVerdict) + if len(stalledComments(h.api)) != 1 { + t.Fatalf("expected the escalation to happen: %v", h.api.comments) + } + var line string + for _, l := range strings.Split(buf.String(), "\n") { + if strings.Contains(l, escalationLogMsg) { + line = l + break + } + } + if !strings.Contains(line, "level=INFO") { + t.Fatalf("escalation log line %q, want level=INFO", line) + } + mu.Lock() + defer mu.Unlock() + for _, m := range fed { + if strings.Contains(m, escalationLogMsg) { + t.Fatalf("the slog feed counted the designed escalation toward a self-report: %q", m) + } + } +} diff --git a/src/pkg/github/automerge/automerge_stage_labels.go b/src/pkg/github/automerge/automerge_stage_labels.go index c698d236a9..468e90393b 100644 --- a/src/pkg/github/automerge/automerge_stage_labels.go +++ b/src/pkg/github/automerge/automerge_stage_labels.go @@ -293,6 +293,7 @@ func (t *StageTracker) drain() [][]stageObservation { } if detail.DCOGateHead != "" && (o.HeadSHA == "" || detail.DCOGateHead == o.HeadSHA) { o.Detail.DCOGate, o.Detail.DCOGateHead = detail.DCOGate, detail.DCOGateHead + o.Detail.SquashSignoffGate = detail.SquashSignoffGate } list = append(list, o) } @@ -550,21 +551,26 @@ func stageActivity(obs stageObservation, stage stagelabels.Stage) stagelabels.Ac // (hivecommons/hive#12542): in the required-check set or reported on the // head by the DCO App / Prow's plugin. It is the detected "this repo // enforces DCO" signal; a head without one is not routed to 072 for a -// missing sign-off unless config says the repo requires it. +// missing sign-off unless config says the repo requires it. It also records +// whether the `Sign-off survives the squash` check is among them, which only +// the sign-off attribution notes read. func (c *Engine) noteStageDCOGate(owner, repo string, number int, head string, required map[string]bool, observed map[string]bool) { if !c.stagePolicy().Enabled || number <= 0 { return } - gate := false + gate, squashGate := false, false for _, set := range []map[string]bool{required, observed} { for name := range set { if hgithub.IsDCOContext(name) { gate = true } + if isSquashSignoffCheck(name) { + squashGate = true + } } } c.stageTrackerOrShared().note(stageKey(owner, repo, number), func(d *stageDetail) { - d.DCOGate, d.DCOGateHead = gate, head + d.DCOGate, d.DCOGateHead, d.SquashSignoffGate = gate, head, squashGate }) } diff --git a/src/pkg/stageengine/prstage/types.go b/src/pkg/stageengine/prstage/types.go index bb7ab7e5d5..09575b1033 100644 --- a/src/pkg/stageengine/prstage/types.go +++ b/src/pkg/stageengine/prstage/types.go @@ -61,6 +61,12 @@ type Detail struct { // no per-repo override is configured. DCOGate bool DCOGateHead string + // SquashSignoffGate records, for the same head as DCOGateHead, whether + // the `Sign-off survives the squash` check + // (.github/workflows/dco-squash-attribution.yml) is required or reported + // there. Only repositories that carry that workflow run it, so the + // sign-off attribution notes name it only where it is. + SquashSignoffGate bool } // RedCheck returns the first red non-required check on head, or "".