✨ fix(reviewer-lane): true creation-time ordering + structured re-escalation hand-off (#5617 items 3–4)

CHANGELOG.md

@@ -15,8 +15,14 @@ Hive did not historically maintain a complete changelog. This file starts a prag
 
 - The quality lane can now opt into agent-authored formal verification with `quality.formal: true` at ACMM L5/L6. Every quality kick receives the same model-worthiness, `formal/<subsystem>/` artifact, expected-verdict drift, reporting-only CI, counterexample narrative/deduplication, and modeled-subsystem maintenance contract even when its ordinary policy prompt is customized. The zero value is off, and the setting becomes inert on an ACMM downgrade below L5 without being discarded ([#5512](https://github.com/kubestellar/hive/issues/5512)).
 
+### Changed
+
+- A PR that re-escalates to `needs-human` **after** the reviewer lane already adjudicated it now carries a structured hand-off note instead of the generic escalation comment ([#5617](https://github.com/kubestellar/hive/issues/5617)). The reviewer lane is a one-pass ladder: a PR carrying `reviewer-passed` is excluded from the reviewer work list permanently, so that second escalation comment is the last thing the machinery will ever say about the PR — yet nothing in it distinguished a terminal hand-off from a first escalation, and nothing told the human that a mechanical repair had already been tried and had not held. The hub never observes a reviewer verdict directly (it is a label edit, usually a direct `gh pr edit`), so the attempt ledger now stamps the head SHA the reviewer left on the branch and the time its verdict was reconciled, and keeps both across the ledger reset that reconciliation performs. The note states that commit, when the PR was returned to the automated lane, that the distinct-attempt count is measured **from** the reviewer's pass rather than from the PR's first failure, and that no further automated pass is coming. It points at the reviewer's own audited record — the `Reviewer adjudication:` relay comment attributed as `agent_pr_reviewed`, plus the matching advisory bead — rather than restating it, because the hub never saw the reviewer's reasoning and summarising it would be invention. The ntfy page on this path is retitled to match: a hand-off with nothing automated behind it is materially more urgent than a first escalation. First escalations are unchanged.
+
 ### Fixed
 
+- The reviewer lane's escalated-PR work list is now ordered by each PR's real creation time instead of a PR-number proxy ([#5617](https://github.com/kubestellar/hive/issues/5617)). The list is capped at three PRs per kick and documented "oldest first", but its rows carried no age signal at all — `ci-failing.json` recorded no creation time, so ordering fell back to (repo name, PR number). PR numbers are monotonic only *within* a repo, so that proxy sorted by repo **name** first, and against the per-kick cap the result was starvation rather than mere cosmetic disorder: on a multi-repo hive, a month-old escalated PR in a late-alphabet repo sat behind newer ones from an early-alphabet repo on every kick, indefinitely. The creation time was already fetched during PR enumeration and simply never threaded into the work-list artifact; it now is, and each row renders an `opened:` line so the reviewer can confirm the ordering without running `gh pr list` (which the kick's invariants forbid). Rows from an older hub that recorded no creation time keep the previous (repo, number) proxy among themselves and sort after every row whose age is known, so a stale artifact degrades to the old behavior rather than being reshuffled.
+
 - Hold-gated PRs can no longer silently carry unreviewed commits into a merge ([#5589](https://github.com/kubestellar/hive/issues/5589)). A PR sitting under a hold label could accumulate commits from other authors or agents (typically a worktree cut from a contaminated base), and once the hold lifted the merge lanes treated it like any other approved PR — the diff a human saw under the hold was not the diff that merged. The governor now snapshots each hold-gated PR's head SHA plus commit/author sets into a PVC ledger (`/data/metrics/hold-guard.json`) and compares at lift time: an unchanged head clears silently, while any drift keeps the PR out of `merge-eligible.json` and `ci-failing.json`, posts a one-time evidence comment naming the unreviewed commits and authors (plain text, never @-mentions), re-applies the `hold` label, and re-pins the snapshot to the drifted head so removing the re-applied hold after reading the evidence is the fresh approval. Both auto-merge sweeps additionally now respect hold and `do-not-merge` labels directly — the self-authored sweep previously listed PRs independently of the enumeration hold gate and would squash a hold-labelled App PR on green. from [#5480](https://github.com/kubestellar/hive/issues/5480): an App-bot comment review submitted through `hive-review` and attributed as `agent_pr_reviewed`, plus an advisory bead included in the advisory digest. `hive-review` now prints its asynchronous result path, and the kick requires a confirmed `ok` result plus the bead before removing `needs-human`, applying a terminal reviewer label, or closing; if either write fails, the PR remains in the human queue for a later retry instead of becoming silently adjudicated.
 - Version flips no longer trap hosted spokes with large, long-lived `/data` PVCs in a startup-probe death loop ([#5525](https://github.com/kubestellar/hive/issues/5525)). The v5 entrypoint synchronously ran recursive per-agent `chown` and shared-home `chmod` passes before the Go server could bind `:3002`; on an NFS/RWX volume with months of worktrees the walk exceeded the probe budget, kubelet killed the container with exit 137, and the next boot restarted the same walk from the beginning. Size-dependent permission repair now runs in a root background worker, so the dashboard and health endpoint start independently of PVC size. Protected completion markers bind each finished pass to the ownership-schema revision and target UID, and the agent manager waits for both the shared-home and per-agent markers before touching that agent's tree, preserving UID isolation without putting the server behind the migration. Completed steady-state boots skip the recursive pass, while a changed UID invalidates its marker and repairs only the affected agent before launch.
 

src/cmd/hive/escalation_sweep_test.go

@@ -306,3 +306,61 @@ func TestRunEscalationSweepAuthorGateAndExcerptFallback(t *testing.T) {
 		t.Fatalf("comment must carry the ledger's stored excerpt, got: %v", fake.comments)
 	}
 }
+
+// #5617 item 3: a PR that re-escalates AFTER a reviewer-lane pass must reach
+// its human with the structured hand-off note — what the reviewer left on the
+// branch, and that no second automated pass is coming — rather than the
+// generic first-escalation body. One reviewer pass per PR is the whole ladder,
+// so this comment is the last thing the machinery will ever say about the PR.
+func TestRunEscalationSweepHandsOffAfterReviewerPass(t *testing.T) {
+	newTestEscalationStore(t)
+	client, fake := newEscalationSweepClient(t)
+	cfg := escalationTestConfig()
+	logger := discardLogger()
+
+	red := func(sha string, labels ...string) *github.ActionableResult {
+		p := redPR("widgets", 7, "hive-agent", sha)
+		p.CIFailureExcerpt = "TestFoo: want 2, got 3"
+		p.Labels = labels
+		return actionableWith(p)
+	}
+
+	// First escalation: three distinct red SHAs cross the threshold.
+	for _, sha := range []string{"sha-1", "sha-2", "sha-3"} {
+		runEscalationSweep(context.Background(), cfg, client, red(sha), nil, logger)
+	}
+	if len(fake.comments) != 1 {
+		t.Fatalf("comments = %d, want the first escalation comment", len(fake.comments))
+	}
+	if strings.Contains(fake.comments[0], "A reviewer already adjudicated") {
+		t.Errorf("a first escalation must not claim a prior reviewer pass:\n%s", fake.comments[0])
+	}
+
+	// The reviewer repairs, drops needs-human and adds reviewer-passed; its
+	// pushed fix is red. Sweep reconciles the verdict and restarts the ledger.
+	runEscalationSweep(context.Background(), cfg, client,
+		red("reviewer-fix", escalation.ReviewerPassedLabel), nil, logger)
+	if len(fake.comments) != 1 {
+		t.Fatalf("comments = %d, want no comment for the reviewer-verdict reconciliation", len(fake.comments))
+	}
+
+	// Two more distinct red SHAs re-cross the threshold on the fresh ledger.
+	for _, sha := range []string{"sha-4", "sha-5"} {
+		runEscalationSweep(context.Background(), cfg, client,
+			red(sha, escalation.ReviewerPassedLabel, escalation.NeedsHumanLabel), nil, logger)
+	}
+	if len(fake.comments) != 2 {
+		t.Fatalf("comments = %d, want a second escalation comment after the reviewer pass", len(fake.comments))
+	}
+	handoff := fake.comments[1]
+	for _, want := range []string{
+		"A reviewer already adjudicated this PR",
+		"`reviewer-fix`",
+		"No further automated pass is coming",
+		"TestFoo: want 2, got 3",
+	} {
+		if !strings.Contains(handoff, want) {
+			t.Errorf("re-escalation comment missing %q:\n%s", want, handoff)
+		}
+	}
+}

src/cmd/hive/main.go

@@ -7881,7 +7881,18 @@ func runEscalationSweep(
 		if excerpt == "" {
 			excerpt = escalationStore.Excerpt(o.Repo, o.Number)
 		}
+		// A PR that re-escalates AFTER a reviewer-lane pass reaches its human
+		// with a structured hand-off note (#5617 item 3): what the reviewer
+		// left on the branch, when, and that no second automated pass is
+		// coming. Before this, the only thing distinguishing that hand-off
+		// from a first escalation was the label set.
 		body := escalation.CommentBody(r.Attempts, meta[key].checks, excerpt)
+		afterReviewerPass := false
+		if sha, at, ok := escalationStore.ReviewerPass(o.Repo, o.Number); ok {
+			body = escalation.HandoffCommentBody(r.Attempts, meta[key].checks, excerpt,
+				escalation.ReviewerHandoff{SHA: sha, At: at})
+			afterReviewerPass = true
+		}
 		if err := writer.CreateIssueComment(ctx, o.Repo, o.Number, body); err != nil {
 			// Retry next pass rather than marking escalated with no comment:
 			// the whole point is that the evidence reaches a human.
@@ -7897,9 +7908,16 @@ func runEscalationSweep(
 			"repo", o.Repo, "pr", o.Number, "attempts", r.Attempts,
 			"failing_checks", strings.Join(meta[key].checks, ","))
 		if notifier != nil {
-			notifier.Send("Fix loop escalated",
-				fmt.Sprintf("%s#%d red on %d fix attempts — needs a human (see PR comment for the raw error)", o.Repo, o.Number, r.Attempts),
-				notify.PriorityHigh)
+			title := "Fix loop escalated"
+			detail := fmt.Sprintf("%s#%d red on %d fix attempts — needs a human (see PR comment for the raw error)", o.Repo, o.Number, r.Attempts)
+			if afterReviewerPass {
+				// Materially more urgent than a first escalation: a reviewer
+				// has already had its one pass, so nothing automated remains
+				// behind this page.
+				title = "Fix loop escalated after reviewer pass"
+				detail = fmt.Sprintf("%s#%d red again on %d fix attempts since the reviewer's pass — no further automated pass will run (see PR comment)", o.Repo, o.Number, r.Attempts)
+			}
+			notifier.Send(title, detail, notify.PriorityHigh)
 		}
 	}
 	return escalated
@@ -8712,6 +8730,14 @@ func writeMergeEligible(actionable *github.ActionableResult, hold github.HoldRes
 		// reviewer-passed — a PR that re-escalates after a reviewer pass
 		// belongs to a true human, never to another automated pass.
 		Labels []string `json:"labels,omitempty"`
+		// CreatedAt is the PR's forge creation time — the reviewer lane's
+		// ordering key (#5617 item 4). Its work list is capped at a few PRs
+		// per kick and documented "oldest first", but until this field the
+		// rows carried no age signal at all and were ordered by (repo name, PR
+		// number). Numbers are monotonic only WITHIN a repo, so that proxy
+		// sorted by repo NAME first and could starve an old escalated PR in a
+		// late-alphabet repo behind newer ones, on every kick, forever.
+		CreatedAt time.Time `json:"created_at"`
 	}
 
 	prAgents := auditPRAgents(org, time.Now().Add(-auditPRAttributionWindow), "")
@@ -8777,6 +8803,7 @@ func writeMergeEligible(actionable *github.ActionableResult, hold github.HoldRes
 					Escalated:     escalatedPRs[escalation.Key(fullRepo, pr.Number)],
 					Agent:         prAgents[fmt.Sprintf("%s#%d", fullRepo, pr.Number)],
 					Labels:        pr.Labels,
+					CreatedAt:     pr.CreatedAt,
 				})
 				continue
 			}

src/docs/adr/0010-escalation-circuit-breaker.md

@@ -24,6 +24,18 @@ For unchanged red heads, track staleness separately and cap re-engagements at
 three per current SHA. A branch that moves resets the re-engagement counter; a
 permanently red, never-moving branch is not nudged forever.
 
+A PR escalating for the SECOND time — after the reviewer lane ([#5480]) already
+repaired or de-escalated it once — gets a structured hand-off note instead of
+the generic body. The ledger stamps the head SHA the reviewer left on the branch
+and when its verdict was reconciled, and keeps both across the reset that
+reconciliation performs, so the comment can say what was already tried, that the
+attempt count is measured from the reviewer's pass, and that no further
+automated pass is coming. One reviewer pass per PR is the whole ladder: without
+the note, nothing distinguished that terminal hand-off from a first escalation
+except the label set.
+
+[#5480]: https://github.com/kubestellar/hive/issues/5480
+
 ## Consequences
 
 The fleet stops spending cycles on fix loops that are not converging and gives a

src/pkg/escalation/escalation.go

@@ -97,6 +97,17 @@ type Entry struct {
 	// were burned. Older-generation entries are granted amnesty (see
 	// MachineryVersion).
 	Machinery int `json:"machinery,omitempty"`
+
+	// ReviewerPassedSHA / ReviewerPassedAt record the reviewer-lane pass that
+	// Sweep reconciled into this entry: the head SHA the reviewer left on the
+	// branch, and when the hub first observed that verdict. They deliberately
+	// SURVIVE the ledger reset that reconciliation performs, so a later
+	// re-escalation can hand the PR to a human with the reviewer's context
+	// attached (#5617 item 3) rather than the bare label set a human used to
+	// get. They are cleared only with the entry itself: a PR that goes green
+	// has converged, and a future regression starts a fresh story.
+	ReviewerPassedSHA string    `json:"reviewer_passed_sha,omitempty"`
+	ReviewerPassedAt  time.Time `json:"reviewer_passed_at,omitempty"`
 }
 
 // Store is the on-PVC attempt ledger. All methods are safe for concurrent use.
@@ -235,6 +246,15 @@ func (s *Store) Sweep(obs []Observation, threshold int) map[string]Result {
 			e.Escalated = false
 			e.RedSHAs = nil
 			e.ReEngagements = 0
+			// Remember WHAT the reviewer left on the branch and WHEN, so a
+			// later re-escalation can hand the human the reviewer's context
+			// instead of a bare label (#5617 item 3). Keyed on the SHA so a
+			// repeat reconciliation of the same verdict cannot walk the
+			// timestamp forward and misdate the hand-off.
+			if e.ReviewerPassedSHA != o.HeadSHA {
+				e.ReviewerPassedSHA = o.HeadSHA
+				e.ReviewerPassedAt = s.now()
+			}
 		}
 		if o.HeadSHA != "" && !containsSHA(e.RedSHAs, o.HeadSHA) {
 			e.RedSHAs = append(e.RedSHAs, o.HeadSHA)
@@ -313,6 +333,21 @@ func (s *Store) Attempts(repo string, number int) int {
 	return 0
 }
 
+// ReviewerPass reports the reviewer-lane pass recorded for a PR: the head SHA
+// the reviewer left on the branch, and when Sweep reconciled that verdict into
+// the ledger. ok is false when no reviewer has ever passed on this PR — which
+// is what routes the escalation sweep between the plain first-escalation
+// comment and the structured hand-off note.
+func (s *Store) ReviewerPass(repo string, number int) (sha string, at time.Time, ok bool) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	e := s.entries[Key(repo, number)]
+	if e == nil || e.ReviewerPassedAt.IsZero() {
+		return "", time.Time{}, false
+	}
+	return e.ReviewerPassedSHA, e.ReviewerPassedAt, true
+}
+
 // nowFn is the store's clock, overridable for tests via SetClock.
 func (s *Store) now() time.Time {
 	if s.clock != nil {
@@ -496,14 +531,61 @@ func containsLabel(labels []string, label string) bool {
 	return false
 }
 
+// ReviewerHandoff is the record of a completed reviewer-lane pass, carried
+// into the escalation comment when a PR the reviewer already adjudicated goes
+// red again. SHA is the head commit the reviewer left on the branch; At is
+// when Sweep reconciled that verdict into the ledger.
+type ReviewerHandoff struct {
+	SHA string
+	At  time.Time
+}
+
 // CommentBody renders the escalation comment posted on the PR. It leads with
 // the raw CI evidence — the whole point is that a human (or the next agent
 // pass) sees the actual error, not just "CI failed".
 func CommentBody(attempts int, failingChecks []string, excerpt string) string {
+	return commentBody(attempts, failingChecks, excerpt, nil)
+}
+
+// HandoffCommentBody renders the escalation comment for a PR that has ALREADY
+// had a reviewer-lane pass (#5617 item 3). The lane is a one-pass ladder: a PR
+// that re-escalates after `reviewer-passed` is excluded from the reviewer work
+// list forever and belongs to a true human. Until now that human's only
+// context was the label set — nothing distinguished a first escalation from a
+// second one, nor said that a mechanical repair had already been tried and had
+// not held. This body says both, and points at the reviewer's own audited
+// record rather than restating it (the hub never saw the reviewer's reasoning;
+// claiming to summarise it would be invention).
+func HandoffCommentBody(attempts int, failingChecks []string, excerpt string, h ReviewerHandoff) string {
+	return commentBody(attempts, failingChecks, excerpt, &h)
+}
+
+func commentBody(attempts int, failingChecks []string, excerpt string, h *ReviewerHandoff) string {
 	var b strings.Builder
 	b.WriteString("## 🛑 Fix loop escalated — human attention needed\n\n")
 	fmt.Fprintf(&b, "This PR has failed CI on **%d distinct fix attempts** (new commits, still red). ", attempts)
 	b.WriteString("The hive has stopped dispatching further automated fixes for it.\n\n")
+	if h != nil {
+		b.WriteString("### A reviewer already adjudicated this PR — this is the second failure\n\n")
+		b.WriteString("The reviewer lane repaired or de-escalated this PR once and returned it to the\n")
+		b.WriteString("automated lane; that is what the `" + ReviewerPassedLabel + "` label records.\n\n")
+		if h.SHA != "" {
+			fmt.Fprintf(&b, "- **Head commit the reviewer left on the branch:** `%s`\n", h.SHA)
+		}
+		if !h.At.IsZero() {
+			fmt.Fprintf(&b, "- **Returned to the automated lane:** %s\n", h.At.UTC().Format(time.RFC3339))
+		}
+		fmt.Fprintf(&b, "- **Since then:** %d distinct fix attempts, all still red. That count is\n", attempts)
+		b.WriteString("  measured FROM the reviewer's pass — the ledger restarts at the verdict.\n\n")
+		b.WriteString("What the reviewer tried, and why it believed the repair sufficient, is recorded on\n")
+		b.WriteString("this PR: look for its `Reviewer adjudication:` comment (posted through the hive\n")
+		b.WriteString("relay and attributed as `agent_pr_reviewed`) and the matching advisory bead. Read\n")
+		b.WriteString("that first — the repair it describes has now been tried and has not held, so\n")
+		b.WriteString("repeating it is the one approach already known to fail here.\n\n")
+		b.WriteString("**No further automated pass is coming.** One reviewer pass per PR is the whole\n")
+		b.WriteString("ladder: the reviewer work list excludes `" + ReviewerPassedLabel + "` rows permanently, so from\n")
+		b.WriteString("here this PR is a human's or it is nobody's.\n\n")
+	}
 	if len(failingChecks) > 0 {
 		sorted := append([]string(nil), failingChecks...)
 		sort.Strings(sorted)