code-review

MANAGED SKILL — Do not edit this file here. It is managed from an external source repository. To make changes, update the source and run "update my skills" to sync.

Code Review

Analyze a GitHub pull request, review each changed file for issues, and post inline comments plus a summary review directly on the PR.

Also run Phase 6: Resolve Review Comments when the user asks to address open PR review comments, e.g.:

• "leia os comentarios e resolva"
• "resolve os comentarios do PR"
• "fix the review comments"

---

Core Workflow

1. Select the PR (user-provided or list open PRs)
2. Fetch diff and file-level changes
3. Read changed files for full context
4. Analyze changes (severity: Critical / Important / Suggestion)
5. Post review (local chat or pending on GitHub)
6. Optionally resolve review comments in follow-up

---

Reference Guide

Topic	Reference	Load When
Pyramid / Cornice / SQLAlchemy / Marshmallow	pyramid-backend-rules.md	PR contains Python files importing Pyramid, Cornice, SQLAlchemy, or Marshmallow

---

Constraints

MUST DO

• Read the full file for context (not just the diff hunk) before commenting
• Post all comments atomically as a single review (never individual comments)
• Include severity prefix on every inline comment
• Respond in Portuguese

MUST NOT DO

• Auto-submit the review (must remain PENDING for user to confirm)
• Comment on formatting/style that a linter handles
• Invent issues to fill space when code is correct
• Post inline comments on lines not visible in the diff (use file-level comment instead)

---

GitHub API Access

Before performing any GitHub operations, determine which access method is available:

1. GitHub MCP (preferred): Check if a GitHub MCP server is configured by scanning the mcps folder for a server matching github or containing github in its name. If found, verify with a get_me call. Note the server name for all subsequent MCP calls.
2. gh CLI (fallback): Run gh auth status to verify authentication.

Also resolve owner and repo from the git remote:

git remote get-url origin

Parse owner/repo from the URL. Store these values — GitHub MCP tools require them as explicit parameters on every call.

Use the detected method consistently for all operations below. Each operation shows both methods: MCP first, then CLI.

---

Phase 1: Select PR

1.0 Review mode

Before anything else, ask the user what kind of review this is:

title: "Review Mode"
questions:
  - id: mode
    prompt: "Is this review local-only or should it be submitted to GitHub?"
    options:
      - id: local
        label: "Local only — show the review here in the chat"
      - id: github
        label: "Submit to GitHub — create as a pending review for me to confirm in the web UI"

Store the selected mode. It affects Phase 4:

• local: perform the full analysis (Phases 2-3) but do NOT post anything to GitHub. Display the complete review (summary + inline comments) directly in the chat.
• github: perform the full analysis (Phases 2-3) and post the review as a PENDING review on GitHub. The user will review and submit it from the web interface.

1.1 Determine the PR

If the user provided a PR number or URL, extract the number and skip to 1.2.

Otherwise, list recent open PRs:

Via MCP:

CallMcpTool: server=github, toolName=list_pull_requests
arguments: { "owner": "<owner>", "repo": "<repo>", "state": "open", "perPage": 15 }

Via CLI:

gh pr list --limit 15 --json number,title,author,headRefName --template '{{range .}}#{{.number}} {{.title}} ({{.author.login}}, {{.headRefName}}){{"\n"}}{{end}}'

Use AskQuestion to let the user pick:

title: "Select PR"
questions:
  - id: pr
    prompt: "Which pull request should I review?"
    options:
      # one option per PR from the list
      - id: "<number>"
        label: "#<number> — <title> (<author>)"

1.2 Fetch PR metadata

Via MCP:

CallMcpTool: server=github, toolName=pull_request_read
arguments: { "owner": "<owner>", "repo": "<repo>", "pullNumber": <number>, "method": "get" }

Via CLI:

gh pr view <number> --json number,title,body,headRefName,baseRefName,headRefOid,url,additions,deletions,changedFiles

Store the metadata — headRefOid, headRefName, baseRefName, url, and number are used in later phases.

Tell the user which PR you're reviewing: number, title, branch, and size (additions/deletions/files changed).

---

Phase 2: Fetch Context

2.1 Get the diff

Via MCP:

CallMcpTool: server=github, toolName=pull_request_read
arguments: { "owner": "<owner>", "repo": "<repo>", "pullNumber": <number>, "method": "get_diff" }

Via CLI:

gh pr diff <number>

2.2 Get file-level change details

Via MCP:

