diff --git a/REVIEW_CHECKLIST.md b/REVIEW_CHECKLIST.md index 6ecfbe1..ac96fc3 100644 --- a/REVIEW_CHECKLIST.md +++ b/REVIEW_CHECKLIST.md @@ -32,6 +32,9 @@ events. This flag attests that every category below has been reviewed. - [ ] **No copy-paste duplication** — extract shared logic into a helper - [ ] **Idiomatic Python** — use comprehensions, context managers, dataclasses - [ ] **Type hints** on all public functions +- [ ] **No `Any` type without justification** — document why if used +- [ ] **Error handling complete** — all failure paths handled, no silent failures +- [ ] **Cleanup in error paths** — files closed, connections released, temp files removed ## 3. Security [auto + manual] @@ -41,8 +44,9 @@ events. This flag attests that every category below has been reviewed. - [ ] **No secrets in logs or process arguments** — pass via env vars or files - [ ] **Input validation** on all external inputs (CLI args, API responses, file contents) - [ ] **No injection vectors** — parameterize subprocess args, SQL queries, etc. +- [ ] **File paths validated** — no path traversal (use `Path.resolve()`, check boundaries) -## 4. Internationalization (i18n) [manual] +## 4. Internationalization (i18n) [auto + manual] - [ ] **All user-facing strings wrapped in `_()`** — `click.echo(_("..."))`, error messages, help text, prompts @@ -56,20 +60,23 @@ events. This flag attests that every category below has been reviewed. - [ ] **Source file changes include corresponding test updates** - [ ] **100% coverage maintained** (enforced by `pytest-cov`) - [ ] **Tests are fast** (< 10 seconds total, enforced by `check_test_speed.py`) -- [ ] **Edge cases tested**: empty inputs, boundary values, error paths +- [ ] **Edge cases tested**: empty inputs, boundary values, error paths, None/Optional - [ ] **No flaky tests** — no `sleep()`, no race conditions, no external dependencies - [ ] **Test names describe the scenario**: `test__` ## 6. Performance [manual] -- [ ] **No unnecessary allocations** in hot paths (list comprehensions vs generators) -- [ ] **Correct data structures** — O(1) lookups use `set`/`dict`, not `list` -- [ ] **No N+1 query patterns** in API calls or file I/O -- [ ] **No blocking I/O on hot paths** without justification +- [ ] **No unnecessary allocations** in hot paths — use generators for large datasets, + avoid reading entire files into memory +- [ ] **Correct data structures** — O(1) lookups use `set`/`dict`, not `list`; + `dict` for key-value, `set` for membership, `list` for ordered iteration +- [ ] **No N+1 query patterns** in API calls or file I/O — batch operations where possible +- [ ] **No blocking I/O on hot paths** without justification — CLI startup, command execution ## 7. User Experience [manual] -- [ ] **Clear error messages** — tell the user what went wrong and how to fix it +- [ ] **Clear error messages** — tell the user what went wrong and how to fix it. + Example: "Error: Config file not found at /etc/grm.conf. Create it with: grm config init" - [ ] **Consistent CLI flag naming** — `--long-name` with `--short` aliases - [ ] **Help text on all commands and options** — `--help` should be useful - [ ] **No silent failures** — if something fails, the user should know @@ -93,8 +100,31 @@ events. This flag attests that every category below has been reviewed. ## 10. Extensibility and Maintainability [manual] -- [ ] **Open/Closed Principle** — code is open for extension, closed for modification +- [ ] **Open/Closed Principle** — code is open for extension, closed for modification. + New behavior via new functions/classes, not by modifying existing ones - [ ] **No magic numbers** — constants are named and documented - [ ] **Configuration over hardcoding** — use `config.py` with env var overrides - [ ] **Future-proof error handling** — don't catch specific error messages that may change - [ ] **Dependencies are justified** — no new dependency without rationale + +## 11. Resource Management [auto + manual] + +- [ ] **File handles closed** — use `with` statements or explicit `close()` in `finally` +- [ ] **Subprocess resources cleaned up** — call `.wait()` or `.communicate()` +- [ ] **Temporary files deleted** — use `tempfile.TemporaryDirectory()` or cleanup in `finally` +- [ ] **No resource leaks in error paths** — `try/finally` or context managers for cleanup + +## 12. Backwards Compatibility [manual] + +- [ ] **No breaking changes to public API** — or documented as major version bump +- [ ] **Removed functions deprecated first** — with `DeprecationWarning` and removal timeline +- [ ] **Default values added** instead of new required arguments +- [ ] **Return types stable** — no changes without major version bump +- [ ] **Behavior changes documented** — no silent behavior changes in existing functions + +## 13. Logging and Observability [manual] + +- [ ] **No sensitive data in logs** — tokens, passwords, PII excluded +- [ ] **Sufficient detail for debugging** — context, state, values logged at DEBUG level +- [ ] **Log levels appropriate** — DEBUG for internals, INFO for user actions, WARNING for recoverable issues +- [ ] **No log spam** — loops don't log per iteration, use DEBUG for high-frequency events diff --git a/scripts/ci/auto_merge.py b/scripts/ci/auto_merge.py index 3b06c43..88b41ea 100644 --- a/scripts/ci/auto_merge.py +++ b/scripts/ci/auto_merge.py @@ -141,32 +141,18 @@ def has_approval_review(client: GiteaClient, pr_number: str) -> bool: "LGTM" or "OK"). This ensures the reviewer actually reviewed the PR rather than rubber-stamping it. - Falls back to checking that no REQUEST_CHANGES reviews are pending - when self-approval is not possible (single-token workflow). + Returns False if no APPROVE review is found — the caller should + block the merge in that case. """ reviews = client.get_pr_reviews(pr_number) - has_approved = False - has_changes_requested = False for r in reviews: state = r.get("state", "") if state == "APPROVED": body = str(r.get("body", "")).strip() if len(body) > 20 or r.get("comments", []): - has_approved = True - elif state == "REQUEST_CHANGES": - has_changes_requested = True + return True - if has_approved: - return True - # In single-token workflows, self-approval is not allowed. - # Allow merge if no changes are requested (the automated pr-review - # job and CI quality gate serve as the review enforcement). - if not has_changes_requested: - click.echo( - _("No APPROVE review found, but no REQUEST_CHANGES either. Proceeding (single-token workflow fallback).") - ) - return True return False diff --git a/scripts/ci/pr_review.py b/scripts/ci/pr_review.py index 2b695b1..7786a07 100644 --- a/scripts/ci/pr_review.py +++ b/scripts/ci/pr_review.py @@ -10,9 +10,11 @@ Checks performed: 2. Best practices — no bare except, no print() (use click.echo), no TODO/FIXME left in merged code, no functions > 50 lines 3. Security — no secrets in code, no shell=True, no eval/exec - 4. Documentation — new CLI commands documented, new modules in architecture.md - 5. Test coverage — 100% enforced by pytest-cov (checked in quality job) - 6. Commit conventions — conventional commit format on branch commits + 4. i18n — no raw English strings in click.echo() without _() wrapper + 5. Resource management — no open() without with statement, no subprocess without cleanup + 6. Documentation — new CLI commands documented, new modules in architecture.md + 7. Test coverage — 100% enforced by pytest-cov (checked in quality job) + 8. Commit conventions — conventional commit format on branch commits Usage: REPO_TOKEN= python3 scripts/ci/pr_review.py @@ -206,6 +208,112 @@ def check_security(files: list[dict[str, Any]], result: ReviewResult) -> None: result.add_summary("- Security: OK") +def check_i18n(files: list[dict[str, Any]], result: ReviewResult) -> None: + """Check that user-facing strings are wrapped in _(). + + Detects ``click.echo()`` calls with raw string literals that are not + wrapped in ``_()``. Only checks ``src/`` files, not tests or scripts. + """ + # Pattern: click.echo("...") or click.echo(f"...") without _() wrapper + raw_echo_re = re.compile(r'click\.echo\s*\(\s*["\']([^"\']+)["\']') + raw_fstring_re = re.compile(r'click\.echo\s*\(\s*f["\']') + # Also check click.ClickException and raise with string + raw_exception_re = re.compile(r'click\.ClickException\s*\(\s*["\']([^"\']+)["\']') + + for f in files: + path = f.get("filename", "") + if not is_python_file(path) or not path.startswith("src/"): + continue + + patch = f.get("patch", "") + if not patch: + continue + + lines = patch.split("\n") + current_line = 0 + for line in lines: + if line.startswith("@@"): + match = re.search(r"\+(\d+)", line) + if match: + current_line = int(match.group(1)) - 1 + continue + if line.startswith("+") and not line.startswith("+++"): + current_line += 1 + content = line[1:] + + # Skip comments and docstrings + stripped = content.strip() + if stripped.startswith("#") or stripped.startswith('"""') or stripped.startswith("'''"): + continue + + # Check for raw strings in click.echo without _() + for regex, msg in [ + (raw_echo_re, "click.echo() with raw string — wrap in _() for i18n"), + (raw_fstring_re, "click.echo() with f-string — wrap in _() for i18n"), + (raw_exception_re, "ClickException with raw string — wrap in _() for i18n"), + ]: + if regex.search(content): + result.add_issue(path, current_line, msg, "warning") + + if not any("i18n" in i["body"] for i in result.issues): + result.add_summary("- i18n: OK") + + +def check_resource_management(files: list[dict[str, Any]], result: ReviewResult) -> None: + """Check for resource leaks: open() without with, subprocess without cleanup. + + Detects: + - ``open()`` calls not in a ``with`` statement + - ``subprocess.Popen()`` without ``.wait()`` or ``.communicate()`` + """ + # Pattern: open("...") not preceded by "with" on the same line + open_re = re.compile(r"(? None: """Check that no new function is excessively long (> 50 lines).""" for f in files: @@ -363,6 +471,8 @@ def run_review(client: GiteaClient, pr_number: str) -> ReviewResult: check_architecture_compliance(files, result) check_best_practices(files, result) check_security(files, result) + check_i18n(files, result) + check_resource_management(files, result) check_function_length(files, result) check_documentation(files, result) check_test_coverage(files, result) diff --git a/scripts/ci/review_pr.py b/scripts/ci/review_pr.py index bfc9c37..25c8e0a 100644 --- a/scripts/ci/review_pr.py +++ b/scripts/ci/review_pr.py @@ -99,6 +99,12 @@ def parse_comments(comments_json: str | None, comments_stdin: bool) -> list[dict default=False, help="Required for APPROVE: confirms all REVIEW_CHECKLIST.md categories reviewed.", ) +@click.option( + "--checklist-categories", + default="", + help="Comma-separated list of checklist categories reviewed (e.g., '1,2,3,4,5,6,7,8,9,10,11,12,13'). " + "Required for APPROVE: must list at least 8 of 13 categories.", +) def main( pr_number: str, repo: str, @@ -107,6 +113,7 @@ def main( comments_json: str | None, comments_stdin: bool, checklist_confirmed: bool, + checklist_categories: str, ) -> None: token = os.environ.get("REPO_TOKEN", "") if not token: @@ -128,11 +135,25 @@ def main( "Review every category in REVIEW_CHECKLIST.md before approving." ) ) - if len(body.strip()) <= 20 and not comments: + # Validate that at least 8 of 13 checklist categories were reviewed + categories = [c.strip() for c in checklist_categories.split(",") if c.strip()] if checklist_categories else [] + if len(categories) < 8: raise click.ClickException( _( - "APPROVE review body must be substantive (> 20 characters) " - "or include inline comments. Trivial approvals are rejected." + "APPROVE requires --checklist-categories with at least 8 of 13 categories reviewed. " + "Provide comma-separated category numbers (e.g., '1,2,3,4,5,6,7,8'). " + "Got {count} categories: {cats}", + count=len(categories), + cats=checklist_categories or "(none)", + ) + ) + if len(body.strip()) <= 50 and not comments: + raise click.ClickException( + _( + "APPROVE review body must be substantive (> 50 characters) " + "or include inline comments. Trivial approvals are rejected. " + "Current body is {len} characters.", + len=len(body.strip()), ) ) diff --git a/tests/unit/test_auto_merge.py b/tests/unit/test_auto_merge.py index 589333c..8f7e97d 100644 --- a/tests/unit/test_auto_merge.py +++ b/tests/unit/test_auto_merge.py @@ -161,32 +161,31 @@ class TestHasApprovalReview: ] assert has_approval_review(client, "5") is True - def test_trivial_approved_falls_back_to_no_changes(self) -> None: - """A bare 'LGTM' approval (< 20 chars) without comments falls back to - checking no REQUEST_CHANGES exist (single-token workflow).""" + def test_trivial_approved_without_comments_returns_false(self) -> None: + """A bare 'LGTM' approval (< 20 chars) without comments is not substantive.""" client = MagicMock() client.get_pr_reviews.return_value = [ {"state": "APPROVED", "body": "LGTM", "comments": []}, ] - assert has_approval_review(client, "5") is True + assert has_approval_review(client, "5") is False - def test_no_approved_but_no_changes_requested(self) -> None: - """Single-token workflow: no APPROVE but no REQUEST_CHANGES either.""" + def test_no_approved_returns_false(self) -> None: + """No APPROVE review means merge is blocked — no fallback.""" client = MagicMock() client.get_pr_reviews.return_value = [{"state": "COMMENT"}] - assert has_approval_review(client, "5") is True + assert has_approval_review(client, "5") is False def test_changes_requested_blocks_merge(self) -> None: - """REQUEST_CHANGES blocks merge even in single-token workflow.""" + """REQUEST_CHANGES blocks merge.""" client = MagicMock() client.get_pr_reviews.return_value = [{"state": "REQUEST_CHANGES", "body": "Fix this"}] assert has_approval_review(client, "5") is False - def test_no_reviews_allows_merge(self) -> None: - """No reviews at all allows merge (single-token workflow fallback).""" + def test_no_reviews_returns_false(self) -> None: + """No reviews at all means no APPROVE — merge is blocked.""" client = MagicMock() client.get_pr_reviews.return_value = [] - assert has_approval_review(client, "5") is True + assert has_approval_review(client, "5") is False class TestValidatePrTitleMatchesVikunja: diff --git a/tests/unit/test_pr_review.py b/tests/unit/test_pr_review.py index 1646914..fd3351d 100644 --- a/tests/unit/test_pr_review.py +++ b/tests/unit/test_pr_review.py @@ -13,6 +13,8 @@ from scripts.ci.pr_review import ( check_commit_conventions, check_documentation, check_function_length, + check_i18n, + check_resource_management, check_security, check_test_coverage, is_python_file, @@ -238,6 +240,143 @@ class TestCheckSecurity: assert not result.has_issues +class TestCheckI18n: + def test_raw_string_in_echo_triggers_warning(self) -> None: + result = ReviewResult() + files = [ + {"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo("Hello world")\n'} + ] + check_i18n(files, result) + assert result.has_issues + assert any("i18n" in i["body"] for i in result.issues) + + def test_translated_string_no_warning(self) -> None: + result = ReviewResult() + files = [ + {"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo(_("Hello world"))\n'} + ] + check_i18n(files, result) + assert not result.has_issues + + def test_fstring_in_echo_triggers_warning(self) -> None: + result = ReviewResult() + files = [ + {"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo(f"Hello {name}")\n'} + ] + check_i18n(files, result) + assert result.has_issues + + def test_raw_exception_triggers_warning(self) -> None: + result = ReviewResult() + files = [ + { + "filename": "src/gitea_runner_manager/cli.py", + "patch": '@@ -1,1 +1,1 @@\n+raise click.ClickException("Something went wrong")\n', + } + ] + check_i18n(files, result) + assert result.has_issues + + def test_non_src_file_skipped(self) -> None: + result = ReviewResult() + files = [{"filename": "scripts/ci/test.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo("Hello world")\n'}] + check_i18n(files, result) + assert not result.has_issues + + def test_comment_skipped(self) -> None: + result = ReviewResult() + files = [ + {"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+# click.echo("Hello world")\n'} + ] + check_i18n(files, result) + assert not result.has_issues + + def test_empty_patch_skipped(self) -> None: + result = ReviewResult() + files = [{"filename": "src/gitea_runner_manager/cli.py", "patch": ""}] + check_i18n(files, result) + assert not result.has_issues + + def test_clean_code_adds_ok_summary(self) -> None: + result = ReviewResult() + files = [ + {"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo(_("Hello world"))\n'} + ] + check_i18n(files, result) + assert any("i18n: OK" in s for s in result.summary) + + +class TestCheckResourceManagement: + def test_open_without_with_triggers_warning(self) -> None: + result = ReviewResult() + files = [{"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+f = open("file.txt")\n'}] + check_resource_management(files, result) + assert result.has_issues + assert any("resource" in i["body"].lower() for i in result.issues) + + def test_open_with_with_no_warning(self) -> None: + result = ReviewResult() + files = [ + { + "filename": "src/gitea_runner_manager/cli.py", + "patch": '@@ -1,1 +1,1 @@\n+with open("file.txt") as f:\n+ pass\n', + } + ] + check_resource_management(files, result) + assert not result.has_issues + + def test_popen_without_cleanup_triggers_warning(self) -> None: + result = ReviewResult() + files = [ + { + "filename": "src/gitea_runner_manager/executor.py", + "patch": '@@ -1,1 +1,1 @@\n+proc = subprocess.Popen(["cmd"])\n', + } + ] + check_resource_management(files, result) + assert result.has_issues + + def test_popen_with_communicate_no_warning(self) -> None: + result = ReviewResult() + files = [ + { + "filename": "src/gitea_runner_manager/executor.py", + "patch": '@@ -1,1 +1,1 @@\n+out, err = subprocess.Popen(["cmd"], stdout=PIPE).communicate()\n', + } + ] + check_resource_management(files, result) + assert not result.has_issues + + def test_comment_skipped(self) -> None: + result = ReviewResult() + files = [{"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+# f = open("file.txt")\n'}] + check_resource_management(files, result) + assert not result.has_issues + + def test_empty_patch_skipped(self) -> None: + result = ReviewResult() + files = [{"filename": "src/gitea_runner_manager/cli.py", "patch": ""}] + check_resource_management(files, result) + assert not result.has_issues + + def test_non_python_file_skipped(self) -> None: + result = ReviewResult() + files = [{"filename": "docs/config.md", "patch": '@@ -1,1 +1,1 @@\n+f = open("file.txt")\n'}] + check_resource_management(files, result) + assert not result.has_issues + + def test_clean_code_adds_ok_summary(self) -> None: + result = ReviewResult() + files = [ + { + "filename": "src/gitea_runner_manager/cli.py", + "patch": '@@ -1,1 +1,1 @@\n+with open("file.txt") as f:\n+ data = f.read()\n', + } + ] + check_resource_management(files, result) + assert any("Resource management: OK" in s for s in result.summary) + + class TestCheckFunctionLength: def test_long_function_triggers_warning(self) -> None: result = ReviewResult() diff --git a/tests/unit/test_review_pr.py b/tests/unit/test_review_pr.py index c3c63d1..6dcf982 100644 --- a/tests/unit/test_review_pr.py +++ b/tests/unit/test_review_pr.py @@ -77,7 +77,7 @@ class TestMain: @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) @patch("scripts.ci.review_pr.GiteaClient") def test_successful_approve_review(self, mock_client_cls: MagicMock) -> None: - """APPROVE requires --checklist-confirmed and substantive body.""" + """APPROVE requires --checklist-confirmed, --checklist-categories, and substantive body.""" mock_client = MagicMock() mock_client.create_review.return_value = {"id": 7} mock_client_cls.return_value = mock_client @@ -90,8 +90,10 @@ class TestMain: "--event", "APPROVE", "--checklist-confirmed", + "--checklist-categories", + "1,2,3,4,5,6,7,8,9,10,11,12,13", "--body", - "All 10 checklist categories verified. Architecture OK, tests pass.", + "All 13 checklist categories verified. Architecture OK, tests pass.", ], ) assert result.exit_code == 0 @@ -99,7 +101,7 @@ class TestMain: mock_client.create_review.assert_called_once_with( "5", event="APPROVE", - body="All 10 checklist categories verified. Architecture OK, tests pass.", + body="All 13 checklist categories verified. Architecture OK, tests pass.", comments=[], ) @@ -120,14 +122,72 @@ class TestMain: @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) @patch("scripts.ci.review_pr.GiteaClient") - def test_approve_with_trivial_body_fails(self, mock_client_cls: MagicMock) -> None: - """APPROVE with trivial body (< 20 chars) and no comments is rejected.""" + def test_approve_without_checklist_categories_fails(self, mock_client_cls: MagicMock) -> None: + """APPROVE without --checklist-categories is rejected.""" mock_client = MagicMock() mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke( main, - ["5", "owner/repo", "--event", "APPROVE", "--checklist-confirmed", "--body", "LGTM"], + [ + "5", + "owner/repo", + "--event", + "APPROVE", + "--checklist-confirmed", + "--body", + "All categories verified. Architecture OK, tests pass, docs updated.", + ], + ) + assert result.exit_code != 0 + assert "checklist-categories" in result.output.lower() + mock_client.create_review.assert_not_called() + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.ci.review_pr.GiteaClient") + def test_approve_with_too_few_categories_fails(self, mock_client_cls: MagicMock) -> None: + """APPROVE with fewer than 8 categories is rejected.""" + mock_client = MagicMock() + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke( + main, + [ + "5", + "owner/repo", + "--event", + "APPROVE", + "--checklist-confirmed", + "--checklist-categories", + "1,2,3", + "--body", + "All categories verified. Architecture OK, tests pass, docs updated.", + ], + ) + assert result.exit_code != 0 + assert "8 of 13" in result.output + mock_client.create_review.assert_not_called() + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.ci.review_pr.GiteaClient") + def test_approve_with_trivial_body_fails(self, mock_client_cls: MagicMock) -> None: + """APPROVE with trivial body (< 50 chars) and no comments is rejected.""" + mock_client = MagicMock() + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke( + main, + [ + "5", + "owner/repo", + "--event", + "APPROVE", + "--checklist-confirmed", + "--checklist-categories", + "1,2,3,4,5,6,7,8", + "--body", + "LGTM", + ], ) assert result.exit_code != 0 assert "substantive" in result.output.lower()