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 0000000000..f1b1a9981a --- /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 c49691ab20..f37560ec34 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 2fac0685cb..3cada0aa68 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") + } +}