CallMcpTool: server=github, toolName=pull_request_read
arguments: { "owner": "<owner>", "repo": "<repo>", "pullNumber": <number>, "method": "get_files", "perPage": 100 }

Via CLI:

gh api repos/{owner}/{repo}/pulls/<number>/files --paginate

This returns JSON with filename, status, patch, additions, deletions, and changes for each file.

If using CLI and you need to determine {owner}/{repo}, extract it from the PR URL or from:

gh repo view --json nameWithOwner -q .nameWithOwner

2.3 Read changed files for context

For each changed file (that is not deleted), read the full file from the PR's head branch so you can understand the surrounding code — not just the diff hunks:

Via MCP:

CallMcpTool: server=github, toolName=get_file_contents
arguments: { "owner": "<owner>", "repo": "<repo>", "path": "<filepath>", "ref": "<headRefName>" }

Via CLI:

gh api repos/{owner}/{repo}/contents/<filepath>?ref=<headRefName> --jq '.content' | base64 -d

For large PRs (more than 20 changed files), prioritize:

1. Files with the most additions/changes
2. Source code files over config, lock files, or generated files
3. Skip binary files, vendored dependencies, and auto-generated files

---

Phase 3: Analyze Changes

Review philosophy

You are an experienced reviewer. Review in a direct, objective, pragmatic style:

• Prioritize business rules and correctness
• Avoid premature abstractions
• Prefer simple, readable code over clever code
• Flag architecture violations only when they are real

Language

Always respond in Portuguese.

Tone

• Direct, no hedging.
• Identify the problem → explain why it's a problem → suggest the fix.
• No unnecessary praise ("great work, but...").
• No artificially softened language ("you might want to consider maybe...").
• No final summary in inline comments.

Severity levels

Record findings using three severity levels:

? Critical — blocks merge

• Logic bugs: unhandled edge cases, incorrect conditions, unexpected behavior in critical flows.
• Bug fix without regression test: if the PR fixes a bug but does not include a test that would have caught the bug before the fix, it must be blocked. Every bug fix needs a test that fails without the fix and passes with it.
• New code without tests: any new view, service, or business logic being added without corresponding test coverage must be blocked. Code that ships untested is a liability — if it's worth writing, it's worth testing.
• Commented-out test: a test function or block that is commented out must never be merged. Either fix the test or delete it — commented tests give false confidence that coverage exists when it doesn't.
• Cross-module database access: a module must never query or write to another module's tables directly. E.g., financing importing models or querying tables from charge is a hard boundary violation. Modules communicate through APIs or explicit contracts, never by reaching into each other's database schema.
• Critical anti-patterns: God class, feature envy, hidden side effects. Train wreck calls (a.b().c().d()) are Critical only when they obscure a critical flow or increase coupling — otherwise flag as Important.
• Security: injection vulnerabilities (SQL, command, template), secrets or credentials committed in code, unsafe deserialization, path traversal, SSRF.

? Important — should be fixed

• Premature abstractions: code made too generic before there's a real need. Prefer concrete and simple. Don't create base classes, mixins, or factories without at least 2–3 real use cases.
• Scattered business rules: the same business decision appears duplicated across views, schemas, or services instead of being centralized.
• Codebase pattern violation: if the project has an established pattern (services, repositories, views), code that diverges from it without clear justification should be flagged. Do not defend an existing pattern just because it exists — if the existing pattern is clearly harmful, flag the objective problem instead of the deviation itself.
• Missing tests on critical changes: business logic or integration flows without test coverage.
• Low-quality tests: happy path only, no edge cases; assertions that don't verify real behavior; tests fragile because they're coupled to implementation; excessive mocking that hides the real behavior being tested.
• Flaky / intermittent test risk: watch closely for tests that depend on timing (sleep, time.time()), ordering of unordered collections (sets, dict keys), hardcoded future dates without time freezing, shared mutable state between tests, or external service calls without mocking. These pass locally but fail randomly in CI. Flag them as Important with a concrete explanation of the flakiness vector.
• Complex logic inside tests: tests must be dumb and linear — Arrange, Act, Assert. If a test contains if/else, for, while, try/except, or any branching logic, it's wrong. A test with control flow can silently skip the assertion or test different things on different runs. Split into separate test cases instead.
• Unnecessary complexity / AI-generated code smell: code harder to read than it needs to be. Flag when you see:
  • Functions or classes that do too many things at once
  • Deeply nested logic (more than 2–3 levels) that could be flattened with early returns or extracted functions
  • Over-engineered solutions for simple problems (factory, strategy pattern, or abstract base class for something that could be a plain function)
  • Unnecessary abstractions or indirection that force the reader to jump between files/classes to understand a simple flow
  • Verbose variable names that add noise instead of clarity (e.g., user_object_instance, process_data_result_value)
  • Long functions (> ~30 lines) that mix concerns and are hard to scan
  • Code that "looks smart" but is hard to maintain — clever one-liners, excessive list comprehensions with side effects, chained calls that obscure intent
