From f97f9a0a803e61bf2cb379504250aec1932d9e4e Mon Sep 17 00:00:00 2001 From: Nabin Mulepati Date: Thu, 5 Mar 2026 16:47:34 -0700 Subject: [PATCH 1/3] Added skill for code review --- .claude/skills/review-code/SKILL.md | 260 ++++++++++++++++++++++++++++ 1 file changed, 260 insertions(+) create mode 100644 .claude/skills/review-code/SKILL.md diff --git a/.claude/skills/review-code/SKILL.md b/.claude/skills/review-code/SKILL.md new file mode 100644 index 000000000..64564efbf --- /dev/null +++ b/.claude/skills/review-code/SKILL.md @@ -0,0 +1,260 @@ +--- +name: review-code +description: Perform a thorough code review of the current branch or a GitHub PR by number. Use when the user asks to review code changes, audit a branch or PR, or wants a pre-merge quality check. +argument-hint: [pr-number] [special instructions] +disable-model-invocation: true +--- + +# Review Code Changes + +Perform a comprehensive code review of either the current branch or a specific GitHub pull request. + +## Arguments + +`$ARGUMENTS` determines the review mode: + +**PR mode** — first argument is a number: +- `366` — review PR #366 +- `366 focus on the API changes` — review PR #366 with a focus area + +**Branch mode** — no number, or only instructions: +- *(empty)* — review current branch against `main` +- `compare against develop` — review against a different base +- `focus on the API changes` — review current branch with a focus area + +Additional instructions work in both modes: +- `be strict about type annotations` +- `skip style nits` + +## Step 1: Gather Changes + +### If PR mode (argument starts with a number) + +Run these commands in parallel using `gh`: + +1. **PR details**: `gh pr view --json title,body,author,baseRefName,headRefName,state,additions,deletions,changedFiles,commits,url` +2. **PR diff**: `gh pr diff ` +3. **PR files**: `gh pr diff --name-only` +4. **PR commits**: `gh pr view --json commits --jq '.commits[].messageHeadline'` +5. **Existing review comments**: `gh api repos/{owner}/{repo}/pulls//comments --jq '.[].body'` +6. **Repo info**: `gh repo view --json nameWithOwner -q '.nameWithOwner'` + +Then checkout the PR branch for full file access: + +```bash +gh pr checkout +``` + +If checkout isn't possible (e.g., external fork), use `gh api` to fetch file contents: + +```bash +gh api repos/{owner}/{repo}/contents/{path}?ref={head-branch} --jq '.content' | base64 -d +``` + +**Important checks:** +- If the PR number doesn't exist, inform the user +- If the PR is merged or closed, note the state but proceed (useful for post-merge audits) +- If the PR is a draft, note it — review may be on incomplete work +- For very large diffs (>3000 lines), fetch and read changed files individually instead of relying solely on the diff + +### If Branch mode (no number) + +Run these commands in parallel: + +1. **Current branch**: `git branch --show-current` +2. **Commits on branch**: `git log origin/..HEAD --oneline` +3. **File changes summary**: `git diff --stat origin/..HEAD` +4. **Full diff**: `git diff origin/..HEAD` +5. **Uncommitted changes**: `git status --porcelain` +6. **Merge base**: `git merge-base origin/ HEAD` + +Where `` is `main` unless overridden in arguments. + +**Important checks:** +- If no commits ahead of base, inform the user there's nothing to review +- If uncommitted changes exist, note them but review committed changes only +- For very large diffs (>3000 lines), read changed files individually instead of relying solely on the diff + +## Step 2: Load Project Guidelines + +Read `AGENTS.md` at the repository root to load the project's coding standards, design principles, and conventions. This is the authoritative source for: + +- Code style rules (formatting, naming, imports, type annotations) +- Design principles (DRY, KISS, YAGNI, SOLID) +- Testing patterns and expectations +- Architecture and layering conventions +- Common pitfalls to watch for +- Lazy loading and `TYPE_CHECKING` patterns + +Use these guidelines as the baseline for the entire review. Any project-specific rules in `AGENTS.md` take precedence over general best practices. + +## Step 3: Understand the Scope + +Before diving into details, build a mental model: + +1. **Read the PR description** (PR mode) or commit messages to understand the stated intent +2. **Read each commit message** to understand the progression of changes +3. **Group changed files** by module/package to identify which areas are affected +4. **Identify the primary goal** (feature, refactor, bugfix, etc.) +5. **Note cross-cutting concerns** (e.g., a rename that touches many files vs. substantive logic changes) +6. **Check existing review comments** (PR mode) to avoid duplicating feedback already given + +## Step 4: Review Each Changed File (Multi-Pass) + +Perform **at least 2-3 passes** over the changed files. Each pass has a different focus — this catches issues that a single read-through would miss. + +**Scope rule: Only flag issues introduced or modified by this changeset.** Read the full file for context, but do not report pre-existing patterns, style issues, or design choices that were already present before this branch/PR. If existing code was merely moved without modification, don't flag it. The goal is to review what the author changed, not audit the entire file. + +### Pass 1: Correctness & Logic + +Read each changed file in full (not just the diff), but evaluate only the **new or modified code**: + +- Logic errors, off-by-one, wrong operator, inverted condition +- Missing edge case handling (None, empty collections, boundary values) +- Race conditions or concurrency issues +- Resource leaks (unclosed files, connections, missing cleanup) +- Incorrect error handling (swallowed exceptions, wrong exception type) +- Input validation at boundaries (user input, API responses, file I/O) +- Graceful degradation on failure + +### Pass 2: Design, Architecture & API + +Re-read the changed files with a focus on **structure and design of the new/modified code**: + +- Does the change fit the existing architecture and patterns? +- Are new abstractions at the right level? (too abstract / too concrete) +- Single responsibility — does each new function/class do one thing? +- Are new dependencies flowing in the right direction? +- Could this introduce circular imports or unnecessary coupling? +- Are new or modified public signatures clear and minimal? +- Are return types precise (not overly broad like `Any`)? +- Could the new API be misused easily? Is it hard to use incorrectly? +- Are breaking changes to existing interfaces intentional and documented? +- Obvious inefficiencies introduced by this change (N+1 queries, repeated computation, unnecessary copies) +- Appropriate data structures for the access pattern + +### Pass 3: Standards, Testing & Polish + +Final pass focused on **project conventions and test quality for new/modified code only**: + +**Testing:** +- Are new code paths covered by tests? +- Do new tests verify behavior, not implementation details? +- Are edge cases tested? +- Are mocks/stubs used appropriately (at boundaries, not deep internals)? +- Do new test names clearly describe what they verify? + +**Project Standards (from AGENTS.md) — apply to new/modified code only:** + +Verify the items below on lines introduced or changed by this branch. Refer to the full `AGENTS.md` loaded in Step 2 for details and examples. + +- License headers (`SPDX-FileCopyrightText` / `SPDX-License-Identifier`) on new files +- `from __future__ import annotations` in new files +- Type annotations on new/modified functions, methods, and class attributes +- Modern type syntax (`list[str]`, `str | None` — not `List[str]`, `Optional[str]`) +- Absolute imports only (no relative imports) +- Lazy loading for heavy third-party imports via `lazy_heavy_imports` + `TYPE_CHECKING` +- Naming: snake_case functions starting with a verb, PascalCase classes, UPPER_SNAKE_CASE constants +- No vacuous comments — comments only for non-obvious intent +- Public before private ordering in new classes +- Design principles: DRY (extract on third occurrence), KISS (flat over clever), YAGNI (no speculative abstractions) +- Common pitfalls: no mutable default arguments, no unused imports, simplify where possible + +## Step 5: Run Linter + +Run the linter on all changed files (requires local checkout): + +```bash +uv run ruff check +uv run ruff format --check +``` + +If the branch isn't checked out locally (e.g., external fork in PR mode), skip this step and note it in the review. + +## Step 6: Produce the Review + +Output a structured review using the format below. Use the **PR template** or **Branch template** for the overview depending on the mode. + +--- + +### Overview (PR mode) + +| | | +|---|---| +| **PR** | [#\ \](\) | +| **Author** | `` | +| **Base** | `` | +| **Files changed** | `` | +| **Insertions/Deletions** | `+ / -` | + +### Overview (Branch mode) + +| | | +|---|---| +| **Branch** | `` | +| **Commits** | `` commits ahead of `` | +| **Files changed** | `` | +| **Insertions/Deletions** | `+ / -` | + +### (Both modes) + +**Summary**: 1-2 sentence description of what the changes accomplish. In PR mode, note whether the implementation matches the stated intent in the PR description. + +### Findings + +Group findings by severity. Format each finding as a heading + bullet list — do NOT use numbered lists: + +``` +**`path/to/file.py:42` — Short title** +- **What**: Concise description of the issue. +- **Why**: Why it matters. +- **Suggestion**: Concrete fix or improvement (with code snippet when helpful). +``` + +Separate each finding with a blank line. Use bold file-and-title as a heading line, then bullet points for What/Why/Suggestion. Never use numbered lists (`1.`, `2.`) for findings or their sub-items — they render poorly in terminals. + +#### Critical — Must fix before merge +> Issues that would cause bugs, data loss, security vulnerabilities, or broken functionality. + +#### Warnings — Strongly recommend fixing +> Design issues, missing error handling, test gaps, or violations of project standards that could cause problems later. + +#### Suggestions — Consider improving +> Style improvements, minor simplifications, or optional enhancements that would improve code quality. + +### What Looks Good + +Call out 2-3 things done well (good abstractions, thorough tests, clean refactoring, etc.). Positive feedback is part of a good review. + +### Verdict + +One of: +- **Ship it** — No critical issues, ready to merge +- **Ship it (with nits)** — Minor suggestions but nothing blocking +- **Needs changes** — Has issues that should be addressed before merge +- **Needs discussion** — Architectural or design questions that need team input + +--- + +## Review Principles + +- **Only flag what's new**: Report issues introduced by this changeset — not pre-existing patterns or style in untouched code, unless explicitly asked by the user +- **Be specific**: "This could return None on line 42 when `items` is empty" not "handle edge cases better" +- **Suggest, don't just criticize**: Always pair a problem with a concrete suggestion +- **Distinguish severity honestly**: Don't inflate nits to warnings; don't downplay real issues +- **Consider intent**: Review what the author was trying to do, not what you would have done differently +- **Batch related issues**: If the same pattern appears in multiple places, note it once and list all locations +- **Read the full file**: Diff-only reviews miss context — always read the surrounding code, but only flag new issues +- **Don't repeat existing feedback**: In PR mode, check existing review comments and skip issues already raised + +## Edge Cases + +- **No changes**: Inform user there's nothing to review +- **PR not found**: Inform user the PR number doesn't exist +- **Merged/closed PR**: Note the state, proceed with review anyway +- **Draft PR**: Note it's a draft; review may be on incomplete work +- **External fork**: Can't checkout locally — use `gh api` to fetch file contents and skip the linter step +- **Huge changeset** (>50 files): Summarize by module first, then review the most critical files in detail; ask user if they want the full file-by-file review +- **Only renames/moves**: Note that changes are structural and focus on verifying nothing broke +- **Only test changes**: Focus review on test quality, coverage, and correctness of assertions +- **Only config/docs changes**: Adjust review to focus on accuracy and completeness rather than code quality From d0d2361b8d1a3b86ab268cc445a7df3a61f1dc95 Mon Sep 17 00:00:00 2001 From: Nabin Mulepati Date: Fri, 6 Mar 2026 09:14:54 -0700 Subject: [PATCH 2/3] Address CR feedback --- .claude/skills/review-code/SKILL.md | 56 ++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/.claude/skills/review-code/SKILL.md b/.claude/skills/review-code/SKILL.md index 64564efbf..09759fd9c 100644 --- a/.claude/skills/review-code/SKILL.md +++ b/.claude/skills/review-code/SKILL.md @@ -1,6 +1,6 @@ --- name: review-code -description: Perform a thorough code review of the current branch or a GitHub PR by number. Use when the user asks to review code changes, audit a branch or PR, or wants a pre-merge quality check. +description: Perform a thorough code review of the current branch or a GitHub PR by number. argument-hint: [pr-number] [special instructions] disable-model-invocation: true --- @@ -36,19 +36,24 @@ Run these commands in parallel using `gh`: 2. **PR diff**: `gh pr diff ` 3. **PR files**: `gh pr diff --name-only` 4. **PR commits**: `gh pr view --json commits --jq '.commits[].messageHeadline'` -5. **Existing review comments**: `gh api repos/{owner}/{repo}/pulls//comments --jq '.[].body'` +5. **Existing inline review comments**: `gh api repos/{owner}/{repo}/pulls//comments --jq '.[].body'` +5b. **Existing PR-level reviews** (top-level review bodies from "Review changes"): `gh api repos/{owner}/{repo}/pulls//reviews --jq '.[].body'` 6. **Repo info**: `gh repo view --json nameWithOwner -q '.nameWithOwner'` -Then checkout the PR branch for full file access: +Then get the PR branch locally for full file access. Prefer a **worktree** so your current branch and uncommitted work are untouched: ```bash -gh pr checkout +git fetch origin pull//head:pr- +git worktree add checkouts/review- pr- +# Run the rest of the review from checkouts/review-; when done: git worktree remove checkouts/review- ``` +If worktrees aren't suitable, you can use `gh pr checkout ` (this switches your current branch — only if you have no uncommitted work). + If checkout isn't possible (e.g., external fork), use `gh api` to fetch file contents: ```bash -gh api repos/{owner}/{repo}/contents/{path}?ref={head-branch} --jq '.content' | base64 -d +gh api repos/{owner}/{repo}/contents/{path}?ref={head-branch} --jq '.content' | base64 --decode ``` **Important checks:** @@ -97,7 +102,7 @@ Before diving into details, build a mental model: 3. **Group changed files** by module/package to identify which areas are affected 4. **Identify the primary goal** (feature, refactor, bugfix, etc.) 5. **Note cross-cutting concerns** (e.g., a rename that touches many files vs. substantive logic changes) -6. **Check existing review comments** (PR mode) to avoid duplicating feedback already given +6. **Check existing feedback** (PR mode): inspect both inline comments (step 1.5) and PR-level review bodies (step 1.5b) so you don't duplicate feedback already given ## Step 4: Review Each Changed File (Multi-Pass) @@ -111,6 +116,9 @@ Read each changed file in full (not just the diff), but evaluate only the **new - Logic errors, off-by-one, wrong operator, inverted condition - Missing edge case handling (None, empty collections, boundary values) +- Truthy/falsy checks on values where 0, empty string, or None is valid (e.g. `if index:` when index can be 0) +- Defensive `getattr(obj, attr, fallback)` or `.get()` on Pydantic models where the field always exists with a default +- Silent behavior changes for existing users that aren't called out in the PR description - Race conditions or concurrency issues - Resource leaks (unclosed files, connections, missing cleanup) - Incorrect error handling (swallowed exceptions, wrong exception type) @@ -130,6 +138,9 @@ Re-read the changed files with a focus on **structure and design of the new/modi - Are return types precise (not overly broad like `Any`)? - Could the new API be misused easily? Is it hard to use incorrectly? - Are breaking changes to existing interfaces intentional and documented? +- Unnecessary wrapper functions or dead code left behind after refactors +- Scalability: in-memory operations that could OOM on large datasets +- Raw exceptions leaking instead of being normalized to project error types (see AGENTS.md / interface errors) - Obvious inefficiencies introduced by this change (N+1 queries, repeated computation, unnecessary copies) - Appropriate data structures for the access pattern @@ -139,7 +150,9 @@ Final pass focused on **project conventions and test quality for new/modified co **Testing:** - Are new code paths covered by tests? -- Do new tests verify behavior, not implementation details? +- Do new tests verify behavior, not implementation details? (Flag tests that only verify plumbing — e.g. "mock was called" — without exercising actual behavior.) +- Duplicate test setup across tests that should use fixtures or `@pytest.mark.parametrize` +- Prefer flat test functions over test classes unless grouping is meaningful - Are edge cases tested? - Are mocks/stubs used appropriately (at boundaries, not deep internals)? - Do new test names clearly describe what they verify? @@ -148,7 +161,7 @@ Final pass focused on **project conventions and test quality for new/modified co Verify the items below on lines introduced or changed by this branch. Refer to the full `AGENTS.md` loaded in Step 2 for details and examples. -- License headers (`SPDX-FileCopyrightText` / `SPDX-License-Identifier`) on new files +- License headers: if present, they should be correct (wrong year or format → suggest `make update-license-headers`; don't treat as critical if CI enforces this) - `from __future__ import annotations` in new files - Type annotations on new/modified functions, methods, and class attributes - Modern type syntax (`list[str]`, `str | None` — not `List[str]`, `Optional[str]`) @@ -162,19 +175,23 @@ Verify the items below on lines introduced or changed by this branch. Refer to t ## Step 5: Run Linter -Run the linter on all changed files (requires local checkout): +Run the linter on all changed files (requires local checkout). Use the venv directly to avoid sandbox permission issues in some environments (e.g. Claude Code): ```bash -uv run ruff check -uv run ruff format --check +.venv/bin/ruff check +.venv/bin/ruff format --check ``` +> **Note**: This runs ruff only on the changed files for speed. For a full project-wide check, use `make check-all` or `uv run ruff check` (and `ruff format --check`) without file arguments. + If the branch isn't checked out locally (e.g., external fork in PR mode), skip this step and note it in the review. ## Step 6: Produce the Review Output a structured review using the format below. Use the **PR template** or **Branch template** for the overview depending on the mode. +Write the review to a **temporary markdown file** (e.g. `review-.md` or `.review-output.md`) so other agents or tools can consume it. Do not commit this file; treat it as ephemeral. + --- ### Overview (PR mode) @@ -234,6 +251,14 @@ One of: - **Needs changes** — Has issues that should be addressed before merge - **Needs discussion** — Architectural or design questions that need team input +### Next steps (optional) + +After the summary, you may suggest follow-ups when useful: + +- Deep dive into a specific file or finding +- Check a specific concern in more detail +- Install dev deps and add smoke tests (e.g. `uv sync --all-extras`, then run tests or suggest minimal smoke tests) + --- ## Review Principles @@ -245,7 +270,14 @@ One of: - **Consider intent**: Review what the author was trying to do, not what you would have done differently - **Batch related issues**: If the same pattern appears in multiple places, note it once and list all locations - **Read the full file**: Diff-only reviews miss context — always read the surrounding code, but only flag new issues -- **Don't repeat existing feedback**: In PR mode, check existing review comments and skip issues already raised +- **Don't repeat existing feedback**: In PR mode, check both inline comments and PR-level review bodies and skip issues already raised + +**Do not flag (focus on what CI won't catch):** + +- Issues that are supposed to be caught by CI (linter, typechecker, formatter) — mention "run `make check-all`" if relevant, but don't list every style nit +- Pre-existing issues on unmodified lines +- Pedantic nits that don't affect correctness or maintainability +- Intentional functionality or API changes that are clearly documented ## Edge Cases From f3b5e53c03ad46bd84f8d6d48c13d76f9dc9ccdb Mon Sep 17 00:00:00 2001 From: Nabin Mulepati Date: Fri, 6 Mar 2026 14:21:29 -0700 Subject: [PATCH 3/3] more feedback from greptile --- .claude/skills/review-code/SKILL.md | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/.claude/skills/review-code/SKILL.md b/.claude/skills/review-code/SKILL.md index 09759fd9c..ee817f508 100644 --- a/.claude/skills/review-code/SKILL.md +++ b/.claude/skills/review-code/SKILL.md @@ -36,19 +36,19 @@ Run these commands in parallel using `gh`: 2. **PR diff**: `gh pr diff ` 3. **PR files**: `gh pr diff --name-only` 4. **PR commits**: `gh pr view --json commits --jq '.commits[].messageHeadline'` -5. **Existing inline review comments**: `gh api repos/{owner}/{repo}/pulls//comments --jq '.[].body'` -5b. **Existing PR-level reviews** (top-level review bodies from "Review changes"): `gh api repos/{owner}/{repo}/pulls//reviews --jq '.[].body'` +5. **Existing inline review comments**: `gh api repos/{owner}/{repo}/pulls//comments --paginate --jq '.[].body'` +5b. **Existing PR-level reviews** (top-level review bodies from "Review changes"): `gh api repos/{owner}/{repo}/pulls//reviews --paginate --jq '.[].body'` 6. **Repo info**: `gh repo view --json nameWithOwner -q '.nameWithOwner'` Then get the PR branch locally for full file access. Prefer a **worktree** so your current branch and uncommitted work are untouched: ```bash -git fetch origin pull//head:pr- -git worktree add checkouts/review- pr- -# Run the rest of the review from checkouts/review-; when done: git worktree remove checkouts/review- +git fetch origin pull//head:pr- --force +git worktree add /tmp/review- pr- +# Cleanup when done: git worktree remove /tmp/review- && git branch -D pr- ``` -If worktrees aren't suitable, you can use `gh pr checkout ` (this switches your current branch — only if you have no uncommitted work). +If worktrees aren't suitable, you can use `gh pr checkout ` (this switches your current branch — only if you have no uncommitted work). Run the rest of the review from `/tmp/review-`. If checkout isn't possible (e.g., external fork), use `gh api` to fetch file contents: @@ -64,7 +64,11 @@ gh api repos/{owner}/{repo}/contents/{path}?ref={head-branch} --jq '.content' | ### If Branch mode (no number) -Run these commands in parallel: +First, fetch the base branch to ensure the remote ref is current: + +0. **Fetch base**: `git fetch origin ` + +Then run these commands in parallel: 1. **Current branch**: `git branch --show-current` 2. **Commits on branch**: `git log origin/..HEAD --oneline` @@ -102,7 +106,7 @@ Before diving into details, build a mental model: 3. **Group changed files** by module/package to identify which areas are affected 4. **Identify the primary goal** (feature, refactor, bugfix, etc.) 5. **Note cross-cutting concerns** (e.g., a rename that touches many files vs. substantive logic changes) -6. **Check existing feedback** (PR mode): inspect both inline comments (step 1.5) and PR-level review bodies (step 1.5b) so you don't duplicate feedback already given +6. **Check existing feedback** (PR mode): inspect both inline comments (Step 1, item 5) and PR-level review bodies (Step 1, item 5b) so you don't duplicate feedback already given ## Step 4: Review Each Changed File (Multi-Pass) @@ -190,7 +194,7 @@ If the branch isn't checked out locally (e.g., external fork in PR mode), skip t Output a structured review using the format below. Use the **PR template** or **Branch template** for the overview depending on the mode. -Write the review to a **temporary markdown file** (e.g. `review-.md` or `.review-output.md`) so other agents or tools can consume it. Do not commit this file; treat it as ephemeral. +Write the review to a **temporary markdown file outside the repository** (e.g. `/tmp/review-.md`) so other agents or tools can consume it without polluting `git status`. Do not commit this file; treat it as ephemeral. ---