diff --git a/README.rst b/README.rst index ad32f72b..3a151181 100644 --- a/README.rst +++ b/README.rst @@ -33,6 +33,28 @@ 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``. + +Silence is not treated as a request to ping more people, and never grants +``/ocabot merge``. + + On pull request close --------------------- @@ -68,12 +90,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 -------- @@ -233,6 +251,7 @@ Contributors * 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..0110ae2d 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 #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..fdae6c52 --- /dev/null +++ b/newsfragments/+review-gate.feature @@ -0,0 +1 @@ +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 f2a03d3f..b6884fc6 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 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/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..4db9ba8b 100644 --- a/src/oca_github_bot/tasks/__init__.py +++ b/src/oca_github_bot/tasks/__init__.py @@ -7,6 +7,7 @@ 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/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..86fc4bec --- /dev/null +++ b/tests/test_review_gate.py @@ -0,0 +1,66 @@ +# Copyright 2026 Maxime Chambreuil - Gray Matter Logic +# Distributed under the MIT License (http://opensource.org/licenses/MIT). + +from pathlib import Path + +from oca_github_bot.review_gate import check_addon, format_comment, merge_results + +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()