• Error handling: exceptions caught and silently swallowed, missing error propagation, unhelpful error messages that hide the root cause.

? Suggestion — optional, but relevant

Do not request cosmetic refactors with low practical value. Only suggest naming, structure, or style changes when they improve correctness, readability, or maintainability in a meaningful way.

• Weak naming: variables, functions, or classes that don't communicate intent.
• Minor simplifications that improve readability without changing behavior.
• Logic duplication that already exists elsewhere in the codebase.
• DRY violations — duplicated logic that should be extracted.
• Language-specific idiom violations.

---

What to look for (general)

Review each modified snippet inline — one comment per relevant snippet. If a snippet has nothing worth commenting on, skip it.

Only comment when there is a concrete problem, real risk, or clear simplification with material value. Do not invent issues to fill space.

Bugs and logic errors

• Incorrect conditions, off-by-one errors, wrong variable used
• Race conditions, missing synchronization
• Null/undefined access without guards

Readability and maintainability

• Unclear naming that requires reading the implementation to understand intent
• Overly complex logic that could be simplified
• Dead code, commented-out code left behind
• Missing context where a brief comment would prevent future confusion

Best practices

• Incorrect or missing type annotations (in typed languages)
• Test coverage gaps for non-trivial logic

---

Backend-specific rules (Pyramid / Cornice / SQLAlchemy)

Load pyramid-backend-rules.md when reviewing Python backend files that import Pyramid, Cornice, SQLAlchemy, or Marshmallow. Apply those rules in addition to the general rules above.

---

Project pattern detection

Before criticizing architecture, read the established pattern in the codebase. Flag a deviation only when:

1. The code clearly diverges from the existing project pattern, OR
2. The anti-pattern is objectively bad regardless of context.

Do not impose an architectural pattern that doesn't exist in the project. Do not defend an existing pattern just because it exists — if it's clearly harmful, flag the objective problem directly.

---

Readability & complexity standard

The bar is: a mid-level dev should be able to read and understand any function in under 30 seconds without asking questions.

If that bar isn't met, it's worth flagging — regardless of whether the code is "technically correct". AI-generated code often passes correctness checks but fails this bar by introducing unnecessary layers, verbose boilerplate, or patterns that don't match the problem's actual complexity.

When the fix is clear, show a simpler version with code. When the fix depends on project context, explain the direction of the change without inventing details.

---

Test evaluation

Don't just check whether a test exists. Evaluate:

• Do the tests cover relevant edge cases beyond the happy path?
• Do assertions verify real behavior, or just that the code ran without an exception?
• Is the test coupled to implementation instead of behavior (fragile)?
• Is there excessive mocking that hides the real behavior being tested?

---

Guidelines

• Be specific — reference the exact line and explain why something is a problem, not just what it is.
• Suggest a fix when possible — use code examples only when they make the fix materially clearer.
• Avoid nitpicking formatting or style that a linter/formatter should handle.
• Acknowledge good patterns — note well-written code in the summary, not as inline noise.
• If a file looks correct and well-written, do not force comments on it.

---

Phase 4: Post Review

If the review mode is local, skip all GitHub API calls. Instead, display the full review directly in the chat using the same format as 4.4 (summary) and 4.5 (inline comments), organized by file. Then skip to Phase 5.

If the review mode is github, continue with 4.1 below.

4.1 Build the review payload

Construct the review using the GitHub Pull Request Reviews API. This posts all inline comments atomically as a single PENDING review — the user will review and submit it from the GitHub web interface.

The owner and repo were resolved in the GitHub API Access section. Get the latest commit SHA:

Via MCP:

CallMcpTool: server=github, toolName=pull_request_read
arguments: { "owner": "<owner>", "repo": "<repo>", "pullNumber": <number>, "method": "get" }

Extract headRefOid from the response.

Via CLI:

gh pr view <number> --json headRefOid -q .headRefOid

4.2 Map line numbers to diff positions

The GitHub API requires diff positions (not file line numbers) for inline comments. For each comment, calculate the position within the file's diff patch.

