From b45de2f3d8b97980b268e80798d8b1c21ae330f9 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 22 Jul 2026 15:56:56 -0700 Subject: [PATCH 1/2] =?UTF-8?q?feat(refresh-reviewers):=20reusable=20workf?= =?UTF-8?q?low=20=E2=80=94=20recompute=20the=20reviewer=20expertise=20map?= =?UTF-8?q?=20from=20git=20history,=20open=20a=20drift=20PR=20(BE-4116)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/refresh-reviewers/README.md | 94 ++ .github/refresh-reviewers/generate.py | 827 ++++++++++++++++++ .../refresh-reviewers/tests/test_generate.py | 411 +++++++++ .github/workflows/refresh-reviewers.yml | 259 ++++++ .github/workflows/test-refresh-reviewers.yml | 39 + AGENTS.md | 10 + README.md | 1 + 7 files changed, 1641 insertions(+) create mode 100644 .github/refresh-reviewers/README.md create mode 100644 .github/refresh-reviewers/generate.py create mode 100644 .github/refresh-reviewers/tests/test_generate.py create mode 100644 .github/workflows/refresh-reviewers.yml create mode 100644 .github/workflows/test-refresh-reviewers.yml diff --git a/.github/refresh-reviewers/README.md b/.github/refresh-reviewers/README.md new file mode 100644 index 0000000..629c248 --- /dev/null +++ b/.github/refresh-reviewers/README.md @@ -0,0 +1,94 @@ +# refresh-reviewers — recompute the reviewer expertise map from git history + +The engine behind [`refresh-reviewers.yml`](../workflows/refresh-reviewers.yml): +a scheduled drift-detector that recomputes the caller repo's +`.github/reviewers.yml` (the config [`assign-reviewers.yml`](../workflows/assign-reviewers.yml) +consumes at PR time) from git history and opens **one reviewable single-file +PR** when the committed map has fallen behind reality. It never assigns anyone +and never merges anything — the drift PR is the whole deliverable, and a human +accepts or edits it. + +## How it scores + +Per commit on the default branch within the window, per rule bucket touched by +≥1 surviving changed file: + +``` +score[rule][login] += 0.5 ** (age_days / half_life_days) +touches[rule][login] += 1 +``` + +- **Buckets are the committed rules.** Each `rules:` entry's path globs are the + bucket definition, matched with the *same* glob semantics as + assign-reviewers.yml's `globToRegExp` (`*` within a segment, `**` across, + `?` one non-slash char) — the map is only correct if it is scored with the + matcher the runtime assigns with. +- **Line counts are intentionally unused.** Recency-decayed commit *touches* + are the signal; numstat is read only for the changed-file list. +- **Excluded:** bot authors (`[bot]@` emails, `noreply@argoproj.io`), + generated/churn paths (ent codegen outside `ent/schema/`, `*.gen.go`, + `*.pb.go`, `vendor/`, Go sum files, JS/generic lockfiles, dynamicconfig + version bumps, `frontend-version.json`, plus caller `extra_exclude_paths` + regexes), and rename noise (numstat's `{old => new}` resolves to the new + path). +- **Emails → logins:** `login@users.noreply.github.com` (and the + `digits+login@` form) decode directly; every other distinct email costs one + `GET /repos/{owner}/{repo}/commits/{sha}` to read `.author.login`. Commits + whose email can't be resolved are dropped and the count is reported in the + PR body. +- **Eligibility = repo collaborators** (paginated `GET .../collaborators`) — + *not* org members, because `addAssignees` silently drops non-collaborators, + so the collaborator set is the exact test the runtime applies. + +## Selection + +Per rule: contributors with `touches >= min_touches` and `score >= min_score`, +ranked by score, capped at `top_k`. Below `floor` qualifiers, the ranked +remainder backfills (needing only `touches >= floor_min_touches`); a rule +still under the floor is **left unchanged** and noted in the PR body — the +runtime already falls back to `default_pool` when a rule can't match, so a +cold bucket keeps its hand-set owners. `default_pool` becomes the top-5 +whole-repo scorers minus anyone already anchoring ≥2 rules (the anti-pile-on +rationale from ComfyUI_frontend#5448) minus `map_exclude`. + +The rewrite is **surgical**: only the `reviewers: [...]` / `default_pool: +[...]` sequences (flow or block) are replaced; every comment and all other +bytes are preserved. The config's comments are its documentation — a +YAML-dump rewrite would be a regression. + +The PR body carries the per-rule before/after table with scores/touches, the +unresolved-email count, the knob values, and a report-only **taxonomy gap** +section: the hottest top-two-level directories matched by *no* rule glob, +with their top contributors — candidates for new rules, never auto-added. + +## Knob defaults (and why) + +| knob | default | rationale | +|---|---|---| +| `window_months` | 12 | Long enough to cover slow-moving subsystems; validated in BE-4114 against cloud history. | +| `half_life_days` | 90 | The decay is what correctly aged out stale expertise (e.g. inference contributors who had moved on); 90d matched observed team reality where flat counts did not. | +| `top_k` / `floor` | 4 / 2 | Enough experts to load-balance across without piling every rule onto the same two people. | +| `min_touches` / `min_score` | 5 / 1.5 | Filters drive-by contributors: one huge recent commit is not sustained expertise. | +| `floor_min_touches` | 2 | Relaxed bar used only to reach the floor. | +| `pr_branch` | `bot/refresh-reviewers` | Stable branch, force-reset each run — re-runs update the one open drift PR instead of stacking duplicates. | + +## `map_exclude` guidance + +`map_exclude` controls who may appear in the **committed map**; it is distinct +from the runtime `vars.REVIEWER_EXCLUDE` (who assign-reviewers skips at PR +time). Seed it with operator logins whose commit volume is largely +**agent-authored** — their history signal is machine throughput, not personal +review expertise, and without the exclusion they would anchor every bucket. + +## Files + +- `generate.py` — the whole engine (stdlib-only; parsing, scoring, selection, + surgical rewrite, report/PR-body emission). Loaded at run time from a pinned + ref of this repo, never from the caller's checkout. +- `tests/test_generate.py` — pure-python tests: glob parity, decay math, + threshold/floor/backfill selection, bot/path/rename filtering, noreply + decoding, byte-preserving rewrite. Run: + +```bash +python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' -v +``` diff --git a/.github/refresh-reviewers/generate.py b/.github/refresh-reviewers/generate.py new file mode 100644 index 0000000..0a57933 --- /dev/null +++ b/.github/refresh-reviewers/generate.py @@ -0,0 +1,827 @@ +#!/usr/bin/env python3 +"""Recompute the reviewer expertise map (reviewers.yml) from git history. + +Backs the reusable `refresh-reviewers.yml` workflow (BE-4114 spike → BE-4116). +Reads the caller repo's committed reviewers.yml (the same config +`assign-reviewers.yml` consumes at runtime), walks the default branch's git +history over a decaying window, scores each collaborator's expertise per rule +bucket, and surgically rewrites ONLY the `reviewers: [...]` / `default_pool: +[...]` lists — every comment and all other bytes are preserved, because the +config's comments are its documentation. + +Signal model (validated against cloud history in the BE-4114 spike): + - per commit, per rule bucket touched by >=1 surviving file: + score[bucket][login] += 0.5 ** (age_days / half_life_days) + touches[bucket][login] += 1 + - line counts are intentionally unused (numstat is only the file list); + recency-decayed commit touches aged out stale expertise correctly where + line-weighted variants did not. + +Exclusions: + - bot authors (email matches `\\[bot\\]@` or noreply@argoproj.io), + - generated/churn paths (codegen, vendored deps, lockfiles, mechanical + version-bump files) plus caller-supplied EXTRA_EXCLUDE_PATHS regexes, + - logins outside the repo's collaborator set (the exact eligibility test the + runtime applies: `addAssignees` silently drops non-collaborators), + - MAP_EXCLUDE logins (e.g. an operator whose commits are agent-authored). + +This script is a drift DETECTOR, never a live mutator: it emits the rewritten +config + a machine-readable report for the workflow's PR step and exits. +Environmental problems (missing config, unreachable API) are downgraded to a +clean no-op with a `::warning::` — same never-fail posture as +assign-reviewers.yml — while real bugs still raise. + +Environment (all optional unless noted): + GITHUB_REPOSITORY owner/repo of the caller (required) + GH_TOKEN app token for the commits/collaborators API (required) + DEFAULT_BRANCH the caller's default branch (required) + REVIEWER_CONFIG_PATH path to reviewers.yml (default .github/reviewers.yml) + WINDOW_MONTHS history window (default 12) + HALF_LIFE_DAYS decay half-life (default 90) + TOP_K max experts per rule (default 4) + FLOOR min experts per rule (default 2) + MIN_TOUCHES qualify: raw touches (default 5) + MIN_SCORE qualify: decayed score (default 1.5) + FLOOR_MIN_TOUCHES backfill touch floor (default 2) + MAP_EXCLUDE whitespace-separated logins never placed in the map + EXTRA_EXCLUDE_PATHS newline-separated regexes appended to the built-ins + RESULTS_DIR where outputs land (default $RUNNER_TEMP or .) + GITHUB_API_URL API base (default https://api.github.com) + GITHUB_OUTPUT step-output file (written when present) + +Outputs (in RESULTS_DIR): reviewers.new.yml, report.json, pr-body.md. +Step outputs: changed=true|false, new_config_path, report_path, pr_body_path. +""" + +import json +import os +import re +import subprocess +import sys +import time +import urllib.error +import urllib.request +from collections import defaultdict + +BOT_EMAIL_RX = re.compile(r"(\[bot\]@|noreply@argoproj\.io)", re.IGNORECASE) +NOREPLY_RX = re.compile(r"^(?:\d+\+)?([^@]+)@users\.noreply\.github\.com$", re.IGNORECASE) +RENAME_BRACE_RX = re.compile(r"\{[^}]* => ([^}]*)\}") + +# Generated/churn paths whose commits carry no expertise signal. Regexes are +# re.search()'d against the (rename-normalized) path. +BUILTIN_EXCLUDE_PATHS = [ + r"(^|/)ent/(?!schema/)", # ent codegen; hand-written schema/ still counts + r"\.gen\.go$", + r"\.pb\.go$", + r"(^|/)vendor/", + r"(^|/)(go\.sum|go\.work[^/]*\.sum)$", + r"(^|/)(package-lock\.json|pnpm-lock\.yaml|yarn\.lock)$", + r"\.lock$", + # mechanical version-bump churn (e.g. "update staging template to 0.9.37") + r"^infrastructure/dynamicconfig/[^/]+/config\.json$", + r"^frontend-version\.json$", +] + + +# --- glob semantics (parity with assign-reviewers.yml's globToRegExp) -------- + +def glob_to_regexp(glob): + """Port of assign-reviewers.yml's globToRegExp: `*` within a segment, + `**` across segments (`**/` -> optional leading dirs), `?` one non-slash + char. Full-string anchored. Must stay byte-for-byte semantics-equal to the + JS original — the map is only correct if it is scored with the same + matcher the runtime assigns with.""" + out = "" + i = 0 + n = len(glob) + while i < n: + c = glob[i] + if c == "*": + if i + 1 < n and glob[i + 1] == "*": + if i + 2 < n and glob[i + 2] == "/": + out += "(?:.*/)?" + i += 3 + continue + out += ".*" + i += 2 + continue + out += "[^/]*" + elif c == "?": + out += "[^/]" + elif c in ".+^${}()|[]\\/": + out += "\\" + c + else: + out += c + i += 1 + return re.compile("^" + out + "$") + + +def matches_any(path, compiled_globs): + return any(rx.match(path) for rx in compiled_globs) + + +# --- reviewers.yml parsing (parity with parseReviewerConfig) ----------------- +# +# Mirrors assign-reviewers.yml's minimal parser (default_pool + rules[{paths, +# reviewers}], flow or block sequences, comment stripping) but ALSO records +# where each reviewers/default_pool list lives so the rewrite can touch only +# those bytes. Location shapes: +# ("flow", line_idx) — `reviewers: [a, b]` (also bare scalar) +# ("block", [line_idx, ...], indent) — `- a` item lines + +def _strip_comment(s): + in_s = in_d = False + for i, ch in enumerate(s): + if ch == "'" and not in_d: + in_s = not in_s + elif ch == '"' and not in_s: + in_d = not in_d + elif ch == "#" and not in_s and not in_d and (i == 0 or s[i - 1].isspace()): + return s[:i] + return s + + +def _unquote(s): + s = s.strip() + if len(s) >= 2 and ((s[0] == '"' and s[-1] == '"') or (s[0] == "'" and s[-1] == "'")): + return s[1:-1] + return s + + +def _parse_flow(s): + s = s.strip() + if not s.startswith("["): + return None + end = s.find("]") + inner = s[1:end] if end != -1 else s[1:] + return [x for x in (_unquote(p) for p in inner.split(",")) if x] + + +def _indent_of(line): + return len(line) - len(line.lstrip(" ")) + + +def parse_reviewer_config(text): + """Return (config, locations). + + config = {"default_pool": [...], "rules": [{"paths": [...], + "reviewers": [...]}, ...]} (parser-parity shape) + locations = {"default_pool": loc-or-None, + "rules": [loc-or-None, ...]} (reviewers-list positions) + """ + raw_lines = text.split("\n") + lines = [_strip_comment(l) for l in raw_lines] + config = {"default_pool": [], "rules": []} + locs = {"default_pool": None, "rules": []} + + i = 0 + n = len(lines) + while i < n: + raw = lines[i] + line = raw.strip() + if not line: + i += 1 + continue + if _indent_of(raw) == 0 and line.startswith("default_pool:"): + rest = line[len("default_pool:"):].strip() + flow = _parse_flow(rest) + if flow is not None: + config["default_pool"] = flow + locs["default_pool"] = ("flow", i) + i += 1 + continue + i += 1 + items = [] + item_lines = [] + indent = 2 + while i < n: + r = lines[i] + if not r.strip(): + i += 1 + continue + if _indent_of(r) == 0: + break + t = r.strip() + if t.startswith("- "): + items.append(_unquote(t[2:])) + item_lines.append(i) + indent = _indent_of(r) + i += 1 + config["default_pool"] = items + if item_lines: + locs["default_pool"] = ("block", item_lines, indent) + continue + if _indent_of(raw) == 0 and line.startswith("rules:"): + i += 1 + current = None + cur_loc = None + list_key = None + rule_indent = -1 + + def set_key(seg, line_idx): + nonlocal list_key + m = re.match(r"^(paths|reviewers):(.*)$", seg) + if not m: + return + key, val = m.group(1), m.group(2).strip() + flow = _parse_flow(val) + if flow is not None: + if current is not None: + current[key] = flow + if key == "reviewers": + cur_loc["reviewers"] = ("flow", line_idx) + list_key = None + elif val == "": + list_key = key + else: + if current is not None: + current[key] = [_unquote(val)] + if key == "reviewers": + # bare scalar — rewritten as a flow sequence + cur_loc["reviewers"] = ("flow", line_idx) + list_key = None + + while i < n: + r = lines[i] + if not r.strip(): + i += 1 + continue + if _indent_of(r) == 0: + break + ind = _indent_of(r) + t = r.strip() + is_dash = t == "-" or t.startswith("- ") + if is_dash and (rule_indent == -1 or ind == rule_indent): + if rule_indent == -1: + rule_indent = ind + current = {"paths": [], "reviewers": []} + cur_loc = {"reviewers": None} + config["rules"].append(current) + locs["rules"].append(cur_loc) + list_key = None + after_dash = t[1:].strip() + if after_dash: + set_key(after_dash, i) + elif is_dash and list_key and current is not None: + current[list_key].append(_unquote(t[1:].strip())) + if list_key == "reviewers": + if cur_loc["reviewers"] is None: + cur_loc["reviewers"] = ("block", [], ind) + if cur_loc["reviewers"][0] == "block": + cur_loc["reviewers"][1].append(i) + else: + set_key(t, i) + i += 1 + continue + i += 1 + # locs["rules"] holds dicts internally; expose just the reviewers loc. + locs["rules"] = [r["reviewers"] for r in locs["rules"]] + return config, locs + + +# --- surgical rewrite -------------------------------------------------------- + +def _rewrite_flow_line(line, key, logins): + """Replace only the `[...]` span on a flow line, preserving everything + else (leading bytes, spacing, trailing comment). A bare-scalar value is + converted to a flow sequence up to the trailing comment. Brackets are + located in the comment-STRIPPED portion so a `[` inside a trailing + comment can never be mistaken for the flow sequence.""" + new_list = "[" + ", ".join(logins) + "]" + stripped = _strip_comment(line) + key_pos = stripped.find(key) + open_idx = stripped.find("[", key_pos) + if open_idx != -1: + close_idx = stripped.find("]", open_idx) + end = close_idx + 1 if close_idx != -1 else len(stripped.rstrip()) + return line[:open_idx] + new_list + line[end:] + # scalar form: `reviewers: alice # note` -> replace the value span only + key_end = key_pos + len(key) + return line[:key_end] + " " + new_list + line[len(stripped.rstrip()):] + + +def rewrite_config(text, locs, rule_replacements, default_pool_replacement): + """Rewrite ONLY the reviewer lists named in the replacement maps. + + rule_replacements: {rule_index: [logins]} — rules absent from the map keep + their bytes untouched. default_pool_replacement: [logins] or None. + Everything outside the replaced flow spans / block item lines — comments + included — is preserved byte-for-byte.""" + lines = text.split("\n") + # (loc, key, logins) for every list being replaced + jobs = [] + if default_pool_replacement is not None and locs["default_pool"] is not None: + jobs.append((locs["default_pool"], "default_pool:", default_pool_replacement)) + for idx, logins in rule_replacements.items(): + if idx < len(locs["rules"]) and locs["rules"][idx] is not None: + jobs.append((locs["rules"][idx], "reviewers:", logins)) + + drop = set() # block item lines to remove + insert_at = {} # first block item line -> replacement item lines + for loc, key, logins in jobs: + if loc[0] == "flow": + lines[loc[1]] = _rewrite_flow_line(lines[loc[1]], key, logins) + else: + _, item_lines, indent = loc + drop.update(item_lines) + insert_at[item_lines[0]] = [" " * indent + "- " + l for l in logins] + + out = [] + for i, line in enumerate(lines): + if i in insert_at: + out.extend(insert_at[i]) + if i in drop: + continue + out.append(line) + return "\n".join(out) + + +# --- git log parsing --------------------------------------------------------- + +def normalize_numstat_path(path): + """Normalize a numstat path: strip git's quoting, resolve both rename + syntaxes (`a/{old => new}/c` and whole-path `old => new`) to the NEW + path.""" + if len(path) >= 2 and path[0] == '"' and path[-1] == '"': + path = path[1:-1] + path = RENAME_BRACE_RX.sub(r"\1", path).replace("//", "/") + if " => " in path: + path = path.rsplit(" => ", 1)[1] + return path + + +def parse_log(lines): + """Parse `git log --format='@%H|%ad|%ae' --numstat --date=unix` output + into (sha, timestamp, email, [normalized paths]) tuples.""" + commits = [] + cur = None + for line in lines: + line = line.rstrip("\n") + if line.startswith("@") and line.count("|") >= 2: + sha, ad, ae = line[1:].split("|", 2) + try: + ts = int(ad) + except ValueError: + cur = None + continue + cur = (sha, ts, ae, []) + commits.append(cur) + elif cur is not None and "\t" in line: + parts = line.split("\t", 2) + if len(parts) == 3: + cur[3].append(normalize_numstat_path(parts[2])) + return commits + + +def email_to_login(email): + """Decode a GitHub noreply email (`login@` or `digits+login@`) to its + login; None for anything else (those need the commits API). Login case is + preserved — membership matching canonicalizes case-insensitively.""" + m = NOREPLY_RX.match(email.strip()) + return m.group(1) if m else None + + +# --- scoring ----------------------------------------------------------------- + +def decay_weight(age_days, half_life_days): + return 0.5 ** (max(age_days, 0.0) / half_life_days) + + +def compute_scores(commits, rule_globs, exclude_rxs, now, half_life_days): + """Score resolved commits against the rule buckets. + + commits: iterable of (login, ts, paths) — already bot-filtered, resolved, + and membership-filtered. rule_globs: per-rule lists of compiled glob + regexes. Returns (score, touches, overall, gap): + score[i][login] decayed per-rule score + touches[i][login] raw per-rule commit touches + overall[login] whole-repo decayed score (any surviving file) + gap[topdir][login] decayed score of files matching NO rule, keyed by + the file's top-two-level directory + """ + score = [defaultdict(float) for _ in rule_globs] + touches = [defaultdict(int) for _ in rule_globs] + overall = defaultdict(float) + gap = defaultdict(lambda: defaultdict(float)) + for login, ts, paths in commits: + w = decay_weight((now - ts) / 86400.0, half_life_days) + touched = set() + any_file = False + for path in paths: + if any(rx.search(path) for rx in exclude_rxs): + continue + any_file = True + matched = False + for i, globs in enumerate(rule_globs): + if matches_any(path, globs): + touched.add(i) + matched = True + if not matched: + parts = path.split("/") + key = "/".join(parts[:2]) if len(parts) > 1 else parts[0] + gap[key][login] += w + if not any_file: + continue + overall[login] += w + for i in touched: + score[i][login] += w + touches[i][login] += 1 + return score, touches, overall, gap + + +# --- selection --------------------------------------------------------------- + +def select_for_rule(score_map, touch_map, top_k, floor, min_touches, min_score, + floor_min_touches): + """Pick a rule's reviewers. Returns (picks, under_floor, starred) where + `starred` marks floor-backfill picks (below the main thresholds). Ranked + by score desc, login asc for determinism.""" + ranked = sorted(score_map.items(), key=lambda kv: (-kv[1], kv[0])) + picks = [l for l, s in ranked + if s >= min_score and touch_map.get(l, 0) >= min_touches][:top_k] + starred = set() + if len(picks) < floor: + for l, _s in ranked: + if len(picks) >= floor: + break + if l not in picks and touch_map.get(l, 0) >= floor_min_touches: + picks.append(l) + starred.add(l) + return picks, len(picks) < floor, starred + + +def select_default_pool(overall, final_rule_lists, map_exclude, size=5, + max_anchored_rules=1): + """Top-`size` whole-repo scorers, skipping anyone already anchoring more + than `max_anchored_rules` rules (the #5448 anti-pile-on rationale) and + MAP_EXCLUDE logins. `overall` is already collaborator-only.""" + anchored = defaultdict(int) + for logins in final_rule_lists: + for l in set(logins): + anchored[l] += 1 + excluded = {x.lower() for x in map_exclude} + ranked = sorted(overall.items(), key=lambda kv: (-kv[1], kv[0])) + return [l for l, _s in ranked + if anchored[l] <= max_anchored_rules and l.lower() not in excluded][:size] + + +# --- GitHub API -------------------------------------------------------------- + +def gh_get(url, token): + """GET a GitHub API URL; None on any error (callers degrade, not crash).""" + req = urllib.request.Request(url, headers={ + "Authorization": "Bearer " + token, + "Accept": "application/vnd.github+json", + "User-Agent": "refresh-reviewers", + }) + try: + with urllib.request.urlopen(req, timeout=30) as resp: + return json.load(resp) + except (urllib.error.URLError, TimeoutError, ValueError) as e: + print(f"::warning::GET {url.split('?')[0]} failed: {e}") + return None + + +def fetch_collaborators(api, repo, token): + """All collaborator logins (paginated); None if the API is unavailable.""" + logins = set() + page = 1 + while True: + data = gh_get(f"{api}/repos/{repo}/collaborators?per_page=100&page={page}", token) + if data is None: + return None + if not isinstance(data, list): + return None + for c in data: + if isinstance(c, dict) and c.get("login"): + logins.add(c["login"]) + if len(data) < 100: + break + page += 1 + return logins + + +def resolve_email_via_api(api, repo, token, sha): + """Resolve a commit author email -> login via one commit fetch; None when + the email has no linked GitHub account (or the call fails).""" + data = gh_get(f"{api}/repos/{repo}/commits/{sha}", token) + if not isinstance(data, dict): + return None + author = data.get("author") or {} + return author.get("login") or None + + +# --- report / PR body -------------------------------------------------------- + +def fmt_login_score(login, score_map, touch_map, starred=frozenset()): + star = "\\*" if login in starred else "" + if login in score_map: + return f"{login}{star} ({score_map[login]:.1f}/{touch_map.get(login, 0)})" + return login + + +def build_pr_body(report): + """Render the drift-PR body from the report dict: per-rule before/after + with scores/touches, unresolved-email count, taxonomy gap report, knobs.""" + k = report["knobs"] + lines = [ + "## Reviewer expertise map refresh", + "", + "Recomputed `" + report["config_path"] + "` from `git log " + + report["default_branch"] + "` — recency-decayed commit touches per " + "rule bucket, collaborators only, bots and generated/churn paths " + "excluded. Entries are `login (decayed score/raw touches)`; `*` marks " + "a floor backfill below the main thresholds. This PR is the whole " + "deliverable of the refresh engine: review the proposed map, edit " + "freely, merge when it looks right.", + "", + "### Proposed map", + "", + "| rule | paths | before | after |", + "|---|---|---|---|", + ] + for r in report["rules"]: + paths = "
".join("`" + p + "`" for p in r["paths"]) or "—" + before = ", ".join(r["before"]) or "—" + if r["under_floor"]: + after = "*(unchanged — fewer than floor qualify; runtime falls " \ + "back to `default_pool` if the rule can't match)*" + else: + after = ", ".join( + fmt_login_score(l, r["scores"], r["touches"], set(r["starred"])) + for l in r["after"]) or "—" + lines.append(f"| {r['index']} | {paths} | {before} | {after} |") + dp = report["default_pool"] + lines += [ + f"| default_pool | — | {', '.join(dp['before']) or '—'} | " + + (", ".join(f"{l} ({dp['scores'].get(l, 0.0):.1f})" for l in dp["after"]) or "—") + + " |", + "", + f"Commits dropped (author email unresolvable to a login): " + f"**{report['unresolved_email_commits']}** · bot commits excluded: " + f"{report['bot_commits_excluded']}", + "", + ] + if report["gaps"]: + lines += [ + "### Taxonomy gaps (report-only)", + "", + "Hottest top-level areas matched by NO rule glob — candidates for " + "new rules (not auto-added):", + "", + "| area | decayed score | top contributors |", + "|---|---|---|", + ] + for g in report["gaps"]: + top = ", ".join(f"{t['login']} ({t['score']:.1f})" for t in g["top"]) + lines.append(f"| `{g['dir']}` | {g['score']:.1f} | {top} |") + lines.append("") + lines += [ + "### Knobs", + "", + f"`window_months={k['window_months']}` `half_life_days={k['half_life_days']}` " + f"`top_k={k['top_k']}` `floor={k['floor']}` `min_touches={k['min_touches']}` " + f"`min_score={k['min_score']}` `floor_min_touches={k['floor_min_touches']}` " + f"`map_exclude={' '.join(k['map_exclude']) or '(none)'}`", + ] + return "\n".join(lines) + "\n" + + +# --- env helpers ------------------------------------------------------------- + +def _env_int(name, default): + try: + return int(os.environ.get(name, "") or default) + except ValueError: + print(f"::warning::{name} is not an integer — using {default}") + return default + + +def _env_float(name, default): + try: + return float(os.environ.get(name, "") or default) + except ValueError: + print(f"::warning::{name} is not a number — using {default}") + return default + + +def write_outputs(outputs): + out_file = os.environ.get("GITHUB_OUTPUT") + if not out_file: + return + with open(out_file, "a", encoding="utf-8") as f: + for key, val in outputs.items(): + f.write(f"{key}={val}\n") + + +def _noop_exit(reason): + print(f"::warning::{reason} — nothing to refresh.") + write_outputs({"changed": "false"}) + return 0 + + +# --- main -------------------------------------------------------------------- + +def main(): + repo = os.environ.get("GITHUB_REPOSITORY", "") + token = os.environ.get("GH_TOKEN", "") + branch = os.environ.get("DEFAULT_BRANCH", "") + if not repo or not token or not branch: + return _noop_exit("GITHUB_REPOSITORY / GH_TOKEN / DEFAULT_BRANCH must be set") + api = os.environ.get("GITHUB_API_URL", "https://api.github.com").rstrip("/") + config_path = os.environ.get("REVIEWER_CONFIG_PATH") or ".github/reviewers.yml" + knobs = { + "window_months": _env_int("WINDOW_MONTHS", 12), + "half_life_days": _env_float("HALF_LIFE_DAYS", 90), + "top_k": _env_int("TOP_K", 4), + "floor": _env_int("FLOOR", 2), + "min_touches": _env_int("MIN_TOUCHES", 5), + "min_score": _env_float("MIN_SCORE", 1.5), + "floor_min_touches": _env_int("FLOOR_MIN_TOUCHES", 2), + "map_exclude": (os.environ.get("MAP_EXCLUDE") or "").split(), + } + results_dir = os.environ.get("RESULTS_DIR") or os.environ.get("RUNNER_TEMP") or "." + os.makedirs(results_dir, exist_ok=True) + + # exclusion regexes (built-ins + caller extras; bad extras warn, not fail) + exclude_rxs = [re.compile(rx) for rx in BUILTIN_EXCLUDE_PATHS] + for rx in (os.environ.get("EXTRA_EXCLUDE_PATHS") or "").splitlines(): + rx = rx.strip() + if not rx: + continue + try: + exclude_rxs.append(re.compile(rx)) + except re.error as e: + print(f"::warning::skipping invalid EXTRA_EXCLUDE_PATHS regex {rx!r}: {e}") + + # --- committed config (from the default branch, not the checkout ref) --- + show = subprocess.run( + ["git", "show", f"refs/remotes/origin/{branch}:{config_path}"], + capture_output=True, text=True) + if show.returncode != 0: + return _noop_exit(f"could not read {config_path} on origin/{branch}") + committed_text = show.stdout + config, locs = parse_reviewer_config(committed_text) + if not config["rules"] and not config["default_pool"]: + return _noop_exit(f"{config_path} has no rules or default_pool") + rule_globs = [[glob_to_regexp(g) for g in r.get("paths", [])] + for r in config["rules"]] + + # --- git history --------------------------------------------------------- + log = subprocess.run( + ["git", "log", f"refs/remotes/origin/{branch}", + f"--since={knobs['window_months']} months ago", "--no-merges", + "--date=unix", "--format=@%H|%ad|%ae", "--numstat"], + capture_output=True, text=True, errors="replace") + if log.returncode != 0: + return _noop_exit(f"git log failed: {log.stderr.strip()[:200]}") + raw_commits = parse_log(log.stdout.splitlines()) + if not raw_commits: + return _noop_exit(f"no commits in the last {knobs['window_months']} months") + + # --- resolve emails -> logins ------------------------------------------- + bot_commits = 0 + unresolved_commits = 0 + email_login = {} # lowercased email -> login or None + email_sha = {} # lowercased email -> a commit sha for API resolution + for sha, _ts, email, _paths in raw_commits: + e = email.strip().lower() + if BOT_EMAIL_RX.search(e) or e in email_login or e in email_sha: + continue + login = email_to_login(email) + if login is not None: + email_login[e] = login + else: + email_sha.setdefault(e, sha) + for e, sha in email_sha.items(): + email_login[e] = resolve_email_via_api(api, repo, token, sha) + + # --- membership filter (collaborators = the runtime's eligibility) ------ + collaborators = fetch_collaborators(api, repo, token) + if collaborators is None: + return _noop_exit("collaborators API unavailable — cannot validate eligibility") + # GitHub logins are case-insensitive but the noreply decode preserves the + # email's casing — canonicalize to the collaborator list's casing so a + # `123+DrJKL@` commit matches collaborator "DrJKL". + canon = {l.lower(): l for l in collaborators} + map_exclude = {x.lower() for x in knobs["map_exclude"]} + + resolved = [] + for _sha, ts, email, paths in raw_commits: + e = email.strip().lower() + if BOT_EMAIL_RX.search(e): + bot_commits += 1 + continue + login = email_login.get(e) + if login is None: + unresolved_commits += 1 + continue + if login.lower() in map_exclude: + continue + login = canon.get(login.lower()) + if login is None: + continue # not a collaborator — the runtime couldn't assign them + resolved.append((login, ts, paths)) + + now = time.time() + score, touches, overall, gap = compute_scores( + resolved, rule_globs, exclude_rxs, now, knobs["half_life_days"]) + + # --- per-rule selection -------------------------------------------------- + rule_reports = [] + replacements = {} + final_lists = [] + for i, rule in enumerate(config["rules"]): + picks, under_floor, starred = select_for_rule( + score[i], touches[i], knobs["top_k"], knobs["floor"], + knobs["min_touches"], knobs["min_score"], knobs["floor_min_touches"]) + before = list(rule.get("reviewers", [])) + if under_floor: + # cold-start fallback: leave the committed reviewers; the runtime + # already falls back to default_pool when a rule can't match. + final_lists.append(before) + after = before + else: + final_lists.append(picks) + after = picks + if picks != before: + replacements[i] = picks + rule_reports.append({ + "index": i, + "paths": rule.get("paths", []), + "before": before, + "after": after, + "changed": i in replacements, + "under_floor": under_floor, + "starred": sorted(starred), + "scores": {l: round(score[i].get(l, 0.0), 2) for l in after}, + "touches": {l: touches[i].get(l, 0) for l in after}, + }) + + # --- default pool -------------------------------------------------------- + dp_before = list(config["default_pool"]) + dp_after = select_default_pool(overall, final_lists, map_exclude) + dp_replacement = None + if locs["default_pool"] is None or not dp_after: + # no default_pool line to rewrite, or nothing scored — report the + # committed pool unchanged rather than advertising an inapplicable one + dp_after = dp_before + elif dp_after != dp_before: + dp_replacement = dp_after + + # --- taxonomy gap report ------------------------------------------------- + gaps = [] + for d, per_login in sorted(gap.items(), key=lambda kv: -sum(kv[1].values()))[:10]: + top3 = sorted(per_login.items(), key=lambda kv: (-kv[1], kv[0]))[:3] + gaps.append({ + "dir": d, + "score": round(sum(per_login.values()), 2), + "top": [{"login": l, "score": round(s, 2)} for l, s in top3], + }) + + # --- rewrite + outputs --------------------------------------------------- + new_text = rewrite_config(committed_text, locs, replacements, dp_replacement) + changed = new_text != committed_text + + report = { + "repo": repo, + "default_branch": branch, + "config_path": config_path, + "knobs": knobs, + "changed": changed, + "bot_commits_excluded": bot_commits, + "unresolved_email_commits": unresolved_commits, + "rules": rule_reports, + "default_pool": { + "before": dp_before, + "after": dp_after, + "changed": dp_replacement is not None, + "scores": {l: round(overall.get(l, 0.0), 2) for l in dp_after}, + }, + "gaps": gaps, + } + + new_config_path = os.path.join(results_dir, "reviewers.new.yml") + report_path = os.path.join(results_dir, "report.json") + pr_body_path = os.path.join(results_dir, "pr-body.md") + with open(new_config_path, "w", encoding="utf-8") as f: + f.write(new_text) + with open(report_path, "w", encoding="utf-8") as f: + json.dump(report, f, indent=2) + with open(pr_body_path, "w", encoding="utf-8") as f: + f.write(build_pr_body(report)) + write_outputs({ + "changed": "true" if changed else "false", + "new_config_path": new_config_path, + "report_path": report_path, + "pr_body_path": pr_body_path, + }) + print(f"drift={'yes' if changed else 'no'} " + f"(rules changed: {sorted(replacements)}, " + f"default_pool changed: {dp_replacement is not None}, " + f"unresolved-email commits: {unresolved_commits})") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/refresh-reviewers/tests/test_generate.py b/.github/refresh-reviewers/tests/test_generate.py new file mode 100644 index 0000000..304d971 --- /dev/null +++ b/.github/refresh-reviewers/tests/test_generate.py @@ -0,0 +1,411 @@ +#!/usr/bin/env python3 +"""Tests for the refresh-reviewers generator (BE-4116). + +Pure-python coverage of the scoring/rewrite core: glob-semantics parity with +assign-reviewers.yml's globToRegExp, decay math, threshold/floor/backfill +selection (including the under-floor leave-unchanged case), bot/generated-path/ +rename-syntax filtering, noreply-email decoding, and the surgical rewrite +preserving every byte outside the edited lists. No network, no git. + +Run: python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' -v +""" + +import importlib.util +import os +import re +import unittest + +_MODULE_PATH = os.path.join(os.path.dirname(__file__), "..", "generate.py") +_spec = importlib.util.spec_from_file_location("refresh_reviewers_generate", _MODULE_PATH) +gen = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(gen) + + +class TestGlobSemantics(unittest.TestCase): + """Parity with assign-reviewers.yml's globToRegExp: `*` within a segment, + `**` across segments, `?` one non-slash char, full-string anchored.""" + + def match(self, glob, path): + return bool(gen.glob_to_regexp(glob).match(path)) + + def test_double_star_prefix_matches_any_depth(self): + # `**/` compiles to an OPTIONAL leading-dirs group, so it also + # matches at the repo root — the property the inference bucket needs. + self.assertTrue(self.match("**/inference/**", "inference/model.go")) + self.assertTrue(self.match("**/inference/**", "services/api/inference/model.go")) + self.assertFalse(self.match("**/inference/**", "inference")) # needs a file under it + self.assertFalse(self.match("**/inference/**", "services/inference.go")) + + def test_single_dir_glob(self): + self.assertTrue(self.match("services/ingest/**", "services/ingest/api/handler.go")) + self.assertTrue(self.match("services/ingest/**", "services/ingest/main.go")) + self.assertFalse(self.match("services/ingest/**", "services/ingest")) + self.assertFalse(self.match("services/ingest/**", "services/ingestx/main.go")) + self.assertFalse(self.match("services/ingest/**", "xservices/ingest/main.go")) + + def test_single_star_stays_within_segment(self): + self.assertTrue(self.match("*.css", "site.css")) + self.assertFalse(self.match("*.css", "styles/site.css")) + self.assertTrue(self.match("docs/*.md", "docs/readme.md")) + self.assertFalse(self.match("docs/*.md", "docs/sub/readme.md")) + + def test_question_mark_single_non_slash_char(self): + self.assertTrue(self.match("v?/api.go", "v1/api.go")) + self.assertFalse(self.match("v?/api.go", "v12/api.go")) + self.assertFalse(self.match("v?/api.go", "v//api.go")) + + def test_bare_double_star_matches_everything(self): + self.assertTrue(self.match("**", "a/b/c.go")) + self.assertTrue(self.match("**", "top.go")) + + def test_regex_chars_escaped(self): + self.assertTrue(self.match("a+b/c.go", "a+b/c.go")) + self.assertFalse(self.match("a+b/c.go", "aab/c.go")) + + +class TestDecayMath(unittest.TestCase): + def test_half_life(self): + self.assertAlmostEqual(gen.decay_weight(0, 90), 1.0) + self.assertAlmostEqual(gen.decay_weight(90, 90), 0.5) + self.assertAlmostEqual(gen.decay_weight(180, 90), 0.25) + self.assertAlmostEqual(gen.decay_weight(45, 90), 0.5 ** 0.5) + + def test_future_commit_clamped_to_full_weight(self): + self.assertAlmostEqual(gen.decay_weight(-5, 90), 1.0) + + +class TestEmailResolution(unittest.TestCase): + def test_plain_noreply_decodes(self): + self.assertEqual(gen.email_to_login("octocat@users.noreply.github.com"), "octocat") + + def test_digits_plus_login_form_decodes(self): + self.assertEqual(gen.email_to_login("583231+octocat@users.noreply.github.com"), "octocat") + + def test_decode_preserves_login_case(self): + # collaborator matching canonicalizes case-insensitively, so the + # decode must not lowercase mixed-case logins like DrJKL away. + self.assertEqual(gen.email_to_login("66172478+DrJKL@users.noreply.github.com"), "DrJKL") + self.assertEqual(gen.email_to_login("DrJKL@Users.Noreply.GitHub.com"), "DrJKL") + + def test_other_emails_do_not_decode(self): + self.assertIsNone(gen.email_to_login("dev@example.com")) + self.assertIsNone(gen.email_to_login("someone@users.noreply.github.com.evil.com")) + + def test_bot_emails_match_exclusion(self): + self.assertTrue(gen.BOT_EMAIL_RX.search("49699333+dependabot[bot]@users.noreply.github.com")) + self.assertTrue(gen.BOT_EMAIL_RX.search("noreply@argoproj.io")) + self.assertFalse(gen.BOT_EMAIL_RX.search("dev@example.com")) + + +class TestPathFiltering(unittest.TestCase): + def setUp(self): + self.rxs = [re.compile(rx) for rx in gen.BUILTIN_EXCLUDE_PATHS] + + def excluded(self, path): + return any(rx.search(path) for rx in self.rxs) + + def test_generated_paths_excluded(self): + for p in [ + "ent/user.go", + "services/api/ent/user_query.go", + "api/types.gen.go", + "proto/svc.pb.go", + "vendor/golang.org/x/net/http2.go", + "go.sum", + "services/api/go.sum", + "go.work.sum", + "go.work.prod.sum", + "package-lock.json", + "web/pnpm-lock.yaml", + "web/yarn.lock", + "Cargo.lock", + "infrastructure/dynamicconfig/staging/config.json", + "frontend-version.json", + ]: + self.assertTrue(self.excluded(p), p) + + def test_hand_written_paths_survive(self): + for p in [ + "ent/schema/user.go", # hand-written ent schema + "services/api/ent/schema/x.go", + "services/api/handler.go", + "docs/lockfiles.md", + "infrastructure/terraform/main.tf", + ]: + self.assertFalse(self.excluded(p), p) + + def test_rename_brace_syntax_uses_new_path(self): + self.assertEqual( + gen.normalize_numstat_path("services/{ingest => intake}/api.go"), + "services/intake/api.go") + self.assertEqual( + gen.normalize_numstat_path("services/{ => new}/api.go"), + "services/new/api.go") + self.assertEqual( + gen.normalize_numstat_path("services/{old => }/api.go"), + "services/api.go") + + def test_whole_path_rename_uses_new_path(self): + self.assertEqual( + gen.normalize_numstat_path("old.go => pkg/new.go"), "pkg/new.go") + + def test_plain_path_untouched(self): + self.assertEqual(gen.normalize_numstat_path("a/b/c.go"), "a/b/c.go") + + +class TestParseLog(unittest.TestCase): + LOG = "\n".join([ + "@aaa1|1700000000|dev@example.com", + "10\t2\tservices/ingest/api.go", + "5\t0\tgo.sum", + "", + "@bbb2|1700086400|49699333+dependabot[bot]@users.noreply.github.com", + "1\t1\tpackage-lock.json", + "", + "@ccc3|1700172800|583231+octocat@users.noreply.github.com", + "-\t-\tassets/logo.png", + "3\t3\tservices/{ingest => intake}/handler.go", + ]) + + def test_parse_shape(self): + commits = gen.parse_log(self.LOG.splitlines()) + self.assertEqual(len(commits), 3) + sha, ts, email, paths = commits[0] + self.assertEqual((sha, ts, email), ("aaa1", 1700000000, "dev@example.com")) + self.assertEqual(paths, ["services/ingest/api.go", "go.sum"]) + # binary numstat (- -) and rename lines both parse + self.assertEqual(commits[2][3], ["assets/logo.png", "services/intake/handler.go"]) + + +class TestSelection(unittest.TestCase): + KNOBS = dict(top_k=4, floor=2, min_touches=5, min_score=1.5, floor_min_touches=2) + + def test_threshold_and_cap(self): + score = {"a": 9.0, "b": 7.0, "c": 6.0, "d": 5.0, "e": 4.0} + touches = {l: 10 for l in score} + picks, under, starred = gen.select_for_rule(score, touches, **self.KNOBS) + self.assertEqual(picks, ["a", "b", "c", "d"]) # top_k caps at 4 + self.assertFalse(under) + self.assertEqual(starred, set()) + + def test_min_touches_disqualifies_high_score(self): + # one huge recent commit != sustained expertise + score = {"drive-by": 9.0, "a": 5.0, "b": 4.0} + touches = {"drive-by": 1, "a": 10, "b": 10} + picks, under, _ = gen.select_for_rule(score, touches, **self.KNOBS) + self.assertEqual(picks, ["a", "b"]) + self.assertFalse(under) + + def test_floor_backfill_from_ranked_remainder(self): + # only one qualifier -> backfill the best remainder with touches >= 2 + score = {"a": 5.0, "b": 1.0, "c": 0.8} + touches = {"a": 10, "b": 3, "c": 4} + picks, under, starred = gen.select_for_rule(score, touches, **self.KNOBS) + self.assertEqual(picks, ["a", "b"]) + self.assertFalse(under) + self.assertEqual(starred, {"b"}) + + def test_backfill_skips_below_floor_min_touches(self): + score = {"a": 5.0, "b": 1.0, "c": 0.8} + touches = {"a": 10, "b": 1, "c": 4} # b under floor_min_touches + picks, under, starred = gen.select_for_rule(score, touches, **self.KNOBS) + self.assertEqual(picks, ["a", "c"]) + self.assertEqual(starred, {"c"}) + + def test_under_floor_reports_unchanged_case(self): + # nobody backfillable -> under_floor True (caller leaves committed list) + score = {"a": 5.0, "b": 0.5} + touches = {"a": 10, "b": 1} + picks, under, _ = gen.select_for_rule(score, touches, **self.KNOBS) + self.assertEqual(picks, ["a"]) + self.assertTrue(under) + + def test_deterministic_tiebreak_by_login(self): + score = {"zed": 3.0, "amy": 3.0} + touches = {"zed": 9, "amy": 9} + picks, _, _ = gen.select_for_rule(score, touches, **self.KNOBS) + self.assertEqual(picks, ["amy", "zed"]) + + def test_default_pool_skips_heavy_anchors_and_excludes(self): + overall = {"a": 50.0, "b": 40.0, "c": 30.0, "d": 20.0, "e": 10.0, + "f": 5.0, "g": 1.0} + final_lists = [["a", "b"], ["a", "c"], ["a"]] # a anchors 3, b/c 1 each + pool = gen.select_default_pool(overall, final_lists, {"d"}) + self.assertEqual(pool, ["b", "c", "e", "f", "g"]) # no a (>=2 rules), no d + + +class TestComputeScores(unittest.TestCase): + def test_bucket_touch_and_overall(self): + rules = [[gen.glob_to_regexp("services/ingest/**")], + [gen.glob_to_regexp("**/inference/**")]] + exclude = [re.compile(rx) for rx in gen.BUILTIN_EXCLUDE_PATHS] + now = 1_800_000_000 + day = 86400 + commits = [ + # fresh commit touching ingest twice (one file excluded) + ("alice", now, ["services/ingest/a.go", "go.sum"]), + # 90-day-old commit touching both buckets + ("alice", now - 90 * day, ["services/ingest/b.go", "api/inference/m.go"]), + # commit whose files are ALL excluded — contributes nothing + ("bob", now, ["vendor/x.go", "package-lock.json"]), + # unmatched path — lands in the gap report, not a bucket + ("carol", now, ["docs/guide.md"]), + ] + score, touches, overall, gap = gen.compute_scores(commits, rules, exclude, now, 90) + self.assertAlmostEqual(score[0]["alice"], 1.5) # 1.0 + 0.5 + self.assertEqual(touches[0]["alice"], 2) + self.assertAlmostEqual(score[1]["alice"], 0.5) + self.assertEqual(touches[1]["alice"], 1) + self.assertNotIn("bob", overall) # all-excluded commit + self.assertAlmostEqual(overall["alice"], 1.5) + self.assertAlmostEqual(gap["docs/guide.md"]["carol"], 1.0) + self.assertNotIn("services/ingest", gap) # covered by a rule + + +CONFIG = """\ +# Reviewer expertise map — hand-tuned, comments are documentation. +# default_pool is the fallback when no rule matches. +default_pool: [old-a, old-b] # keep small + +rules: + # Ingest service — the API front door. + - paths: ["services/ingest/**"] + reviewers: [old-a, old-c] # ingest folk + # Inference — anywhere in the tree. + - paths: + - "**/inference/**" + reviewers: + - old-d + - old-e + # Cold-start rule — nobody active enough; must stay untouched. + - paths: ["services/quiet/**"] + reviewers: [old-f] # keep: cold-start +""" + + +class TestSurgicalRewrite(unittest.TestCase): + def test_parse_shapes(self): + config, locs = gen.parse_reviewer_config(CONFIG) + self.assertEqual(config["default_pool"], ["old-a", "old-b"]) + self.assertEqual([r["reviewers"] for r in config["rules"]], + [["old-a", "old-c"], ["old-d", "old-e"], ["old-f"]]) + self.assertEqual([r["paths"] for r in config["rules"]], + [["services/ingest/**"], ["**/inference/**"], ["services/quiet/**"]]) + self.assertEqual(locs["default_pool"][0], "flow") + self.assertEqual(locs["rules"][0][0], "flow") + self.assertEqual(locs["rules"][1][0], "block") + + def test_flow_rewrite_preserves_comments(self): + config, locs = gen.parse_reviewer_config(CONFIG) + out = gen.rewrite_config(CONFIG, locs, {0: ["new-x", "new-y"]}, None) + self.assertIn("reviewers: [new-x, new-y] # ingest folk\n", out) + # rule 1 (block) and rule 2 byte-identical; header comments intact + self.assertIn(" - old-d\n - old-e\n", out) + self.assertIn("reviewers: [old-f] # keep: cold-start", out) + self.assertIn("# Reviewer expertise map — hand-tuned", out) + + def test_block_rewrite_replaces_items_at_same_indent(self): + config, locs = gen.parse_reviewer_config(CONFIG) + out = gen.rewrite_config(CONFIG, locs, {1: ["new-p", "new-q", "new-r"]}, None) + self.assertIn(" reviewers:\n - new-p\n - new-q\n - new-r\n", out) + self.assertNotIn("old-d", out) + # untouched lists keep their bytes + self.assertIn("reviewers: [old-a, old-c] # ingest folk", out) + + def test_default_pool_rewrite(self): + config, locs = gen.parse_reviewer_config(CONFIG) + out = gen.rewrite_config(CONFIG, locs, {}, ["pool-1", "pool-2"]) + self.assertIn("default_pool: [pool-1, pool-2] # keep small\n", out) + + def test_everything_outside_edited_lists_is_byte_identical(self): + config, locs = gen.parse_reviewer_config(CONFIG) + out = gen.rewrite_config(CONFIG, locs, {0: ["n1", "n2"]}, ["p1"]) + orig_lines = CONFIG.split("\n") + new_lines = out.split("\n") + self.assertEqual(len(orig_lines), len(new_lines)) + edited = {2, 7} # default_pool line, rule-0 reviewers line + for i, (a, b) in enumerate(zip(orig_lines, new_lines)): + if i in edited: + self.assertNotEqual(a, b, f"line {i} should have changed") + else: + self.assertEqual(a, b, f"line {i} changed unexpectedly") + + def test_noop_rewrite_is_byte_identical(self): + config, locs = gen.parse_reviewer_config(CONFIG) + self.assertEqual(gen.rewrite_config(CONFIG, locs, {}, None), CONFIG) + + def test_block_default_pool(self): + cfg = ("default_pool:\n" + " - old-a # anchor\n" + " - old-b\n" + "rules:\n" + " - paths: [\"x/**\"]\n" + " reviewers: [r1]\n") + config, locs = gen.parse_reviewer_config(cfg) + self.assertEqual(config["default_pool"], ["old-a", "old-b"]) + out = gen.rewrite_config(cfg, locs, {}, ["new-a"]) + self.assertIn("default_pool:\n - new-a\nrules:\n", out) + self.assertNotIn("old-a", out) + + def test_scalar_reviewers_becomes_flow(self): + cfg = ("rules:\n" + " - paths: [\"x/**\"]\n" + " reviewers: solo # single owner\n") + config, locs = gen.parse_reviewer_config(cfg) + self.assertEqual(config["rules"][0]["reviewers"], ["solo"]) + out = gen.rewrite_config(cfg, locs, {0: ["a", "b"]}, None) + self.assertIn(" reviewers: [a, b] # single owner\n", out) + + def test_bracket_inside_comment_is_not_the_flow(self): + # a `[` in the trailing comment must never be mistaken for the list + cfg = ("rules:\n" + " - paths: [\"x/**\"]\n" + " reviewers: solo # [see docs]\n") + config, locs = gen.parse_reviewer_config(cfg) + out = gen.rewrite_config(cfg, locs, {0: ["a"]}, None) + self.assertIn(" reviewers: [a] # [see docs]\n", out) + + def test_default_pool_exclude_is_case_insensitive(self): + pool = gen.select_default_pool({"DrJKL": 9.0, "b": 5.0}, [], {"drjkl"}) + self.assertEqual(pool, ["b"]) + + +class TestPrBody(unittest.TestCase): + def test_body_carries_the_contract_pieces(self): + report = { + "repo": "o/r", "default_branch": "main", + "config_path": ".github/reviewers.yml", + "knobs": {"window_months": 12, "half_life_days": 90, "top_k": 4, + "floor": 2, "min_touches": 5, "min_score": 1.5, + "floor_min_touches": 2, "map_exclude": ["op-login"]}, + "changed": True, + "bot_commits_excluded": 7, + "unresolved_email_commits": 3, + "rules": [ + {"index": 0, "paths": ["services/ingest/**"], + "before": ["old-a"], "after": ["new-a", "new-b"], + "changed": True, "under_floor": False, "starred": ["new-b"], + "scores": {"new-a": 9.1, "new-b": 1.2}, + "touches": {"new-a": 20, "new-b": 3}}, + {"index": 1, "paths": ["services/quiet/**"], + "before": ["old-f"], "after": ["old-f"], + "changed": False, "under_floor": True, "starred": [], + "scores": {}, "touches": {}}, + ], + "default_pool": {"before": ["old-a"], "after": ["new-a"], + "changed": True, "scores": {"new-a": 30.0}}, + "gaps": [{"dir": "docs/site", "score": 12.5, + "top": [{"login": "carol", "score": 8.0}]}], + } + body = gen.build_pr_body(report) + self.assertIn("new-a (9.1/20)", body) # score/touch table + self.assertIn("new-b\\* (1.2/3)", body) # starred backfill + self.assertIn("unchanged — fewer than floor qualify", body) + self.assertIn("**3**", body) # unresolved-email count + self.assertIn("docs/site", body) # gap report + self.assertIn("window_months=12", body) # knob values + self.assertIn("map_exclude=op-login", body) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/refresh-reviewers.yml b/.github/workflows/refresh-reviewers.yml new file mode 100644 index 0000000..ba0420c --- /dev/null +++ b/.github/workflows/refresh-reviewers.yml @@ -0,0 +1,259 @@ +name: Refresh Reviewers (reusable) + +# Recomputes the caller repo's reviewer expertise map (`.github/reviewers.yml`, +# the config assign-reviewers.yml consumes) from git history and opens a drift +# PR when the committed map has fallen behind reality. This engine is a drift +# DETECTOR, never a live mutator: nothing is assigned, nothing merges — the +# whole deliverable is one reviewable single-file PR a human accepts or edits. +# +# HOW IT SCORES (validated in the BE-4114 spike against cloud history): +# recency-decayed commit touches per rule bucket — per commit, per rule glob +# matched by >=1 surviving changed file, score += 0.5^(age_days/half_life_days) +# and touches += 1. Line counts are intentionally unused. Bot authors, +# generated/churn paths, and non-collaborators are excluded (collaborators — +# not org members — because `addAssignees` silently drops non-collaborators, +# so that is the exact eligibility test the runtime applies). The generator +# rewrites ONLY the `reviewers: [...]` / `default_pool: [...]` lists, +# preserving every comment: the config's comments are its documentation. +# +# WHY AN APP TOKEN: the drift PR is created with the CLOUD_CODE_BOT app token +# (vars.APP_ID + secrets.CLOUD_CODE_BOT_PRIVATE_KEY) — the branch push needs +# `contents: write` and the PR open/edit needs `pull-requests: write`, both of +# which the app carries; the workflow's own `permissions:` block stays +# `contents: read` because every mutation goes through the app token (same +# pattern as assign-reviewers.yml). +# +# IDEMPOTENT: re-runs force-reset the same `pr_branch` from the default branch +# and edit the one open drift PR in place — duplicate PRs never stack. When +# the recomputed map equals the committed one, the run exits quietly. +# +# The generator script is loaded from THIS repo (public, pinned via +# `workflows_ref`) — never from the caller's checkout — so a caller-side +# change can't rewrite the logic scoring it. +# +# Example caller (consumer repo), e.g. on a weekly schedule: +# +# name: CI - Refresh Reviewers +# on: +# schedule: +# - cron: '0 6 * * 1' +# workflow_dispatch: +# jobs: +# refresh: +# permissions: +# contents: read +# uses: Comfy-Org/github-workflows/.github/workflows/refresh-reviewers.yml@ # v1 +# with: +# workflows_ref: # pin the generator to the same ref as `uses:` +# map_exclude: some-operator-login +# secrets: +# CLOUD_CODE_BOT_PRIVATE_KEY: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} +# +# Caller-side configuration: +# vars.APP_ID - CLOUD_CODE_BOT app id (same as assign-reviewers.yml) +# +# NOTE: `map_exclude` (who may appear in the COMMITTED map) is distinct from +# the runtime `vars.REVIEWER_EXCLUDE` (who assign-reviewers skips at PR time). +# Seed map_exclude with operator logins whose commits are largely +# agent-authored — their commit volume is not personal expertise. + +on: + workflow_call: + inputs: + reviewer_config_path: + description: >- + Path in the caller repo to the expertise/path-glob reviewer config + (same meaning as in assign-reviewers.yml). + type: string + required: false + default: .github/reviewers.yml + window_months: + description: How many months of git history to score. + type: number + required: false + default: 12 + half_life_days: + description: Decay half-life in days for commit recency weighting. + type: number + required: false + default: 90 + top_k: + description: Max experts per rule. + type: number + required: false + default: 4 + floor: + description: >- + Min experts per rule; below-threshold candidates backfill up to this, + and a rule that still can't reach it is left unchanged. + type: number + required: false + default: 2 + min_touches: + description: Raw commit-touches needed to qualify for a rule. + type: number + required: false + default: 5 + min_score: + description: Decayed score needed to qualify for a rule. + type: number + required: false + default: 1.5 + floor_min_touches: + description: Relaxed touch threshold used only for floor backfill. + type: number + required: false + default: 2 + map_exclude: + description: >- + Whitespace-separated logins never to place in the map (distinct from + the runtime vars.REVIEWER_EXCLUDE — e.g. an operator login whose + commits are largely agent-authored). + type: string + required: false + default: '' + extra_exclude_paths: + description: >- + Newline-separated regexes appended to the built-in generated/churn + path exclusion list. + type: string + required: false + default: '' + pr_branch: + description: Head branch for the drift PR (reset + reused across runs). + type: string + required: false + default: bot/refresh-reviewers + workflows_ref: + description: >- + Ref of Comfy-Org/github-workflows to load the generator script from. + Pin this to the same ref you pin `uses:` to for reproducibility. + type: string + required: false + default: main + secrets: + CLOUD_CODE_BOT_PRIVATE_KEY: + description: >- + Private key for the CLOUD_CODE_BOT GitHub App (app id = vars.APP_ID). + Required: the drift-PR branch push and PR open/edit are made by the + app (contents: write + pull-requests: write), so the workflow's own + token stays read-only. + required: true + +permissions: + contents: read + +jobs: + refresh: + name: Recompute reviewer map + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: Generate GitHub App token + id: app-token + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} + + - name: Checkout caller repo (full history) + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + # Full history IS the input signal for the expertise scoring. + fetch-depth: 0 + # The app token is persisted so the drift-PR push below can use + # plain `git push origin` (the default GITHUB_TOKEN stays unused). + token: ${{ steps.app-token.outputs.token }} + persist-credentials: true + + - name: Load generator script + # The generator comes from THIS repo (public, pinned via + # workflows_ref), never from the caller's checkout, so a caller-side + # change can't rewrite the scoring logic. + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + repository: Comfy-Org/github-workflows + ref: ${{ inputs.workflows_ref }} + path: _refresh_reviewers + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6 + with: + python-version: '3.12' + + - name: Resolve default branch + id: default-branch + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + EVENT_DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + run: | + BRANCH="$EVENT_DEFAULT_BRANCH" + if [ -z "$BRANCH" ]; then + BRANCH="$(gh api "repos/$GITHUB_REPOSITORY" --jq .default_branch)" + fi + echo "branch=$BRANCH" >> "$GITHUB_OUTPUT" + + - name: Recompute reviewer map + id: generate + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + DEFAULT_BRANCH: ${{ steps.default-branch.outputs.branch }} + REVIEWER_CONFIG_PATH: ${{ inputs.reviewer_config_path }} + WINDOW_MONTHS: ${{ inputs.window_months }} + HALF_LIFE_DAYS: ${{ inputs.half_life_days }} + TOP_K: ${{ inputs.top_k }} + FLOOR: ${{ inputs.floor }} + MIN_TOUCHES: ${{ inputs.min_touches }} + MIN_SCORE: ${{ inputs.min_score }} + FLOOR_MIN_TOUCHES: ${{ inputs.floor_min_touches }} + MAP_EXCLUDE: ${{ inputs.map_exclude }} + EXTRA_EXCLUDE_PATHS: ${{ inputs.extra_exclude_paths }} + RESULTS_DIR: ${{ runner.temp }}/refresh-reviewers + run: | + python3 "$GITHUB_WORKSPACE/_refresh_reviewers/.github/refresh-reviewers/generate.py" + if [ -f "$RESULTS_DIR/pr-body.md" ]; then + cat "$RESULTS_DIR/pr-body.md" >> "$GITHUB_STEP_SUMMARY" + fi + + - name: Open or update the drift PR + if: steps.generate.outputs.changed == 'true' + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + APP_SLUG: ${{ steps.app-token.outputs.app-slug }} + DEFAULT_BRANCH: ${{ steps.default-branch.outputs.branch }} + PR_BRANCH: ${{ inputs.pr_branch }} + CONFIG_PATH: ${{ inputs.reviewer_config_path }} + NEW_CONFIG: ${{ steps.generate.outputs.new_config_path }} + PR_BODY: ${{ steps.generate.outputs.pr_body_path }} + run: | + set -euo pipefail + # Commit as the app's bot identity — the `[bot]@` email also keeps + # these refresh commits out of future expertise scoring. + git config user.name "${APP_SLUG}[bot]" + git config user.email "${APP_SLUG}[bot]@users.noreply.github.com" + + # Reset the (stable) drift branch from the default branch, land the + # single-file change, and force-push — re-runs update in place, so + # duplicate PRs never stack. + git checkout -B "$PR_BRANCH" "refs/remotes/origin/$DEFAULT_BRANCH" + cp "$NEW_CONFIG" "$CONFIG_PATH" + git add -- "$CONFIG_PATH" + if git diff --cached --quiet; then + echo "Recomputed map matches origin/$DEFAULT_BRANCH — nothing to open." + exit 0 + fi + git commit -m "chore: refresh reviewer expertise map from git history" + git push --force origin "HEAD:refs/heads/$PR_BRANCH" + + EXISTING="$(gh pr list --head "$PR_BRANCH" --base "$DEFAULT_BRANCH" \ + --state open --json number --jq '.[0].number // empty')" + if [ -n "$EXISTING" ]; then + gh pr edit "$EXISTING" --body-file "$PR_BODY" + echo "Updated existing drift PR #$EXISTING." + else + gh pr create --head "$PR_BRANCH" --base "$DEFAULT_BRANCH" \ + --title "chore: refresh reviewer expertise map" \ + --body-file "$PR_BODY" + fi diff --git a/.github/workflows/test-refresh-reviewers.yml b/.github/workflows/test-refresh-reviewers.yml new file mode 100644 index 0000000..4ddece9 --- /dev/null +++ b/.github/workflows/test-refresh-reviewers.yml @@ -0,0 +1,39 @@ +name: Test refresh-reviewers generator + +# Runs the Python unit tests for the refresh-reviewers generator (glob parity +# with assign-reviewers.yml, decay/selection math, the surgical rewrite). The +# generator proposes every consumer repo's reviewer map, so a scoring or +# rewrite regression silently corrupts routing everywhere — cheap to guard +# with a unit run on change. + +on: + pull_request: + paths: + - '.github/refresh-reviewers/**' + - '.github/workflows/test-refresh-reviewers.yml' + push: + branches: [main] + paths: + - '.github/refresh-reviewers/**' + - '.github/workflows/test-refresh-reviewers.yml' + +permissions: + contents: read + +jobs: + test: + name: unittest + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v6 + with: + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: '3.12' + + - name: Run refresh-reviewers generator tests + run: python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' -v diff --git a/AGENTS.md b/AGENTS.md index 134ae61..2cd5473 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,6 +23,9 @@ python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py' # groom dedup/rejection ledger tests python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v +# refresh-reviewers generator tests +python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' -v + # bump-callers shell tests + lint (gh is stubbed; no network) shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh bash .github/bump-callers/tests/test_bump_callers.sh @@ -54,6 +57,10 @@ tests — run the matching command above for whatever you touched. — and (BE-4003) recognizes auto-builder PR state (open/merged/closed) so a built finding is never re-proposed. It uses GitHub issue+PR state as the store — no new secret. Tests in `tests/`. +- `.github/refresh-reviewers/` — `generate.py`, the engine behind + `refresh-reviewers.yml`: recomputes a caller's reviewers.yml from git history + (decayed commit touches, assigner-parity globs, collaborator-only) and + surgically rewrites just the reviewer lists for a drift PR. Tests in `tests/`. - `.github/bump-callers/` — `bump-callers.sh`, the ONE fleet-agnostic script that opens SHA-bump PRs in consumer repos when a reusable workflow changes. Tests in `tests/`. @@ -76,6 +83,9 @@ tests — run the matching command above for whatever you touched. auto-merged) via a credential-free patch job + a separate bot PR job. - `agents-md-integrity.yml` — enforces the AGENTS.md standard on the caller repo. - `assign-reviewers.yml` — expertise-aware, load-balanced reviewer requests. +- `refresh-reviewers.yml` — scheduled drift-detector: recomputes the caller's + reviewers.yml from git history and opens one idempotent drift PR (never a + live mutator). - `assign-prs-to-author.yml` — assigns unassigned open PRs to their author. - `detect-unreviewed-merge.yml` — SOC 2: flags PRs merged without approval. - `bump-cursor-review-callers.yml` / `bump-agents-md-callers.yml` / diff --git a/README.md b/README.md index a752ace..e298d8b 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,7 @@ This repo is **public** so any repo — public or private, inside or outside the | [`cursor-review.yml`](.github/workflows/cursor-review.yml) | Label-triggered multi-model code review. A 4-lab × 2-review-type cursor-agent panel runs adversarial + edge-case passes, a judge model consolidates them into one PR review with per-finding severity badges, and the triggerer gets Slack start/complete DMs. Advisory by default; opt in with `blocking: true` to fail a (required-status-check) gate while findings stay unresolved. Prompts and scripts live in [`.github/cursor-review/`](.github/cursor-review) — the single source of truth, so consumer repos carry only a thin caller. Self-hostable via `runs_on` (JSON, default `ubuntu-latest`) and panel models overridable via `models` (JSON array) for accounts lacking a default provider. Requires `CURSOR_API_KEY` (+ optional `SLACK_BOT_TOKEN`). | | [`cursor-review-auto-label.yml`](.github/workflows/cursor-review-auto-label.yml) | Companion to `cursor-review.yml`. On PR assignment, applies the review label for an opted-in reviewer (via the CLOUD_CODE_BOT app token, so the label actually triggers the review). The opt-in roster lives in the caller's `vars.CURSOR_REVIEW_OPTED_IN_LOGINS` — no roster is baked into the workflow. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. | | [`assign-reviewers.yml`](.github/workflows/assign-reviewers.yml) | Auto-requests expertise-aware, load-balanced PR reviewers with new-folk randomization. Matches changed paths against a caller-repo `.github/reviewers.yml` (path-glob → reviewers, plus a `default_pool`), drops the author + `vars.REVIEWER_EXCLUDE`, ranks candidates by open review load (steering off anyone at/over `vars.REVIEWER_LOAD_CAP`), and may swap a slot for a `vars.REVIEWER_GROWTH_POOL` member. Requests go through the CLOUD_CODE_BOT app token so they work on fork PRs. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. | +| [`refresh-reviewers.yml`](.github/workflows/refresh-reviewers.yml) | Companion to `assign-reviewers.yml` — a scheduled drift-detector that recomputes the caller's `.github/reviewers.yml` from git history (recency-decayed commit touches per rule bucket, same glob semantics as the assigner, collaborators only, bots and generated/churn paths excluded) and opens ONE idempotent single-file PR when the committed map drifts. The rewrite is surgical (only the `reviewers:`/`default_pool:` lists change — comments preserved), the PR body carries per-rule before/after scores plus a report-only taxonomy-gap section, and a rule with too few qualifiers is left unchanged. Never a live mutator. Engine + knob rationale in [`.github/refresh-reviewers/`](.github/refresh-reviewers). Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. | | [`assign-prs-to-author.yml`](.github/workflows/assign-prs-to-author.yml) | Housekeeping — assigns every open PR with no assignees to its author (bot-authored PRs skipped by default). Run on a schedule from a thin caller; useful when a team tracks PR ownership via assignees. The calling job needs `pull-requests: write` and `issues: write`. | | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | From 0b3c716d1787744513c8e82e6620c2b312f8f50f Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 22 Jul 2026 16:41:46 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(refresh-reviewers):=20harden=20per=20re?= =?UTF-8?q?view=20panel=20=E2=80=94=20knob=20validation,=20API-failure=20n?= =?UTF-8?q?o-op,=20branch=20guards,=20stale-PR=20reconcile,=20md=20escapin?= =?UTF-8?q?g=20(BE-4116)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the cursor-review panel findings on #63: - validate HALF_LIFE_DAYS strictly positive (zero divided by zero, negative inverted the decay) — falls back to the default with a warning - distinguish API failure from "no linked account" via an API_ERROR sentinel; any failed email->login lookup downgrades the run to the documented clean no-op instead of silently dropping contributors - workflow: refuse pr_branch == default branch, add a per-repo concurrency group, and never force-reset an open drift PR whose tip a human authored - workflow: a genuine no-drift run (report present) closes a stale still-bot-authored drift PR so an obsolete proposal can't linger mergeable - workflows_ref is now required (defaulting to mutable main defeated the pin-by-SHA rule); no consumers exist yet so nothing breaks - gap report: accumulate once per commit per key (parity with rule scores) and key by top-two-level DIRECTORY, never the filename - decode git's C-style path quoting properly (tabs, quotes, octal) - escape | and backticks in repo-derived text in the PR-body tables - document the accepted trust boundary (forgeable author metadata, bounded by human review) and the caller-trusted EXTRA_EXCLUDE_PATHS regexes Co-Authored-By: Claude Opus 4.8 --- .github/refresh-reviewers/README.md | 8 +- .github/refresh-reviewers/generate.py | 122 +++++++++++++++--- .../refresh-reviewers/tests/test_generate.py | 82 +++++++++++- .github/workflows/refresh-reviewers.yml | 85 ++++++++++-- README.md | 2 +- 5 files changed, 264 insertions(+), 35 deletions(-) diff --git a/.github/refresh-reviewers/README.md b/.github/refresh-reviewers/README.md index 629c248..4b78198 100644 --- a/.github/refresh-reviewers/README.md +++ b/.github/refresh-reviewers/README.md @@ -34,8 +34,10 @@ touches[rule][login] += 1 - **Emails → logins:** `login@users.noreply.github.com` (and the `digits+login@` form) decode directly; every other distinct email costs one `GET /repos/{owner}/{repo}/commits/{sha}` to read `.author.login`. Commits - whose email can't be resolved are dropped and the count is reported in the - PR body. + whose email has **no linked account** are dropped and the count is reported + in the PR body — but an API **failure** (rate limit, 5xx, timeout) is never + scored as "no account": any failed lookup downgrades the whole run to the + clean no-op, so a transient outage can't emit a biased map. - **Eligibility = repo collaborators** (paginated `GET .../collaborators`) — *not* org members, because `addAssignees` silently drops non-collaborators, so the collaborator set is the exact test the runtime applies. @@ -70,7 +72,7 @@ with their top contributors — candidates for new rules, never auto-added. | `top_k` / `floor` | 4 / 2 | Enough experts to load-balance across without piling every rule onto the same two people. | | `min_touches` / `min_score` | 5 / 1.5 | Filters drive-by contributors: one huge recent commit is not sustained expertise. | | `floor_min_touches` | 2 | Relaxed bar used only to reach the floor. | -| `pr_branch` | `bot/refresh-reviewers` | Stable branch, force-reset each run — re-runs update the one open drift PR instead of stacking duplicates. | +| `pr_branch` | `bot/refresh-reviewers` | Stable branch, force-reset each run — re-runs update the one open drift PR instead of stacking duplicates. Guarded: it may never equal the default branch, a tip a human has pushed to is never force-reset ("edit freely" means it), and a no-drift run closes a stale still-bot-authored PR. | ## `map_exclude` guidance diff --git a/.github/refresh-reviewers/generate.py b/.github/refresh-reviewers/generate.py index 0a57933..c54f884 100644 --- a/.github/refresh-reviewers/generate.py +++ b/.github/refresh-reviewers/generate.py @@ -25,6 +25,14 @@ runtime applies: `addAssignees` silently drops non-collaborators), - MAP_EXCLUDE logins (e.g. an operator whose commits are agent-authored). +Trust model: scoring reads unauthenticated git author metadata. Anyone with +push access to the default branch can forge another user's noreply email (the +commits API attributes by email too, so API resolution inherits the same +limit) or a future author date (clamped to full weight). That forger is by +definition already a collaborator, and the only output is a drift PR a human +reviews — so the exposure is accepted rather than "fixed" with signature +checks the underlying data can't support. + This script is a drift DETECTOR, never a live mutator: it emits the rewritten config + a machine-readable report for the workflow's PR step and exits. Environmental problems (missing config, unreachable API) are downgraded to a @@ -338,12 +346,30 @@ def rewrite_config(text, locs, rule_replacements, default_pool_replacement): # --- git log parsing --------------------------------------------------------- +def _git_unquote(path): + """Decode git's C-style path quoting (core.quotePath: `\\t`, `\\"`, + `\\\\`, `\\ooo` octal for non-ASCII bytes). Quoted output is ASCII, so + decode escapes then reassemble the raw bytes as UTF-8; malformed input + falls back to the bare inner string.""" + if not (len(path) >= 2 and path[0] == '"' and path[-1] == '"'): + return path + inner = path[1:-1] + try: + return (inner.encode("ascii", "backslashreplace") + .decode("unicode_escape") + .encode("latin-1").decode("utf-8", "replace")) + except (UnicodeDecodeError, UnicodeEncodeError): + return inner + + def normalize_numstat_path(path): - """Normalize a numstat path: strip git's quoting, resolve both rename + """Normalize a numstat path: decode git's quoting, resolve both rename syntaxes (`a/{old => new}/c` and whole-path `old => new`) to the NEW - path.""" - if len(path) >= 2 and path[0] == '"' and path[-1] == '"': - path = path[1:-1] + path. A filename legitimately containing ` => ` is indistinguishable from + the whole-path rename form in numstat's line output — accepted ambiguity + (vanishingly rare, and the cost is one file scored against the wrong + bucket).""" + path = _git_unquote(path) path = RENAME_BRACE_RX.sub(r"\1", path).replace("//", "/") if " => " in path: path = path.rsplit(" => ", 1)[1] @@ -396,8 +422,11 @@ def compute_scores(commits, rule_globs, exclude_rxs, now, half_life_days): score[i][login] decayed per-rule score touches[i][login] raw per-rule commit touches overall[login] whole-repo decayed score (any surviving file) - gap[topdir][login] decayed score of files matching NO rule, keyed by - the file's top-two-level directory + gap[topdir][login] decayed score of commits touching >=1 file matching + NO rule, keyed by the file's top-two-level DIRECTORY + ("(root)" for top-level files). Accumulated once per + commit per key — same commit-touch semantics as the + rule scores, so the two columns stay comparable. """ score = [defaultdict(float) for _ in rule_globs] touches = [defaultdict(int) for _ in rule_globs] @@ -406,6 +435,7 @@ def compute_scores(commits, rule_globs, exclude_rxs, now, half_life_days): for login, ts, paths in commits: w = decay_weight((now - ts) / 86400.0, half_life_days) touched = set() + gap_keys = set() any_file = False for path in paths: if any(rx.search(path) for rx in exclude_rxs): @@ -417,12 +447,13 @@ def compute_scores(commits, rule_globs, exclude_rxs, now, half_life_days): touched.add(i) matched = True if not matched: - parts = path.split("/") - key = "/".join(parts[:2]) if len(parts) > 1 else parts[0] - gap[key][login] += w + dirs = path.split("/")[:-1] + gap_keys.add("/".join(dirs[:2]) if dirs else "(root)") if not any_file: continue overall[login] += w + for key in gap_keys: + gap[key][login] += w for i in touched: score[i][login] += w touches[i][login] += 1 @@ -467,8 +498,18 @@ def select_default_pool(overall, final_rule_lists, map_exclude, size=5, # --- GitHub API -------------------------------------------------------------- +# Sentinel distinct from None: "the API call FAILED" (timeout / 5xx / 403 / +# bad JSON) vs "the API answered and the answer is empty". Callers must never +# collapse the two — a transient failure that masquerades as "author has no +# linked account" silently drops that contributor's history and biases the +# proposal (the documented posture for environmental problems is a clean +# no-op, not a skewed map). +API_ERROR = object() + + def gh_get(url, token): - """GET a GitHub API URL; None on any error (callers degrade, not crash).""" + """GET a GitHub API URL; API_ERROR on any transport/HTTP failure so + callers can tell an outage from a genuinely empty answer.""" req = urllib.request.Request(url, headers={ "Authorization": "Bearer " + token, "Accept": "application/vnd.github+json", @@ -479,7 +520,7 @@ def gh_get(url, token): return json.load(resp) except (urllib.error.URLError, TimeoutError, ValueError) as e: print(f"::warning::GET {url.split('?')[0]} failed: {e}") - return None + return API_ERROR def fetch_collaborators(api, repo, token): @@ -488,9 +529,7 @@ def fetch_collaborators(api, repo, token): page = 1 while True: data = gh_get(f"{api}/repos/{repo}/collaborators?per_page=100&page={page}", token) - if data is None: - return None - if not isinstance(data, list): + if data is API_ERROR or not isinstance(data, list): return None for c in data: if isinstance(c, dict) and c.get("login"): @@ -502,9 +541,12 @@ def fetch_collaborators(api, repo, token): def resolve_email_via_api(api, repo, token, sha): - """Resolve a commit author email -> login via one commit fetch; None when - the email has no linked GitHub account (or the call fails).""" + """Resolve a commit author email -> login via one commit fetch. Returns + the login, None when the email has no linked GitHub account, or API_ERROR + when the call itself failed (callers must not treat that as no-account).""" data = gh_get(f"{api}/repos/{repo}/commits/{sha}", token) + if data is API_ERROR: + return API_ERROR if not isinstance(data, dict): return None author = data.get("author") or {} @@ -513,6 +555,15 @@ def resolve_email_via_api(api, repo, token, sha): # --- report / PR body -------------------------------------------------------- +def md_code(s): + """Wrap repo-derived text (paths, directory names) in a Markdown code + span that cannot break out of its table cell: GFM honors `\\|` even + inside code spans within tables, and a backtick would close the span so + it is swapped for a plain apostrophe. Keeps a crafted filename from + injecting rows/links/mentions into the bot-authored PR body.""" + return "`" + s.replace("|", "\\|").replace("`", "'") + "`" + + def fmt_login_score(login, score_map, touch_map, starred=frozenset()): star = "\\*" if login in starred else "" if login in score_map: @@ -541,7 +592,7 @@ def build_pr_body(report): "|---|---|---|---|", ] for r in report["rules"]: - paths = "
".join("`" + p + "`" for p in r["paths"]) or "—" + paths = "
".join(md_code(p) for p in r["paths"]) or "—" before = ", ".join(r["before"]) or "—" if r["under_floor"]: after = "*(unchanged — fewer than floor qualify; runtime falls " \ @@ -574,7 +625,7 @@ def build_pr_body(report): ] for g in report["gaps"]: top = ", ".join(f"{t['login']} ({t['score']:.1f})" for t in g["top"]) - lines.append(f"| `{g['dir']}` | {g['score']:.1f} | {top} |") + lines.append(f"| {md_code(g['dir'])} | {g['score']:.1f} | {top} |") lines.append("") lines += [ "### Knobs", @@ -605,6 +656,18 @@ def _env_float(name, default): return default +def _env_pos_float(name, default): + """_env_float, but the knob must be strictly positive — a zero half-life + would divide by zero and a negative one would invert the decay (older + commits gaining weight), so both fall back to the default with a warning + instead of crashing the never-fail run.""" + v = _env_float(name, default) + if v <= 0: + print(f"::warning::{name} must be positive (got {v}) — using {default}") + return float(default) + return v + + def write_outputs(outputs): out_file = os.environ.get("GITHUB_OUTPUT") if not out_file: @@ -632,7 +695,7 @@ def main(): config_path = os.environ.get("REVIEWER_CONFIG_PATH") or ".github/reviewers.yml" knobs = { "window_months": _env_int("WINDOW_MONTHS", 12), - "half_life_days": _env_float("HALF_LIFE_DAYS", 90), + "half_life_days": _env_pos_float("HALF_LIFE_DAYS", 90), "top_k": _env_int("TOP_K", 4), "floor": _env_int("FLOOR", 2), "min_touches": _env_int("MIN_TOUCHES", 5), @@ -643,7 +706,11 @@ def main(): results_dir = os.environ.get("RESULTS_DIR") or os.environ.get("RUNNER_TEMP") or "." os.makedirs(results_dir, exist_ok=True) - # exclusion regexes (built-ins + caller extras; bad extras warn, not fail) + # exclusion regexes (built-ins + caller extras; bad extras warn, not + # fail). The extras are trusted caller workflow config — a caller can + # already run arbitrary code in their own workflow, so a pathological + # (catastrophic-backtracking) pattern only stalls the caller's own run + # until the job timeout; no complexity bound is attempted here. exclude_rxs = [re.compile(rx) for rx in BUILTIN_EXCLUDE_PATHS] for rx in (os.environ.get("EXTRA_EXCLUDE_PATHS") or "").splitlines(): rx = rx.strip() @@ -693,8 +760,21 @@ def main(): email_login[e] = login else: email_sha.setdefault(e, sha) + failed_lookups = 0 for e, sha in email_sha.items(): - email_login[e] = resolve_email_via_api(api, repo, token, sha) + login = resolve_email_via_api(api, repo, token, sha) + if login is API_ERROR: + failed_lookups += 1 + login = None + email_login[e] = login + if failed_lookups: + # A transient API failure (rate limit, 5xx, timeout) must not be + # scored as "these authors have no linked account" — that would + # silently drop their entire history and emit a biased proposal. + # Environmental problem -> the documented clean no-op; the next + # scheduled run retries with a fresh quota. + return _noop_exit( + f"email->login resolution failed for {failed_lookups} email(s)") # --- membership filter (collaborators = the runtime's eligibility) ------ collaborators = fetch_collaborators(api, repo, token) diff --git a/.github/refresh-reviewers/tests/test_generate.py b/.github/refresh-reviewers/tests/test_generate.py index 304d971..c5febd2 100644 --- a/.github/refresh-reviewers/tests/test_generate.py +++ b/.github/refresh-reviewers/tests/test_generate.py @@ -152,6 +152,13 @@ def test_whole_path_rename_uses_new_path(self): def test_plain_path_untouched(self): self.assertEqual(gen.normalize_numstat_path("a/b/c.go"), "a/b/c.go") + def test_quoted_path_escapes_are_decoded(self): + # git C-style quoting (core.quotePath): \t, \", \\, octal non-ASCII + self.assertEqual(gen.normalize_numstat_path('"a\\ttab.go"'), "a\ttab.go") + self.assertEqual(gen.normalize_numstat_path('"quo\\"te.go"'), 'quo"te.go') + self.assertEqual(gen.normalize_numstat_path('"back\\\\slash.go"'), "back\\slash.go") + self.assertEqual(gen.normalize_numstat_path('"sp\\303\\244th.go"'), "späth.go") + class TestParseLog(unittest.TestCase): LOG = "\n".join([ @@ -258,9 +265,28 @@ def test_bucket_touch_and_overall(self): self.assertEqual(touches[1]["alice"], 1) self.assertNotIn("bob", overall) # all-excluded commit self.assertAlmostEqual(overall["alice"], 1.5) - self.assertAlmostEqual(gap["docs/guide.md"]["carol"], 1.0) + self.assertAlmostEqual(gap["docs"]["carol"], 1.0) self.assertNotIn("services/ingest", gap) # covered by a rule + def test_gap_dedupes_per_commit_and_keys_by_directory(self): + # a single commit touching many unmatched files in one directory must + # add its weight ONCE per gap key (same commit-touch semantics as the + # rule scores — else the gap column is incomparable), and the key is + # the top-two-level DIRECTORY, never the filename. + rules = [[gen.glob_to_regexp("services/ingest/**")]] + now = 1_800_000_000 + commits = [ + ("carol", now, ["docs/a.md", "docs/b.md", "docs/sub/c.md"]), + ("dave", now, ["README.md"]), # root-level file + ("erin", now, ["web/src/components/App.tsx"]), # deep path + ] + _s, _t, _o, gap = gen.compute_scores(commits, rules, [], now, 90) + self.assertAlmostEqual(gap["docs"]["carol"], 1.0) # not 2.0 + self.assertAlmostEqual(gap["docs/sub"]["carol"], 1.0) + self.assertAlmostEqual(gap["(root)"]["dave"], 1.0) + self.assertAlmostEqual(gap["web/src"]["erin"], 1.0) + self.assertNotIn("docs/a.md", gap) + CONFIG = """\ # Reviewer expertise map — hand-tuned, comments are documentation. @@ -370,6 +396,60 @@ def test_default_pool_exclude_is_case_insensitive(self): self.assertEqual(pool, ["b"]) +class TestEnvKnobs(unittest.TestCase): + def _with_env(self, value, default=90): + os.environ["_RR_TEST_KNOB"] = value + self.addCleanup(os.environ.pop, "_RR_TEST_KNOB", None) + return gen._env_pos_float("_RR_TEST_KNOB", default) + + def test_zero_half_life_falls_back_to_default(self): + # half_life_days: 0 would divide by zero in decay_weight + self.assertEqual(self._with_env("0"), 90.0) + + def test_negative_half_life_falls_back_to_default(self): + # a negative half-life inverts the decay (older commits gain weight) + self.assertEqual(self._with_env("-5"), 90.0) + + def test_positive_value_passes_through(self): + self.assertEqual(self._with_env("30"), 30.0) + + +class TestApiErrorSentinel(unittest.TestCase): + """A transient API failure must stay distinguishable from 'this email has + no linked account' — collapsing the two silently drops contributors.""" + + def _patch_gh_get(self, fake): + orig = gen.gh_get + gen.gh_get = fake + self.addCleanup(setattr, gen, "gh_get", orig) + + def test_resolve_email_propagates_api_error(self): + self._patch_gh_get(lambda url, token: gen.API_ERROR) + self.assertIs(gen.resolve_email_via_api("a", "o/r", "t", "sha"), + gen.API_ERROR) + + def test_resolve_email_none_for_unlinked_account(self): + self._patch_gh_get(lambda url, token: {"author": None}) + self.assertIsNone(gen.resolve_email_via_api("a", "o/r", "t", "sha")) + + def test_resolve_email_returns_login(self): + self._patch_gh_get(lambda url, token: {"author": {"login": "octocat"}}) + self.assertEqual(gen.resolve_email_via_api("a", "o/r", "t", "sha"), + "octocat") + + def test_fetch_collaborators_unavailable_on_api_error(self): + self._patch_gh_get(lambda url, token: gen.API_ERROR) + self.assertIsNone(gen.fetch_collaborators("a", "o/r", "t")) + + +class TestMarkdownEscaping(unittest.TestCase): + def test_md_code_escapes_table_breakers(self): + # `|` would split the table cell, a backtick would close the span + self.assertEqual(gen.md_code("a|b"), "`a\\|b`") + self.assertEqual(gen.md_code("a`b`c"), "`a'b'c`") + self.assertEqual(gen.md_code("plain/path.go"), "`plain/path.go`") + + class TestPrBody(unittest.TestCase): def test_body_carries_the_contract_pieces(self): report = { diff --git a/.github/workflows/refresh-reviewers.yml b/.github/workflows/refresh-reviewers.yml index ba0420c..1882dd7 100644 --- a/.github/workflows/refresh-reviewers.yml +++ b/.github/workflows/refresh-reviewers.yml @@ -24,8 +24,12 @@ name: Refresh Reviewers (reusable) # pattern as assign-reviewers.yml). # # IDEMPOTENT: re-runs force-reset the same `pr_branch` from the default branch -# and edit the one open drift PR in place — duplicate PRs never stack. When -# the recomputed map equals the committed one, the run exits quietly. +# and edit the one open drift PR in place — duplicate PRs never stack. Guard +# rails: `pr_branch` may never equal the default branch (hard error), a +# concurrency group serializes runs per repo, a drift branch whose tip a +# human has pushed to is never force-reset (the PR body says "edit freely" +# and means it), and a no-drift run closes a stale still-bot-authored drift +# PR so an obsolete proposal can't linger mergeable. # # The generator script is loaded from THIS repo (public, pinned via # `workflows_ref`) — never from the caller's checkout — so a caller-side @@ -44,7 +48,7 @@ name: Refresh Reviewers (reusable) # contents: read # uses: Comfy-Org/github-workflows/.github/workflows/refresh-reviewers.yml@ # v1 # with: -# workflows_ref: # pin the generator to the same ref as `uses:` +# workflows_ref: # required — pin the generator to the same SHA as `uses:` # map_exclude: some-operator-login # secrets: # CLOUD_CODE_BOT_PRIVATE_KEY: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} @@ -127,10 +131,12 @@ on: workflows_ref: description: >- Ref of Comfy-Org/github-workflows to load the generator script from. - Pin this to the same ref you pin `uses:` to for reproducibility. + Pin this to the same SHA you pin `uses:` to. REQUIRED with no + default on purpose: defaulting to the mutable `main` would load the + generator from a moving ref even when the caller pinned `uses:` by + SHA, defeating the pin-everything-by-SHA rule. type: string - required: false - default: main + required: true secrets: CLOUD_CODE_BOT_PRIVATE_KEY: description: >- @@ -147,6 +153,12 @@ jobs: refresh: name: Recompute reviewer map runs-on: ubuntu-latest + # One refresh per caller repo at a time — overlapping runs would race the + # force-reset of the shared drift branch. Queued, never cancelled: the + # newest queued run recomputes from the newest history anyway. + concurrency: + group: refresh-reviewers-${{ github.repository }} + cancel-in-progress: false permissions: contents: read steps: @@ -229,10 +241,32 @@ jobs: PR_BODY: ${{ steps.generate.outputs.pr_body_path }} run: | set -euo pipefail + # A misconfigured `pr_branch: main` would force-push the generated + # commit straight onto the default branch — refuse loudly. + if [ "$PR_BRANCH" = "$DEFAULT_BRANCH" ]; then + echo "::error::pr_branch ('$PR_BRANCH') must not be the default branch" + exit 1 + fi + # Commit as the app's bot identity — the `[bot]@` email also keeps # these refresh commits out of future expertise scoring. + BOT_EMAIL="${APP_SLUG}[bot]@users.noreply.github.com" git config user.name "${APP_SLUG}[bot]" - git config user.email "${APP_SLUG}[bot]@users.noreply.github.com" + git config user.email "$BOT_EMAIL" + + # The PR body advertises "edit freely" — never clobber a human's + # commits on the drift branch. If an OPEN drift PR's tip is not + # bot-authored, a human is driving it: leave it alone. (A closed or + # merged PR's leftover branch is nobody's — reset it freely.) + EXISTING="$(gh pr list --head "$PR_BRANCH" --base "$DEFAULT_BRANCH" \ + --state open --json number --jq '.[0].number // empty')" + if [ -n "$EXISTING" ] && git fetch origin "refs/heads/$PR_BRANCH" 2>/dev/null; then + TIP_AUTHOR="$(git log -1 --format=%ae FETCH_HEAD)" + if [ "$TIP_AUTHOR" != "$BOT_EMAIL" ]; then + echo "::warning::open drift PR #$EXISTING has a tip authored by $TIP_AUTHOR (not the bot) — leaving human edits in place, not force-pushing." + exit 0 + fi + fi # Reset the (stable) drift branch from the default branch, land the # single-file change, and force-push — re-runs update in place, so @@ -247,8 +281,6 @@ jobs: git commit -m "chore: refresh reviewer expertise map from git history" git push --force origin "HEAD:refs/heads/$PR_BRANCH" - EXISTING="$(gh pr list --head "$PR_BRANCH" --base "$DEFAULT_BRANCH" \ - --state open --json number --jq '.[0].number // empty')" if [ -n "$EXISTING" ]; then gh pr edit "$EXISTING" --body-file "$PR_BODY" echo "Updated existing drift PR #$EXISTING." @@ -257,3 +289,38 @@ jobs: --title "chore: refresh reviewer expertise map" \ --body-file "$PR_BODY" fi + + - name: Close stale drift PR (no drift) + # A later run that finds NO drift must still reconcile an + # already-open bot PR: new history or decay may have made a + # previously proposed map obsolete, and leaving that PR open and + # mergeable would silently reintroduce an outdated reviewer map. + # Only a still-bot-authored PR is closed — one a human has pushed to + # is theirs to drive. Gated on report_path so the generator's + # environmental no-op (API outage, missing config — which also emits + # changed=false but no report) can never close a PR the run failed + # to recompute. + if: >- + steps.generate.outputs.changed == 'false' && + steps.generate.outputs.report_path != '' + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + APP_SLUG: ${{ steps.app-token.outputs.app-slug }} + DEFAULT_BRANCH: ${{ steps.default-branch.outputs.branch }} + PR_BRANCH: ${{ inputs.pr_branch }} + run: | + set -euo pipefail + EXISTING="$(gh pr list --head "$PR_BRANCH" --base "$DEFAULT_BRANCH" \ + --state open --json number --jq '.[0].number // empty')" + if [ -z "$EXISTING" ]; then + echo "No open drift PR — nothing to reconcile." + exit 0 + fi + BOT_EMAIL="${APP_SLUG}[bot]@users.noreply.github.com" + if git fetch origin "refs/heads/$PR_BRANCH" 2>/dev/null && \ + [ "$(git log -1 --format=%ae FETCH_HEAD)" = "$BOT_EMAIL" ]; then + gh pr close "$EXISTING" --comment "The latest refresh run found no drift — the committed map matches the recomputed one, so this proposal is obsolete. A future run will open a fresh PR if drift reappears." + echo "Closed stale drift PR #$EXISTING." + else + echo "Drift PR #$EXISTING has non-bot commits — leaving it to its human author." + fi diff --git a/README.md b/README.md index e298d8b..2df25a5 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ This repo is **public** so any repo — public or private, inside or outside the | [`cursor-review.yml`](.github/workflows/cursor-review.yml) | Label-triggered multi-model code review. A 4-lab × 2-review-type cursor-agent panel runs adversarial + edge-case passes, a judge model consolidates them into one PR review with per-finding severity badges, and the triggerer gets Slack start/complete DMs. Advisory by default; opt in with `blocking: true` to fail a (required-status-check) gate while findings stay unresolved. Prompts and scripts live in [`.github/cursor-review/`](.github/cursor-review) — the single source of truth, so consumer repos carry only a thin caller. Self-hostable via `runs_on` (JSON, default `ubuntu-latest`) and panel models overridable via `models` (JSON array) for accounts lacking a default provider. Requires `CURSOR_API_KEY` (+ optional `SLACK_BOT_TOKEN`). | | [`cursor-review-auto-label.yml`](.github/workflows/cursor-review-auto-label.yml) | Companion to `cursor-review.yml`. On PR assignment, applies the review label for an opted-in reviewer (via the CLOUD_CODE_BOT app token, so the label actually triggers the review). The opt-in roster lives in the caller's `vars.CURSOR_REVIEW_OPTED_IN_LOGINS` — no roster is baked into the workflow. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. | | [`assign-reviewers.yml`](.github/workflows/assign-reviewers.yml) | Auto-requests expertise-aware, load-balanced PR reviewers with new-folk randomization. Matches changed paths against a caller-repo `.github/reviewers.yml` (path-glob → reviewers, plus a `default_pool`), drops the author + `vars.REVIEWER_EXCLUDE`, ranks candidates by open review load (steering off anyone at/over `vars.REVIEWER_LOAD_CAP`), and may swap a slot for a `vars.REVIEWER_GROWTH_POOL` member. Requests go through the CLOUD_CODE_BOT app token so they work on fork PRs. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. | -| [`refresh-reviewers.yml`](.github/workflows/refresh-reviewers.yml) | Companion to `assign-reviewers.yml` — a scheduled drift-detector that recomputes the caller's `.github/reviewers.yml` from git history (recency-decayed commit touches per rule bucket, same glob semantics as the assigner, collaborators only, bots and generated/churn paths excluded) and opens ONE idempotent single-file PR when the committed map drifts. The rewrite is surgical (only the `reviewers:`/`default_pool:` lists change — comments preserved), the PR body carries per-rule before/after scores plus a report-only taxonomy-gap section, and a rule with too few qualifiers is left unchanged. Never a live mutator. Engine + knob rationale in [`.github/refresh-reviewers/`](.github/refresh-reviewers). Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. | +| [`refresh-reviewers.yml`](.github/workflows/refresh-reviewers.yml) | Companion to `assign-reviewers.yml` — a scheduled drift-detector that recomputes the caller's `.github/reviewers.yml` from git history (recency-decayed commit touches per rule bucket, same glob semantics as the assigner, collaborators only, bots and generated/churn paths excluded) and opens ONE idempotent single-file PR when the committed map drifts. The rewrite is surgical (only the `reviewers:`/`default_pool:` lists change — comments preserved), the PR body carries per-rule before/after scores plus a report-only taxonomy-gap section, and a rule with too few qualifiers is left unchanged. Never a live mutator. Engine + knob rationale in [`.github/refresh-reviewers/`](.github/refresh-reviewers). Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`, and `workflows_ref` (required) must pin the generator to the same SHA as `uses:`. | | [`assign-prs-to-author.yml`](.github/workflows/assign-prs-to-author.yml) | Housekeeping — assigns every open PR with no assignees to its author (bot-authored PRs skipped by default). Run on a schedule from a thin caller; useful when a team tracks PR ownership via assignees. The calling job needs `pull-requests: write` and `issues: write`. | | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. |