-
Notifications
You must be signed in to change notification settings - Fork 191
Add Claude AI code review workflow #14708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pointed out above, this is valid case to use yield |
||
| - 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 | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -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: <ORG>/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 | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would change to something like this
Suggested change
|
||||||
| 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 | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As pointed bellow , this probably should not be a blocker. But if it's used we should log the exception out: something like this. So if something will debugged we see what is happening. |
||||||
| 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 | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why yield is not acceptable? I think it's a valid scenario which we use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| 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. | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l don't see this as blocker, sometime we use this.
We definitely should not allow diabolic anti pattern like:
Better to always use specific exception whenever it's possible, but sometimes there are so many possibilities that catching generic Exception is only one solution to avoid many different unpredictable exceptions.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tool should only add comments, not change requests, so it is not real blocker. using braod exceptions makes debug much harder and sometimes lets test that should fail to pass