Parse the patch field from the file data retrieved in Phase 2.2. The position is the 1-based line index within the patch (counting from the first @@ line, which is position 1). Only lines visible in the diff can receive comments.

If a comment targets a line not present in the diff, convert it to a file-level comment by omitting the line field and prepending the line reference to the comment body.

4.3 Post the review as PENDING

The review is created as PENDING — it is only visible to the author until they submit it from the GitHub web interface. Do NOT submit the review automatically.

Via MCP:

Create the pending review (omit event to keep it pending):

CallMcpTool: server=github, toolName=pull_request_review_write
arguments: { "owner": "<owner>", "repo": "<repo>", "pullNumber": <number>, "method": "create", "body": "<summary>", "commitID": "<headRefOid>" }

Then add each inline comment to the pending review:

CallMcpTool: server=github, toolName=add_comment_to_pending_review
arguments: { "owner": "<owner>", "repo": "<repo>", "pullNumber": <number>, "path": "app/views/partner.py", "line": 42, "side": "RIGHT", "subjectType": "line", "body": "**[? Critical]** Description of the issue.\n\nSuggested fix." }

Do NOT call submit — the review stays pending for the user to review and submit manually.

Via CLI:

Omit the event field so the review is created as PENDING:

gh api repos/{owner}/{repo}/pulls/<number>/reviews \
  --method POST \
  -f body="<summary>" \
  -f commit_id="<headRefOid>" \
  --input <json-file>

Where <json-file> contains the comments array:

{
  "body": "<summary text>",
  "commit_id": "<sha>",
  "comments": [
    {
      "path": "app/views/partner.py",
      "line": 42,
      "side": "RIGHT",
      "body": "**[? Critical]** Description of the issue.\n\nSuggested fix."
    }
  ]
}

4.4 Summary review format

The summary body posted with the review should follow this structure:

## Code Review Summary

**PR**: #<number> — <title>
**Branch**: <headRefName> → <baseRefName>
**Files reviewed**: <count>

### Findings

- ? **Critical**: <count>
- ? **Important**: <count>
- ? **Suggestion**: <count>

### Overview

<2-3 sentences on what the PR does and the overall approach>

### Key Observations

<Bullet points covering the most important findings>

### What Looks Good

<Bullet points on well-written parts of the PR>

4.5 Inline comment format

Each inline comment uses this format:

**[? Critical | ? Important | ? Suggestion]** — <description>

<suggested fix or code example, if applicable>

Use code examples only when they make the fix materially clearer. Prefer short comments when the issue is obvious.

4.6 No findings

If the review finds no issues, still post a summary review acknowledging the PR looks good. Skip the inline comments — post only the summary with a note that no issues were found.

---

Phase 5: Summary

If mode is github:

• Link to the PR
• How many inline comments were posted, broken down by severity (?/?/?)
• Inform that the review was created as PENDING — the user must go to the GitHub web interface to review the comments and click "Submit review" to publish

If mode is local:

• How many findings were identified, broken down by severity (?/?/?)
• The full review was already displayed in the chat during Phase 4

---

Phase 6: Resolve Review Comments

When to use

Run this phase when the user asks to fix open review comments on a PR. This handles fetching comments, applying fixes, running tests, and replying on GitHub.

6.1 Fetch open comments

gh api repos/{owner}/{repo}/pulls/<number>/comments \
  --jq '.[] | {id: .id, path: .path, body: .body, position: .position, line: .line}'

Read every comment body carefully before touching any file.

6.2 Apply the fixes

For each comment, make the necessary code change. Group related fixes logically (same file or same concern) so they can be committed together.

6.3 Run tests before pushing

NEVER push or mark a comment as resolved before running the test suite.

make test

If any test fails, fix it before proceeding. Only move to the next step when tests pass.

6.4 Commit and push

Follow the git-commit skill guidelines (Conventional Commits, group by context).

git push

6.5 Ask the user for confirmation before submitting to GitHub

STOP here. Do NOT reply or resolve any comment yet.

Present a summary of what was done for each comment, e.g.:

Todos os comentários foram resolvidos e os testes passando. Posso submeter as respostas e marcar os threads como resolvidos no GitHub?

• ? views/partner.py — validação de CNPJ duplicado adicionada
• ? schemas/partner.py — campo razao_social renomeado para company_name
• ...

Wait for the user to explicitly confirm before proceeding to 6.6.

6.6 Reply and resolve each comment

After a successful push, for each comment that was addressed:

6.6a — Reply with a short message:

