✨ feat(reviewer-lane): ship the adjudication contract as a template (#5617 item 2)

CHANGELOG.md

@@ -17,6 +17,8 @@ Hive did not historically maintain a complete changelog. This file starts a prag
 
 ### Changed
 
+- The reviewer lane's adjudication contract now ships as an editable template, `reviewer-lane.md`, instead of a compiled-in string ([#5617](https://github.com/kubestellar/hive/issues/5617), the last of the four follow-ups deferred from the #5480 delivery). The lane reaches it by **role** — an operator enables the lane by adding a cadence agent with `role: reviewer`, under any name and with no `kick_template`, so the ordinary template-resolution order never reached it and the contract was only changeable by rebuilding the binary. It now resolves through the same paths as every other template, so an operator-saved copy or a policies-checkout copy wins over the embedded default. This is a pure extraction: the rendered template is asserted **byte-identical** to the builder it replaces, at both ACMM close-authority levels, so no wording moved. Because this contract governs an agent acting on PRs in a human queue, three decisions are deliberately kept in Go where an override cannot reach them — whether the lane is awake at all (below ACMM L5 the kick is a stand-down), whether there is work (an empty escalated queue is a stand-down), and whether the agent may close a PR (operator-only below ACMM L6, rendered into `${REVIEWER_CLOSE_AUTHORITY}`). Both gates are evaluated before any template is read, so the worst an override can do is change the wording of a kick that was already going to be sent. The compiled-in contract is retained as the resolution fallback for the case the template cannot be resolved at all, since a reviewer with no contract is worse than one running the previous wording; a parity test fails if the two ever drift. Not to be confused with `reviewer-advisory.md`, which belongs to the pack-defined on-demand reviewer that votes on healthy PRs pre-merge and never touches the needs-human queue.
+
 - 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

src/docs/agent-configuration.md

@@ -573,6 +573,22 @@ The fetched file must be a valid portable `AgentDefinition`: `kind: AgentDefinit
 
 Resolution order: the agent's explicit `kick_template` wins; otherwise the ACMM pack's template for that agent at the current level; otherwise convention — `/data/agents/<name>/CLAUDE.md`, then `<name>.md` in the policies checkout, then the embedded default. Pack templates carry the level's policy in their names — `scanner-holdgated.md` is scanner-at-L5; the same scanner at L6 gets `scanner-automerge.md`.
 
+### The reviewer lane's template, and what an override cannot change
+
+One agent reaches its template by **role** rather than by `kick_template`: an agent with `role: reviewer` runs the escalated-PR adjudication lane ([#5480](https://github.com/kubestellar/hive/issues/5480)) and renders `reviewer-lane.md`. An operator enables that lane by adding a cadence agent with that role, under any name and with no `kick_template`, so the ordinary resolution order above never reaches it.
+
+`reviewer-lane.md` resolves through the same paths as every other template, so the contract **is** editable — the operator-saved copy wins, then the policies checkout, then the embedded default. That matters because this contract governs an agent acting on PRs sitting in a human queue, so three decisions are deliberately **not** in the template and cannot be changed by editing it ([#5617](https://github.com/kubestellar/hive/issues/5617)):
+
+| Decision | Where it lives | Why |
+|---|---|---|
+| Is the lane awake? | Go, before any template is read | Below ACMM L5 the kick is a stand-down. An edited template must not be able to wake a lane on a low-trust hive. |
+| Is there work? | Go, before any template is read | An empty escalated queue is a stand-down. A template must not be able to manufacture a contract with nothing to adjudicate. |
+| May this agent close a PR? | Go, rendered into `${REVIEWER_CLOSE_AUTHORITY}` | Closing is operator-only below ACMM L6. Whether an agent may close a human-queued PR is a trust decision, not wording. |
+
+So the worst an override can do is change the *wording* of a kick that was already going to be sent. The template-specific variables are `${REVIEWER_WORK_LIST}`, `${REVIEWER_MAX_PRS}`, `${REVIEWER_PASSED_LABEL}`, `${REVIEWER_RECOMMEND_CLOSE_LABEL}` and `${REVIEWER_CLOSE_AUTHORITY}`, alongside the usual built-ins.
+
+Do not confuse it with `reviewer-advisory.md`, which belongs to the pack-defined on-demand reviewer: that agent votes on *healthy* PRs pre-merge in advisory mode and never touches the needs-human queue.
+
 Portable agents bundle everything — config plus a `promptTemplate` — in a single `AgentDefinition` YAML you can import from a URL in the dashboard. The reference schema is [`../AGENT-DEFINITION.md`](../AGENT-DEFINITION.md), and a worked example lives at [`../examples/agents/customized-agent.yaml`](../examples/agents/customized-agent.yaml).
 
 ## Label policy: which issues agents may work

src/pkg/policies/defaults/reviewer-lane.md

@@ -0,0 +1,53 @@
+[agent:${AGENT_NAME}]
+REVIEWER — adjudicate escalated (needs-human) hive-authored PRs.
+
+${GH_AUTH}ESCALATED PRs AWAITING ADJUDICATION (max ${REVIEWER_MAX_PRS} per kick, oldest first):
+${REVIEWER_WORK_LIST}
+ADJUDICATION CONTRACT — for EACH PR above, deliver EXACTLY ONE verdict:
+  1. REPAIR — the change is still wanted and can be made lossless:
+     a. Verify still-wanted: the problem it solves still exists on the base branch
+        and no merged PR supersedes it.
+     b. Verify lossless vs main: compare the PR's diff and its TEST COUNT against
+        the base branch (diff/test-count parity). A branch that drops code or tests
+        present on main is lossy — do NOT repair it; use RECOMMEND-CLOSE instead.
+     c. Fix on the SAME branch, working from the CI evidence above:
+        gh pr checkout <number> → fix → commit -s → git push
+        Do NOT open a replacement PR.
+     d. After completing the mandatory audit below, return it to the automated lane and mark your pass:
+        gh pr edit <number> --remove-label needs-human --add-label ${REVIEWER_PASSED_LABEL}
+  2. DE-ESCALATE — the failure was environmental (base-branch regression since
+     fixed, infra flake): rebase the branch on its base, push, complete the
+     mandatory audit below, then
+        gh pr edit <number> --remove-label needs-human --add-label ${REVIEWER_PASSED_LABEL}
+  3. RECOMMEND-CLOSE — duplicate, superseded, or irreparably lossy: complete the
+     mandatory audit below using its recommend-close review body, then mark the
+     verdict delivered so it is never repeated:
+        gh pr edit <number> --add-label ${REVIEWER_RECOMMEND_CLOSE_LABEL}
+${REVIEWER_CLOSE_AUTHORITY}
+
+MANDATORY AUDIT — complete BOTH records for EVERY verdict before relabeling or closing:
+  1. Submit a comment review through Hive's relay so the adjudication is attributed
+     as `agent_pr_reviewed` (direct `gh pr comment` / `gh pr review` is not sufficient):
+        REPAIR / DE-ESCALATE:
+          hive-review <number> --repo <owner/repo> --comment --body "Reviewer adjudication: <REPAIR|DE-ESCALATE> — evidence: <why>; changes: <what>; tests: <commands/results>; remaining risk: <risk or none>"
+        RECOMMEND-CLOSE (this is the one close-recommend record; do not post a second comment):
+          hive-review <number> --repo <owner/repo> --comment --body "[reviewer] recommend close: <duplicate/superseded/lossy rationale>; evidence: <why>; tests: <checks>; remaining risk: <risk or none>"
+     `hive-review` is asynchronous: poll the `.result.json` path it prints and do
+     not proceed until that file reports `"ok": true`. A queued request is not yet
+     an audit record; an error result must leave the PR in `needs-human`.
+  2. Create the matching advisory bead so the outcome reaches the advisory digest:
+        bd create --title "Reviewer adjudication: <owner/repo>#<number> — <outcome>" --type advisory --priority 2 --actor ${AGENT_NAME} --external-ref "gh-<owner/repo>#<number>"
+     If either record fails, leave `needs-human` in place, do not add a reviewer verdict
+     label, and do not close; report the audit failure for a later retry.
+
+INVARIANTS:
+  ⛔ Adjudicate AT MOST ${REVIEWER_MAX_PRS} PRs this kick, oldest first — the list above is
+     already capped and ordered; work it top to bottom.
+  ⛔ NEVER touch a human-authored PR. The work list contains ONLY hive-authored
+     PRs; if you nonetheless encounter a PR not authored by this hive's bot
+     identity, skip it — it is not yours to adjudicate.
+  ⛔ NEVER adjudicate a PR whose ledger shows a prior reviewer pass (the
+     `${REVIEWER_PASSED_LABEL}` label). Such PRs are excluded from your list; a PR that
+     re-escalates after your pass belongs to a true human. That is why you add
+     the label on every repair/de-escalate.
+  ⛔ NEVER run gh pr list / gh search — the work list above is your ONLY source.

src/pkg/scheduler/reviewer_lane.go

@@ -72,6 +72,21 @@ const (
 	// like ReviewerPassedLabel; the kick contract instructs the reviewer to
 	// add the label alongside the recommend-close comment.
 	ReviewerRecommendCloseLabel = "reviewer-recommend-close"
+
+	// reviewerLaneTemplate is the shipped kick template for the reviewer LANE
+	// (kubestellar/hive#5617 item 2). It is deliberately a different file from
+	// reviewer-advisory.md, which belongs to the pack-defined on-demand reviewer
+	// described in the file header — that agent votes on HEALTHY PRs pre-merge
+	// and never touches the needs-human queue, so the two must never resolve to
+	// each other's prompt.
+	//
+	// The lane reaches this template through buildReviewerMessage rather than the
+	// ordinary kick_template chain, because an operator enables the lane by ROLE
+	// on an agent that may be named anything and carries no kick_template. It
+	// ships in pkg/policies/defaults, so loadNamedTemplate finds it after the
+	// operator override and git-clone paths — an operator can still customise the
+	// contract, and the safety gates below are the reason that is safe.
+	reviewerLaneTemplate = "reviewer-lane.md"
 )
 
 // agentRole resolves the effective role for an agent name: the configured
@@ -94,31 +109,103 @@ func (s *Scheduler) agentRole(agentName string) string {
 // hive-authored PR work list (from ci-failing.json) and the adjudication
 // contract — or a dormant notice when the hive's ACMM level is below the gate.
 func (s *Scheduler) buildReviewerMessage(agentName string, actionable *github.ActionableResult) string {
-	var b strings.Builder
-	b.WriteString(fmt.Sprintf("[agent:%s]\n", agentName))
-	b.WriteString("REVIEWER — adjudicate escalated (needs-human) hive-authored PRs.\n\n")
-
 	level := 0
 	if s.cfg.ACMMLevel != nil {
 		level = *s.cfg.ACMMLevel
 	}
+
+	// THE TWO GATES BELOW ARE EVALUATED IN GO, BEFORE ANY TEMPLATE IS READ, AND
+	// THAT IS THE WHOLE REASON A TEMPLATE IS SAFE TO SHIP (#5617 item 2).
+	//
+	// loadNamedTemplate honours operator overrides on disk, so the contract text
+	// is editable — which is the point of templating it. Neither of these
+	// decisions is: an edited template must not be able to un-dormant the lane on
+	// a low-trust hive, and must not be able to invent work when the queue is
+	// empty. Keeping both in code means the worst an operator can do to this
+	// template is change the WORDING of a kick that was already going to be sent.
 	if level < reviewerLaneMinACMMLevel {
-		b.WriteString(fmt.Sprintf(
+		return reviewerKickHeader(agentName) + fmt.Sprintf(
 			"⛔ REVIEWER LANE DORMANT: this hive runs at ACMM level %d; the escalated-PR\n"+
 				"adjudication lane requires level %d or above. The needs-human queue belongs\n"+
 				"entirely to human operators at this level. Do NOT touch, comment on, relabel,\n"+
 				"rebase, or close any PR. Stand down this kick.\n",
-			level, reviewerLaneMinACMMLevel))
-		return b.String()
+			level, reviewerLaneMinACMMLevel)
 	}
 
 	workList := s.buildReviewerWorkList()
 	if workList == "" {
-		b.WriteString("ESCALATED PRs AWAITING ADJUDICATION: (none)\n")
-		b.WriteString("Nothing to adjudicate — stand down this kick. Do NOT go hunting for other work.\n")
-		return b.String()
+		return reviewerKickHeader(agentName) +
+			"ESCALATED PRs AWAITING ADJUDICATION: (none)\n" +
+			"Nothing to adjudicate — stand down this kick. Do NOT go hunting for other work.\n"
+	}
+
+	// The shipped template renders the same contract from
+	// pkg/policies/defaults/reviewer-lane.md. It is proven byte-identical to the
+	// fallback below by TestReviewerLaneTemplate_ByteIdenticalToBuilder, so this
+	// is an extraction and not a rewrite. The fallback stays for the case the
+	// template cannot be resolved at all — a stripped binary, or an operator
+	// override that reads as empty — because a reviewer with no contract is far
+	// worse than one with the compiled-in wording.
+	if rendered := s.renderReviewerLaneTemplate(agentName, actionable, level, workList); rendered != "" {
+		return rendered
+	}
+	return s.buildReviewerMessageHardcoded(agentName, level, workList)
+}
+
+// reviewerKickHeader is the two-line preamble every reviewer kick opens with,
+// including the two stand-down cases that never reach a template.
+func reviewerKickHeader(agentName string) string {
+	return fmt.Sprintf("[agent:%s]\nREVIEWER — adjudicate escalated (needs-human) hive-authored PRs.\n\n", agentName)
+}
+
+// renderReviewerLaneTemplate renders the shipped reviewer-lane template, or ""
+// when it cannot be resolved or renders empty so the caller falls back.
+//
+// The reviewer-specific values are passed in rather than recomputed inside the
+// substitution: buildReviewerMessage has ALREADY decided, from this exact work
+// list, that there is work to adjudicate. Re-reading ci-failing.json here would
+// let a rewrite between the two reads render a full adjudication contract over
+// an empty list.
+func (s *Scheduler) renderReviewerLaneTemplate(agentName string, actionable *github.ActionableResult, level int, workList string) string {
+	tmpl := s.loadNamedTemplate(reviewerLaneTemplate)
+	if tmpl == "" {
+		return ""
+	}
+	extra := map[string]func() string{
+		"REVIEWER_WORK_LIST":             func() string { return workList },
+		"REVIEWER_MAX_PRS":               func() string { return fmt.Sprintf("%d", reviewerMaxPRsPerKick) },
+		"REVIEWER_PASSED_LABEL":          func() string { return ReviewerPassedLabel },
+		"REVIEWER_RECOMMEND_CLOSE_LABEL": func() string { return ReviewerRecommendCloseLabel },
+		"REVIEWER_CLOSE_AUTHORITY":       func() string { return reviewerCloseAuthority(level) },
 	}
+	body, failClosed := s.substituteTemplateWithVars(tmpl, actionable, agentName, nil, extra)
+	if failClosed || strings.TrimSpace(body) == "" {
+		return ""
+	}
+	return body
+}
 
+// reviewerCloseAuthority renders the close-authority clause for a hive's ACMM
+// level. It is computed HERE, not expressed as template logic, for the same
+// reason the dormancy gate is: whether this agent may close a human-queued PR is
+// a trust decision, and an operator editing prompt wording must not be able to
+// grant it. Returns no trailing newline; the template supplies the spacing.
+func reviewerCloseAuthority(level int) string {
+	if level >= reviewerCloseACMMLevel {
+		return fmt.Sprintf("     At this hive's ACMM level (%d) you MAY then close the PR yourself\n", level) +
+			"     (gh pr close <number>) after the audited review and bead are recorded."
+	}
+	return fmt.Sprintf("     ⛔ NEVER close the PR yourself: closing is operator-only below ACMM level %d.\n", reviewerCloseACMMLevel) +
+		"     The audited recommendation is your entire verdict; leave needs-human in place."
+}
+
+// buildReviewerMessageHardcoded is the compiled-in contract the template was
+// extracted from, kept as the resolution fallback. Any edit here must be mirrored
+// in pkg/policies/defaults/reviewer-lane.md — the parity test fails otherwise,
+// which is exactly the drift guard that makes keeping both safe.
+func (s *Scheduler) buildReviewerMessageHardcoded(agentName string, level int, workList string) string {
+	var b strings.Builder
+	b.WriteString(reviewerKickHeader(agentName))
 	b.WriteString(s.ghAuthInstructions())
 	b.WriteString(fmt.Sprintf("ESCALATED PRs AWAITING ADJUDICATION (max %d per kick, oldest first):\n", reviewerMaxPRsPerKick))
 	b.WriteString(workList)