feat(rules): add code-review.md rule to common rules#927
feat(rules): add code-review.md rule to common rules#927xingzihai wants to merge 1 commit intoaffaan-m:mainfrom
Conversation
- Add comprehensive code review standards for all languages - Define when to review (after code changes, before commits) - Include security review triggers and severity levels - Reference relevant agents (code-reviewer, security-reviewer, etc.) - Add review checklist covering security, quality, and performance - Define approval criteria (Approve/Warning/Block) This rule complements the existing code-reviewer agent by providing clear guidelines on when and how to conduct code reviews.
📝 WalkthroughWalkthroughA new documentation file establishing code review standards has been added, defining mandatory review triggers, pre-merge checklists covering readability and quality checks, security review conditions, severity levels with corresponding actions, an agent delegation table, review workflow steps, and common issues to address during reviews. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds Key findings:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Code Written / Modified] --> B{Security-sensitive\nchange?}
B -- Yes --> C[Use security-reviewer agent]
B -- No --> D[Run git diff]
C --> D
D --> E[Security Checklist]
E --> F[Code Quality Checklist]
F --> G[Run Tests\nVerify ≥80% coverage]
G --> H[Use appropriate agent\ncode-reviewer / language-reviewer]
H --> I{Issues Found?}
I -- CRITICAL --> J[🚫 BLOCK - Must fix before merge]
I -- HIGH --> K[⚠️ WARN - Should fix before merge]
I -- MEDIUM --> L[ℹ️ INFO - Consider fixing]
I -- LOW --> M[📝 NOTE - Optional]
I -- None --> N[✅ APPROVE]
K --> O{Proceed?}
O -- Fixed --> N
O -- Merge with caution --> P[⚠️ WARNING decision]
Reviews (1): Last reviewed commit: "feat(rules): add code-review.md rule to ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - Unbounded queries - add constraints | ||
| - Missing caching - cache expensive operations | ||
|
|
||
| ## Approval Criteria |
There was a problem hiding this comment.
The severity table on line 48 defines HIGH as "Should fix before merge" with action WARN, yet the Approval Criteria here says HIGH issues only result in a "Warning (merge with caution)". These two definitions directly contradict each other: one says a HIGH issue must be resolved before merging, while the other allows merging with a warning.
Pick one consistent stance. If HIGH issues truly block merging, change the Approval Criteria to:
| - Unbounded queries - add constraints | |
| - Missing caching - cache expensive operations | |
| ## Approval Criteria | |
| - **Approve**: No CRITICAL or HIGH issues | |
| - **Warning**: Only MEDIUM issues (merge with caution) | |
| - **Block**: CRITICAL or HIGH issues found |
Alternatively, if merging with caution on HIGH issues is acceptable, update the severity table's action column for HIGH from "Should fix before merge" to "May merge with caution".
| |-------|---------| | ||
| | **code-reviewer** | General code quality, patterns, best practices | | ||
| | **security-reviewer** | Security vulnerabilities, OWASP Top 10 | | ||
| | **typescript-reviewer** | TypeScript/JavaScript specific issues | | ||
| | **python-reviewer** | Python specific issues | | ||
| | **go-reviewer** | Go specific issues | | ||
| | **rust-reviewer** | Rust specific issues | |
There was a problem hiding this comment.
agents.md
The master rules/common/agents.md rule defines the canonical list of available agents. The agents typescript-reviewer, python-reviewer, and go-reviewer are listed here but are absent from that master list (which only includes rust-reviewer among language-specific reviewers).
Additionally, several language-specific agents that exist in the agents/ directory — kotlin-reviewer, cpp-reviewer, java-reviewer, flutter-reviewer — are silently omitted. Readers may not know they are available.
Consider either:
- Aligning this table with the full list in
agents.md, or - Adding a note such as "See
agents.mdfor the complete list of available agents" instead of duplicating a partial subset.
Context Used: rules/common/agents.md (source)
| - [testing.md](testing.md) - Test coverage requirements | ||
| - [security.md](security.md) - Security checklist | ||
| - [git-workflow.md](git-workflow.md) - Commit standards | ||
| - [agents.md](agents.md) - Agent delegation No newline at end of file |
There was a problem hiding this comment.
The file does not end with a newline character. Most editors and POSIX tools expect a trailing newline, and it is consistent with every other file in rules/common/.
| - [agents.md](agents.md) - Agent delegation | |
| - [agents.md](agents.md) - Agent delegation |
(Add a blank line after this line to ensure the file ends with a newline.)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rules/common/code-review.md (1)
12-13: Tighten reviewer-trigger wording to match repo practice.Current wording scopes commit-time checks to shared branches/security triggers. Consider explicitly requiring
code-reviewerafter code changes andsecurity-reviewerbefore commits involving sensitive areas, so this rule stays aligned with existing team workflow guidance.Based on learnings: “Use code-reviewer agent immediately after writing or modifying code…” and “Use security-reviewer agent before committing any code, especially for security-sensitive code…”.
Also applies to: 33-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rules/common/code-review.md` around lines 12 - 13, Update the rule wording to explicitly require running the code-reviewer agent immediately after writing or modifying code and to require running the security-reviewer agent before committing any changes that touch security-sensitive areas (auth, payments, user data); replace the current vague phrases ("Before any commit to shared branches" / "When security-sensitive code is changed") with explicit instructions referencing the code-reviewer and security-reviewer agents, and apply the same wording change to the related section covering lines 33-41.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rules/common/code-review.md`:
- Around line 106-107: The document has conflicting guidance: the rule that
"HIGH issues should be fixed before merge" and the approval criteria bullet
"**Warning**: Only HIGH issues (merge with caution)" disagree; update the
approval criteria so HIGH severity is treated as blocking (or else change the
severity rule) to be consistent. Specifically, replace the approval-criteria
bullet "**Warning**: Only HIGH issues (merge with caution)" with wording that
HIGH issues must be resolved before merge (or change the severity policy to
match the existing warning text), ensuring the "**Block**: CRITICAL issues
found" line remains accurate and the two statements are consistent.
---
Nitpick comments:
In `@rules/common/code-review.md`:
- Around line 12-13: Update the rule wording to explicitly require running the
code-reviewer agent immediately after writing or modifying code and to require
running the security-reviewer agent before committing any changes that touch
security-sensitive areas (auth, payments, user data); replace the current vague
phrases ("Before any commit to shared branches" / "When security-sensitive code
is changed") with explicit instructions referencing the code-reviewer and
security-reviewer agents, and apply the same wording change to the related
section covering lines 33-41.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f48a7124-af87-4a0d-a027-7c7cecbdc0fa
📒 Files selected for processing (1)
rules/common/code-review.md
| - **Warning**: Only HIGH issues (merge with caution) | ||
| - **Block**: CRITICAL issues found |
There was a problem hiding this comment.
Approval criteria conflicts with your HIGH-severity rule.
Line 48 says HIGH issues should be fixed before merge, but Line 106 allows merge with only HIGH issues present. This creates contradictory enforcement and weakens the standard.
Suggested doc fix
-- **Warning**: Only HIGH issues (merge with caution)
+- **Warning**: Only MEDIUM/LOW issues (merge with caution)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Warning**: Only HIGH issues (merge with caution) | |
| - **Block**: CRITICAL issues found | |
| - **Warning**: Only MEDIUM/LOW issues (merge with caution) | |
| - **Block**: CRITICAL issues found |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rules/common/code-review.md` around lines 106 - 107, The document has
conflicting guidance: the rule that "HIGH issues should be fixed before merge"
and the approval criteria bullet "**Warning**: Only HIGH issues (merge with
caution)" disagree; update the approval criteria so HIGH severity is treated as
blocking (or else change the severity rule) to be consistent. Specifically,
replace the approval-criteria bullet "**Warning**: Only HIGH issues (merge with
caution)" with wording that HIGH issues must be resolved before merge (or change
the severity policy to match the existing warning text), ensuring the
"**Block**: CRITICAL issues found" line remains accurate and the two statements
are consistent.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="rules/common/code-review.md">
<violation number="1" location="rules/common/code-review.md:106">
P2: HIGH severity handling is inconsistent: one section says fix before merge, another allows merge with caution for HIGH-only findings.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ## Approval Criteria | ||
|
|
||
| - **Approve**: No CRITICAL or HIGH issues | ||
| - **Warning**: Only HIGH issues (merge with caution) |
There was a problem hiding this comment.
P2: HIGH severity handling is inconsistent: one section says fix before merge, another allows merge with caution for HIGH-only findings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At rules/common/code-review.md, line 106:
<comment>HIGH severity handling is inconsistent: one section says fix before merge, another allows merge with caution for HIGH-only findings.</comment>
<file context>
@@ -0,0 +1,116 @@
+## Approval Criteria
+
+- **Approve**: No CRITICAL or HIGH issues
+- **Warning**: Only HIGH issues (merge with caution)
+- **Block**: CRITICAL issues found
+
</file context>
|
thanks, queued for review. |
2 participants
Summary
This PR adds a new
code-review.mdrule to therules/common/directory.What's Added
A comprehensive code review standards rule that:
Why This Rule?
While the project has a detailed
code-revieweragent, there was no corresponding rule file inrules/common/that defines:This rule fills that gap and works alongside the existing agent.
Integration
This rule references and integrates with:
testing.md- Test coverage requirementssecurity.md- Security checklistgit-workflow.md- Commit standardsagents.md- Agent delegationTesting
rules/common/)Added by contributor as part of open source contribution efforts.
Summary by CodeRabbit