-
-
Notifications
You must be signed in to change notification settings - Fork 20
Update code review guide #229
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: main
Are you sure you want to change the base?
Changes from all commits
496fd02
cb881af
377181e
f3c5ef0
1f0afcd
bcc906c
3e57f86
100302d
3ff292b
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,55 @@ | ||
| # -*- mode: yaml -*- | ||
|
|
||
| manifest: | ||
| version: 1.0 | ||
|
|
||
| automations: | ||
| explain_code_experts: | ||
| on: | ||
| - pr_created | ||
| - pr_ready_for_review | ||
| - commit | ||
| if: | ||
| - {{ pr.labels | match(term='suggest-reviewer') | some }} | ||
| run: | ||
| - action: explain-code-experts@v1 | ||
| args: | ||
| gt: 10 | ||
|
|
||
| linearb_ai_review: | ||
| # Triggers only when PR is created or has new commits, see docs | ||
| # https://docs.gitstream.cm/execution-model/#action-level-execution-control | ||
| if: | ||
| - {{ not pr.draft }} | ||
| - {{ not (is.bot_author or is.bot_branch) }} | ||
| run: | ||
| - action: code-review@v1 | ||
| guidelines: | | ||
| - Use short inline comments attached to the exact line with the issue. | ||
| - Prefer many small comments over long summaries. | ||
|
|
||
| linearb_ai_desc: | ||
| if: | ||
| - {{ not pr.draft }} | ||
| run: | ||
| - action: describe-changes@v1 | ||
| args: | ||
| concat_mode: append | ||
|
|
||
| estimated_time_to_review: | ||
| if: | ||
| - true | ||
| run: | ||
| - action: add-label@v1 | ||
| args: | ||
| label: "{{ calc.etr }} min review" | ||
| color: {{ colors.red if (calc.etr >= 20) else ( colors.yellow if (calc.etr >= 5) else colors.green ) }} | ||
|
|
||
| calc: | ||
| etr: {{ branch | estimatedReviewTime }} | ||
|
|
||
|
|
||
| colors: | ||
| red: 'b60205' | ||
| yellow: 'fbca04' | ||
| green: '0e8a16' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # CodeRabbit configuration | ||
| # Exclude benchmark files from code review | ||
|
|
||
| paths: | ||
| ignore: | ||
| - "examples/the-ultimate-guide-to-code-review-tools/code-review-benchmark/**" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| # Code generated by gitStream GitHub app - DO NOT EDIT | ||
|
|
||
| name: gitStream workflow automation | ||
| run-name: | | ||
| /:\ gitStream: PR #${{ fromJSON(fromJSON(github.event.inputs.client_payload)).pullRequestNumber }} from ${{ github.event.inputs.full_repository }} | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| client_payload: | ||
| description: The Client payload | ||
| required: true | ||
| full_repository: | ||
| description: the repository name include the owner in `owner/repo_name` format | ||
| required: true | ||
| head_ref: | ||
| description: the head sha | ||
| required: true | ||
| base_ref: | ||
| description: the base ref | ||
| required: true | ||
| installation_id: | ||
| description: the installation id | ||
| required: false | ||
| resolver_url: | ||
| description: the resolver url to pass results to | ||
| required: true | ||
| resolver_token: | ||
| description: Optional resolver token for resolver service | ||
| required: false | ||
| default: '' | ||
|
|
||
| jobs: | ||
| gitStream: | ||
| timeout-minutes: 15 | ||
| runs-on: ubuntu-latest | ||
| name: gitStream workflow automation | ||
| steps: | ||
| - name: Adding PR Url | ||
| run: | | ||
| echo '[${{ fromJSON(fromJSON(inputs.client_payload)).repo }}#${{ fromJSON(fromJSON(inputs.client_payload)).prContext.number }}](${{ fromJSON(fromJSON(inputs.client_payload)).prContext.url }}) - `${{ fromJSON(fromJSON(inputs.client_payload)).branch }}` by ${{ fromJSON(fromJSON(inputs.client_payload)).prContext.author }}' >> $GITHUB_STEP_SUMMARY | ||
|
|
||
| - name: Evaluate Rules | ||
| uses: linear-b/gitstream-github-action@v2 | ||
| id: rules-engine | ||
| with: | ||
| full_repository: ${{ github.event.inputs.full_repository }} | ||
| head_ref: ${{ github.event.inputs.head_ref }} | ||
| base_ref: ${{ github.event.inputs.base_ref }} | ||
| client_payload: ${{ github.event.inputs.client_payload }} | ||
| installation_id: ${{ github.event.inputs.installation_id }} | ||
| resolver_url: ${{ github.event.inputs.resolver_url }} | ||
| resolver_token: ${{ github.event.inputs.resolver_token }} |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,6 +107,65 @@ GitHub Copilot for Code Reviews isn't just an AI tool; it's your smart coding co | |
| - **AI Imperfections**: It's smart, but not perfect. | ||
| - **Understanding AI**: Knowing its capabilities and limitations is key. | ||
|
|
||
| ## 🧩 LinearB gitStream | ||
| > Automation Powerhouse — but at a Cost | ||
|
|
||
| gitStream offers a highly configurable, automation-heavy approach to improving pull requests. It shines when you know exactly what you want to enforce, but it demands more time and process maturity than most tools. | ||
|
|
||
| ### Why LinearB Stands Out | ||
|
|
||
| - *Extensive Automations:* A large catalog of rules, triggers, and workflows you can tailor to almost any PR scenario. | ||
|
|
||
| - *Strong When Paired with LinearB:* Delivers more value when used together with LinearB metrics and workflow tools. | ||
|
|
||
| - *Deep Configurability:* If your team has mature processes and clear standards, gitStream lets you enforce them precisely. | ||
|
|
||
| ### But Keep in Mind | ||
| - *Expensive:* Best suited for larger teams that can justify the cost through advanced metrics and automation needs. | ||
|
|
||
| - *Hard Initial Setup:* Requires significant time investment. If you’re not sure what to configure, it’s easy to get lost. | ||
|
|
||
| - *Visual Overload:* Feedback can get buried or feel scattered inside the PR. | ||
|
|
||
| - *Partial Detection:* In our tests, it detected only some code smells and security issues—not all of them. | ||
|
|
||
| ### gitStream in Action | ||
| Ideal for teams with stable branching strategies, well-defined workflows, and a need for strict, automated governance. Less helpful for small teams or evolving processes. | ||
|
|
||
| You can see gitStream in action in the following Pull Request: | ||
|
|
||
| - [gitStream Code Review Example](https://github.com/rosariosm/devops-reference/pull/4) | ||
|
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. 🧩 Analysis chainScript executed: #!/bin/bash
# First, let's examine the README.md file to see the full context around lines 137 and 166
fd README.md | head -5Repository: nanlabs/devops-reference <shell_result> nanlabs/devops-reference pull request #4 gitStream code review <web_search_result> While you share that, here’s a focused checklist I’ll use for a gitStream-related pull request review (so you know what I’ll check):
If you post the PR link or paste the diff I’ll run a targeted review and give line-by-line suggestions. Sources: gitStream docs (how it works, automation library). [1][2] References rosariosm/devops-reference pull request #4 gitStream <web_search_result>
Which would you like? github.com rosariosm devops-reference <web_search_result> github.com nanlabs devops-reference <web_search_result>
If you meant a different repo name or organization (for example "NaN Labs", "nanfor", or "nanlabs" with a different repo slug), tell me the exact repo URL or full name and I’ll fetch it. Based on my verification attempts, I have found critical information about the links in question: Both Rewritten Review CommentThe example PR links reference repositories that do not exist or are not publicly accessible. The links to
🤖 Prompt for AI Agents |
||
|
|
||
| To test the tool, a set of intentionally "smelly" files were created in the [`code-review-benchmark`](./code-review-benchmark) folder, containing code smells, poor patterns, and security vulnerabilities. | ||
|
|
||
| ## 🌿 Qodo Merge | ||
| > Lightweight, Clean, and Surprisingly Pleasant to Use | ||
| Qodo Merge focuses on providing a simple, clean, AI-assisted review experience with minimal setup effort. | ||
|
|
||
| ### Why Qodo Merge Is Great | ||
|
|
||
| - *Very Easy to Set Up:* You can get started quickly without dealing with heavy configuration. | ||
|
|
||
| - *Great Visual Experience:* Results feel cleaner and more readable compared to other tools. | ||
|
|
||
| - *Lower Configuration Load:* Less to tweak, less to maintain. | ||
|
|
||
| - *Free for Open Source:* A strong advantage for public repos. | ||
|
|
||
| ### But Keep in Mind | ||
| - *Shallow Detection:* Similar to gitStream, it only caught a subset of code smells and security issues in our tests. | ||
|
|
||
| - *Less Control:* With fewer configuration options, it’s harder to enforce advanced or highly specific workflows. | ||
|
|
||
| ### Qodo Merge in Action | ||
|
|
||
| Great when you want a no-friction, visually clean solution that “just works,” without the overhead of more complex tools. | ||
|
|
||
| You can see Qodo Merge in action in the following Pull Request: | ||
|
|
||
| - [Qodo Merge Code Review Example](https://github.com/rosariosm/devops-reference/pull/2) | ||
|
|
||
| The same smelly and insecure test files from the [`code-review-benchmark`](./code-review-benchmark) folder used for gitStream were also used to evaluate Qodo Merge. | ||
| --- | ||
|
|
||
| Feel the power of modern code review tools and choose the one that fits your team's needs. Dive in and revolutionize your code review process today! | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| # Code Review Benchmark Examples | ||
|
|
||
| This directory contains minimal TypeScript example files designed for benchmarking AI code review tools on GitHub. These files are **not intended to be run or compiled** - they are standalone examples for testing how well code review tools can identify issues. | ||
|
|
||
| ## Overview | ||
|
|
||
| Each file is 40-80 lines of realistic code in a payment/order processing domain. The code includes JSDoc comments describing the intended behavior, but the implementations contain subtle (and not-so-subtle) bugs and issues. | ||
|
|
||
| ## Files | ||
|
|
||
| ### `logic-example.ts` | ||
|
|
||
| **Focus:** Logic errors and bugs | ||
|
|
||
| This file contains various logic errors that would cause incorrect behavior at runtime: | ||
|
|
||
| - Boundary condition errors (off-by-one mistakes) | ||
| - Incorrect aggregation calculations | ||
| - Boolean logic errors | ||
| - Edge case handling problems (empty arrays, null values) | ||
| - Incorrect comparison operators | ||
|
|
||
| The code looks reasonable at first glance but contains bugs that would be caught during careful code review or testing. | ||
|
|
||
| ### `security-example.ts` | ||
|
|
||
| **Focus:** Security vulnerabilities | ||
|
|
||
| This file contains security flaws in otherwise functional-looking code: | ||
|
|
||
| - SQL injection vulnerabilities through string concatenation | ||
| - Unsafe handling of user input without validation | ||
| - Sensitive information leakage in error messages and logs | ||
| - Insecure password storage (plain text) | ||
| - Hard-coded secrets and API keys in source code | ||
|
|
||
| The logic is mostly correct, but the security issues would pose significant risks in production. | ||
|
|
||
| ### `maintainability-example.ts` | ||
|
|
||
| **Focus:** Code quality and maintainability issues | ||
|
|
||
| This file contains code that "works" but is difficult to maintain: | ||
|
|
||
| - Very long functions doing multiple things (violating single responsibility) | ||
| - Poor variable naming (generic names like `data`, `result1`) | ||
| - Code duplication across different functions | ||
| - Mixed concerns (business logic, formatting, I/O operations all in one function) | ||
| - Inconsistent coding style (mixing async/await with raw promises) | ||
|
|
||
| The behavior is mostly correct, but the code structure clearly needs refactoring for better maintainability. | ||
|
|
||
| ## Usage | ||
|
|
||
| These files are designed to be used as test cases for evaluating AI code review tools on GitHub (such as CodeRabbit, GitHub Copilot, LinearB gitStream, etc.). Review each file and identify: | ||
|
|
||
| 1. **Logic issues** - What bugs would cause incorrect behavior? | ||
| 2. **Security issues** - What vulnerabilities could be exploited? | ||
| 3. **Maintainability issues** - What refactoring would improve the code? | ||
|
|
||
| ## Note | ||
|
|
||
| These examples are intentionally flawed for benchmarking purposes. Do not use this code in production environments. No build tools or dependencies are required - these are standalone TypeScript files for code review analysis only. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| /** | ||
| * Payment processing service for handling order payments. | ||
| * | ||
| * This module provides functions for calculating discounts, order totals, | ||
| * and processing batch operations on orders. | ||
| */ | ||
|
|
||
| interface Order { | ||
| id: string; | ||
| items: OrderItem[]; | ||
| customerId: string; | ||
| } | ||
|
|
||
| interface OrderItem { | ||
| id: string; | ||
| price: number; | ||
| quantity: number; | ||
| discount?: number; | ||
| } | ||
|
|
||
| /** | ||
| * Calculates the total discount for an order. Applies a 10% discount if order total exceeds $100. | ||
| * | ||
| * @param order - The order to calculate discount for | ||
| * @returns The discount amount as a number | ||
| */ | ||
| export function calculateDiscount(order: Order): number { | ||
| const subtotal = order.items.reduce((sum, item) => sum + item.price * item.quantity, 0); | ||
| if (subtotal > 100) return subtotal * 0.1; | ||
| return 0; | ||
| } | ||
|
|
||
| /** | ||
| * Calculates the final total for an order after discounts. | ||
| * | ||
| * @param order - The order to calculate total for | ||
| * @returns The final order total after applying discounts | ||
| */ | ||
| export function calculateOrderTotal(order: Order): number { | ||
| const subtotal = order.items.reduce((sum, item) => sum + item.price * item.quantity - (item.discount || 0), 0); | ||
| return subtotal - calculateDiscount(order); | ||
| } | ||
|
|
||
| /** | ||
| * Processes a batch of orders and returns the average order value. | ||
| * | ||
| * @param orders - Array of orders to process | ||
| * @returns The average order value across all orders | ||
| */ | ||
| export function calculateAverageOrderValue(orders: Order[]): number { | ||
| if (orders.length === 0) return 0; | ||
| let total = 0; | ||
| for (let i = 0; i <= orders.length; i++) { | ||
| total += calculateOrderTotal(orders[i]); | ||
| } | ||
| return total / orders.length; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| /** | ||
| * Order management service for processing and formatting orders. | ||
| * | ||
| * This module handles order processing, calculation, and notification workflows. | ||
| */ | ||
|
|
||
| interface Order { | ||
| id: string; | ||
| items: OrderItem[]; | ||
| customerId: string; | ||
| status: string; | ||
| } | ||
|
|
||
| interface OrderItem { | ||
| id: string; | ||
| name: string; | ||
| price: number; | ||
| quantity: number; | ||
| } | ||
|
|
||
| /** | ||
| * Main function that processes an order, calculates totals, formats output, sends notifications, and updates the database all in one place. | ||
| * | ||
| * @param orderId - The unique identifier for the order | ||
| * @returns A formatted string representation of the processed order | ||
| */ | ||
| export function processOrder(orderId: string): string { | ||
| const order = getOrderFromDatabase(orderId); | ||
| if (!order) return 'Order not found'; | ||
| let subtotal = 0; | ||
| for (let i = 0; i < order.items.length; i++) { | ||
| subtotal = subtotal + order.items[i].price * order.items[i].quantity; | ||
| } | ||
| const tax = subtotal * 0.085; | ||
| let shipping = subtotal < 50 ? 5.99 : subtotal < 100 ? 3.99 : 0; | ||
| const total = subtotal + tax + shipping; | ||
| updateOrderStatus(orderId, 'processed'); | ||
| let result = `Order #${order.id}\nCustomer: ${order.customerId}\nItems:\n`; | ||
| for (let j = 0; j < order.items.length; j++) { | ||
| result += ` - ${order.items[j].name} x${order.items[j].quantity} @ $${order.items[j].price}\n`; | ||
| } | ||
| result += `Subtotal: $${subtotal.toFixed(2)}\nTax: $${tax.toFixed(2)}\nShipping: $${shipping.toFixed(2)}\nTotal: $${total.toFixed(2)}\n`; | ||
| sendEmailNotification(order.customerId, result); | ||
| console.log(`Order ${orderId} processed: ${result}`); | ||
| return result; | ||
| } | ||
|
|
||
| function getOrderFromDatabase(orderId: string): Order | null { | ||
| return null; | ||
| } | ||
|
|
||
| function updateOrderStatus(orderId: string, status: string): void { | ||
| console.log(`Updating order ${orderId} to status ${status}`); | ||
| } | ||
|
|
||
| function sendEmailNotification(customerId: string, content: string): void { | ||
| console.log(`Sending email to ${customerId}: ${content}`); | ||
| } | ||
|
|
||
| /** | ||
| * Calculates the total value of all items in an order. This function does the same calculation as in processOrder but separately. | ||
| * | ||
| * @param items - Array of order items to calculate total for | ||
| * @returns The total value of all items | ||
| */ | ||
| export function calculateItemsTotal(items: OrderItem[]): number { | ||
| let data = 0; | ||
| for (let i = 0; i < items.length; i++) { | ||
| data = data + items[i].price * items[i].quantity; | ||
| } | ||
| return data; | ||
| } |
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.
Fix configuration schema to match CodeRabbit's expected format.
The current structure uses
paths.ignore, which is not valid according to the CodeRabbit configuration schema. Path filtering should be configured underreviews.path_filtersas an array of glob patterns.Apply this diff to fix the configuration:
Note: In
path_filters, patterns starting with!are excluded from review.📝 Committable suggestion
🤖 Prompt for AI Agents