Add LLM-assisted reviewer router (Kubernetes-only v1)#5637
Add LLM-assisted reviewer router (Kubernetes-only v1)#5637ChrisJBurns wants to merge 17 commits into
Conversation
Coarse path-glob CODEOWNERS forces every listed owner onto a PR as a required, blocking, notified reviewer, conflating gatekeeping with awareness. Owners get pinged on irrelevant churn within paths they own, while static lists go stale. Introduce an advisory, context-aware reviewer router that complements a slim deterministic gate: - REVIEWERS.md: natural-language ownership doc with Required/Notify tiers and per-area conditions (e.g. skip doc-regen, scope to a sub-feature) so routing can be more precise than a glob. - assign-reviewers.yml: pull_request-triggered workflow using a brain/hands split. The "brain" reasons over the diff with no token and no write access; a deterministic "hands" script performs all API calls. - assign-reviewers.sh: validates the brain's picks against an allowlist of handles in REVIEWERS.md, drops the author, requests reviewers, and upserts one notification comment. - CODEOWNERS.proposed: slim deterministic gate for sensitive paths only. Fork PRs receive no secret and fall back to the deterministic gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pare the first iteration down to a single area so the mechanism can prove out before expanding. REVIEWERS.md now routes only Kubernetes changes; everything else falls through to the default owner. Since routing is advisory and the merge gate stays in CODEOWNERS, the brain/hands split is more machinery than this scope needs: let the action request reviewers directly with tools restricted to reading files and the GitHub API, and a token scoped to pull-requests:write. Remove the deterministic assignment script and the precompute step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The k8s-only advisory router leaves the existing CODEOWNERS as the deterministic gate, so a slimmed-gate draft is unused until the router's coverage grows. Remove it; revisit when expanding scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5637 +/- ##
==========================================
- Coverage 70.42% 70.35% -0.07%
==========================================
Files 651 651
Lines 66285 66285
==========================================
- Hits 46678 46638 -40
- Misses 16232 16286 +54
+ Partials 3375 3361 -14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Address three review findings: - M4: read REVIEWERS.md from the base commit, not the PR head. The head can rewrite the routing rules and the handle allowlist in the same PR, so the "edits are gated" guarantee only held at merge, not at CI evaluation. Mirror how GitHub evaluates CODEOWNERS (from the base branch). - H1: narrow allowed_tools from mcp__github__* to PR inspection plus reviewer request, and correct the security comment so it no longer overstates what the step can do. - L1: add an author_association guard for consistency with claude.yml. The exact GitHub MCP reviewer-request tool name (H2) is still unverified and needs a live run to confirm reviewers are actually requested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The GitHub MCP server has no reliable human-reviewer-request tool, so the prior MCP-based write was unverified and likely a no-op. Switch to a deterministic gh api call: - Brain (Claude) now reads only local files (trusted REVIEWERS.md + a precomputed changed-file list) and writes a decision file. Its tools are Read/Glob/Grep/Write only -- no Bash, no GitHub access. - Hands: a final bash step validates the decision against the REVIEWERS.md allowlist, drops the author, and POSTs to .../requested_reviewers. This makes the write mechanism verifiable on first run (resolves H2) while keeping the brain unable to act on the PR directly. The script is inline in the workflow (retest.yaml style), not a separate file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The base-ref overwrite guarded against an author rewriting the routing rules in their own PR. But routing is advisory: rerouting only changes who is requested, and an author cannot approve their own PR or grant an out-of-org approval, so CODEOWNERS still gates the merge. The incentive runs toward getting the correct reviewer. Drop the fetch/git-show/overwrite and simply read the head copy; skip routing when REVIEWERS.md is absent. If CODEOWNERS is ever slimmed so the router carries gate weight, base-ref reads must come back. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer requests alone give no visibility into why someone was pinged, which matters for a new LLM-driven mechanism and for spotting REVIEWERS.md mis-tuning. Have Claude attach a one-sentence reason per pick, and have the hands step upsert a single marker comment listing the reviewers and reasons. The comment is found by an HTML-comment marker and edited in place on later pushes rather than re-posted, so it does not spam the PR. Reasons are filtered through jq (and stripped of '@') so free-text cannot inject mentions or break the comment payload. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The first live run failed two ways:
- Without github_token the action falls back to OIDC and dies ("Could not
fetch an OIDC token"). Pass github_token like issue-triage.yml does.
- `allowed_tools` is not a valid input on this action version and was
silently ignored. Move the restriction into claude_args --allowedTools.
The brain stays handless at the tool level: allowed tools are
Read/Glob/Grep/Write only, so Claude cannot act on the PR despite the token
being present; the gh api step still performs the request.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Throwaway commit to exercise the happy path of the reviewer router on a Kubernetes change. Will be reverted.
The blanket "Required: <everyone>" lists were copied from CODEOWNERS, so the router reproduced the exact spam it exists to prevent (a one-line operator change pinged all 8 k8s owners). Remove the blanket lists entirely: a reviewer is requested ONLY when a specific rule names them for that kind of change, and unmatched changes request no one. CODEOWNERS remains the safety net. Update the prompt to match (no default list; minimal, rule-named set). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Controller/reconcile-logic changes now route to @ChrisJBurns and @JAORMX. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the prep step that fetched the changed-file list via the API. Give the brain a read-only, scoped `Bash(git diff:*)` tool and full checkout history (fetch-depth 0) so it determines what changed itself, and can inspect actual diffs for content-based rules. Tools stay limited to file reads + git diff (no other commands, no GitHub access), so the brain still cannot act on the PR; the deterministic gh api step continues to do the request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A two-dot `git diff base HEAD` against the checked-out merge ref pulled in files that changed on main since the branch point (a stray CRD-types file showed up in routing). Switch to the three-dot form between explicit base and head SHAs, which diffs from the merge-base and shows only what the PR itself changed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🤖 Reviewer router requested the following based on
Advisory only — merge gating is governed by CODEOWNERS. |
aponcedeleonch
left a comment
There was a problem hiding this comment.
Flagging this as request-changes mainly to open a discussion before approval, not as a hard block. The PR works as framed; I'd just like to talk through an alternative direction first.
The problem you're naming is real. CODEOWNERS makes every path owner a required, blocking, notified reviewer, and that conflates two jobs: gatekeeping ("must approve") and awareness ("want to know"). The fatigue and the silent staleness are both worth fixing, and I like the care you put into the brain/hands split and the security model.
My hesitation is with the mechanism, so let me pitch a simpler alternative that keeps your goals but leans on CODEOWNERS instead of paralleling it.
Where I'm hesitant
REVIEWERS.mdis still a hand-maintained owner list. In v1 it names two people across two rules, so the ownership isn't smarter than CODEOWNERS, it's the same hardcoded mapping in prose. The new capability is really conditional firing, not smarter ownership.- Most of those conditions are path globs plus simple exclusions a deterministic script could do. The one case needing semantic judgment is "comment-only vs logic change," which feels thin for an LLM call, an API-key dependency, and nondeterminism on every PR.
- It rides alongside CODEOWNERS rather than replacing it, so today it's net-added complexity. The spam payoff only lands once CODEOWNERS is slimmed, which this defers.
The counter-proposal: keep CODEOWNERS, add one weekly job
CODEOWNERS stays the single source GitHub already enforces, carrying both personas. The only thing we add is a weekly job that recomputes the file and opens a bot-drafted PR for a human to approve. Nothing reshapes ownership without that review, same as today, you're just reviewing a draft instead of writing globs by hand.
People who want to follow an area opt in by name, never by path. The job handles the path expansion.
.github/CODEOWNERS:
# ---- area definitions: friendly name -> globs (job-maintained) ----
# Names are stable and human-chosen. The globs under each name are recomputed
# weekly, so subscribing by name never goes stale.
#@area operator-controllers: /cmd/thv-operator/controllers/** /pkg/operator/**
#@area auth-registry: /pkg/registry/auth/**
#@area cli: /cmd/thv/**
# ---- subscriptions: opt in by area name, never by path ----
# Add yourself with a one-line PR. You never touch globs.
#@subscribe auth-registry: @alejandro
#@subscribe operator-controllers: @JAORMX
# ===================== GENERATED BELOW, do not hand-edit =====================
# must-review owners (history-derived)
# BEGIN auto-owners
/cmd/thv-operator/controllers/ @ChrisJBurns
/pkg/operator/ @ChrisJBurns
/pkg/registry/auth/ @johnford
# END auto-owners
# want-to-know subscribers (expanded from the @subscribe directives above)
# BEGIN auto-subscribers
/pkg/registry/auth/ @alejandro
/cmd/thv-operator/controllers/ @JAORMX
/pkg/operator/ @JAORMX
# END auto-subscribers
GitHub ignores every #@... line, so the directives are just friendly input. The BEGIN/END blocks are the compiled output that actually takes effect.
What the weekly job does
All three steps land in one reviewed PR:
- Recompute areas. Refresh the globs under each
#@areaname from the current directory structure, so new and moved code stays mapped. Names stay fixed, only the globs move, so existing subscriptions never break. - Recompute must-review owners. Read git history per area, filter against the active roster, and rewrite the
auto-ownersblock with the dominant active author. - Re-expand subscribers. For each
#@subscribe <area>: @handle, look up that area's refreshed globs and rewrite theauto-subscribersblock.
The job only ever rewrites between the markers. Area definitions and subscription directives above them are yours, edited by normal PRs.
Opting in by area name
This is the part you wanted to keep easy. Say you want to follow the auth registry. You add one line, #@subscribe auth-registry: @alejandro, and open a PR. You never work out which paths that means. The job resolves the name to globs and writes the real /pkg/registry/auth/ @alejandro line for you. Because people subscribe to the name and not the paths, the weekly glob refresh can move files around underneath without orphaning anyone.
The one caveat: renaming an area is the thing that breaks subscriptions, since the name is the join key. The job should fail the PR (or warn loudly) if a #@subscribe points at an area name that no longer exists, rather than silently dropping it.
Why I'd lean this way
It's CODEOWNERS doing what it already does, plus one job, in one file. No parallel ownership doc, no per-PR LLM, no API key, no nondeterminism on the critical path. The staleness you called out gets fixed by the weekly recompute. The want-to-know case becomes a one-line, by-name opt-in. And both owners and subscribers still land through a reviewed PR, so a human is always between the heuristic and the merge gate.
To be fair, the history-derived owners are the part with the most edge cases, which is exactly why that half stays human-approved rather than auto-merged. Curious what you think.
Summary
Today's
.github/CODEOWNERSassigns reviewers purely by path glob, which makes every listed owner a required, blocking, notified reviewer. That conflates two different jobs — gatekeeping ("must approve") and awareness ("want to know") — and the result is notification fatigue: owners get pinged on churn within paths they own but no longer actively work, while static lists quietly go stale.This PR introduces an advisory, context-aware reviewer router that rides on top of the existing CODEOWNERS gate (which is left untouched and still governs merges). Instead of a coarse glob, an LLM reads a natural-language ownership doc and requests reviewers based on what actually changed — e.g. it can require the controller owner on reconcile-logic changes but skip the full operator list for a pure CRD-doc regeneration.
Scope is intentionally limited to Kubernetes changes for this first iteration so the mechanism can prove out before expanding.
.github/REVIEWERS.md— natural-language ownership doc (Kubernetes-only for v1) with per-area conditions that let routing be more precise than a glob (skip doc-regen/chart-version-bump/generated-only changes; always include@ChrisJBurnson controller changes)..github/workflows/assign-reviewers.yml— apull_request-triggered workflow that runsclaude-code-action(same pattern as the existing issue-triage workflow) to request the appropriate reviewers.Type of change
Test plan
This is CI/automation config (no Go code). Verification so far: YAML structure and action/SHA pins match the existing
issue-triage.ymlandclaude.ymlworkflows. End-to-end behavior (the action requesting reviewers on a real Kubernetes PR) needs to be observed once merged or tested on a throwaway PR, since the workflow only runs with repository secrets available.Does this introduce a user-facing change?
No. This only affects the reviewer-assignment experience for contributors; it does not change ToolHive's runtime behavior.
Special notes for reviewers
Security model. The workflow uses
pull_request(notpull_request_target, which this repo deliberately avoids), so fork/dependabot PRs receive noANTHROPIC_API_KEYand the Claude step skips — those fall back to the deterministic CODEOWNERS gate. The token is scoped topull-requests: writeandallowed_toolsis restricted to reading repo files plus the GitHub API. Routing is advisory: the worst a prompt-injected diff can do is request the wrong reviewer, which CODEOWNERS still gates against.Known follow-ups (intentionally out of scope here):
allowed_toolsis currentlymcp__github__*; it should be narrowed to just the reviewer-request tool. Left broad for the v1 trial.Generated with Claude Code