From 5c1d848311dfd0332c104a2ce92b931826561899 Mon Sep 17 00:00:00 2001 From: emil Date: Sun, 21 Jun 2026 06:02:50 +0000 Subject: [PATCH] GRM-33: feat: add mandatory PR review step to workflow --- AGENTS.md | 94 ++++++++++++- CHANGELOG.md | 11 ++ README.md | 24 +++- scripts/auto_merge.py | 68 ++++++++-- scripts/post_merge.py | 9 +- scripts/review_pr.py | 140 +++++++++++++++++++ src/gitea_runner_manager/api_clients.py | 36 +++++ tests/unit/test_api_clients.py | 72 ++++++++++ tests/unit/test_auto_merge.py | 130 +++++++++++++++--- tests/unit/test_post_merge.py | 7 +- tests/unit/test_review_pr.py | 170 ++++++++++++++++++++++++ 11 files changed, 726 insertions(+), 35 deletions(-) create mode 100644 scripts/review_pr.py create mode 100644 tests/unit/test_review_pr.py diff --git a/AGENTS.md b/AGENTS.md index 700d044..431a7dd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,17 +16,103 @@ make test-all # pytest-cov + molecule - **Python CLI** (`src/gitea_runner_manager/`) — Click-based CLI that delegates to Ansible - **Ansible Role** (`ansible/roles/gitea-runner/`) — Idempotent role for rootless Docker runner setup -- **CI Scripts** (`scripts/`) — Automation for auto-merge, post-merge, publishing, molecule distribution +- **CI Scripts** (`scripts/`) — Automation for auto-merge, post-merge, publishing, molecule distribution, PR reviews + +## PR Workflow (Mandatory) + +Every change to master goes through this workflow. No exceptions. + +### 1. Create Vikunja Task +Create a task in Vikunja project 6 to get a `GRM-N` identifier. + +### 2. Create Branch +```bash +git checkout master && git pull +git checkout -b GRM-N-short-description +``` + +### 3. Implement Changes +- Write code following conventions below +- Write/update tests (100% coverage required) +- Update documentation (CHANGELOG, README, AGENTS.md as needed) + +### 4. Commit (Conventional Commits) +Branch commits use conventional commit format (no `GRM-N:` prefix): +``` +feat: add new feature +fix: resolve bug +docs: update README +``` + +### 5. Push and Create PR +- **PR title format**: `GRM-N: ` (colon-separated) +- PR body: summary of changes, `Closes GRM-N` +- Add `ready-to-merge` label **only after review is complete** + +### 6. Review the PR (Mandatory — Before Adding ready-to-merge Label) +Review the full diff (`git diff master...HEAD`) focusing on: + +- **Functional completeness**: Does the code do what it claims? Are all requirements met? +- **Edge cases**: Are boundary conditions, empty inputs, error paths handled? +- **Technical excellence**: + - Architecture compliance and evolution + - Single Responsibility Principle (SRP) + - Deduplication (no copy-paste, single source of truth) + - Code smells detection and removal + - Best industry practices + - Industry-grade code quality + - Reusability + - Clean code + - Readability + - Maintainability + - Extensibility +- **Performance**: No unnecessary allocations, O(n) vs O(n²), efficient data structures +- **Security**: No secrets in logs/process list, input validation, no injection vectors +- **User experience**: Clear error messages, intuitive CLI flags, helpful output +- **Documentation**: Completeness and relevance of docs, CHANGELOG entries, AGENTS.md updates + +Post review comments using `scripts/review_pr.py`: +```bash +REPO_TOKEN= python3 scripts/review_pr.py \ + --event REQUEST_CHANGES \ + --body "Review summary" \ + --comments-json comments.json +``` + +### 7. Address Review Comments +Fix each comment one by one, commit, and push. Re-review until satisfied. + +### 8. Approve and Merge +Once all comments are addressed: +```bash +REPO_TOKEN= python3 scripts/review_pr.py \ + --event APPROVE \ + --body "All comments addressed. LGTM." +``` + +Then add the `ready-to-merge` label. The auto-merge workflow will: +1. Wait for all CI checks to pass +2. Squash-merge with title: `GRM-N ` (space-separated) +3. The post-merge workflow marks the Vikunja task as done + +### Title Format Summary + +| What | Format | Example | +|------|--------|---------| +| Branch name | `GRM-N-short-description` | `GRM-33-add-pr-review-step` | +| Branch commits | `` | `feat: add review script` | +| PR title | `GRM-N: ` | `GRM-33: Add mandatory PR review step` | +| Merge commit | `GRM-N ` | `GRM-33 feat: add review script` | ## Key Conventions - Python 3.12+ required (ruff/pyright target `py312`) - 100% test coverage required (`--cov-fail-under=100`) - Conventional commits on feature branches (no `GRM-N:` prefix) -- `GRM-N:` prefix on master branch (added by auto-merge) - Branch names must include `GRM-N` task ID - Line length: 120 chars - Secrets are passed via temp JSON files, never on the command line (CWE-214) +- CI triggers only on `opened` and `synchronize` PR events (not `labeled`) ## Ansible Role Structure @@ -36,8 +122,8 @@ main.yml → systemd_check → user_setup → rootless_docker → install_runner - `install_runner.yml` handles: download, config, validate, register, service - `main.yml` handles: prune, integration_test (NOT install_runner — avoids duplicates) -- All `systemctl --user` tasks must be guarded by `docker_rootless_setup` -- All template creation tasks must be guarded by `docker_rootless_setup` +- `systemctl --user` tasks must be guarded by `docker_rootless_setup` +- Template creation tasks are NOT guarded by `docker_rootless_setup` (they just create files) ## Molecule Scenarios diff --git a/CHANGELOG.md b/CHANGELOG.md index d3cedc4..bde6616 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,17 @@ All notable changes to this project will be documented in this file. ### Added +- **Mandatory PR review step**: `scripts/review_pr.py` — CLI to post Gitea PR reviews (COMMENT, APPROVE, REQUEST_CHANGES) with inline comments via `--comments-json` or `--comments-stdin`. +- `GiteaClient.get_pr_files`, `GiteaClient.get_pr_commits`, `GiteaClient.create_review` — API methods for PR review workflow. +- `VikunjaClient.get_task` — fetch a single task by numeric ID. +- PR title format: `GRM-N: ` (colon-separated, human-friendly). +- Merge commit format: `GRM-N ` (space-separated, conventional). +- `auto_merge.py` now extracts the conventional commit message from PR commits and constructs the merge title as `GRM-N `. +- `post_merge.py` `extract_conventional_msg` now handles both legacy (`GRM-N: `) and current (`GRM-N `) merge commit formats. +- Full PR workflow documented in `AGENTS.md` and `README.md` (Vikunja task → branch → implement → commit → PR → review → address comments → approve → merge). + +### Changed + - Parameterized all hardcoded configuration values as Ansible variables in `defaults/main.yml`: - `gitea_runner_data_dir` — Runtime data directory - `gitea_runner_config_dir` — Config directory diff --git a/README.md b/README.md index 8c9ce58..1dc8896 100644 --- a/README.md +++ b/README.md @@ -10,10 +10,28 @@ Each runner runs in an isolated **rootless Docker** environment under a dedicate ## Commit Convention & Branch Naming -This project uses **conventional commits** and **GRM-N branch prefixes**. See [CONTRIBUTING.md](CONTRIBUTING.md) for details. +This project uses **conventional commits** and **GRM-N branch prefixes**. See [AGENTS.md](AGENTS.md) for the full workflow. -- Branches: `GRM-N` or `GRM-N-brief-description` (required for CI automation) -- Commits: `feat:`, `fix:`, `chore:`, etc. (no `GRM-N:` prefix on feature branches) +| What | Format | Example | +|------|--------|---------| +| Branch name | `GRM-N-short-description` | `GRM-33-add-pr-review-step` | +| Branch commits | `` | `feat: add review script` | +| PR title | `GRM-N: ` | `GRM-33: Add mandatory PR review step` | +| Merge commit | `GRM-N ` | `GRM-33 feat: add review script` | + +### PR Workflow + +Every change to master goes through a mandatory review workflow: + +1. **Create Vikunja task** — get a `GRM-N` identifier +2. **Create branch** — `GRM-N-short-description` +3. **Implement** — write code, tests (100% coverage), update docs +4. **Commit** — conventional commits (no `GRM-N:` prefix on branch) +5. **Push & create PR** — title: `GRM-N: ` +6. **Review** — review the full diff focusing on: functional completeness, edge cases, technical excellence (architecture, SRP, deduplication, code smells, best practices, code quality, reusability, clean code, readability, maintainability, extensibility), performance, security, UX, documentation completeness/relevance. Post review comments via `scripts/review_pr.py`. +7. **Address comments** — fix each comment, commit, push, re-review +8. **Approve** — post an `APPROVE` review via `scripts/review_pr.py` +9. **Add `ready-to-merge` label** — auto-merge workflow squash-merges with title `GRM-N `, post-merge workflow marks the Vikunja task as done ## Features diff --git a/scripts/auto_merge.py b/scripts/auto_merge.py index 2d2b1b9..1152125 100644 --- a/scripts/auto_merge.py +++ b/scripts/auto_merge.py @@ -1,14 +1,23 @@ #!/usr/bin/env python3 -"""Auto-merge PR by extracting task ID from branch and validating PR title. +"""Auto-merge PR by extracting task ID from branch and constructing merge title. Waits for CI checks to complete before attempting the merge. +PR title format: ``GRM-N: `` +Merge commit format: ``GRM-N `` + +The conventional commit message is taken from the first commit on the PR +branch (the branch HEAD). This allows the PR title to be a human-friendly +Vikunja task title while the squashed commit follows conventional commits. + Usage: REPO_TOKEN= python3 scripts/auto_merge.py [label_name] """ import os +import re import time +from typing import Any import click from dotenv import load_dotenv # pyright: ignore[reportMissingImports,reportUnknownVariableType] @@ -22,6 +31,9 @@ READY_TO_MERGE = "ready-to-merge" MAX_WAIT_SECONDS = 900 # 15 minutes POLL_INTERVAL_SECONDS = 30 +# PR title: GRM-N: +PR_TITLE_RE = re.compile(r"^GRM-\d+:\s+.+") + load_dotenv(override=True) @@ -31,17 +43,50 @@ def extract_task_id(branch: str) -> str: return match.group(0) if match else "" -def validate_pr_title(pr_title: str) -> None: - """Raise ClickException if PR title does not follow conventional commits.""" - if not CONVENTIONAL_RE.match(pr_title): +def validate_pr_title(pr_title: str, task_id: str) -> None: + """Raise ClickException if PR title does not follow the required format. + + Expected: ``GRM-N: `` + """ + if not PR_TITLE_RE.match(pr_title): raise click.ClickException( _( - "Oops! PR title must follow conventional commit format.\n" - " Expected: : \n" + "Oops! PR title must follow format 'GRM-N: '.\n" + " Expected: {task_id}: \n" " Got: {pr_title}", + task_id=task_id, pr_title=pr_title, ) ) + if not pr_title.startswith(f"{task_id}:"): + raise click.ClickException( + _( + "Oops! PR title task ID mismatch.\n" + " Branch task ID: {task_id}\n" + " PR title: {pr_title}", + task_id=task_id, + pr_title=pr_title, + ) + ) + + +def extract_conventional_msg(commits: list[dict[str, Any]]) -> str: + """Extract the conventional commit message from PR commits. + + Iterates commits in reverse order (newest first) to find the first + message matching the conventional commit format. Falls back to the + newest commit message if none match. + """ + for commit in reversed(commits): + commit_info = commit.get("commit", {}) + message = str(commit_info.get("message", "") if isinstance(commit_info, dict) else "").split("\n")[0] + if CONVENTIONAL_RE.match(message): + return message + # Fallback: use the newest commit's first line + if commits: + commit_info = commits[-1].get("commit", {}) + return str(commit_info.get("message", "") if isinstance(commit_info, dict) else "").split("\n")[0] + return "" def has_ready_to_merge_label(client: GiteaClient, pr_number: str) -> bool: @@ -138,7 +183,7 @@ def main(branch: str, pr_title: str, repo: str, pr_number: str, label_name: str) ) ) - validate_pr_title(pr_title) + validate_pr_title(pr_title, task_id) # Wait for CI checks to complete before attempting merge. pr = client.get_pr(pr_number) @@ -152,7 +197,14 @@ def main(branch: str, pr_title: str, repo: str, pr_number: str, label_name: str) else: click.echo(_("Warning: could not determine PR head SHA, proceeding without CI wait.")) - merge_title = f"{task_id}: {pr_title}" + # Build merge title: GRM-N + commits = client.get_pr_commits(pr_number) + conv_msg = extract_conventional_msg(commits) + if not conv_msg: + raise click.ClickException( + _("Could not extract conventional commit message from PR commits.") + ) + merge_title = f"{task_id} {conv_msg}" try: client.merge_pr(pr_number, merge_title) diff --git a/scripts/post_merge.py b/scripts/post_merge.py index b7f8d62..e761611 100644 --- a/scripts/post_merge.py +++ b/scripts/post_merge.py @@ -27,9 +27,14 @@ def extract_task_id(commit_msg: str) -> str: def extract_conventional_msg(commit_msg: str) -> str: - """Strip the GRM-N prefix from the commit subject.""" + """Strip the GRM-N prefix from the commit subject. + + Handles both formats: + - ``GRM-N: `` (legacy, colon-separated) + - ``GRM-N `` (current, space-separated) + """ first_line = commit_msg.split("\n")[0] - return re.sub(r"^GRM-\d+:\s*", "", first_line) + return re.sub(r"^GRM-\d+[:\s]\s*", "", first_line) def resolve_task_id(client: VikunjaClient, task_id: str) -> int: diff --git a/scripts/review_pr.py b/scripts/review_pr.py new file mode 100644 index 0000000..6329dcb --- /dev/null +++ b/scripts/review_pr.py @@ -0,0 +1,140 @@ +#!/usr/bin/env python3 +"""Post a review on a Gitea pull request. + +Used by the GRM workflow to post structured PR reviews. The review body +is provided via --body and inline comments via a JSON file +(--comments-json) or stdin (--comments-stdin). This script is a thin +CLI wrapper around ``GiteaClient.create_review`` — the actual review +analysis is performed by the agent before invoking this tool. + +Usage: + REPO_TOKEN= python3 scripts/review_pr.py \ + --event COMMENT \ + --body "Review body text" \ + --comments-json comments.json + +The comments JSON file is a list of objects with keys: + - path: file path in the repo + - body: comment text + - new_position: line number in the new file (1-based) + - old_position: (optional) line number in the old file + +Review focus areas (for the reviewer, not enforced by this script): + - Functional completeness + - Edge cases + - Technical excellence: architecture compliance, SRP, deduplication, + code smells, best practices, code quality, reusability, clean code, + readability, maintainability, extensibility + - Performance + - Security + - User experience + - Documentation completeness and relevance +""" + +from __future__ import annotations + +import json +import os +import sys +from typing import Any + +import click +from dotenv import load_dotenv # pyright: ignore[reportMissingImports,reportUnknownVariableType] + +from gitea_runner_manager.api_clients import GiteaClient +from gitea_runner_manager.config import GITEA_API_URL +from gitea_runner_manager.exceptions import APIError +from gitea_runner_manager.i18n import _ + +load_dotenv(override=True) + +VALID_EVENTS = ("APPROVE", "REQUEST_CHANGES", "COMMENT") + + +def parse_comments(comments_json: str | None, comments_stdin: bool) -> list[dict[str, Any]]: + """Parse inline comments from a JSON file or stdin.""" + if comments_json: + try: + with open(comments_json) as f: + data = json.load(f) + except json.JSONDecodeError as e: + raise click.ClickException(_("Invalid JSON in comments file: {error}", error=str(e))) from None + if not isinstance(data, list): + raise click.ClickException(_("Comments JSON must be a list of objects.")) + return data + if comments_stdin: + raw = sys.stdin.read().strip() + if not raw: + return [] + try: + data = json.loads(raw) + except json.JSONDecodeError as e: + raise click.ClickException(_("Invalid JSON on stdin: {error}", error=str(e))) from None + if not isinstance(data, list): + raise click.ClickException(_("Stdin comments JSON must be a list of objects.")) + return data + return [] + + +@click.command() +@click.argument("pr_number") +@click.argument("repo") +@click.option( + "--event", + default="COMMENT", + type=click.Choice(VALID_EVENTS), + help="Review event type: APPROVE, REQUEST_CHANGES, or COMMENT.", +) +@click.option("--body", default="", help="Top-level review body text.") +@click.option("--comments-json", default=None, help="Path to JSON file with inline comments.") +@click.option( + "--comments-stdin", + is_flag=True, + default=False, + help="Read inline comments JSON from stdin.", +) +def main( + pr_number: str, + repo: str, + event: str, + body: str, + comments_json: str | None, + comments_stdin: bool, +) -> None: + token = os.environ.get("REPO_TOKEN", "") + if not token: + raise click.ClickException(_("ERROR: REPO_TOKEN is not set.")) + + owner, repo_name = repo.split("/") + client = GiteaClient(GITEA_API_URL, token, owner, repo_name) + + comments = parse_comments(comments_json, comments_stdin) + + if event != "APPROVE" and not body and not comments: + raise click.ClickException(_("Review body or inline comments are required for event '{event}'.", event=event)) + + try: + review = client.create_review(pr_number, event=event, body=body, comments=comments) + except APIError as e: + raise click.ClickException( + _( + "Failed to post review: HTTP {status} — {message}", + status=e.status, + message=e.message, + ) + ) from None + + review_id = review.get("id", "?") + click.echo( + _( + "Review #{review_id} posted on PR #{pr_number} with event '{event}' ({num_comments} inline comments).", + review_id=review_id, + pr_number=pr_number, + event=event, + num_comments=len(comments), + ) + ) + + +if __name__ == "__main__": # pragma: no cover + main() diff --git a/src/gitea_runner_manager/api_clients.py b/src/gitea_runner_manager/api_clients.py index 36e6fa7..54e7920 100644 --- a/src/gitea_runner_manager/api_clients.py +++ b/src/gitea_runner_manager/api_clients.py @@ -126,6 +126,37 @@ class GiteaClient: r = self._request("GET", f"/pulls/{pr_number}") return r.json() + def get_pr_files(self, pr_number: str | int) -> list[dict[str, Any]]: + """Fetch the list of files changed in a pull request.""" + r = self._request("GET", f"/pulls/{pr_number}/files") + return r.json() + + def get_pr_commits(self, pr_number: str | int) -> list[dict[str, Any]]: + """Fetch the commits included in a pull request.""" + r = self._request("GET", f"/pulls/{pr_number}/commits") + return r.json() + + def create_review( + self, + pr_number: str | int, + event: str = "COMMENT", + body: str = "", + comments: list[dict[str, Any]] | None = None, + ) -> dict[str, Any]: + """Post a review on a pull request. + + Args: + event: ``APPROVE``, ``REQUEST_CHANGES``, or ``COMMENT``. + body: Top-level review body text. + comments: Line-level comments with ``path``, ``body``, + ``new_position`` (and optionally ``old_position``). + """ + payload: dict[str, Any] = {"event": event, "body": body} + if comments: + payload["comments"] = comments + r = self._request("POST", f"/pulls/{pr_number}/reviews", json=payload) + return r.json() + def create_release( self, tag: str, @@ -167,6 +198,11 @@ class VikunjaClient: r = self._request("GET", "/tasks", params=params) return r.json() + def get_task(self, task_id: int) -> dict[str, Any]: + """Fetch a single task by its numeric ID.""" + r = self._request("GET", f"/tasks/{task_id}") + return r.json() + def list_project_tasks(self, project_id: int, **params: Any) -> list[dict[str, Any]]: """List tasks in a specific project (more efficient than listing all tasks).""" r = self._request("GET", f"/projects/{project_id}/tasks", params=params) diff --git a/tests/unit/test_api_clients.py b/tests/unit/test_api_clients.py index 44d2482..154737b 100644 --- a/tests/unit/test_api_clients.py +++ b/tests/unit/test_api_clients.py @@ -228,6 +228,63 @@ class TestGiteaClient: timeout=DEFAULT_TIMEOUT, ) + def test_get_pr_files(self) -> None: + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + client._session.request = MagicMock( + return_value=_mock_response([{"filename": "src/main.py", "status": "modified"}]) + ) + + result = client.get_pr_files(7) + assert len(result) == 1 + assert result[0]["filename"] == "src/main.py" + client._session.request.assert_called_once_with( + "GET", + "https://git.example.com/repos/owner/repo/pulls/7/files", + timeout=DEFAULT_TIMEOUT, + ) + + def test_get_pr_commits(self) -> None: + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + client._session.request = MagicMock( + return_value=_mock_response([{"sha": "abc123", "commit": {"message": "fix: bug"}}]) + ) + + result = client.get_pr_commits(7) + assert len(result) == 1 + assert result[0]["commit"]["message"] == "fix: bug" + client._session.request.assert_called_once_with( + "GET", + "https://git.example.com/repos/owner/repo/pulls/7/commits", + timeout=DEFAULT_TIMEOUT, + ) + + def test_create_review_comment(self) -> None: + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + client._session.request = MagicMock(return_value=_mock_response({"id": 42})) + + result = client.create_review(7, event="COMMENT", body="Looks good") + assert result["id"] == 42 + client._session.request.assert_called_once_with( + "POST", + "https://git.example.com/repos/owner/repo/pulls/7/reviews", + timeout=DEFAULT_TIMEOUT, + json={"event": "COMMENT", "body": "Looks good"}, + ) + + def test_create_review_with_inline_comments(self) -> None: + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + client._session.request = MagicMock(return_value=_mock_response({"id": 43})) + + comments = [{"path": "src/main.py", "body": "Fix this", "new_position": 10}] + result = client.create_review(7, event="REQUEST_CHANGES", body="Please fix", comments=comments) + assert result["id"] == 43 + client._session.request.assert_called_once_with( + "POST", + "https://git.example.com/repos/owner/repo/pulls/7/reviews", + timeout=DEFAULT_TIMEOUT, + json={"event": "REQUEST_CHANGES", "body": "Please fix", "comments": comments}, + ) + def test_update_repo_settings(self) -> None: client = GiteaClient("https://git.example.com", "tok", "owner", "repo") client._session.request = MagicMock(return_value=_mock_response({"default_delete_branch_after_merge": True})) @@ -289,6 +346,21 @@ class TestVikunjaClient: params={"page": 1, "per_page": DEFAULT_PER_PAGE}, ) + def test_get_task(self) -> None: + client = VikunjaClient("https://work.example.com", "tok") + client._session.request = MagicMock( + return_value=_mock_response({"id": 292, "identifier": "GRM-32", "title": "Some task"}) + ) + + result = client.get_task(292) + assert result["identifier"] == "GRM-32" + assert result["title"] == "Some task" + client._session.request.assert_called_once_with( + "GET", + "https://work.example.com/tasks/292", + timeout=DEFAULT_TIMEOUT, + ) + def test_post_comment(self) -> None: client = VikunjaClient("https://work.example.com", "tok") client._session.request = MagicMock(return_value=_mock_response()) diff --git a/tests/unit/test_auto_merge.py b/tests/unit/test_auto_merge.py index ac1719f..800c56b 100644 --- a/tests/unit/test_auto_merge.py +++ b/tests/unit/test_auto_merge.py @@ -10,6 +10,8 @@ from click.testing import CliRunner from gitea_runner_manager.config import CONVENTIONAL_RE, TASK_ID_RE from gitea_runner_manager.exceptions import APIError from scripts.auto_merge import ( + PR_TITLE_RE, + extract_conventional_msg, extract_task_id, has_ready_to_merge_label, main, @@ -26,6 +28,10 @@ def _status(context: str, status: str, updated_at: str = "2026-01-01T00:00:00Z") return {"context": context, "status": status, "updated_at": updated_at} +def _commit(message: str) -> dict[str, dict[str, str]]: + return {"commit": {"message": message}} + + class TestRegexes: def test_task_id_re_matches(self) -> None: assert TASK_ID_RE.search("GRM-19-fix-bug") @@ -42,6 +48,15 @@ class TestRegexes: assert not CONVENTIONAL_RE.match("random message") assert not CONVENTIONAL_RE.match("feat:") + def test_pr_title_re_matches(self) -> None: + assert PR_TITLE_RE.match("GRM-19: Some task title") + assert PR_TITLE_RE.match("GRM-42: double space title") + + def test_pr_title_re_rejects(self) -> None: + assert not PR_TITLE_RE.match("fix: resolve timeout") + assert not PR_TITLE_RE.match("GRM-19:No space after colon") + assert not PR_TITLE_RE.match("random message") + class TestExtractTaskId: def test_extracts_from_branch(self) -> None: @@ -56,15 +71,58 @@ class TestExtractTaskId: class TestValidatePrTitle: def test_valid_title_passes(self) -> None: - validate_pr_title("fix: resolve timeout") + validate_pr_title("GRM-19: Some task title", "GRM-19") def test_valid_title_with_scope_passes(self) -> None: - validate_pr_title("feat(cli): add --url option") + validate_pr_title("GRM-42: Add --url option", "GRM-42") - def test_invalid_title_raises(self) -> None: + def test_invalid_format_raises(self) -> None: with pytest.raises(click.ClickException) as exc: - validate_pr_title("random message") - assert "conventional" in str(exc.value) + validate_pr_title("random message", "GRM-19") + assert "GRM-N" in str(exc.value) + + def test_invalid_format_conventional_raises(self) -> None: + with pytest.raises(click.ClickException) as exc: + validate_pr_title("fix: resolve timeout", "GRM-19") + assert "GRM-N" in str(exc.value) + + def test_task_id_mismatch_raises(self) -> None: + with pytest.raises(click.ClickException) as exc: + validate_pr_title("GRM-42: Some task title", "GRM-19") + assert "mismatch" in str(exc.value) + + +class TestExtractConventionalMsg: + def test_returns_newest_conventional(self) -> None: + """Iterates in reverse — picks the newest conventional commit.""" + commits = [ + _commit("fix: resolve timeout"), + _commit("random message"), + _commit("feat: add thing"), + ] + assert extract_conventional_msg(commits) == "feat: add thing" + + def test_falls_back_to_newest_commit(self) -> None: + commits = [ + _commit("another random"), + _commit("random message"), + ] + assert extract_conventional_msg(commits) == "random message" + + def test_uses_first_line_only(self) -> None: + commits = [_commit("fix: resolve timeout\n\nBody text here")] + assert extract_conventional_msg(commits) == "fix: resolve timeout" + + def test_empty_commits_returns_empty(self) -> None: + assert extract_conventional_msg([]) == "" + + def test_commit_with_scope(self) -> None: + commits = [_commit("feat(api): new endpoint")] + assert extract_conventional_msg(commits) == "feat(api): new endpoint" + + def test_missing_commit_key(self) -> None: + commits = [{}] # type: ignore[list-item] + assert extract_conventional_msg(commits) == "" class TestHasReadyToMergeLabel: @@ -161,6 +219,10 @@ def _mock_ci_passing() -> list[dict[str, str]]: return [_status("CI / quality (pull_request)", CI_SUCCESS)] +def _mock_commits() -> list[dict[str, dict[str, str]]]: + return [_commit("fix: resolve timeout")] + + class TestMain: @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) @patch("scripts.auto_merge.GiteaClient") @@ -169,14 +231,15 @@ class TestMain: mock_client_cls.return_value = mock_client mock_client.get_pr.return_value = _mock_pr() mock_client.get_commit_status.return_value = _mock_ci_passing() + mock_client.get_pr_commits.return_value = _mock_commits() runner = CliRunner() result = runner.invoke( main, - ["GRM-19-fix-bug", "fix: resolve timeout", "owner/repo", "7", "ready-to-merge"], + ["GRM-19-fix-bug", "GRM-19: Some task title", "owner/repo", "7", "ready-to-merge"], ) assert result.exit_code == 0 assert "squash-merged" in result.output - mock_client.merge_pr.assert_called_once_with("7", "GRM-19: fix: resolve timeout") + mock_client.merge_pr.assert_called_once_with("7", "GRM-19 fix: resolve timeout") @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) @patch("scripts.auto_merge.GiteaClient") @@ -186,15 +249,16 @@ class TestMain: mock_client.get_pr_labels.return_value = [{"name": "ready-to-merge"}] mock_client.get_pr.return_value = _mock_pr() mock_client.get_commit_status.return_value = _mock_ci_passing() + mock_client.get_pr_commits.return_value = _mock_commits() mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke( main, - ["GRM-19-fix-bug", "fix: resolve timeout", "owner/repo", "7"], + ["GRM-19-fix-bug", "GRM-19: Some task title", "owner/repo", "7"], ) assert result.exit_code == 0 assert "squash-merged" in result.output - mock_client.merge_pr.assert_called_once_with("7", "GRM-19: fix: resolve timeout") + mock_client.merge_pr.assert_called_once_with("7", "GRM-19 fix: resolve timeout") @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) @patch("scripts.auto_merge.GiteaClient") @@ -206,7 +270,7 @@ class TestMain: runner = CliRunner() result = runner.invoke( main, - ["GRM-19-fix-bug", "fix: resolve timeout", "owner/repo", "7", "bug"], + ["GRM-19-fix-bug", "GRM-19: Some task title", "owner/repo", "7", "bug"], ) assert result.exit_code == 0 assert "skipping" in result.output @@ -220,11 +284,12 @@ class TestMain: mock_client.get_pr_labels.return_value = [{"name": "ready-to-merge"}] mock_client.get_pr.return_value = _mock_pr() mock_client.get_commit_status.return_value = _mock_ci_passing() + mock_client.get_pr_commits.return_value = _mock_commits() mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke( main, - ["GRM-19-fix-bug", "fix: resolve timeout", "owner/repo", "7", ""], + ["GRM-19-fix-bug", "GRM-19: Some task title", "owner/repo", "7", ""], ) assert result.exit_code == 0 assert "squash-merged" in result.output @@ -243,7 +308,7 @@ class TestMain: mock_client.get_pr_labels.return_value = [{"name": "ready-to-merge"}] mock_client_cls.return_value = mock_client runner = CliRunner() - result = runner.invoke(main, ["feature-no-id", "fix: bug", "owner/repo", "1"]) + result = runner.invoke(main, ["feature-no-id", "GRM-19: bug", "owner/repo", "1"]) assert result.exit_code == 1 assert "task ID" in result.output @@ -256,7 +321,35 @@ class TestMain: runner = CliRunner() result = runner.invoke(main, ["GRM-19-fix", "random title", "owner/repo", "1"]) assert result.exit_code == 1 - assert "conventional" in result.output + assert "GRM-N" in result.output + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.auto_merge.GiteaClient") + def test_pr_title_task_id_mismatch_exits(self, mock_client_cls: MagicMock) -> None: + """PR title has a different task ID than the branch.""" + mock_client = MagicMock() + mock_client.get_pr_labels.return_value = [{"name": "ready-to-merge"}] + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke(main, ["GRM-19-fix", "GRM-42: Some task title", "owner/repo", "1"]) + assert result.exit_code == 1 + assert "mismatch" in result.output + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.auto_merge.GiteaClient") + def test_empty_commits_exits(self, mock_client_cls: MagicMock) -> None: + """PR has no commits — cannot extract conventional message.""" + mock_client = MagicMock() + mock_client.get_pr_labels.return_value = [{"name": "ready-to-merge"}] + mock_client.get_pr.return_value = _mock_pr() + mock_client.get_commit_status.return_value = _mock_ci_passing() + mock_client.get_pr_commits.return_value = [] + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke(main, ["GRM-19-fix", "GRM-19: Some task title", "owner/repo", "1"]) + assert result.exit_code == 1 + assert "conventional commit" in result.output + mock_client.merge_pr.assert_not_called() @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) @patch("scripts.auto_merge.GiteaClient") @@ -265,10 +358,11 @@ class TestMain: mock_client.get_pr_labels.return_value = [{"name": "ready-to-merge"}] mock_client.get_pr.return_value = _mock_pr() mock_client.get_commit_status.return_value = _mock_ci_passing() + mock_client.get_pr_commits.return_value = _mock_commits() mock_client.merge_pr.side_effect = APIError(http.HTTPStatus.INTERNAL_SERVER_ERROR, "server error") mock_client_cls.return_value = mock_client runner = CliRunner() - result = runner.invoke(main, ["GRM-19-fix", "fix: bug", "owner/repo", "1"]) + result = runner.invoke(main, ["GRM-19-fix", "GRM-19: Some task title", "owner/repo", "1"]) assert result.exit_code == 1 assert "HTTP" in result.output @@ -279,10 +373,11 @@ class TestMain: mock_client.get_pr_labels.return_value = [{"name": "ready-to-merge"}] mock_client.get_pr.return_value = _mock_pr() mock_client.get_commit_status.return_value = _mock_ci_passing() + mock_client.get_pr_commits.return_value = _mock_commits() mock_client.merge_pr.side_effect = APIError(http.HTTPStatus.BAD_GATEWAY, "bad gateway") mock_client_cls.return_value = mock_client runner = CliRunner() - result = runner.invoke(main, ["GRM-19-fix", "fix: bug", "owner/repo", "1"]) + result = runner.invoke(main, ["GRM-19-fix", "GRM-19: Some task title", "owner/repo", "1"]) assert result.exit_code == 1 assert str(http.HTTPStatus.BAD_GATEWAY) in result.output @@ -298,7 +393,7 @@ class TestMain: ] mock_client_cls.return_value = mock_client runner = CliRunner() - result = runner.invoke(main, ["GRM-19-fix", "fix: bug", "owner/repo", "1"]) + result = runner.invoke(main, ["GRM-19-fix", "GRM-19: Some task title", "owner/repo", "1"]) assert result.exit_code == 1 assert "CI checks did not pass" in result.output mock_client.merge_pr.assert_not_called() @@ -310,8 +405,9 @@ class TestMain: mock_client = MagicMock() mock_client.get_pr_labels.return_value = [{"name": "ready-to-merge"}] mock_client.get_pr.return_value = {"head": {}} + mock_client.get_pr_commits.return_value = _mock_commits() mock_client_cls.return_value = mock_client runner = CliRunner() - result = runner.invoke(main, ["GRM-19-fix", "fix: bug", "owner/repo", "1"]) + result = runner.invoke(main, ["GRM-19-fix", "GRM-19: Some task title", "owner/repo", "1"]) assert result.exit_code == 0 assert "squash-merged" in result.output diff --git a/tests/unit/test_post_merge.py b/tests/unit/test_post_merge.py index 91405a3..6e590f1 100644 --- a/tests/unit/test_post_merge.py +++ b/tests/unit/test_post_merge.py @@ -26,9 +26,14 @@ class TestExtractTaskId: class TestExtractConventionalMsg: - def test_strips_task_id_prefix(self) -> None: + def test_strips_colon_prefix(self) -> None: + """Legacy format: GRM-N: """ assert extract_conventional_msg("GRM-19: fix: resolve bug") == "fix: resolve bug" + def test_strips_space_prefix(self) -> None: + """Current format: GRM-N """ + assert extract_conventional_msg("GRM-19 fix: resolve bug") == "fix: resolve bug" + def test_returns_unchanged_without_prefix(self) -> None: assert extract_conventional_msg("fix: resolve bug") == "fix: resolve bug" diff --git a/tests/unit/test_review_pr.py b/tests/unit/test_review_pr.py new file mode 100644 index 0000000..140876b --- /dev/null +++ b/tests/unit/test_review_pr.py @@ -0,0 +1,170 @@ +"""Unit tests for scripts/review_pr.py.""" + +import http +import json +from unittest.mock import MagicMock, patch + +import click +import pytest +from click.testing import CliRunner + +from gitea_runner_manager.exceptions import APIError +from scripts.review_pr import main, parse_comments + + +class TestParseComments: + def test_parse_from_json_file(self, tmp_path) -> None: + comments = [{"path": "a.py", "body": "fix", "new_position": 1}] + f = tmp_path / "comments.json" + f.write_text(json.dumps(comments)) + assert parse_comments(str(f), False) == comments + + def test_parse_from_stdin(self) -> None: + comments = [{"path": "a.py", "body": "fix", "new_position": 1}] + with patch("scripts.review_pr.sys.stdin") as mock_stdin: + mock_stdin.read.return_value = json.dumps(comments) + assert parse_comments(None, True) == comments + + def test_no_comments_returns_empty(self) -> None: + assert parse_comments(None, False) == [] + + def test_invalid_json_file_raises(self, tmp_path) -> None: + f = tmp_path / "comments.json" + f.write_text("not json{") + with pytest.raises(click.ClickException): + parse_comments(str(f), False) + + def test_non_list_json_raises(self, tmp_path) -> None: + f = tmp_path / "comments.json" + f.write_text(json.dumps({"path": "a.py"})) + with pytest.raises(click.ClickException): + parse_comments(str(f), False) + + def test_stdin_non_list_raises(self) -> None: + with patch("scripts.review_pr.sys.stdin") as mock_stdin: + mock_stdin.read.return_value = json.dumps({"path": "a.py"}) + with pytest.raises(click.ClickException): + parse_comments(None, True) + + def test_stdin_invalid_json_raises(self) -> None: + with patch("scripts.review_pr.sys.stdin") as mock_stdin: + mock_stdin.read.return_value = "not json{" + with pytest.raises(click.ClickException): + parse_comments(None, True) + + def test_stdin_empty_returns_empty(self) -> None: + with patch("scripts.review_pr.sys.stdin") as mock_stdin: + mock_stdin.read.return_value = " " + assert parse_comments(None, True) == [] + + +class TestMain: + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.review_pr.GiteaClient") + def test_successful_comment_review(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client.create_review.return_value = {"id": 42} + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke( + main, + ["5", "owner/repo", "--event", "COMMENT", "--body", "LGTM"], + ) + assert result.exit_code == 0 + assert "Review #42" in result.output + mock_client.create_review.assert_called_once_with("5", event="COMMENT", body="LGTM", comments=[]) + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.review_pr.GiteaClient") + def test_successful_approve_review(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client.create_review.return_value = {"id": 7} + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke(main, ["5", "owner/repo", "--event", "APPROVE"]) + assert result.exit_code == 0 + assert "Review #7" in result.output + mock_client.create_review.assert_called_once_with("5", event="APPROVE", body="", comments=[]) + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.review_pr.GiteaClient") + def test_successful_with_inline_comments(self, mock_client_cls: MagicMock, tmp_path) -> None: + comments = [{"path": "a.py", "body": "fix", "new_position": 1}] + f = tmp_path / "comments.json" + f.write_text(json.dumps(comments)) + mock_client = MagicMock() + mock_client.create_review.return_value = {"id": 9} + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke( + main, + ["5", "owner/repo", "--comments-json", str(f)], + ) + assert result.exit_code == 0 + mock_client.create_review.assert_called_once_with("5", event="COMMENT", body="", comments=comments) + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.review_pr.GiteaClient") + def test_successful_with_stdin_comments(self, mock_client_cls: MagicMock) -> None: + comments = [{"path": "a.py", "body": "fix", "new_position": 1}] + mock_client = MagicMock() + mock_client.create_review.return_value = {"id": 11} + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke( + main, + ["5", "owner/repo", "--comments-stdin"], + input=json.dumps(comments), + ) + assert result.exit_code == 0 + mock_client.create_review.assert_called_once_with("5", event="COMMENT", body="", comments=comments) + + @patch.dict("os.environ", {"REPO_TOKEN": ""}, clear=True) + def test_missing_token_exits(self) -> None: + runner = CliRunner() + result = runner.invoke(main, ["5", "owner/repo", "--body", "x"]) + assert result.exit_code == 1 + assert "REPO_TOKEN" in result.output + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.review_pr.GiteaClient") + def test_no_body_or_comments_for_comment_event(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke(main, ["5", "owner/repo", "--event", "COMMENT"]) + assert result.exit_code == 1 + assert "required" in result.output + mock_client.create_review.assert_not_called() + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.review_pr.GiteaClient") + def test_no_body_or_comments_for_request_changes(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke(main, ["5", "owner/repo", "--event", "REQUEST_CHANGES"]) + assert result.exit_code == 1 + assert "required" in result.output + mock_client.create_review.assert_not_called() + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.review_pr.GiteaClient") + def test_api_error_raises_click(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client.create_review.side_effect = APIError(http.HTTPStatus.INTERNAL_SERVER_ERROR, "server error") + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke(main, ["5", "owner/repo", "--body", "x"]) + assert result.exit_code == 1 + assert "HTTP" in result.output + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.review_pr.GiteaClient") + def test_invalid_event_choice(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke(main, ["5", "owner/repo", "--event", "Bogus"]) + assert result.exit_code != 0 + mock_client.create_review.assert_not_called()