From 2493a5538a560386c721df832e1fc5dd755e69d3 Mon Sep 17 00:00:00 2001 From: Aviadp Date: Tue, 17 Mar 2026 14:59:38 +0200 Subject: [PATCH] Add Claude AI code review workflow Signed-off-by: Aviadp --- .github/claude-review-guide.md | 82 +++++++++++++++++++++++++++++ .github/workflows/claude-review.yml | 53 +++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 .github/claude-review-guide.md create mode 100644 .github/workflows/claude-review.yml diff --git a/.github/claude-review-guide.md b/.github/claude-review-guide.md new file mode 100644 index 00000000000..6fcf0d316c5 --- /dev/null +++ b/.github/claude-review-guide.md @@ -0,0 +1,82 @@ +# Claude Review Guide for OCS-CI + +This guide complements `.claude/CLAUDE.md` — it defines **how to review** code, not how to write it. + +## Review Scope + +- Only review code added or modified by the PR — never flag existing code +- If the codebase already uses a pattern, don't flag it in new code that follows the same pattern +- Read docstrings and comments to understand the author's intent before flagging design decisions +- Ask "is this a realistic failure scenario?" — not "could this theoretically fail?" + +## Severity Calibration + +Map findings to severity levels based on real impact, not theoretical risk. + +### BLOCKER + +These violate critical project rules and must always be flagged: + +- `except Exception` or bare `except:` — must use specific exceptions +- UI locators defined outside `ocs_ci/ocs/ui/views.py` (unless the locator contains a variable) +- Fixtures using `yield` — must use `request.addfinalizer()` instead +- Tier marker at class level when `parametrize` marks have different tiers +- Test classes that create resources without a `teardown()` method +- Version-specific CSS selectors (`.pf-c-` instead of `[class*='c-']`) + +### HIGH + +- Missing type hints on new public method parameters or return values +- Missing docstrings on new public methods +- Magic numbers (unnamed timeout values like `time.sleep(2)`) +- Use of `@pytest.mark.usefixtures` instead of passing fixtures as parameters +- Global variables used for sharing state between tests + +### MEDIUM + +- Missing `log_step()` in UI test methods (non-UI tests use `logger.info()`) +- Missing fallback selectors for UI elements known to be flaky +- Inconsistent return types (method returns different types based on a flag) + +### LOW + +- Missing exception chaining (`from e`) +- Minor formatting issues not caught by black/flake8 + +## File-Type Guidance + +### conftest.py + +- Focus on fixture teardown: are resources cleaned up via `request.addfinalizer()`? +- Check for `yield` usage — this is a BLOCKER +- Verify no `request.node.cls` usage for setting/reading class attributes +- Ignore docstrings on fixtures (per project convention) + +### Page Objects (`ocs_ci/ocs/ui/page_objects/`) + +- Verify locators are in `views.py`, not inline +- Check navigation pattern: destination pages should NOT navigate to themselves +- Look for lazy imports to prevent circular dependencies +- Verify selectors are version-agnostic (`[class*='c-']` not `.pf-c-`) + +### Test Files (`tests/`) + +- Verify `teardown()` exists if the class creates resources +- Check tier markers are only in `parametrize` marks, not at class level +- Verify `log_step()` usage in UI tests +- Check that factories are used for resource creation (automatic cleanup) +- Verify `create_unique_resource_name()` for naming resources + +### views.py (`ocs_ci/ocs/ui/views.py`) + +- Verify selectors follow the locator priority order: `data-test` > `id` > `aria-label` > text+ancestor > class > index +- Check for version-agnostic patterns +- Verify fallback selector lists for unreliable elements + +## What NOT to Flag + +- Style issues caught by black or flake8 (formatting, import ordering, line length) +- Existing codebase patterns used consistently in new code +- Missing docstrings on fixtures (project convention exempts them) +- Import ordering or grouping +- Use of `time.sleep()` when the timeout value IS a named constant diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml new file mode 100644 index 00000000000..cefff63d3e9 --- /dev/null +++ b/.github/workflows/claude-review.yml @@ -0,0 +1,53 @@ +name: Claude Code Review + +on: + pull_request: + types: [labeled] + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] + +jobs: + review: + if: > + (github.event_name == 'pull_request' && github.event.label.name == 'claude-review') || + (github.event_name == 'issue_comment' && contains(github.event.comment.body, '@claude')) || + (github.event_name == 'pull_request_review_comment' && contains(github.event.comment.body, '@claude')) + runs-on: ubuntu-latest + timeout-minutes: 20 + permissions: + contents: read + pull-requests: write + issues: write + + steps: + - name: Run Claude Review + uses: /claude-review-action@v1 + with: + anthropic-api-key: ${{ secrets.ANTHROPIC_API_KEY }} + model: claude-sonnet-4-6 + review-authority: comment-only + review-guide-path: .github/claude-review-guide.md + max-diff-lines: '4000' + max-diff-bytes: '120000' + max-files: '40' + context-intro: > + You are reviewing a PR in ocs-ci — a Python/pytest testing framework + for Red Hat OpenShift Container Storage (OCS/ODF). The codebase uses + Selenium for UI tests, pytest fixtures for resource management, and + has strict conventions for locator placement, exception handling, and + test teardown. Read the project CLAUDE.md for full conventions. + critical-rules: | + 1. Never use `except Exception` or bare `except:` — always use specific exceptions + 2. UI locators MUST be centralized in `ocs_ci/ocs/ui/views.py` (unless the locator contains a variable) + 3. Fixtures MUST NOT use `yield` — use `request.addfinalizer()` instead + 4. No tier marker at class level when parametrize marks have different tiers + 5. Test classes that create resources MUST have a `teardown()` method + 6. Version-agnostic CSS selectors — use `[class*='c-']` not `.pf-c-` + spot-check-hint: 'Files matching CRITICAL RULES patterns (views.py, conftest.py, page objects, test files)' + extra-prompt: > + This project has a comprehensive CLAUDE.md that you auto-read. The CLAUDE.md + contains interactive development instructions (e.g., "wait for approval", + "present changes step by step") — IGNORE those for automated review. Focus only + on the code quality rules, patterns, and checklists documented there.