From 64a58874b646f43820a7050c6b7612d3d94b76db Mon Sep 17 00:00:00 2001 From: emil Date: Sun, 28 Jun 2026 00:17:33 +0000 Subject: [PATCH] DEVX-91: feat: add manual review support to pr_review (--event, --body, --checklist-confirmed) --- AGENTS.md | 3 +- src/devx/ci/auto_merge.py | 20 +++- src/devx/ci/pr_review.py | 98 ++++++++++++++++++- src/devx/make/devx.mak | 12 ++- src/devx/translations.json | 40 ++++++++ tests/unit/test_auto_merge.py | 23 +++++ tests/unit/test_pr_review.py | 176 ++++++++++++++++++++++++++++++++++ 7 files changed, 363 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a42c799..2ba064c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,7 +62,7 @@ src/devx/ │ ├── classify_changes.py # User-facing vs workflow-only change detection │ ├── detect_release_commit.py # Detect release commits on master │ ├── validate_commit_msg.py # Conventional commit validation -│ ├── pr_review.py # Automated PR review +│ ├── pr_review.py # Automated PR review + manual reviews (--event, --body, --checklist-confirmed) │ ├── post_merge.py # Vikunja task updates after merge │ ├── sync_wiki.py # Sync documentation to Gitea wiki │ ├── push_badges.py # Generate and push quality badges (--retries for retry on git push failures) @@ -406,6 +406,7 @@ projects. | `devx-pr-status` | Check CI status for a PR (`PR=`, `WAIT=`, `TIMEOUT=`) | | `devx-pr-logs` | Fetch logs for failed CI jobs (`PR=`, `JOB=`, `TAIL=`) | | `devx-pr-label` | Add a label to a PR (`PR=`, `LABEL=ready-to-merge`) | +| `devx-pr-review` | Post a review on a PR (`PR=`, `EVENT=`, `BODY=`, `CHECKLIST=`) | | `devx-check-config` | Validate devx configuration | | `devx-configure-gitea-pypi` | Configure Gitea private PyPI registry | | `devx-env` | Create .env from .env.example | diff --git a/src/devx/ci/auto_merge.py b/src/devx/ci/auto_merge.py index e9ba114..1da28b5 100644 --- a/src/devx/ci/auto_merge.py +++ b/src/devx/ci/auto_merge.py @@ -174,15 +174,25 @@ def validate_pr_title_matches_vikunja(pr_title: str, task_id: str) -> None: 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. + Picks the highest-priority conventional commit message from the PR. + Priority: feat > fix > refactor > docs > chore > other. + Falls back to the newest commit message if none match. """ + priority = {"feat": 5, "fix": 4, "refactor": 3, "docs": 2, "chore": 1, "ci": 1, "style": 1, "test": 1} + best_msg = "" + best_score = 0 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 + m = CONVENTIONAL_RE.match(message) + if m: + prefix = m.group(1).split("(")[0].strip() # e.g. "feat" from "feat(scope)" + score = priority.get(prefix, 0) + if score > best_score: + best_score = score + best_msg = message + if best_msg: + return best_msg # Fallback: use the newest commit's first line if commits: commit_info = commits[-1].get("commit", {}) diff --git a/src/devx/ci/pr_review.py b/src/devx/ci/pr_review.py index 071383c..245dc9f 100644 --- a/src/devx/ci/pr_review.py +++ b/src/devx/ci/pr_review.py @@ -520,12 +520,102 @@ def post_review(client: GiteaClient, pr_number: str, result: ReviewResult) -> di return client.create_review(pr_number, event=event, body=body, comments=comments) +def _post_manual_review( + client: GiteaClient, + pr_number: str, + event: str, + body: str | None, + checklist_confirmed: bool, + checklist_categories: str | None, + dry_run: bool, +) -> None: + """Post a manual review with validation for APPROVE events.""" + if not body or len(body) < 50: + raise click.ClickException(_("Review body must be at least 50 characters.")) + + if event == "APPROVE": + if not checklist_confirmed: + raise click.ClickException( + _("--checklist-confirmed is required for APPROVE events."), + ) + cats = [c.strip() for c in (checklist_categories or "").split(",") if c.strip()] + cat_nums: list[int] = [] + for c in cats: + try: + cat_nums.append(int(c)) + except ValueError: + raise click.ClickException( + _("Invalid checklist category: {cat}. Must be numbers.", cat=c), + ) from None + if len(cat_nums) < 8: + raise click.ClickException( + _("--checklist-categories must list at least 8 of 13 categories. Got {count}.", count=len(cat_nums)), + ) + + click.echo(f"Manual review event: {event}") + click.echo(f"Body: {body[:80]}...") + if checklist_confirmed: + click.echo(f"Checklist confirmed: {checklist_categories}") + + if dry_run: + click.echo("\n[dry-run] Review not posted.") + return + + try: + review = client.create_review(pr_number, event=event, body=body) + except APIError as e: + if "approve" in e.message.lower() or "422" in str(e.status): + click.echo(_("Note: Self-approval not allowed. Posting COMMENT instead.")) + review = client.create_review(pr_number, event="COMMENT", body=body) + else: + raise + review_id = review.get("id", "?") + click.echo( + _( + "\nReview #{review_id} posted on PR #{pr_number} with event '{event}'.", + review_id=review_id, + pr_number=pr_number, + event=event, + ) + ) + + @click.command() @click.argument("pr_number") @click.argument("repo") @click.option("--dry-run", is_flag=True, default=False, help="Print review without posting.") -def main(pr_number: str, repo: str, dry_run: bool) -> None: - """Run automated PR review and post results to Gitea.""" +@click.option( + "--event", + type=click.Choice(["APPROVE", "REQUEST_CHANGES", "COMMENT"], case_sensitive=False), + default=None, + help="Post a manual review with the given event (skips automated checks).", +) +@click.option("--body", default=None, help="Review body text (required with --event).") +@click.option( + "--checklist-confirmed", + is_flag=True, + default=False, + help="Attest that REVIEW_CHECKLIST.md categories were checked (required for APPROVE).", +) +@click.option( + "--checklist-categories", + default=None, + help="Comma-separated checklist category numbers (required for APPROVE, min 8 of 13).", +) +def main( + pr_number: str, + repo: str, + dry_run: bool, + event: str | None, + body: str | None, + checklist_confirmed: bool, + checklist_categories: str | None, +) -> None: + """Run automated PR review and post results to Gitea. + + Without --event: runs automated checks and posts COMMENT/REQUEST_CHANGES. + With --event: posts a manual review (skips automated checks). + """ token = os.environ.get("CI_GITEA_TOKEN", "") if not token: raise click.ClickException(_("ERROR: CI_GITEA_TOKEN is not set.")) @@ -533,6 +623,10 @@ def main(pr_number: str, repo: str, dry_run: bool) -> None: owner, repo_name = repo.split("/") client = GiteaClient(GITEA_API_URL, token, owner, repo_name) + if event is not None: + _post_manual_review(client, pr_number, event.upper(), body, checklist_confirmed, checklist_categories, dry_run) + return + result = run_review(client, pr_number) body = build_review_body(result) diff --git a/src/devx/make/devx.mak b/src/devx/make/devx.mak index a7ad179..fae5f1a 100644 --- a/src/devx/make/devx.mak +++ b/src/devx/make/devx.mak @@ -63,7 +63,7 @@ DEVX_PIP_INSTALL := if [ -z "$$CI_GITEA_TOKEN" ]; then . ./.env 2>/dev/null; fi; $(DEVX_BIN)/pip .PHONY: devx-create-task devx-create-pr devx-push devx-push-with-pr devx-check-config -.PHONY: devx-pr-status devx-pr-logs devx-pr-label +.PHONY: devx-pr-status devx-pr-logs devx-pr-label devx-pr-review .PHONY: devx-configure-gitea-pypi devx-install-tools devx-install-checkmake devx-checkmake .PHONY: devx-workflow-lint devx-workflow-dryrun devx-workflow-dryrun-safe devx-workflow-check .PHONY: devx-notify-failure devx-install-hooks devx-activate-scripts @@ -124,6 +124,16 @@ devx-pr-label: $(if $(PR),--pr $(PR)) \ --label $(or $(LABEL),ready-to-merge) +# Usage: make devx-pr-review PR=42 EVENT=APPROVE BODY="..." CHECKLIST=1,2,3,4,5,6,7,8,9,10,11,12,13 +# make devx-pr-review PR=42 EVENT=REQUEST_CHANGES BODY="..." +# make devx-pr-review PR=42 (auto review) +devx-pr-review: + @$(DEVX_PYTHON) -m devx.ci.pr_review \ + $(PR) $(DEVX_REPO_OWNER)/$(DEVX_REPO_NAME) \ + $(if $(EVENT),--event $(EVENT)) \ + $(if $(BODY),--body "$(BODY)") \ + $(if $(CHECKLIST),--checklist-confirmed --checklist-categories $(CHECKLIST)) + # ── Environment setup ───────────────────────────────────────────────────────── # Configure Gitea private PyPI registry so pip can find devx and other diff --git a/src/devx/translations.json b/src/devx/translations.json index 87f84c9..11543f2 100644 --- a/src/devx/translations.json +++ b/src/devx/translations.json @@ -2214,5 +2214,45 @@ "pl": "Missing tests for changed files.", "ru": "Missing tests for changed files.", "zh": "Missing tests for changed files." + }, + "\nReview #{review_id} posted on PR #{pr_number} with event '{event}'.": { + "en": "\nReview #{review_id} posted on PR #{pr_number} with event '{event}'.", + "bg": "\nReview #{review_id} posted on PR #{pr_number} with event '{event}'.", + "de": "\nReview #{review_id} posted on PR #{pr_number} with event '{event}'.", + "pl": "\nReview #{review_id} posted on PR #{pr_number} with event '{event}'.", + "ru": "\nReview #{review_id} posted on PR #{pr_number} with event '{event}'.", + "zh": "\nReview #{review_id} posted on PR #{pr_number} with event '{event}'." + }, + "--checklist-categories must list at least 8 of 13 categories. Got {count}.": { + "en": "--checklist-categories must list at least 8 of 13 categories. Got {count}.", + "bg": "--checklist-categories must list at least 8 of 13 categories. Got {count}.", + "de": "--checklist-categories must list at least 8 of 13 categories. Got {count}.", + "pl": "--checklist-categories must list at least 8 of 13 categories. Got {count}.", + "ru": "--checklist-categories must list at least 8 of 13 categories. Got {count}.", + "zh": "--checklist-categories must list at least 8 of 13 categories. Got {count}." + }, + "--checklist-confirmed is required for APPROVE events.": { + "en": "--checklist-confirmed is required for APPROVE events.", + "bg": "--checklist-confirmed is required for APPROVE events.", + "de": "--checklist-confirmed is required for APPROVE events.", + "pl": "--checklist-confirmed is required for APPROVE events.", + "ru": "--checklist-confirmed is required for APPROVE events.", + "zh": "--checklist-confirmed is required for APPROVE events." + }, + "Invalid checklist category: {cat}. Must be numbers.": { + "en": "Invalid checklist category: {cat}. Must be numbers.", + "bg": "Invalid checklist category: {cat}. Must be numbers.", + "de": "Invalid checklist category: {cat}. Must be numbers.", + "pl": "Invalid checklist category: {cat}. Must be numbers.", + "ru": "Invalid checklist category: {cat}. Must be numbers.", + "zh": "Invalid checklist category: {cat}. Must be numbers." + }, + "Review body must be at least 50 characters.": { + "en": "Review body must be at least 50 characters.", + "bg": "Review body must be at least 50 characters.", + "de": "Review body must be at least 50 characters.", + "pl": "Review body must be at least 50 characters.", + "ru": "Review body must be at least 50 characters.", + "zh": "Review body must be at least 50 characters." } } diff --git a/tests/unit/test_auto_merge.py b/tests/unit/test_auto_merge.py index 6451e44..20f1e5e 100644 --- a/tests/unit/test_auto_merge.py +++ b/tests/unit/test_auto_merge.py @@ -195,6 +195,29 @@ class TestExtractConventionalMsg: ] assert extract_conventional_msg(commits) == "feat: add feature" + def test_prefers_feat_over_refactor(self) -> None: + """When both feat and refactor commits exist, feat wins.""" + commits = [ + {"commit": {"message": "refactor: add find_task_by_identifier"}}, + {"commit": {"message": "fix: remove hardcoded fallbacks"}}, + {"commit": {"message": "feat: add manual review support"}}, + ] + assert extract_conventional_msg(commits) == "feat: add manual review support" + + def test_prefers_fix_over_docs(self) -> None: + commits = [ + {"commit": {"message": "docs: update README"}}, + {"commit": {"message": "fix: resolve bug"}}, + ] + assert extract_conventional_msg(commits) == "fix: resolve bug" + + def test_scope_in_prefix(self) -> None: + commits = [ + {"commit": {"message": "refactor(ci): cleanup code"}}, + {"commit": {"message": "feat(api): add endpoint"}}, + ] + assert extract_conventional_msg(commits) == "feat(api): add endpoint" + # -- run_cmd -- diff --git a/tests/unit/test_pr_review.py b/tests/unit/test_pr_review.py index d25fbc7..b5d9247 100644 --- a/tests/unit/test_pr_review.py +++ b/tests/unit/test_pr_review.py @@ -733,6 +733,182 @@ class TestMain: assert "CI_GITEA_TOKEN" in result.output +class TestManualReview: + @patch("devx.ci.pr_review.GiteaClient") + def test_approve_success(self, mock_client_class: MagicMock) -> None: + mock_client_class.return_value.create_review.return_value = {"id": 200} + runner = CliRunner() + result = runner.invoke( + main, + [ + "42", + "oblachno-oss/devx", + "--event", + "APPROVE", + "--body", + "All 13 REVIEW_CHECKLIST.md categories verified. Architecture: clean. Security: no issues.", + "--checklist-confirmed", + "--checklist-categories", + "1,2,3,4,5,6,7,8,9,10,11,12,13", + ], + env={"CI_GITEA_TOKEN": "fake"}, + ) + assert result.exit_code == 0 + assert "Review #200" in result.output + + @patch("devx.ci.pr_review.GiteaClient") + def test_approve_without_checklist_confirmed_fails(self, mock_client_class: MagicMock) -> None: + runner = CliRunner() + result = runner.invoke( + main, + [ + "42", + "oblachno-oss/devx", + "--event", + "APPROVE", + "--body", + "x" * 60, + "--checklist-categories", + "1,2,3,4,5,6,7,8", + ], + env={"CI_GITEA_TOKEN": "fake"}, + ) + assert result.exit_code != 0 + assert "checklist-confirmed" in result.output + + @patch("devx.ci.pr_review.GiteaClient") + def test_approve_with_too_few_categories_fails(self, mock_client_class: MagicMock) -> None: + runner = CliRunner() + result = runner.invoke( + main, + [ + "42", + "oblachno-oss/devx", + "--event", + "APPROVE", + "--body", + "x" * 60, + "--checklist-confirmed", + "--checklist-categories", + "1,2,3", + ], + env={"CI_GITEA_TOKEN": "fake"}, + ) + assert result.exit_code != 0 + assert "at least 8" in result.output + + @patch("devx.ci.pr_review.GiteaClient") + def test_approve_with_short_body_fails(self, mock_client_class: MagicMock) -> None: + runner = CliRunner() + result = runner.invoke( + main, + [ + "42", + "oblachno-oss/devx", + "--event", + "APPROVE", + "--body", + "LGTM", + "--checklist-confirmed", + "--checklist-categories", + "1,2,3,4,5,6,7,8", + ], + env={"CI_GITEA_TOKEN": "fake"}, + ) + assert result.exit_code != 0 + assert "50 characters" in result.output + + @patch("devx.ci.pr_review.GiteaClient") + def test_approve_with_invalid_category_fails(self, mock_client_class: MagicMock) -> None: + runner = CliRunner() + result = runner.invoke( + main, + [ + "42", + "oblachno-oss/devx", + "--event", + "APPROVE", + "--body", + "x" * 60, + "--checklist-confirmed", + "--checklist-categories", + "1,2,abc,4", + ], + env={"CI_GITEA_TOKEN": "fake"}, + ) + assert result.exit_code != 0 + assert "Invalid" in result.output + + @patch("devx.ci.pr_review.GiteaClient") + def test_request_changes_success(self, mock_client_class: MagicMock) -> None: + mock_client_class.return_value.create_review.return_value = {"id": 201} + runner = CliRunner() + result = runner.invoke( + main, + [ + "42", + "oblachno-oss/devx", + "--event", + "REQUEST_CHANGES", + "--body", + "Please fix the architecture issues in the CLI module before merging.", + ], + env={"CI_GITEA_TOKEN": "fake"}, + ) + assert result.exit_code == 0 + assert "Review #201" in result.output + + @patch("devx.ci.pr_review.GiteaClient") + def test_manual_review_dry_run(self, mock_client_class: MagicMock) -> None: + runner = CliRunner() + result = runner.invoke( + main, + ["42", "oblachno-oss/devx", "--event", "COMMENT", "--body", "x" * 60, "--dry-run"], + env={"CI_GITEA_TOKEN": "fake"}, + ) + assert result.exit_code == 0 + assert "[dry-run]" in result.output + mock_client_class.return_value.create_review.assert_not_called() + + @patch("devx.ci.pr_review.GiteaClient") + def test_manual_review_self_approval_fallback(self, mock_client_class: MagicMock) -> None: + client = mock_client_class.return_value + client.create_review.side_effect = [ + APIError(422, "approve your own pull is not allowed"), + {"id": 202}, + ] + runner = CliRunner() + result = runner.invoke( + main, + [ + "42", + "oblachno-oss/devx", + "--event", + "APPROVE", + "--body", + "x" * 60, + "--checklist-confirmed", + "--checklist-categories", + "1,2,3,4,5,6,7,8", + ], + env={"CI_GITEA_TOKEN": "fake"}, + ) + assert result.exit_code == 0 + assert "Review #202" in result.output + + @patch("devx.ci.pr_review.GiteaClient") + def test_manual_review_other_error_re_raises(self, mock_client_class: MagicMock) -> None: + client = mock_client_class.return_value + client.create_review.side_effect = APIError(500, "Internal server error") + runner = CliRunner() + result = runner.invoke( + main, + ["42", "oblachno-oss/devx", "--event", "COMMENT", "--body", "x" * 60], + env={"CI_GITEA_TOKEN": "fake"}, + ) + assert result.exit_code != 0 + + def test_main_module_block() -> None: import devx.ci.pr_review as pr