✨ feat(reviewer-lane): ship the adjudication contract as a template (#5617 item 2) - #5720
Conversation
Item 2 of hivecommons#5617, the last of the four follow-ups deferred from the hivecommons#5480 delivery, and the one the dispatch on that issue asked for: extract buildReviewerMessage into pkg/policies/defaults/reviewer-lane.md. The lane reaches its prompt by ROLE, not by kick_template. An operator enables it by adding a cadence agent with `role: reviewer` — any name, no kick_template — so the ordinary resolution order never reached it and the contract was changeable only 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 an EXTRACTION, not a rewrite, and that is asserted rather than claimed: TestReviewerLaneTemplate_ByteIdenticalToBuilder renders the template and the builder it came from and compares them byte for byte, at both close-authority levels. Every line of this contract is a rule an agent follows against a HUMAN queue, so "looks right" is not a standard worth shipping — and the pre-existing TestBuildReviewerMessage_ContractAtL5 can only check the ~25 strings somebody thought to list. Those tests now exercise the template path too, since that is what buildReviewerMessage returns. Three decisions are deliberately NOT in the template, because the template is editable and these are not wording: - Is the lane awake? Below ACMM L5 the kick is a stand-down. Evaluated in Go BEFORE any template is read, so an override cannot wake a lane on a low-trust hive. - Is there work? An empty escalated queue is a stand-down. Also evaluated first, so an override cannot manufacture a contract with nothing to adjudicate. - May this agent close a PR? Operator-only below ACMM L6. Computed in Go and rendered into ${REVIEWER_CLOSE_AUTHORITY} rather than expressed as template logic: whether an agent may close a human-queued PR is a trust decision. So the worst an override can do is change the wording of a kick that was already going to be sent. All three are pinned by tests that install a rogue operator template and prove it cannot wake the lane, invent work, or grant close authority. The work list is passed INTO the substitution rather than recomputed inside it. buildReviewerMessage has already gated on that exact list; re-reading ci-failing.json during rendering would reopen a window where a rewrite between the two reads renders a full adjudication contract over an empty list. That needed a substituteTemplateWithVars variant taking caller-supplied ${VAR}s, in which the built-ins win a name clash so no call site can redefine ${GH_AUTH}. The compiled-in contract is kept as the resolution fallback — a stripped binary or an override that reads as empty must not leave the reviewer with no contract at all, which is worse than the previous wording. The parity test is what makes keeping both safe: the two cannot drift without failing it. Refs hivecommons#5617 (item 2 of 4 — the last outstanding one) Signed-off-by: Doug Baggett <doug.baggett@gmail.com>
agent-configuration.md's "Kick templates" section described a resolution order that the reviewer lane does not use — it reaches its prompt by `role: reviewer`, not by `kick_template` — so an operator reading it had no way to learn that the lane's contract is now editable, or where it lives. Adds that, and the part that actually matters: a table of the three decisions that stay in Go and cannot be changed by editing the template — whether the lane is awake, whether there is work, and whether the agent may close a PR — with why each is a trust decision rather than wording. An operator who edits this file should know exactly how much rope they have. Also disambiguates it from `reviewer-advisory.md`, which belongs to the pack-defined on-demand reviewer and never touches the needs-human queue. The two route to entirely different agents. Refs hivecommons#5617 Signed-off-by: Doug Baggett <doug.baggett@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
clubanderson
left a comment
There was a problem hiding this comment.
Reviewed the diff against the requested base in a detached worktree. Validation: cd src && go build ./... plus targeted package tests for changed Go packages (and relay tests where applicable) passed locally. I found no blocking issues.
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: c3a1021896f6447689c13676160878951b75f258 |
1 similar comment
|
LGTM label has been added. DetailsGit tree hash: c3a1021896f6447689c13676160878951b75f258 |
Summary
Item 2 of #5617 — the embedded kick template — which is the last of the four deferred follow-ups still outstanding, and the one @clubanderson's dispatch on the issue asked for. The reviewer lane's adjudication contract moves from a compiled-in string into
pkg/policies/defaults/reviewer-lane.md.The lane reaches its prompt by role, not by
kick_template: an operator enables it by adding a cadence agent withrole: reviewer— any name, nokick_template— so the ordinary resolution order never reached it and the contract was changeable only 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 an extraction, and that is asserted rather than claimed
TestReviewerLaneTemplate_ByteIdenticalToBuilderrenders the template and the builder it came from and compares them byte for byte, at both close-authority levels, reporting the first differing line on failure.That test is the load-bearing one. Every line of this contract is a rule an agent follows against a human queue, so "the template looks right" is not a standard worth shipping — and the pre-existing
TestBuildReviewerMessage_ContractAtL5can only check the ~25 strings somebody thought to list. Those existing tests now exercise the template path too, since that is whatbuildReviewerMessagereturns, which is the parity coverage step 3 of the dispatch asked for.Three decisions deliberately stay in Go
The template is editable — that is the point of shipping one. These are not wording, so an override must not be able to reach them:
${REVIEWER_CLOSE_AUTHORITY}So the worst an override can do is change the wording of a kick that was already going to be sent. All three are pinned by tests that install a rogue operator template — one that unconditionally issues a contract and offers
gh pr close— and prove it cannot wake the lane, invent work, or grant close authority.Two smaller decisions worth flagging
The work list is passed into the substitution, not recomputed inside it.
buildReviewerMessagehas already gated on that exact list; re-readingci-failing.jsonduring rendering would reopen a window where a rewrite between the two reads renders a full adjudication contract over an empty list. That needed asubstituteTemplateWithVarsvariant taking caller-supplied${VAR}s — in which the built-ins win a name clash, so no call site can redefine${GH_AUTH}. A test pins that.The compiled-in contract is kept as the resolution fallback. Step 4 of the dispatch says to remove it "only after template coverage is green"; I have kept it, because a stripped binary or an override that reads as empty must not leave the reviewer with no contract, which is worse than the previous wording. The parity test is what makes keeping both safe — they cannot drift without failing it. Removing it is a follow-up, not this PR.
Cluster claimed (files/functions)
src/pkg/policies/defaults/reviewer-lane.md(new) — the extracted contract.src/pkg/scheduler/reviewer_lane.go—reviewerLaneTemplate,reviewerKickHeader,renderReviewerLaneTemplate,reviewerCloseAuthority, andbuildReviewerMessageHardcoded(the former body, unchanged text, now the fallback).src/pkg/scheduler/scheduler.go—substituteTemplateWithVars;substituteTemplateWithPolicydelegates to it withnil, so every existing caller is untouched.src/pkg/scheduler/reviewer_lane_template_test.go(new, 6 tests),src/docs/agent-configuration.md,CHANGELOG.md.No behaviour change: this is the "pure template extraction plus parity tests" scope the dispatch specified.
Validation
go build ./...,go vet ./pkg/scheduler/ ./pkg/policies/,gofmt -lclean on every file I touched. (gofmtflags three otherpkg/schedulertest files — pre-existing drift, untouched here.)go test ./pkg/scheduler/ ./pkg/policies/— both packages pass in full, including every pre-existing reviewer-lane contract test.src/scripts/check-docs-links.py: all links and anchors resolve.Mutation-checked, and both mutations are caught by the new tests and by the pre-existing ones:
_ByteIdenticalToBuilderandTestBuildReviewerMessage_ContractAtL5_GatesAreNotTemplatedandTestBuildReviewerMessage_ACMMGateDormantBelowL5and_EmptyQueueStandsDownRelated issues
Refs #5617 (item 2 of 4 — items 1, 3 and 4 landed in #5634 and #5682). Related: #5480 (the reviewer lane), #5511 (the one-pass ladder the contract encodes).
Testing
cd src && go build ./...cd src && go test ./...— ran./pkg/scheduler/and./pkg/policies/in full (the only packages touched); both pass.— hive: backend=claude model=claude-opus-5