gh api --method POST \
  repos/{owner}/{repo}/pulls/<number>/comments/<comment_id>/replies \
  --field body="Resolvido: <one-line description of the fix>"

6.6b — Mark the thread as resolved (requires the GraphQL thread ID, not the comment ID):

First, fetch all review thread IDs and match them by comment databaseId:

gh api graphql -f query='
  query($owner:String!, $repo:String!, $pr:Int!) {
    repository(owner:$owner, name:$repo) {
      pullRequest(number:$pr) {
        reviewThreads(first:50) {
          nodes {
            id
            isResolved
            comments(first:1) {
              nodes { databaseId body }
            }
          }
        }
      }
    }
  }
' -f owner={owner} -f repo={repo} -F pr=<number>

Then resolve each thread by its GraphQL id:

gh api graphql -f query='
  mutation($threadId:ID!) {
    resolveReviewThread(input:{threadId:$threadId}) {
      thread { id isResolved }
    }
  }
' -f threadId=<thread_id>

Batch with a Python script to avoid repetition:

python3 << 'PYEOF'
import subprocess, json

OWNER = "{owner}"
REPO  = "{repo}"
PR    = <number>

# Map comment databaseId → reply message
resolutions = {
    <comment_id_1>: "Resolvido: <descrição do que foi feito>",
    <comment_id_2>: "Resolvido: <descrição do que foi feito>",
}

# Fetch all review threads
result = subprocess.run(
    ["gh", "api", "graphql",
     "-f", f"query=query($owner:String!,$repo:String!,$pr:Int!){{repository(owner:$owner,name:$repo){{pullRequest(number:$pr){{reviewThreads(first:50){{nodes{{id isResolved comments(first:1){{nodes{{databaseId}}}}}}}}}}}}}}",
     "-f", f"owner={OWNER}", "-f", f"repo={REPO}", "-F", f"pr={PR}"],
    capture_output=True, text=True
)
data = json.loads(result.stdout)
threads = data["data"]["repository"]["pullRequest"]["reviewThreads"]["nodes"]

for thread in threads:
    if thread["isResolved"]:
        continue
    comment_id = thread["comments"]["nodes"][0]["databaseId"]
    thread_id  = thread["id"]

    if comment_id not in resolutions:
        continue

    # Reply
    subprocess.run(
        ["gh", "api", "--method", "POST",
         f"repos/{OWNER}/{REPO}/pulls/{PR}/comments/{comment_id}/replies",
         "--field", f"body={resolutions[comment_id]}"],
        capture_output=True
    )

    # Resolve
    subprocess.run(
        ["gh", "api", "graphql",
         "-f", f"query=mutation($t:ID!){{resolveReviewThread(input:{{threadId:$t}}){{thread{{id isResolved}}}}}}",
         "-f", f"t={thread_id}"],
        capture_output=True
    )
    print(f"✓ comment {comment_id} replied and resolved")
PYEOF

Do not reply or resolve before the push succeeds.

---

Edge Cases

Large PRs

For PRs with more than 30 changed files, warn the user and offer to focus on specific directories or file patterns:

title: "Large PR"
questions:
  - id: scope
    prompt: "This PR has <N> changed files. Review all of them or focus on specific areas?"
    options:
      - id: all
        label: "Review all files"
      - id: focus
        label: "Let me specify which files or directories to focus on"

Binary and generated files

Skip binary files, lock files (package-lock.json, poetry.lock, etc.), and auto-generated files. Mention skipped files in the summary.

Insufficient permissions

If GitHub API calls fail with 403/404 (whether via MCP or gh api), tell the user they may lack write access to the repository and suggest reviewing locally (print findings to the chat instead of posting).

---

Error Recovery

GitHub API returns 403/404:

• Verify authentication (gh auth status or MCP get_me)
• If permissions are insufficient, fall back to local-only mode (display review in chat)
• If the PR does not exist, confirm the number with the user

Diff too large (>2000 lines or >50 files):

• Offer to scope the review to specific directories or file patterns
• Prioritize source files over config, lock files, and generated files
• Skip binary files and vendored dependencies without asking

Pending review already exists:

• Fetch existing pending reviews via gh api repos/{owner}/{repo}/pulls/<number>/reviews
• Warn the user and ask whether to delete the stale review or add to it

Rate limit hit mid-review:

• Post what was collected so far as a partial review
• Tell the user which files remain unreviewed
• Suggest resuming later or switching to local-only mode

Comment targets a line not in the diff:

• Convert to a file-level comment by omitting the line field
• Prepend the line reference to the comment body (e.g., "Line 42: ...")