feat: scheduled review-escalation Action + always-on-host docs (#230 follow-up) - #232
Conversation
…llow-up) Addresses @tangym's review note on #230: the 24h/72h escalation windows are wall-clock thresholds, but the loop was documented as running on the maintainer's local workstation — which is offline in exactly the scenario the escalation is for (maintainer away). Document running it on an always-on host (small VM, CI cron, or scheduled GitHub Action) instead, and note that .github/CODEOWNERS already covers the baseline independent of the loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…uting) Reference implementation of the always-on escalation loop the docs now call for. .github/workflows/review-escalation.yml runs every 6h (plus manual workflow_dispatch with a dry-run default) and calls .github/scripts/escalate_reviews.py, which applies the 24h/72h/7d windows and CODEOWNERS routing deterministically — no LLM, so it runs reliably in CI on GitHub's always-on schedule. Routing mirrors AGENTS.md: last-match CODEOWNERS, exclude author + OOO owners, fallback admin only as last resort, prefer the owner covering the most changed paths. Validated in dry-run against all open PRs (correctly routes #228->@minthigpen, #226->@AaronAspinwall123, observes PRs that already have reviewers, excludes authors). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jakepresent
left a comment
There was a problem hiding this comment.
Requesting changes because the always-on host idea is right, but the deterministic escalation script does not yet match the governance contract it says it implements.
Blocking issues:
-
The routing policy differs from
AGENTS.md/dev-maintainer.md. The docs say to exclude the author, exclude OOO users, keep fallback admin as last resort, then prefer admins / the owner pinged least recently. The script instead ranks by changed-file coverage and alphabetic tie-break, with no admin preference and no ping-history state. That may be a reasonable deterministic policy, but then the source-of-truth docs need to say that. Right now the PR body says the script mirrorsAGENTS.md, and it does not. -
The 7-day fallback path is effectively unreachable for the normal stale-request case. In
evaluate_pr, therequested and not has_reviewed and age >= WINDOW_72Hbranch runs before theage >= WINDOW_7Dbranch. So a PR with requested reviewers and no response at 7+ days either keeps adding another non-fallback owner every run, or if no additional owner exists, returns “72h reached but no additional eligible owner” with no fallback request. That contradicts the documented “≥ 7 days, still no response -> fallback admin” rule. -
The fallback branch can request the PR author. The candidate pool excludes the author, but if the pool is empty the code sets
chosen = [FALLBACK_LOGIN]anyway. For a PR authored by the fallback admin that only matches the catch-all owner, this would try to request review from the author, violating the first routing rule. It should no-op / log an anomaly / require manual escalation instead.
I verified the script py_compiles and a dry run works, so this is not a syntax/build issue. It is specifically the escalation semantics versus the governance text.
…review) Three blocking issues from the review: 1. Routing now matches AGENTS.md / dev-maintainer.md. Updated both docs to state the deterministic tie-break the stateless reference Action uses (most changed-path coverage, then alphabetical) in place of the stateful 'least recently pinged', and documented the author-guard. Docs and code now agree. 2. The 7-day fallback is reachable. evaluate_pr now cascades by severity: never-requested -> first owner; requested + >=7d -> fallback admin; requested + >=72h -> second non-fallback owner; else observe. A requested-but-silent PR at 7d+ now reaches the fallback branch instead of looping on the 72h step. The fallback admin is reserved for 7d (72h uses non-fallback owners only). 3. The fallback never requests the PR author. New _safe_fallback() returns None when the fallback admin is the author (or already requested/reviewed); the caller then widens to another owner or emits a 'manual escalation' warning instead of pinging the author. Verified in dry-run: PR #88 (authored by the catch-all owner) now routes to a real co-owner, not the author. Adds tests/test_escalate_reviews.py (7 cases incl. all three regressions). Dry-run across all open PRs confirmed no PR targets its own author. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks @jakepresent — all three are real and all three are fixed in 1. Routing now matches the governance docs (and vice-versa). You're right: the script ranked by changed-file coverage + alphabetical, while
If you'd prefer the script instead grow real least-recently-pinged behavior (derivable from the PR timeline's 2. The 7-day fallback is now reachable. Confirmed bug — the old So a requested-but-silent PR at 7d+ now hits the fallback branch. I also tightened 72h to use non-fallback owners only, so the fallback admin isn't pulled in early. 3. The fallback can no longer request the PR author. New Verified in a dry-run across all open PRs: PR #88 (authored by Tests: added Re-requesting review when you have a sec. Genuinely good catch on the semantics-vs-docs gap. 🙏 |
jakepresent
left a comment
There was a problem hiding this comment.
Re-reviewed after the follow-up fixes. The blockers from my previous review are addressed:
- The docs now describe the deterministic stateless routing policy instead of claiming ping-history behavior.
- The 7-day fallback branch is reachable before the 72h branch for requested-but-silent PRs.
- The fallback path now avoids requesting the PR author and logs/manual-escalates when fallback would be invalid.
Validation:
python -m pytest tests/test_escalate_reviews.py -q->7 passedpython -m py_compile .github/scripts/escalate_reviews.pypassed
Looks good to me.
What
Follow-up to @tangym's review note on #230: the dev-maintainer escalation windows (24h / 72h / 7d) are wall-clock thresholds, but the loop was documented as running on the maintainer's local workstation — which is offline in exactly the scenario the escalation exists for (the maintainer being away). This PR (1) fixes the docs to require an always-on host and (2) ships that host as a scheduled GitHub Action so the escalation actually fires unattended.
Changes
Docs (
AGENTS.md,.github/agents/dev-maintainer.md).github/CODEOWNERSalready covers the baseline independent of the loop.Reference implementation (
.github/workflows/review-escalation.yml+.github/scripts/escalate_reviews.py)workflow_dispatch(with a dry-run default for safe manual runs).escalate_reviews.pyis the deterministic half of the dev-maintainer agent — pure CODEOWNERS routing, no LLM — so it runs reliably in CI. The LLMaudit-prpass stays a separate concern a maintainer runs from any host.AGENTS.md/dev-maintainer.mdcontract: last-match CODEOWNERS → exclude the PR author → exclude OOO owners (best-effort via GitHub user status) → fallback admin only as last resort, never the author. Deterministic tie-break (now documented in both files): most changed-path coverage, then alphabetical — the stateless Action uses this in place of 'least recently pinged'. Cascade is evaluated by severity (7d → 72h → 24h) so the 7-day fallback is always reachable.permissions: { contents: read, pull-requests: write }; uses the built-inGITHUB_TOKEN.Test plan
Validated by running the exact script in
--dry-runagainst all open PRs (aworkflow_dispatchon a brand-new workflow can't be triggered until it's on the default branch, so the script is locally runnable for this reason):observe; routing preview ranks the correct owners:@jakepresent, @AaronAspinwall123, @tangym, @minthigpen, @sooyeonni, author@changliu2excluded. No double-ping.@minthigpen; chore: pre-launch demo polish (quieter logs + faster default concurrency) #226 / Add variant-c-unguarded-prompted run to demo branch #206 →@AaronAspinwall123.observe; authors always excluded; fallback admin only when they are the sole owner.YAML well-formed, script
py_compiles clean.