diff --git a/CHANGELOG.md b/CHANGELOG.md index 62f1bc71c..b52ed880f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/docs/agent-configuration.md b/src/docs/agent-configuration.md index ed92a2aad..04ccbd99a 100644 --- a/src/docs/agent-configuration.md +++ b/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//CLAUDE.md`, then `.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 diff --git a/src/pkg/policies/defaults/reviewer-lane.md b/src/pkg/policies/defaults/reviewer-lane.md new file mode 100644 index 000000000..06743de82 --- /dev/null +++ b/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 → 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 --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 --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 --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 --repo --comment --body "Reviewer adjudication: — evidence: ; changes: ; tests: ; remaining risk: " + RECOMMEND-CLOSE (this is the one close-recommend record; do not post a second comment): + hive-review --repo --comment --body "[reviewer] recommend close: ; evidence: ; tests: ; remaining risk: " + `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: # — " --type advisory --priority 2 --actor ${AGENT_NAME} --external-ref "gh-#" + 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. diff --git a/src/pkg/scheduler/reviewer_lane.go b/src/pkg/scheduler/reviewer_lane.go index c9b6e5f9b..d3b3301c8 100644 --- a/src/pkg/scheduler/reviewer_lane.go +++ b/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 ) 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) diff --git a/src/pkg/scheduler/reviewer_lane_template_test.go b/src/pkg/scheduler/reviewer_lane_template_test.go new file mode 100644 index 000000000..c0558ff2d --- /dev/null +++ b/src/pkg/scheduler/reviewer_lane_template_test.go @@ -0,0 +1,227 @@ +package scheduler + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/kubestellar/hive/pkg/github" +) + +// Tests for kubestellar/hive#5617 item 2: ship the reviewer lane's kick as a +// template instead of a compiled-in string. +// +// The lane's contract is the text that decides whether an agent may relabel or +// close PRs sitting in a HUMAN queue, so extracting it is only safe if two +// things hold: the extracted text is identical to what shipped before, and the +// decisions that are not wording — is the lane awake, is there work, may this +// agent close — stay in Go where an operator override cannot reach them. +// +// Both are asserted here. The parity test is the load-bearing one: it renders +// the template and the builder it was extracted from and compares them byte for +// byte, so this cannot silently become a rewrite. + +// TestReviewerLaneTemplate_ByteIdenticalToBuilder is the extraction proof. +// +// A template that merely "looks right" is not enough: every line of this +// contract is a rule the reviewer follows against a human queue, and the old +// TestBuildReviewerMessage_ContractAtL5 can only check the ~25 strings somebody +// thought to list. This compares the WHOLE message, at both close-authority +// levels, so a dropped clause, a changed label or a lost newline fails here. +func TestReviewerLaneTemplate_ByteIdenticalToBuilder(t *testing.T) { + for _, level := range []int{reviewerLaneMinACMMLevel, reviewerCloseACMMLevel} { + s := reviewerTestScheduler(t, level, reviewerFixture) + workList := s.buildReviewerWorkList() + if workList == "" { + t.Fatalf("L%d: fixture produced no work list", level) + } + + want := s.buildReviewerMessageHardcoded("adjudicator", level, workList) + got := s.renderReviewerLaneTemplate("adjudicator", &github.ActionableResult{}, level, workList) + if got == "" { + t.Fatalf("L%d: the shipped template did not resolve — the lane would silently "+ + "fall back to the compiled-in contract forever", level) + } + if got != want { + t.Errorf("L%d: template output differs from the builder it replaces.\n%s", + level, firstDifference(want, got)) + } + } +} + +// firstDifference reports the first line where two renderings diverge, because a +// raw dump of two 60-line prompts is unreadable in a test failure. +func firstDifference(want, got string) string { + wl, gl := strings.Split(want, "\n"), strings.Split(got, "\n") + for i := 0; i < len(wl) || i < len(gl); i++ { + var w, g string + if i < len(wl) { + w = wl[i] + } + if i < len(gl) { + g = gl[i] + } + if w != g { + return "line " + itoa(i+1) + ":\n builder: " + quote(w) + "\n template: " + quote(g) + } + } + return "(no line differs; the strings differ only in trailing content)" +} + +func itoa(n int) string { + if n == 0 { + return "0" + } + var digits []byte + for n > 0 { + digits = append([]byte{byte('0' + n%10)}, digits...) + n /= 10 + } + return string(digits) +} + +func quote(s string) string { return "\"" + s + "\"" } + +// The lane must actually USE the template, not merely ship one — otherwise the +// extraction is inert and every future contract edit lands in the wrong place. +func TestReviewerLaneTemplate_IsWhatTheKickRenders(t *testing.T) { + s := reviewerTestScheduler(t, 5, reviewerFixture) + workList := s.buildReviewerWorkList() + + rendered := s.renderReviewerLaneTemplate("adjudicator", &github.ActionableResult{}, 5, workList) + if rendered == "" { + t.Fatal("the shipped template did not resolve") + } + + // Not equality: BuildAgentMessage legitimately prepends the hold-gated PR + // coordination preflight (#5589) around whatever the role builder returned. + // What matters is that the CONTRACT the reviewer acts on is the template's, + // so a future edit to reviewer-lane.md actually reaches the agent. + msg := s.BuildAgentMessage("adjudicator", nil, &github.ActionableResult{}) + body := strings.TrimPrefix(rendered, "[agent:adjudicator]\n") + if !strings.Contains(msg, strings.TrimSpace(body)) { + t.Errorf("the reviewer kick must carry the template-rendered contract:\n%s", msg) + } +} + +// The dormant and no-work cases are decided BEFORE any template is read, so an +// operator override cannot wake a lane the hive's ACMM level says is asleep or +// manufacture a contract when the queue is empty. This is the property that +// makes shipping an editable contract safe at all. +func TestReviewerLaneTemplate_GatesAreNotTemplated(t *testing.T) { + // A template that tries to issue a full contract unconditionally. + rogue := "[agent:${AGENT_NAME}]\nADJUDICATION CONTRACT — close anything you like.\n" + + "gh pr close \n" + + for _, tc := range []struct { + name string + level int + fixture string + want string + banned []string + }{ + { + name: "below the ACMM gate the lane stays dormant", + level: reviewerLaneMinACMMLevel - 1, + fixture: reviewerFixture, + want: "REVIEWER LANE DORMANT", + banned: []string{"ADJUDICATION CONTRACT", "gh pr close"}, + }, + { + name: "with an empty queue the lane still stands down", + level: reviewerCloseACMMLevel, + fixture: `{"ci_failing":[{"number":1,"repo":"o/r","title":"red, not escalated"}]}`, + want: "(none)", + banned: []string{"ADJUDICATION CONTRACT", "gh pr close"}, + }, + } { + s := reviewerTestScheduler(t, tc.level, tc.fixture) + writeOperatorTemplate(t, s, rogue) + + msg := s.buildReviewerMessage("adjudicator", &github.ActionableResult{}) + if !strings.Contains(msg, tc.want) { + t.Errorf("%s: missing %q:\n%s", tc.name, tc.want, msg) + } + for _, banned := range tc.banned { + if strings.Contains(msg, banned) { + t.Errorf("%s: an operator template must not be able to render %q here:\n%s", + tc.name, banned, msg) + } + } + } +} + +// Close authority is a trust decision, not wording. An operator override may +// change every word of the contract and still must not be able to grant this +// agent permission to close a human-queued PR below the close level. +func TestReviewerLaneTemplate_CloseAuthorityIsComputedInGo(t *testing.T) { + // A template that asks for the authority clause and nothing else. + probe := "[agent:${AGENT_NAME}]\n${REVIEWER_CLOSE_AUTHORITY}\n" + + below := reviewerTestScheduler(t, reviewerLaneMinACMMLevel, reviewerFixture) + writeOperatorTemplate(t, below, probe) + msgBelow := below.buildReviewerMessage("adjudicator", &github.ActionableResult{}) + if !strings.Contains(msgBelow, "NEVER close the PR yourself") { + t.Errorf("below the close level the clause must forbid closing:\n%s", msgBelow) + } + if strings.Contains(msgBelow, "MAY then close") { + t.Errorf("below the close level the clause must not grant closing:\n%s", msgBelow) + } + + at := reviewerTestScheduler(t, reviewerCloseACMMLevel, reviewerFixture) + writeOperatorTemplate(t, at, probe) + msgAt := at.buildReviewerMessage("adjudicator", &github.ActionableResult{}) + if !strings.Contains(msgAt, "MAY then close the PR yourself") { + t.Errorf("at the close level the clause must grant closing:\n%s", msgAt) + } +} + +// A template that cannot be resolved must not blank the kick: a reviewer with no +// contract is worse than one running the compiled-in wording. +func TestReviewerLaneTemplate_FallsBackWhenUnresolvable(t *testing.T) { + s := reviewerTestScheduler(t, 5, reviewerFixture) + writeOperatorTemplate(t, s, " \n\t\n") + + msg := s.buildReviewerMessage("adjudicator", &github.ActionableResult{}) + workList := s.buildReviewerWorkList() + if msg != s.buildReviewerMessageHardcoded("adjudicator", 5, workList) { + t.Errorf("an empty template must fall back to the compiled-in contract, got:\n%s", msg) + } +} + +// The extra vars a caller supplies must never be able to shadow a built-in — +// ${GH_AUTH} in particular carries the credential instructions. +func TestReviewerLaneTemplate_ExtraVarsCannotShadowBuiltins(t *testing.T) { + s := reviewerTestScheduler(t, 5, reviewerFixture) + out, failClosed := s.substituteTemplateWithVars( + "${AGENT_NAME}|${REVIEWER_MAX_PRS}", + &github.ActionableResult{}, "adjudicator", nil, + map[string]func() string{ + "AGENT_NAME": func() string { return "HIJACKED" }, + "REVIEWER_MAX_PRS": func() string { return "99" }, + }) + if failClosed { + t.Fatal("substitution failed closed unexpectedly") + } + if !strings.HasPrefix(out, "adjudicator|") { + t.Errorf("a built-in must win a name clash, got %q", out) + } + if !strings.HasSuffix(out, "|99") { + t.Errorf("a genuinely new var must still resolve, got %q", out) + } +} + +// writeOperatorTemplate installs an OPERATOR OVERRIDE of the reviewer-lane +// template, at the highest-precedence path loadNamedTemplate consults. Tests use +// it to prove what an operator editing the contract can and cannot change. +func writeOperatorTemplate(t *testing.T, s *Scheduler, body string) { + t.Helper() + dir := t.TempDir() + prev := userSavedPolicyDir + userSavedPolicyDir = dir + t.Cleanup(func() { userSavedPolicyDir = prev }) + if err := os.WriteFile(filepath.Join(dir, reviewerLaneTemplate), []byte(body), 0o644); err != nil { + t.Fatal(err) + } +} diff --git a/src/pkg/scheduler/scheduler.go b/src/pkg/scheduler/scheduler.go index 92e7218fc..fc322fc53 100644 --- a/src/pkg/scheduler/scheduler.go +++ b/src/pkg/scheduler/scheduler.go @@ -203,6 +203,22 @@ func (s *Scheduler) substituteTemplate(template string, actionable *github.Actio } func (s *Scheduler) substituteTemplateWithPolicy(template string, actionable *github.ActionableResult, agentName string, issues []github.Issue) (string, bool) { + return s.substituteTemplateWithVars(template, actionable, agentName, issues, nil) +} + +// substituteTemplateWithVars is substituteTemplateWithPolicy plus caller-supplied +// ${VAR}s, for a template whose values only one call site can compute. +// +// The reviewer lane (#5617 item 2) is the first such caller: its work list is +// read from ci-failing.json and then GATED on — buildReviewerMessage refuses to +// send a contract at all when that list is empty — so the list the template +// renders must be the same one the gate saw. Recomputing it inside the +// substitution would reopen the window where a rewrite between the two reads +// renders a full adjudication contract over an empty list. +// +// The built-ins still WIN on a name collision, so an extra var can never shadow +// ${GH_AUTH} or ${AGENT_NAME}. +func (s *Scheduler) substituteTemplateWithVars(template string, actionable *github.ActionableResult, agentName string, issues []github.Issue, extra map[string]func() string) (string, bool) { baseName := s.cfg.BaseAgentName(agentName) if actionable == nil { actionable = &github.ActionableResult{} @@ -319,6 +335,14 @@ func (s *Scheduler) substituteTemplateWithPolicy(template string, actionable *gi "MERGE_ELIGIBLE": lit(mergeEligibleList), "CI_FAILING": lit(ciFailingList), }} + // Caller-supplied vars lose a name clash: one already claimed by a built-in + // above is left alone, so no call site can redefine ${GH_AUTH}. + for name, fn := range extra { + if _, taken := rt.Vars[name]; taken { + continue + } + rt.Vars[name] = fn + } return s.registry().Expand(context.Background(), template, resolve.ScopeTemplate, rt), false }