From fb46e6ccd3d9dbe36c2ccb5fdac8e1a0c02de805 Mon Sep 17 00:00:00 2001 From: Maxime Chambreuil Date: Wed, 30 Sep 2026 16:56:58 +0000 Subject: [PATCH 1/3] [IMP] mechanical review gate and silent-PR escalation Add a volume filter in front of human reviewers: missing development_status, obvious ACL holes on new addons, and warnings for non-Alpha / possible name collisions. The bot comments a checklist and never merges or counts as a review. Escalate PRs that stay on needs review with no human reply (7 / 14 / 21 days) to a wider team. Optional REVIEW_GATE_AS_FIRST_FILTER delays maintainer pings until checks and CI are green. --- README.rst | 34 +++- environment.sample | 7 +- newsfragments/+review-gate.feature | 1 + src/oca_github_bot/config.py | 6 +- src/oca_github_bot/cron.py | 5 + src/oca_github_bot/review_gate.py | 143 ++++++++++++++++ src/oca_github_bot/tasks/__init__.py | 2 + src/oca_github_bot/tasks/escalate_review.py | 152 ++++++++++++++++++ src/oca_github_bot/tasks/review_gate.py | 80 +++++++++ src/oca_github_bot/tasks/tag_needs_review.py | 14 +- .../tasks/tag_ready_to_merge.py | 3 +- src/oca_github_bot/webhooks/__init__.py | 1 + .../webhooks/on_pr_open_mention_maintainer.py | 4 + .../webhooks/on_pr_review_gate.py | 15 ++ tests/test_on_pr_review_gate.py | 21 +++ tests/test_review_gate.py | 96 +++++++++++ 16 files changed, 571 insertions(+), 13 deletions(-) create mode 100644 newsfragments/+review-gate.feature create mode 100644 src/oca_github_bot/review_gate.py create mode 100644 src/oca_github_bot/tasks/escalate_review.py create mode 100644 src/oca_github_bot/tasks/review_gate.py create mode 100644 src/oca_github_bot/webhooks/on_pr_review_gate.py create mode 100644 tests/test_on_pr_review_gate.py create mode 100644 tests/test_review_gate.py diff --git a/README.rst b/README.rst index ad32f72b..3fd8fdf4 100644 --- a/README.rst +++ b/README.rst @@ -33,6 +33,29 @@ Mention declared maintainers that addons they maintain are being modified. Comment with a call for maintainers if there are no declared maintainer. +Run **mechanical checks** (volume filter, not a review): missing +``development_status``, new addons without ``ir.model.access.csv`` when they +define models, flag addons that are not Alpha / that look like a name collision. +The bot comments a checklist, sets or removes the ``bot checks passed`` label, +and never treats this as an approval or a merge. + +When ``REVIEW_GATE_AS_FIRST_FILTER`` is enabled, maintainer pings and the +``needs review`` label wait until those checks pass **and** CI is green, so +reviewer time is spent on ready PRs. + +On pull request CI status +------------------------- + +When the CI in a Pull Request goes green, set the ``needs review`` label, +unless it has ``wip:`` or ``[wip]`` in it's title. + +If the first-filter is enabled, also require ``bot checks passed``. + +If a PR stays on ``needs review`` with no human review, a daily job escalates +who is asked (7 days: repo maintainer team, 14 days: PSC link, +21 days: core maintainers). Silence does **not** grant ``/ocabot merge``. + + On pull request close --------------------- @@ -68,12 +91,8 @@ On Pull Request review When there are two approvals, set the ``approved`` label. When the PR is at least 5 days old, set the ``ready to merge`` label. - -On Pull Request CI status -------------------------- - -When the CI in a Pull Request goes green, set the ``needs review`` label, -unless it has ``wip:`` or ``[wip]`` in it's title. +The ready-to-merge comment includes the PSC list: +https://oca.github.io/repo-maintainer-conf/repos.html Commands -------- @@ -231,8 +250,7 @@ Contributors * Jose Angel Fentanez * Simone Rubino * Sylvain Le Gal (https://twitter.com/legalsylvain) -* Tecnativa - Pedro M. Baeza -* Tecnativa - Víctor Martínez +* Maxime Chambreuil Maintainers =========== diff --git a/environment.sample b/environment.sample index 19e51453..4ff3af58 100644 --- a/environment.sample +++ b/environment.sample @@ -34,6 +34,10 @@ ODOO_PASSWORD= # Number of days before the proposal can be marked as "Approved" #MIN_PR_AGE=5 +# If true, do not ping maintainers or set `needs review` until mechanical +# checks (review_gate) pass and CI is green. Default: false (current behaviour). +#REVIEW_GATE_AS_FIRST_FILTER=false + # Color of the github label that contains the name of the module #MODULE_LABEL_COLOR=#ffc @@ -42,7 +46,8 @@ ODOO_PASSWORD= # Available tasks: # delete_branch,tag_approved,tag_ready_to_merge,gen_addons_table, # gen_addons_readme,gen_addons_icon,setuptools_odoo,mention_maintainer, -# merge_bot,rebase_bot,merge_bot_towncrier,tag_needs_review +# merge_bot,rebase_bot,merge_bot_towncrier,tag_needs_review, +# review_gate,escalate_review #BOT_TASKS=all # Coma separated list of task to ignore diff --git a/newsfragments/+review-gate.feature b/newsfragments/+review-gate.feature new file mode 100644 index 00000000..ba3844f5 --- /dev/null +++ b/newsfragments/+review-gate.feature @@ -0,0 +1 @@ +Add a mechanical review gate (volume filter, not a human review) and escalate silent PRs to a wider human set without merging. diff --git a/src/oca_github_bot/config.py b/src/oca_github_bot/config.py index f2a03d3f..ffdeec5b 100644 --- a/src/oca_github_bot/config.py +++ b/src/oca_github_bot/config.py @@ -59,7 +59,8 @@ def func_wrapper(*args, **kwargs): # Available tasks: # delete_branch,tag_approved,tag_ready_to_merge,gen_addons_table, # gen_addons_readme,gen_addons_icon,setuptools_odoo,merge_bot,tag_needs_review, -# migration_issue_bot,whool_init,gen_metapackage,label_modified_addons +# migration_issue_bot,whool_init,gen_metapackage,label_modified_addons, +# review_gate,escalate_review BOT_TASKS = os.environ.get("BOT_TASKS", "all").split(",") BOT_TASKS_DISABLED = os.environ.get("BOT_TASKS_DISABLED", "").split(",") @@ -100,6 +101,9 @@ def func_wrapper(*args, **kwargs): APPROVALS_REQUIRED = int(os.environ.get("APPROVALS_REQUIRED", "2")) MIN_PR_AGE = int(os.environ.get("MIN_PR_AGE", "5")) +REVIEW_GATE_AS_FIRST_FILTER = os.environ.get( + "REVIEW_GATE_AS_FIRST_FILTER", "" +).lower() in ("1", "true", "yes") MODULE_LABEL_COLOR = os.environ.get("MODULE_LABEL_COLOR", "#ffc") diff --git a/src/oca_github_bot/cron.py b/src/oca_github_bot/cron.py index 708722b3..5021ecd5 100644 --- a/src/oca_github_bot/cron.py +++ b/src/oca_github_bot/cron.py @@ -28,6 +28,11 @@ "args": (org,), "schedule": crontab(minute="0"), }, + "escalate_review": { + "task": "oca_github_bot.tasks.escalate_review.escalate_review", + "args": (org,), + "schedule": crontab(hour="8", minute="0"), + }, } ) diff --git a/src/oca_github_bot/review_gate.py b/src/oca_github_bot/review_gate.py new file mode 100644 index 00000000..1d927b9a --- /dev/null +++ b/src/oca_github_bot/review_gate.py @@ -0,0 +1,143 @@ +# Copyright 2026 Maxime Chambreuil - Gray Matter Logic +# Distributed under the MIT License (http://opensource.org/licenses/MIT). + +"""Mechanical review-gate checks (volume filter, not a human review).""" + +from __future__ import annotations + +import os +import re +from dataclasses import dataclass, field + +from .manifest import NoManifestFound, get_addon_name, get_manifest + +COMMENT_MARKER = "" +LABEL_BOT_CHECKS_PASSED = "bot checks passed" + +STABLE_STATUSES = {"Production/Stable", "Mature"} +MODEL_NAME_RE = re.compile(r"""_name\s*=\s*['"]([a-z0-9.]+)['"]""") + + +@dataclass +class GateResult: + blocking: list[str] = field(default_factory=list) + warnings: list[str] = field(default_factory=list) + + @property + def passed(self) -> bool: + return not self.blocking + + +def _has_models(addon_dir: str) -> bool: + models_dir = os.path.join(addon_dir, "models") + if not os.path.isdir(models_dir): + return False + for name in os.listdir(models_dir): + if name.endswith(".py") and name != "__init__.py": + return True + return False + + +def _defines_models(addon_dir: str) -> bool: + models_dir = os.path.join(addon_dir, "models") + if not os.path.isdir(models_dir): + return False + for root, _dirs, files in os.walk(models_dir): + for name in files: + if not name.endswith(".py"): + continue + path = os.path.join(root, name) + with open(path, encoding="utf-8", errors="replace") as fh: + if MODEL_NAME_RE.search(fh.read()): + return True + return False + + +def _has_acl_file(addon_dir: str) -> bool: + return os.path.isfile(os.path.join(addon_dir, "security", "ir.model.access.csv")) + + +def check_addon(addon_dir: str, *, is_new: bool) -> GateResult: + result = GateResult() + name = get_addon_name(addon_dir) + try: + manifest = get_manifest(addon_dir) + except NoManifestFound: + result.blocking.append(f"`{name}`: no `__manifest__.py` found") + return result + status = (manifest.get("development_status") or "").strip() + if not status: + result.blocking.append( + f"`{name}`: missing `development_status` in the manifest" + ) + elif status in STABLE_STATUSES: + result.warnings.append( + f"`{name}` has `development_status` {status}; " + "a human review is required before merge (bot checks are not a review)" + ) + elif status != "Alpha": + result.warnings.append( + f"`{name}` has `development_status` {status} " + "(not Alpha); keep the normal 2-review path" + ) + if is_new: + result.warnings.append( + f"`{name}` is a new addon on this branch; confirm the name " + "does not collide with another OCA addon on the same series" + ) + if _defines_models(addon_dir) and not _has_acl_file(addon_dir): + result.blocking.append( + f"`{name}`: new models without `security/ir.model.access.csv` " + "(obvious ACL hole)" + ) + elif _has_models(addon_dir) and not _has_acl_file(addon_dir): + result.warnings.append( + f"`{name}`: `models/` present but no `security/ir.model.access.csv`" + ) + return result + + +def merge_results(results: list[GateResult]) -> GateResult: + merged = GateResult() + for result in results: + merged.blocking.extend(result.blocking) + merged.warnings.extend(result.warnings) + return merged + + +def format_comment(result: GateResult) -> str: + lines = [ + COMMENT_MARKER, + "## Mechanical checks", + "", + ( + "This is a **volume filter**, not a review. " + "It does not count as an approval and the bot will not merge." + ), + "", + ] + if result.passed: + lines.append("**Result:** ready for a human reviewer / maintainer.") + else: + lines.append( + "**Result:** not ready for reviewers yet. " + "Please fix the blocking items; maintainers will not be pinged " + "for review while this stays red (when the first-filter is enabled)." + ) + if result.blocking: + lines.extend(["", "### Must fix"]) + lines.extend(f"- {item}" for item in result.blocking) + if result.warnings: + lines.extend(["", "### Please check"]) + lines.extend(f"- {item}" for item in result.warnings) + lines.extend( + [ + "", + ( + "Merge still requires 2 human approvals (or 3 within 5 days) " + "including a PSC, then `/ocabot merge`." + ), + "PSC list: https://oca.github.io/repo-maintainer-conf/repos.html", + ] + ) + return "\n".join(lines) diff --git a/src/oca_github_bot/tasks/__init__.py b/src/oca_github_bot/tasks/__init__.py index 29b5a562..610416c3 100644 --- a/src/oca_github_bot/tasks/__init__.py +++ b/src/oca_github_bot/tasks/__init__.py @@ -2,11 +2,13 @@ # Distributed under the MIT License (http://opensource.org/licenses/MIT). from . import ( + escalate_review, heartbeat, label_modified_addons, main_branch_bot, mention_maintainer, migration_issue_bot, + review_gate, tag_approved, tag_needs_review, tag_ready_to_merge, diff --git a/src/oca_github_bot/tasks/escalate_review.py b/src/oca_github_bot/tasks/escalate_review.py new file mode 100644 index 00000000..dd8a686f --- /dev/null +++ b/src/oca_github_bot/tasks/escalate_review.py @@ -0,0 +1,152 @@ +# Copyright 2026 Maxime Chambreuil - Gray Matter Logic +# Distributed under the MIT License (http://opensource.org/licenses/MIT). + +"""Escalate silent PRs: ask a wider human set, never merge.""" + +from datetime import datetime, timezone + +from .. import github +from ..config import GITHUB_LOGIN, GITHUB_ORG, switchable +from ..github import gh_call +from ..queue import getLogger, task + +_logger = getLogger(__name__) + +BOT_LOGINS = { + (GITHUB_LOGIN or "").lower(), + "oca-git-bot", + "github-actions[bot]", + "codecov", + "codecov-commenter", + "pre-commit-ci[bot]", +} + +STEP_MARKERS = ( + "", + "", + "", +) + +STEP_DELAYS = (7, 14, 21) + +PSC_LIST = "https://oca.github.io/repo-maintainer-conf/repos.html" + + +def _is_bot(login): + if not login: + return True + low = login.lower() + return low in BOT_LOGINS or low.endswith("[bot]") + + +def _step_done(bodies, step): + marker = STEP_MARKERS[step] + return any(marker in (body or "") for body in bodies) + + +def next_escalate_step(last_human_at, now, comments_bodies): + """Return 0, 1 or 2 for the next ping, or None if none is due.""" + if last_human_at is None: + return None + elapsed = (now - last_human_at).total_seconds() / 86400 + for step, days in enumerate(STEP_DELAYS): + if elapsed >= days and not _step_done(comments_bodies, step): + return step + return None + + +def escalate_comment(repo, step): + team = f"@OCA/{repo}-maintainers" + if step == 0: + return ( + f"{STEP_MARKERS[0]}\n" + f"No human review yet after {STEP_DELAYS[0]} days " + f"(mechanical checks / `needs review`).\n\n" + f"Pinging the repo maintainer team: {team}\n" + "This is not a merge and does not count as a review." + ) + if step == 1: + return ( + f"{STEP_MARKERS[1]}\n" + f"Still no human review after {STEP_DELAYS[1]} days.\n\n" + "Pinging the PSC for this repository. " + f"See {PSC_LIST}\n" + f"(team guess: {team})\n" + "Please review or say if this should wait. The bot will not merge." + ) + return ( + f"{STEP_MARKERS[2]}\n" + f"Still no human review after {STEP_DELAYS[2]} days.\n\n" + "@OCA/oca-core-maintainers review wanted.\n" + "Silence escalates who is asked, not who may `/ocabot merge`." + ) + + +def _comment_dt(comment): + created = getattr(comment, "created_at", None) + if created is None: + return None + if created.tzinfo is None: + return created.replace(tzinfo=timezone.utc) + return created.astimezone(timezone.utc) + + +def _last_human_at(comments, issue_created): + last_human = None + for comment in comments: + login = comment.user.login if comment.user else None + if _is_bot(login): + continue + created = _comment_dt(comment) + if created and (last_human is None or created > last_human): + last_human = created + if last_human is not None: + return last_human + if issue_created is None: + return None + if issue_created.tzinfo is None: + return issue_created.replace(tzinfo=timezone.utc) + return issue_created.astimezone(timezone.utc) + + +def _org_repo_from_issue(issue): + full = issue.html_url # https://github.com/org/repo/pull/N + parts = full.split("/") + return parts[3], parts[4] + + +@task() +@switchable("escalate_review") +def escalate_review(org=None, dry_run=False): + """Ping a wider human set when a green PR gets no review.""" + orgs = [org] if org else list(GITHUB_ORG) + now = datetime.now(timezone.utc) + with github.login() as gh: + for org_name in orgs: + query = [ + "type:pr", + "state:open", + 'label:"needs review"', + "-label:approved", + '-label:"work in progress"', + '-label:"ready to merge"', + f"org:{org_name}", + ] + for issue in gh.search_issues(" ".join(query)): + _escalate_one(issue, now, dry_run) + + +def _escalate_one(issue, now, dry_run): + comments = list(issue.issue.comments()) + bodies = [c.body or "" for c in comments] + last_human = _last_human_at(comments, getattr(issue, "created_at", None)) + step = next_escalate_step(last_human, now, bodies) + if step is None: + return + _org_name, repo_name = _org_repo_from_issue(issue) + body = escalate_comment(repo_name, step) + if dry_run: + _logger.info("DRY-RUN escalate step %s on %s", step, issue.html_url) + return + _logger.info("escalate step %s on %s", step, issue.html_url) + gh_call(issue.issue.create_comment, body) diff --git a/src/oca_github_bot/tasks/review_gate.py b/src/oca_github_bot/tasks/review_gate.py new file mode 100644 index 00000000..71a03465 --- /dev/null +++ b/src/oca_github_bot/tasks/review_gate.py @@ -0,0 +1,80 @@ +# Copyright 2026 Maxime Chambreuil - Gray Matter Logic +# Distributed under the MIT License (http://opensource.org/licenses/MIT). + +from .. import github +from ..config import REVIEW_GATE_AS_FIRST_FILTER, switchable +from ..github import gh_call +from ..manifest import ( + addon_dirs_in, + get_addon_name, + git_modified_addon_dirs, + is_addon_dir, +) +from ..process import check_call +from ..queue import getLogger, task +from ..review_gate import ( + COMMENT_MARKER, + LABEL_BOT_CHECKS_PASSED, + check_addon, + format_comment, + merge_results, +) +from .tag_needs_review import tag_needs_review + +_logger = getLogger(__name__) + + +def _upsert_comment(gh_issue, body): + for comment in gh_issue.comments(): + if COMMENT_MARKER in (comment.body or ""): + return gh_call(comment.edit, body) + return gh_call(gh_issue.create_comment, body) + + +@task() +@switchable("review_gate") +def review_gate(org, repo, pr, dry_run=False): + """Run mechanical checks and comment on the PR (not a human review).""" + with github.login() as gh: + gh_pr = gh.pull_request(org, repo, pr) + target_branch = gh_pr.base.ref + with github.temporary_clone(org, repo, target_branch) as clonedir: + base_names = { + get_addon_name(d) + for d in addon_dirs_in(clonedir, installable_only=False) + } + pr_branch = f"tmp-pr-{pr}" + check_call( + ["git", "fetch", "origin", f"refs/pull/{pr}/head:{pr_branch}"], + cwd=clonedir, + ) + check_call(["git", "checkout", pr_branch], cwd=clonedir) + modified_addon_dirs, _, _ = git_modified_addon_dirs(clonedir, target_branch) + modified_addon_dirs = [ + d + for d in modified_addon_dirs + if is_addon_dir(d, installable_only=False) + ] + results = [] + for addon_dir in modified_addon_dirs: + name = get_addon_name(addon_dir) + results.append(check_addon(addon_dir, is_new=name not in base_names)) + result = merge_results(results) + body = format_comment(result) + gh_issue = gh_pr.issue() + labels = [label.name for label in gh_issue.labels()] + if dry_run: + _logger.info("DRY-RUN review gate on %s/%s#%s: %s", org, repo, pr, body) + return + _upsert_comment(gh_issue, body) + if result.passed: + if LABEL_BOT_CHECKS_PASSED not in labels: + gh_call(gh_issue.add_labels, LABEL_BOT_CHECKS_PASSED) + if REVIEW_GATE_AS_FIRST_FILTER: + sha = gh_pr.head.sha + gh_repo = gh.repository(org, repo) + combined = gh_call(gh_repo.combined_status, sha) + if getattr(combined, "state", None) == "success": + tag_needs_review.delay(org, pr, repo, "success") + elif LABEL_BOT_CHECKS_PASSED in labels: + gh_call(gh_issue.remove_label, LABEL_BOT_CHECKS_PASSED) diff --git a/src/oca_github_bot/tasks/tag_needs_review.py b/src/oca_github_bot/tasks/tag_needs_review.py index 337a490d..619dc5b8 100644 --- a/src/oca_github_bot/tasks/tag_needs_review.py +++ b/src/oca_github_bot/tasks/tag_needs_review.py @@ -1,8 +1,10 @@ # Distributed under the MIT License (http://opensource.org/licenses/MIT). -from ..config import switchable +from ..config import REVIEW_GATE_AS_FIRST_FILTER, switchable from ..github import gh_call, repository from ..queue import getLogger, task +from ..review_gate import LABEL_BOT_CHECKS_PASSED +from .mention_maintainer import mention_maintainer _logger = getLogger(__name__) @@ -26,7 +28,15 @@ def tag_needs_review(org, pr, repo, status, dry_run=False): gh_pr.title.lower().startswith(("wip:", "[wip]")) or LABEL_WIP in labels ) if status == "success" and not has_wip: + if REVIEW_GATE_AS_FIRST_FILTER and LABEL_BOT_CHECKS_PASSED not in labels: + _logger.info( + "skip %s: mechanical checks have not passed yet", LABEL_NEEDS_REVIEW + ) + return + already = LABEL_NEEDS_REVIEW in labels if dry_run: _logger.info(f"DRY-RUN add {LABEL_NEEDS_REVIEW} label") - else: + elif not already: gh_call(gh_issue.add_labels, LABEL_NEEDS_REVIEW) + if REVIEW_GATE_AS_FIRST_FILTER and not already: + mention_maintainer.delay(org, repo, pr) diff --git a/src/oca_github_bot/tasks/tag_ready_to_merge.py b/src/oca_github_bot/tasks/tag_ready_to_merge.py index 25e199c0..5c7e12a1 100644 --- a/src/oca_github_bot/tasks/tag_ready_to_merge.py +++ b/src/oca_github_bot/tasks/tag_ready_to_merge.py @@ -17,7 +17,8 @@ "has been created more than 5 days ago. " "It should therefore be ready to merge by a maintainer " "(or a PSC member if the concerned addon has " - "no declared maintainer). 🤖" + "no declared maintainer). 🤖\n\n" + "Who to ping: https://oca.github.io/repo-maintainer-conf/repos.html" ) diff --git a/src/oca_github_bot/webhooks/__init__.py b/src/oca_github_bot/webhooks/__init__.py index 8a2d6e1f..7362575a 100644 --- a/src/oca_github_bot/webhooks/__init__.py +++ b/src/oca_github_bot/webhooks/__init__.py @@ -9,6 +9,7 @@ on_pr_open_label_new_contributor, on_pr_open_mention_maintainer, on_pr_review, + on_pr_review_gate, on_push_to_main_branch, on_status_merge_bot, ) diff --git a/src/oca_github_bot/webhooks/on_pr_open_mention_maintainer.py b/src/oca_github_bot/webhooks/on_pr_open_mention_maintainer.py index 2bca46bf..11cd284f 100644 --- a/src/oca_github_bot/webhooks/on_pr_open_mention_maintainer.py +++ b/src/oca_github_bot/webhooks/on_pr_open_mention_maintainer.py @@ -3,6 +3,7 @@ import logging +from ..config import REVIEW_GATE_AS_FIRST_FILTER from ..router import router from ..tasks.mention_maintainer import mention_maintainer @@ -15,6 +16,9 @@ async def on_pr_open_mention_maintainer(event, *args, **kwargs): """ Whenever a PR is opened, mention the maintainers of modified addons. """ + if REVIEW_GATE_AS_FIRST_FILTER: + # Mentions wait until mechanical checks + CI are green. + return org, repo = event.data["repository"]["full_name"].split("/") pr = event.data["pull_request"]["number"] mention_maintainer.delay(org, repo, pr) diff --git a/src/oca_github_bot/webhooks/on_pr_review_gate.py b/src/oca_github_bot/webhooks/on_pr_review_gate.py new file mode 100644 index 00000000..bf1d1fab --- /dev/null +++ b/src/oca_github_bot/webhooks/on_pr_review_gate.py @@ -0,0 +1,15 @@ +# Copyright 2026 Maxime Chambreuil - Gray Matter Logic +# Distributed under the MIT License (http://opensource.org/licenses/MIT). + +from ..router import router +from ..tasks.review_gate import review_gate + + +@router.register("pull_request", action="opened") +@router.register("pull_request", action="reopened") +@router.register("pull_request", action="synchronize") +async def on_pr_review_gate(event, *args, **kwargs): + """Run mechanical checks whenever the PR is opened or updated.""" + org, repo = event.data["repository"]["full_name"].split("/") + pr = event.data["pull_request"]["number"] + review_gate.delay(org, repo, pr) diff --git a/tests/test_on_pr_review_gate.py b/tests/test_on_pr_review_gate.py new file mode 100644 index 00000000..495a9363 --- /dev/null +++ b/tests/test_on_pr_review_gate.py @@ -0,0 +1,21 @@ +# Copyright 2026 Maxime Chambreuil - Gray Matter Logic +# Distributed under the MIT License (http://opensource.org/licenses/MIT). + +import pytest + +from oca_github_bot.webhooks import on_pr_review_gate + +from .common import EventMock + + +@pytest.mark.asyncio +async def test_on_pr_review_gate(mocker): + mocker.patch("oca_github_bot.webhooks.on_pr_review_gate.review_gate.delay") + event = EventMock( + data={ + "repository": {"full_name": "OCA/some-repo"}, + "pull_request": {"number": 12}, + } + ) + await on_pr_review_gate.on_pr_review_gate(event) + on_pr_review_gate.review_gate.delay.assert_called_once_with("OCA", "some-repo", 12) diff --git a/tests/test_review_gate.py b/tests/test_review_gate.py new file mode 100644 index 00000000..beff852d --- /dev/null +++ b/tests/test_review_gate.py @@ -0,0 +1,96 @@ +# Copyright 2026 Maxime Chambreuil - Gray Matter Logic +# Distributed under the MIT License (http://opensource.org/licenses/MIT). + +from datetime import datetime, timedelta, timezone +from pathlib import Path + +from oca_github_bot.review_gate import check_addon, format_comment, merge_results +from oca_github_bot.tasks.escalate_review import ( + STEP_MARKERS, + escalate_comment, + next_escalate_step, +) + +from .common import make_addon + + +def test_missing_development_status_is_blocking(git_clone): + addon_dir = make_addon(git_clone, "addon_alpha") + result = check_addon(addon_dir, is_new=False) + assert result.passed is False + assert any("development_status" in item for item in result.blocking) + + +def test_alpha_existing_addon_passes(git_clone): + addon_dir = make_addon(git_clone, "addon_alpha", development_status="Alpha") + result = check_addon(addon_dir, is_new=False) + assert result.passed is True + assert result.blocking == [] + + +def test_stable_addon_is_warning_not_blocking(git_clone): + addon_dir = make_addon( + git_clone, "addon_stable", development_status="Production/Stable" + ) + result = check_addon(addon_dir, is_new=False) + assert result.passed is True + assert any("Production/Stable" in item for item in result.warnings) + + +def test_new_addon_models_without_acl_is_blocking(git_clone): + addon_dir = Path(make_addon(git_clone, "addon_new", development_status="Alpha")) + models = addon_dir / "models" + models.mkdir() + (models / "__init__.py").write_text("from . import res_partner\n") + (models / "res_partner.py").write_text( + "from odoo import models\n\n" + "class ResPartner(models.Model):\n" + " _name = 'res.partner.extra'\n" + ) + result = check_addon(str(addon_dir), is_new=True) + assert result.passed is False + assert any("ir.model.access.csv" in item for item in result.blocking) + + +def test_new_addon_with_acl_passes(git_clone): + addon_dir = Path(make_addon(git_clone, "addon_new", development_status="Alpha")) + models = addon_dir / "models" + models.mkdir() + (models / "res_partner.py").write_text("_name = 'x.y'\n") + security = addon_dir / "security" + security.mkdir() + (security / "ir.model.access.csv").write_text("id,name,model_id:id,group_id:id\n") + result = check_addon(str(addon_dir), is_new=True) + assert result.passed is True + assert any("new addon" in item for item in result.warnings) + + +def test_format_comment_contains_marker(): + result = merge_results([]) + body = format_comment(result) + assert "ocabot-review-gate" in body + assert "not a review" in body.lower() or "volume filter" in body.lower() + + +def test_escalate_steps_ladder(): + now = datetime(2026, 9, 30, tzinfo=timezone.utc) + created = now - timedelta(days=8) + assert next_escalate_step(created, now, []) == 0 + assert next_escalate_step(created, now, [STEP_MARKERS[0]]) is None + created14 = now - timedelta(days=15) + assert next_escalate_step(created14, now, [STEP_MARKERS[0]]) == 1 + created21 = now - timedelta(days=22) + assert ( + next_escalate_step( + created21, now, [STEP_MARKERS[0], STEP_MARKERS[1]] + ) + == 2 + ) + assert next_escalate_step(now - timedelta(days=2), now, []) is None + + +def test_escalate_comment_does_not_claim_merge(): + body = escalate_comment("field-service", 0) + assert "ocabot-escalate:1" in body + assert "will not merge" in body.lower() or "not a merge" in body.lower() + assert "/ocabot merge" in escalate_comment("field-service", 2) From cd0c2aa4e1be954d9ea5dd2aa550d1fecdfefc2f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 30 Sep 2026 16:57:17 +0000 Subject: [PATCH 2/3] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_review_gate.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/test_review_gate.py b/tests/test_review_gate.py index beff852d..64ced597 100644 --- a/tests/test_review_gate.py +++ b/tests/test_review_gate.py @@ -80,12 +80,7 @@ def test_escalate_steps_ladder(): created14 = now - timedelta(days=15) assert next_escalate_step(created14, now, [STEP_MARKERS[0]]) == 1 created21 = now - timedelta(days=22) - assert ( - next_escalate_step( - created21, now, [STEP_MARKERS[0], STEP_MARKERS[1]] - ) - == 2 - ) + assert next_escalate_step(created21, now, [STEP_MARKERS[0], STEP_MARKERS[1]]) == 2 assert next_escalate_step(now - timedelta(days=2), now, []) is None From 8c2f36c13e67b23d7b5653ba2c5dc906aae9e92c Mon Sep 17 00:00:00 2001 From: Maxime Chambreuil Date: Wed, 30 Sep 2026 18:33:08 +0000 Subject: [PATCH 3/3] [IMP] drop silent-PR escalation pings Holger Brunn (contributors@, 2026-09-30): silence can mean the PR is uninteresting; do not add a 7/14/21 ping ladder. Keep the mechanical review gate and the ready-to-merge PSC link. Restore Tecnativa credits dropped in the previous README edit. --- README.rst | 7 +- environment.sample | 2 +- newsfragments/+review-gate.feature | 2 +- src/oca_github_bot/config.py | 2 +- src/oca_github_bot/cron.py | 5 - src/oca_github_bot/tasks/__init__.py | 1 - src/oca_github_bot/tasks/escalate_review.py | 152 -------------------- tests/test_review_gate.py | 25 ---- 8 files changed, 7 insertions(+), 189 deletions(-) delete mode 100644 src/oca_github_bot/tasks/escalate_review.py diff --git a/README.rst b/README.rst index 3fd8fdf4..3a151181 100644 --- a/README.rst +++ b/README.rst @@ -51,9 +51,8 @@ unless it has ``wip:`` or ``[wip]`` in it's title. If the first-filter is enabled, also require ``bot checks passed``. -If a PR stays on ``needs review`` with no human review, a daily job escalates -who is asked (7 days: repo maintainer team, 14 days: PSC link, -21 days: core maintainers). Silence does **not** grant ``/ocabot merge``. +Silence is not treated as a request to ping more people, and never grants +``/ocabot merge``. On pull request close @@ -250,6 +249,8 @@ Contributors * Jose Angel Fentanez * Simone Rubino * Sylvain Le Gal (https://twitter.com/legalsylvain) +* Tecnativa - Pedro M. Baeza +* Tecnativa - Víctor Martínez * Maxime Chambreuil Maintainers diff --git a/environment.sample b/environment.sample index 4ff3af58..0110ae2d 100644 --- a/environment.sample +++ b/environment.sample @@ -47,7 +47,7 @@ ODOO_PASSWORD= # delete_branch,tag_approved,tag_ready_to_merge,gen_addons_table, # gen_addons_readme,gen_addons_icon,setuptools_odoo,mention_maintainer, # merge_bot,rebase_bot,merge_bot_towncrier,tag_needs_review, -# review_gate,escalate_review +# review_gate #BOT_TASKS=all # Coma separated list of task to ignore diff --git a/newsfragments/+review-gate.feature b/newsfragments/+review-gate.feature index ba3844f5..fdae6c52 100644 --- a/newsfragments/+review-gate.feature +++ b/newsfragments/+review-gate.feature @@ -1 +1 @@ -Add a mechanical review gate (volume filter, not a human review) and escalate silent PRs to a wider human set without merging. +Add a mechanical review gate (volume filter, not a human review). Silence is not escalated into extra reviewer pings, and the bot never merges. diff --git a/src/oca_github_bot/config.py b/src/oca_github_bot/config.py index ffdeec5b..b6884fc6 100644 --- a/src/oca_github_bot/config.py +++ b/src/oca_github_bot/config.py @@ -60,7 +60,7 @@ def func_wrapper(*args, **kwargs): # delete_branch,tag_approved,tag_ready_to_merge,gen_addons_table, # gen_addons_readme,gen_addons_icon,setuptools_odoo,merge_bot,tag_needs_review, # migration_issue_bot,whool_init,gen_metapackage,label_modified_addons, -# review_gate,escalate_review +# review_gate BOT_TASKS = os.environ.get("BOT_TASKS", "all").split(",") BOT_TASKS_DISABLED = os.environ.get("BOT_TASKS_DISABLED", "").split(",") diff --git a/src/oca_github_bot/cron.py b/src/oca_github_bot/cron.py index 5021ecd5..708722b3 100644 --- a/src/oca_github_bot/cron.py +++ b/src/oca_github_bot/cron.py @@ -28,11 +28,6 @@ "args": (org,), "schedule": crontab(minute="0"), }, - "escalate_review": { - "task": "oca_github_bot.tasks.escalate_review.escalate_review", - "args": (org,), - "schedule": crontab(hour="8", minute="0"), - }, } ) diff --git a/src/oca_github_bot/tasks/__init__.py b/src/oca_github_bot/tasks/__init__.py index 610416c3..4db9ba8b 100644 --- a/src/oca_github_bot/tasks/__init__.py +++ b/src/oca_github_bot/tasks/__init__.py @@ -2,7 +2,6 @@ # Distributed under the MIT License (http://opensource.org/licenses/MIT). from . import ( - escalate_review, heartbeat, label_modified_addons, main_branch_bot, diff --git a/src/oca_github_bot/tasks/escalate_review.py b/src/oca_github_bot/tasks/escalate_review.py deleted file mode 100644 index dd8a686f..00000000 --- a/src/oca_github_bot/tasks/escalate_review.py +++ /dev/null @@ -1,152 +0,0 @@ -# Copyright 2026 Maxime Chambreuil - Gray Matter Logic -# Distributed under the MIT License (http://opensource.org/licenses/MIT). - -"""Escalate silent PRs: ask a wider human set, never merge.""" - -from datetime import datetime, timezone - -from .. import github -from ..config import GITHUB_LOGIN, GITHUB_ORG, switchable -from ..github import gh_call -from ..queue import getLogger, task - -_logger = getLogger(__name__) - -BOT_LOGINS = { - (GITHUB_LOGIN or "").lower(), - "oca-git-bot", - "github-actions[bot]", - "codecov", - "codecov-commenter", - "pre-commit-ci[bot]", -} - -STEP_MARKERS = ( - "", - "", - "", -) - -STEP_DELAYS = (7, 14, 21) - -PSC_LIST = "https://oca.github.io/repo-maintainer-conf/repos.html" - - -def _is_bot(login): - if not login: - return True - low = login.lower() - return low in BOT_LOGINS or low.endswith("[bot]") - - -def _step_done(bodies, step): - marker = STEP_MARKERS[step] - return any(marker in (body or "") for body in bodies) - - -def next_escalate_step(last_human_at, now, comments_bodies): - """Return 0, 1 or 2 for the next ping, or None if none is due.""" - if last_human_at is None: - return None - elapsed = (now - last_human_at).total_seconds() / 86400 - for step, days in enumerate(STEP_DELAYS): - if elapsed >= days and not _step_done(comments_bodies, step): - return step - return None - - -def escalate_comment(repo, step): - team = f"@OCA/{repo}-maintainers" - if step == 0: - return ( - f"{STEP_MARKERS[0]}\n" - f"No human review yet after {STEP_DELAYS[0]} days " - f"(mechanical checks / `needs review`).\n\n" - f"Pinging the repo maintainer team: {team}\n" - "This is not a merge and does not count as a review." - ) - if step == 1: - return ( - f"{STEP_MARKERS[1]}\n" - f"Still no human review after {STEP_DELAYS[1]} days.\n\n" - "Pinging the PSC for this repository. " - f"See {PSC_LIST}\n" - f"(team guess: {team})\n" - "Please review or say if this should wait. The bot will not merge." - ) - return ( - f"{STEP_MARKERS[2]}\n" - f"Still no human review after {STEP_DELAYS[2]} days.\n\n" - "@OCA/oca-core-maintainers review wanted.\n" - "Silence escalates who is asked, not who may `/ocabot merge`." - ) - - -def _comment_dt(comment): - created = getattr(comment, "created_at", None) - if created is None: - return None - if created.tzinfo is None: - return created.replace(tzinfo=timezone.utc) - return created.astimezone(timezone.utc) - - -def _last_human_at(comments, issue_created): - last_human = None - for comment in comments: - login = comment.user.login if comment.user else None - if _is_bot(login): - continue - created = _comment_dt(comment) - if created and (last_human is None or created > last_human): - last_human = created - if last_human is not None: - return last_human - if issue_created is None: - return None - if issue_created.tzinfo is None: - return issue_created.replace(tzinfo=timezone.utc) - return issue_created.astimezone(timezone.utc) - - -def _org_repo_from_issue(issue): - full = issue.html_url # https://github.com/org/repo/pull/N - parts = full.split("/") - return parts[3], parts[4] - - -@task() -@switchable("escalate_review") -def escalate_review(org=None, dry_run=False): - """Ping a wider human set when a green PR gets no review.""" - orgs = [org] if org else list(GITHUB_ORG) - now = datetime.now(timezone.utc) - with github.login() as gh: - for org_name in orgs: - query = [ - "type:pr", - "state:open", - 'label:"needs review"', - "-label:approved", - '-label:"work in progress"', - '-label:"ready to merge"', - f"org:{org_name}", - ] - for issue in gh.search_issues(" ".join(query)): - _escalate_one(issue, now, dry_run) - - -def _escalate_one(issue, now, dry_run): - comments = list(issue.issue.comments()) - bodies = [c.body or "" for c in comments] - last_human = _last_human_at(comments, getattr(issue, "created_at", None)) - step = next_escalate_step(last_human, now, bodies) - if step is None: - return - _org_name, repo_name = _org_repo_from_issue(issue) - body = escalate_comment(repo_name, step) - if dry_run: - _logger.info("DRY-RUN escalate step %s on %s", step, issue.html_url) - return - _logger.info("escalate step %s on %s", step, issue.html_url) - gh_call(issue.issue.create_comment, body) diff --git a/tests/test_review_gate.py b/tests/test_review_gate.py index 64ced597..86fc4bec 100644 --- a/tests/test_review_gate.py +++ b/tests/test_review_gate.py @@ -1,15 +1,9 @@ # Copyright 2026 Maxime Chambreuil - Gray Matter Logic # Distributed under the MIT License (http://opensource.org/licenses/MIT). -from datetime import datetime, timedelta, timezone from pathlib import Path from oca_github_bot.review_gate import check_addon, format_comment, merge_results -from oca_github_bot.tasks.escalate_review import ( - STEP_MARKERS, - escalate_comment, - next_escalate_step, -) from .common import make_addon @@ -70,22 +64,3 @@ def test_format_comment_contains_marker(): body = format_comment(result) assert "ocabot-review-gate" in body assert "not a review" in body.lower() or "volume filter" in body.lower() - - -def test_escalate_steps_ladder(): - now = datetime(2026, 9, 30, tzinfo=timezone.utc) - created = now - timedelta(days=8) - assert next_escalate_step(created, now, []) == 0 - assert next_escalate_step(created, now, [STEP_MARKERS[0]]) is None - created14 = now - timedelta(days=15) - assert next_escalate_step(created14, now, [STEP_MARKERS[0]]) == 1 - created21 = now - timedelta(days=22) - assert next_escalate_step(created21, now, [STEP_MARKERS[0], STEP_MARKERS[1]]) == 2 - assert next_escalate_step(now - timedelta(days=2), now, []) is None - - -def test_escalate_comment_does_not_claim_merge(): - body = escalate_comment("field-service", 0) - assert "ocabot-escalate:1" in body - assert "will not merge" in body.lower() or "not a merge" in body.lower() - assert "/ocabot merge" in escalate_comment("field-service", 2)