From 5755b285ffbd4b01182b5fbc5202e86429ebb353 Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Thu, 17 Sep 2026 22:44:35 -0400 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20feat(review):=20route=20blocking=20?= =?UTF-8?q?verdicts=20to=20a=20human?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A hive at ACMM L5 cannot merge, so the only possible outcome of a review is a human acting on it. Reviewers already produce well-cited comments with requires_human and reject verdicts, but nothing carried those to a person: a blocking finding landed in a queue of hundreds looking exactly like routine review noise, and the two reviews posted on the projectbluefin spoke contained zero mentions and no way to filter for "a human must decide this". Add a routing block to the publish half of the kick. A requires_human or reject verdict must open the comment with a pinned `**HUMAN DECISION NEEDED**` marker, which is the string a maintainer triaging a long queue can actually search on; its whole value is being identical everywhere, so it is pinned by test. Mention the PR author only when the author is a person. On a hive fleet most PRs are agent-authored, so the author is an App ("app/") or a bot ("[bot]"), and @-mentioning either notifies no one while still reading as though the review had been routed somewhere. That false signal is worse than no mention, so for those authors the prompt explicitly suppresses the mention and leans on the marker instead. Also ask the reviewer to report the limits of its own review. An honest "I could not verify X" is more useful than a confident guess, and leaving it out is how an unreviewed change gets waved through on the strength of an automated approval. Routing rides the existing review.post_comments switch, so a hive that has not opted into commenting sees no prompt change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Andrew Anderson --- .../added-reviewer-routes-to-humans.md | 1 + src/pkg/review/prompts.go | 39 ++++++++++ src/pkg/review/publish_prompt_test.go | 78 +++++++++++++++++++ 3 files changed, 118 insertions(+) create mode 100644 changelog.d/added-reviewer-routes-to-humans.md diff --git a/changelog.d/added-reviewer-routes-to-humans.md b/changelog.d/added-reviewer-routes-to-humans.md new file mode 100644 index 000000000..f1b1a9981 --- /dev/null +++ b/changelog.d/added-reviewer-routes-to-humans.md @@ -0,0 +1 @@ +- Review comments now route: a `requires_human` or `reject` verdict opens with a pinned `**HUMAN DECISION NEEDED**` marker maintainers can filter on, mentions the PR author when that author is a person (never an app/bot account, which notifies nobody), and asks the reviewer to state plainly what it could not judge instead of approving around it. diff --git a/src/pkg/review/prompts.go b/src/pkg/review/prompts.go index c49691ab2..f37560ec3 100644 --- a/src/pkg/review/prompts.go +++ b/src/pkg/review/prompts.go @@ -86,9 +86,48 @@ func buildPublishInstruction(pr PullRequest) string { b.WriteString("Say nothing rather than pad. Do NOT post a comment that is only nits, only praise, or a restatement of the diff. If this perspective found nothing a human needs, skip the comment entirely and just return the JSON.\n") b.WriteString("If the PR body claims behavior the diff does not implement, say so with file:line — that gap is one of the most useful things you can report.\n") b.WriteString("Be brief and specific. One comment, at most a few findings, worst first.\n") + b.WriteString(buildRoutingInstruction(pr)) return b.String() } +// buildRoutingInstruction is what turns a verdict into a decision. +// +// A hive that cannot merge produces reviews whose only possible outcome is a +// human acting on them. But a correct, well-cited comment buried in a queue of +// hundreds is not actionable: nothing distinguishes "a human must decide this" +// from routine review noise. Routing is therefore not a nicety on top of the +// review — it is the step that makes the review reach anyone. +func buildRoutingInstruction(pr PullRequest) string { + var b strings.Builder + b.WriteString("\nROUTING — make a needed human decision findable.\n") + b.WriteString("If your verdict is requires_human or reject, the FIRST line of the comment must be exactly:\n") + b.WriteString(" **HUMAN DECISION NEEDED** — \n") + b.WriteString("A maintainer triaging a long queue filters on that marker; without it a blocking finding reads as one more comment and is skipped.\n") + if handle := mentionableAuthor(pr.Author); handle != "" { + fmt.Fprintf(&b, "On that same line, mention @%s (the PR author) so the person who can act is notified.\n", handle) + } else { + b.WriteString("Do NOT @-mention the PR author: this PR was opened by an app or bot account, so a mention notifies nobody. The marker line is the routing.\n") + } + b.WriteString("Report the limits of your own review. If you could not judge part of this PR — missing context, an unfamiliar subsystem, an ambiguous requirement, a change you cannot test — say so plainly and use requires_human. Naming what you could not verify is more useful than a confident guess, and omitting it is how an unreviewed change gets waved through on the strength of an automated approval.\n") + return b.String() +} + +// mentionableAuthor returns the bare @-handle for a PR author when mentioning +// it would reach a person, and "" when it would not. +// +// On a hive fleet most PRs are agent-authored, so the author is an App +// ("app/") or a bot ("[bot]"). @-mentioning either notifies no one +// — it renders as a link and nothing else — while still looking to a reader +// like the review was routed somewhere. That false signal is worse than no +// mention at all, because it suggests a human is already on it. +func mentionableAuthor(author string) string { + a := strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(author), "@")) + if a == "" || strings.HasSuffix(a, "[bot]") || strings.Contains(a, "/") { + return "" + } + return a +} + func BuildPerspectivePrompts(pr PullRequest, perspectives []Perspective) map[Perspective]string { if len(perspectives) == 0 { perspectives = DefaultPerspectives diff --git a/src/pkg/review/publish_prompt_test.go b/src/pkg/review/publish_prompt_test.go index 2fac0685c..3cada0aa6 100644 --- a/src/pkg/review/publish_prompt_test.go +++ b/src/pkg/review/publish_prompt_test.go @@ -111,3 +111,81 @@ func TestPromptPublishAppliesToEveryPerspective(t *testing.T) { } } } + +// TestRoutingMarkerIsPinned pins the exact marker a maintainer filters on. The +// value of the marker is entirely in its being identical across every repo and +// every reviewer; a reworded variant is unsearchable and therefore useless. +func TestRoutingMarkerIsPinned(t *testing.T) { + got := BuildPerspectivePromptOpts(PerspectiveCorrectness, testPR(), true) + + if !strings.Contains(got, "**HUMAN DECISION NEEDED**") { + t.Error("publish prompt does not pin the HUMAN DECISION NEEDED marker") + } + if !strings.Contains(got, "requires_human or reject") { + t.Error("publish prompt does not tie the marker to the blocking verdicts") + } +} + +// TestRoutingDoesNotMentionBotAuthors is the point of the author check: the +// common case on a hive is an agent-authored PR, and mentioning the App that +// opened it notifies nobody while looking like the review was routed. +func TestRoutingDoesNotMentionBotAuthors(t *testing.T) { + for _, author := range []string{ + "kubestellar-hive[bot]", + "app/kubestellar-hive", + "", + } { + pr := testPR() + pr.Author = author + got := BuildPerspectivePromptOpts(PerspectiveCorrectness, pr, true) + + if strings.Contains(got, "(the PR author) so the person who can act") { + t.Errorf("author %q: prompt asks the reviewer to mention a non-human author", author) + } + if !strings.Contains(got, "Do NOT @-mention the PR author") { + t.Errorf("author %q: prompt omits the do-not-mention instruction", author) + } + } +} + +// TestRoutingMentionsHumanAuthors is the converse: when a person opened the PR, +// the mention is the fastest path from finding to decision. +func TestRoutingMentionsHumanAuthors(t *testing.T) { + pr := testPR() + pr.Author = "@clubanderson" + got := BuildPerspectivePromptOpts(PerspectiveCorrectness, pr, true) + + if !strings.Contains(got, "mention @clubanderson (the PR author)") { + t.Error("prompt does not ask the reviewer to mention the human PR author") + } + // The leading @ must not be doubled when the author already carries one. + if strings.Contains(got, "@@") { + t.Error("prompt double-prefixed the author handle with @") + } + if strings.Contains(got, "Do NOT @-mention the PR author") { + t.Error("prompt suppressed the mention for a human author") + } +} + +// TestRoutingAsksForReviewLimits covers the capability-honesty half: a reviewer +// that cannot judge something must say so rather than approve around it. +func TestRoutingAsksForReviewLimits(t *testing.T) { + got := BuildPerspectivePromptOpts(PerspectiveCorrectness, testPR(), true) + + if !strings.Contains(got, "Report the limits of your own review") { + t.Error("publish prompt does not ask the reviewer to report what it could not judge") + } +} + +// TestRoutingIsOptInWithPublish keeps routing on the same switch as publishing: +// a hive that has not opted into comments must not have its prompt changed. +func TestRoutingIsOptInWithPublish(t *testing.T) { + got := BuildPerspectivePrompt(PerspectiveCorrectness, testPR()) + + if strings.Contains(got, "HUMAN DECISION NEEDED") { + t.Error("default prompt contains the routing marker; routing must be opt-in with publishing") + } + if strings.Contains(got, "ROUTING") { + t.Error("default prompt contains the routing block; routing must be opt-in with publishing") + } +}