diff --git a/CHANGELOG.md b/CHANGELOG.md index d50ec3553..e397c003c 100644 --- a/CHANGELOG.md +++ b/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//` 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. diff --git a/src/cmd/hive/escalation_sweep_test.go b/src/cmd/hive/escalation_sweep_test.go index 1e0421eb8..ebccde849 100644 --- a/src/cmd/hive/escalation_sweep_test.go +++ b/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) + } + } +} diff --git a/src/cmd/hive/main.go b/src/cmd/hive/main.go index 1a307232d..1ccdb3f04 100644 --- a/src/cmd/hive/main.go +++ b/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 } diff --git a/src/docs/adr/0010-escalation-circuit-breaker.md b/src/docs/adr/0010-escalation-circuit-breaker.md index cc40fda52..ca88df3e4 100644 --- a/src/docs/adr/0010-escalation-circuit-breaker.md +++ b/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 diff --git a/src/pkg/escalation/escalation.go b/src/pkg/escalation/escalation.go index cca1e47ca..f890b47c6 100644 --- a/src/pkg/escalation/escalation.go +++ b/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) diff --git a/src/pkg/escalation/escalation_test.go b/src/pkg/escalation/escalation_test.go index a80b7fc0b..b23b79a01 100644 --- a/src/pkg/escalation/escalation_test.go +++ b/src/pkg/escalation/escalation_test.go @@ -469,3 +469,95 @@ func TestObserveRed_PendingDoesNotClearStalenessClock(t *testing.T) { t.Fatal("green must still clear the staleness/re-engagement record") } } + +// #5617 item 3: the reviewer-verdict reset must REMEMBER the pass it is +// reconciling — the head SHA the reviewer left and when — because that reset +// is the only moment the hub can observe a reviewer verdict at all. Without +// the record, a PR re-escalating after a reviewer pass reaches its human with +// nothing but the label set to say a repair was already tried. +func TestSweep_RecordsReviewerPassForHandoff(t *testing.T) { + s := Load(filepath.Join(t.TempDir(), "ledger.json")) + at := time.Date(2026, 9, 2, 10, 0, 0, 0, time.UTC) + s.SetClock(func() time.Time { return at }) + + if _, _, ok := s.ReviewerPass("o/r", 7); ok { + t.Fatal("no reviewer has passed yet; ReviewerPass must report ok=false") + } + + // Burn the ledger down to an escalated entry. + for _, sha := range []string{"a", "b", "c"} { + s.Sweep([]Observation{obs("o/r", 7, sha, true)}, 3) + } + s.MarkEscalated("o/r", 7) + + // The reviewer repaired and relabeled; its pushed fix is red again. + s.Sweep([]Observation{obsLabeled("o/r", 7, "reviewer-fix", true, ReviewerPassedLabel)}, 3) + + sha, got, ok := s.ReviewerPass("o/r", 7) + if !ok { + t.Fatal("the reviewer pass must be recorded for the later hand-off note") + } + if sha != "reviewer-fix" { + t.Errorf("recorded SHA = %q, want the head the reviewer left behind", sha) + } + if !got.Equal(at) { + t.Errorf("recorded time = %s, want %s", got, at) + } + // The record must survive the ledger reset it accompanies. + if s.Attempts("o/r", 7) != 1 { + t.Errorf("attempts = %d, want the reviewer pass to have restarted the count at 1", s.Attempts("o/r", 7)) + } + + // It must also stay pinned to the verdict: the hand-off note dates the + // reviewer's pass, not the most recent sweep that saw the same red head. + later := at.Add(2 * time.Hour) + s.SetClock(func() time.Time { return later }) + s.Sweep([]Observation{obsLabeled("o/r", 7, "reviewer-fix", true, ReviewerPassedLabel)}, 3) + if _, got, _ := s.ReviewerPass("o/r", 7); !got.Equal(at) { + t.Errorf("reviewer-pass timestamp moved to %s; it must stay pinned to the verdict at %s", got, at) + } +} + +// The hand-off body must tell the human the three things the label set cannot: +// that a reviewer already had its one pass, what it left behind, and that +// nothing automated is coming after this (#5617 item 3). +func TestHandoffCommentBody_CarriesReviewerContext(t *testing.T) { + at := time.Date(2026, 9, 2, 10, 0, 0, 0, time.UTC) + body := HandoffCommentBody(2, []string{"build-gate"}, "panic: assignment to entry in nil map", + ReviewerHandoff{SHA: "deadbeef", At: at}) + for _, want := range []string{ + "2 distinct fix attempts", + "A reviewer already adjudicated this PR", + "`deadbeef`", + "2026-09-02T10:00:00Z", + ReviewerPassedLabel, + "Reviewer adjudication:", + "agent_pr_reviewed", + "No further automated pass is coming", + // The generic evidence the plain body carries must not be lost. + "build-gate", + "panic: assignment to entry in nil map", + "Remove the `needs-human` label", + } { + if !strings.Contains(body, want) { + t.Errorf("hand-off comment missing %q:\n%s", want, body) + } + } + + // A FIRST escalation must never claim a reviewer pass that never happened. + plain := CommentBody(2, []string{"build-gate"}, "panic: assignment to entry in nil map") + for _, banned := range []string{"A reviewer already adjudicated", "No further automated pass"} { + if strings.Contains(plain, banned) { + t.Errorf("first-escalation comment must not mention a reviewer pass (%q):\n%s", banned, plain) + } + } + + // Partial records still render: a ledger entry written before the SHA was + // observable must not emit an empty backtick pair or a zero timestamp. + bare := HandoffCommentBody(1, nil, "", ReviewerHandoff{}) + for _, banned := range []string{"``", "0001-01-01"} { + if strings.Contains(bare, banned) { + t.Errorf("empty hand-off record must omit the field, not render %q:\n%s", banned, bare) + } + } +} diff --git a/src/pkg/scheduler/reviewer_lane.go b/src/pkg/scheduler/reviewer_lane.go index e7f119165..c9b6e5f9b 100644 --- a/src/pkg/scheduler/reviewer_lane.go +++ b/src/pkg/scheduler/reviewer_lane.go @@ -24,6 +24,7 @@ import ( "os" "sort" "strings" + "time" "github.com/kubestellar/hive/pkg/escalation" "github.com/kubestellar/hive/pkg/github" @@ -196,19 +197,20 @@ func (s *Scheduler) buildReviewerWorkList() string { // enumeration with per-agent attribution, so every row is hive work; rows // whose Labels carry ReviewerPassedLabel or ReviewerRecommendCloseLabel are // excluded (one reviewer pass per PR, ever — whatever the verdict was). -// Output is capped at reviewerMaxPRsPerKick rows, oldest first -// (ascending PR number per repo — the enumeration carries no creation time, -// and numbers are monotonic per repo). +// Output is capped at reviewerMaxPRsPerKick rows, oldest first by the PR's +// real forge creation time; rows written by a hub that recorded none fall back +// to the old (repo, number) proxy and sort last (#5617 item 4). func formatReviewerWorkList(data []byte) string { type ciFailingRow struct { - Number int `json:"number"` - Repo string `json:"repo"` - Title string `json:"title"` - Agent string `json:"agent"` - Labels []string `json:"labels"` - FailingChecks []string `json:"failing_checks"` - Excerpt string `json:"excerpt"` - Escalated bool `json:"escalated"` + Number int `json:"number"` + Repo string `json:"repo"` + Title string `json:"title"` + Agent string `json:"agent"` + Labels []string `json:"labels"` + FailingChecks []string `json:"failing_checks"` + Excerpt string `json:"excerpt"` + Escalated bool `json:"escalated"` + CreatedAt time.Time `json:"created_at"` } var payload struct { Items []ciFailingRow `json:"ci_failing"` @@ -240,13 +242,28 @@ func formatReviewerWorkList(data []byte) string { if len(rows) == 0 { return "" } - // Oldest first: PR numbers are monotonic per repo, the only age signal - // these rows carry. + // Oldest first, by the PR's TRUE creation time (#5617 item 4). The old key + // carried no age signal at all: PR numbers are monotonic only WITHIN a + // repo, so ordering on (repo, number) sorted by repo NAME first. Against + // the reviewerMaxPRsPerKick cap that is a starvation bug rather than a + // cosmetic one — a month-old escalated PR in "zeta/service" sits behind + // three newer ones in "alpha/console" on every kick, forever, and the + // reviewer never reaches it. Rows whose creation time is unknown (a + // ci-failing.json written by an older hub, or a forge that omitted the + // field) sort LAST and keep the old proxy among themselves: an unproven + // age must not jump ahead of a measured one. sort.Slice(rows, func(i, j int) bool { - if rows[i].Repo != rows[j].Repo { - return rows[i].Repo < rows[j].Repo + a, b := rows[i], rows[j] + if a.CreatedAt.IsZero() != b.CreatedAt.IsZero() { + return !a.CreatedAt.IsZero() } - return rows[i].Number < rows[j].Number + if !a.CreatedAt.IsZero() && !a.CreatedAt.Equal(b.CreatedAt) { + return a.CreatedAt.Before(b.CreatedAt) + } + if a.Repo != b.Repo { + return a.Repo < b.Repo + } + return a.Number < b.Number }) var b strings.Builder @@ -262,6 +279,12 @@ func formatReviewerWorkList(data []byte) string { } b.WriteString(fmt.Sprintf(" %s#%d — %s\n", pr.Repo, pr.Number, pr.Title)) b.WriteString(fmt.Sprintf(" original author agent: %s\n", author)) + if !pr.CreatedAt.IsZero() { + // The ordering key, shown so the reviewer can see for itself that + // the list really is oldest-first — the INVARIANTS below forbid + // running `gh pr list` to check. + b.WriteString(fmt.Sprintf(" opened: %s\n", pr.CreatedAt.UTC().Format(time.RFC3339))) + } b.WriteString(fmt.Sprintf(" checkout: gh pr checkout %d --repo %s\n", pr.Number, pr.Repo)) if len(pr.FailingChecks) > 0 { b.WriteString(fmt.Sprintf(" failing: %s\n", strings.Join(pr.FailingChecks, ", "))) diff --git a/src/pkg/scheduler/reviewer_lane_test.go b/src/pkg/scheduler/reviewer_lane_test.go index ce267e15f..9ed0307bb 100644 --- a/src/pkg/scheduler/reviewer_lane_test.go +++ b/src/pkg/scheduler/reviewer_lane_test.go @@ -244,3 +244,53 @@ func TestBuildAgentMessage_RoutesReviewerRole(t *testing.T) { t.Errorf("scanner must not receive the reviewer kick:\n%s", scanner) } } + +// #5617 item 4: "oldest first" now means the PR's real creation time. The old +// (repo, number) proxy sorted by repo NAME first, so against the per-kick cap +// an old escalated PR in a late-alphabet repo was starved behind newer ones in +// an early-alphabet repo on every kick, forever. +func TestFormatReviewerWorkList_TrueCreationTimeOrdering(t *testing.T) { + data := `{"ci_failing":[ + {"number":9001,"repo":"alpha/console","title":"newest","escalated":true,"created_at":"2026-09-01T00:00:00Z"}, + {"number":12,"repo":"zeta/service","title":"oldest","escalated":true,"created_at":"2026-06-01T00:00:00Z"}, + {"number":9000,"repo":"alpha/console","title":"middle","escalated":true,"created_at":"2026-07-01T00:00:00Z"} + ]}` + out := formatReviewerWorkList([]byte(data)) + oldest := strings.Index(out, "zeta/service#12") + middle := strings.Index(out, "alpha/console#9000") + newest := strings.Index(out, "alpha/console#9001") + if oldest < 0 || middle < 0 || newest < 0 { + t.Fatalf("every escalated row must be listed:\n%s", out) + } + // Under the old key the two alpha/console rows came first purely because + // "alpha" < "zeta", and the genuinely oldest PR came last. + if oldest > middle || middle > newest { + t.Errorf("rows must order by creation time, not (repo, number):\n%s", out) + } + if !strings.Contains(out, "opened: 2026-06-01T00:00:00Z") { + t.Errorf("the ordering key must be visible in the row so the reviewer can check it:\n%s", out) + } +} + +// A ci-failing.json from a hub that recorded no creation times still works: +// those rows keep the old (repo, number) proxy among themselves and sort AFTER +// every row whose age is actually known — an unproven age must not jump ahead +// of a measured one. +func TestFormatReviewerWorkList_UnknownCreationTimeSortsLast(t *testing.T) { + data := `{"ci_failing":[ + {"number":1,"repo":"aaa/repo","title":"no timestamp","escalated":true}, + {"number":2,"repo":"aaa/repo","title":"also none","escalated":true}, + {"number":900,"repo":"zzz/repo","title":"known age","escalated":true,"created_at":"2026-08-01T00:00:00Z"} + ]}` + out := formatReviewerWorkList([]byte(data)) + known := strings.Index(out, "zzz/repo#900") + if known < 0 || known > strings.Index(out, "aaa/repo#1") { + t.Errorf("a row with a known creation time must precede every row without one:\n%s", out) + } + if strings.Index(out, "aaa/repo#1") > strings.Index(out, "aaa/repo#2") { + t.Errorf("timestamp-less rows must keep the (repo, number) proxy among themselves:\n%s", out) + } + if n := strings.Count(out, "opened:"); n != 1 { + t.Errorf("only the row with a known creation time may render an opened line (got %d):\n%s", n, out) + } +}