Menú
Reviews a GitHub PR diff for correctness, security, tests, architecture. Use when asked to review a PR or pull request.
Instalar con Codex o Claude Copia este prompt, pégalo en Codex, Claude u otro asistente, y deja que revise la página de la skill y la instale por ti.
Basado en la clasificación ocupacional SOC
| name | pr-review |
| description | Reviews a GitHub PR diff for correctness, security, tests, architecture. Use when asked to review a PR or pull request. |
| allowed-tools | Bash, Read, Grep, Glob |
<when_to_activate>
/pr and wanting self-review before requesting teammates
</when_to_activate>If the user's request includes BOTH unresolved bot/reviewer comments AND a CI run, address comments FIRST, then check CI:
gh api repos/<o>/<r>/pulls/<n>/comments on GitHub). Triage into FIX / WONT_FIX / ALREADY_FIXED, batch all FIX edits into one focused commit, then reply per-thread linking the resolving SHA: FIX → Addressed in <short-sha> — <one-line of what changed>; WONT_FIX → lead with Won't fix and give the reasoning (cost/benefit, abstraction threshold, breaks shared pattern), don't be apologetic — a reasoned no closes the thread better than a defensive yes; ALREADY_FIXED → Addressed in <prior-sha> — <pointer>. Don't extract a dedup abstraction on review feedback alone if only two callers share the pattern; the abstraction threshold is three. Don't reply to spec-reviewer "met" items unless the user asks — those are informational.Bot comments often explain or contextualize the CI failure. Reading CI first risks fixing a symptom that the bot already proposed a different fix for.
Always fetch the canonical comment text before reasoning about it. When the user pastes a bot comment excerpt and asks to address it, run gh api repos/<o>/<r>/pulls/<n>/comments and read the full body — user-pasted excerpts truncate. Arguing against a "fabricated" file reference that turns out to be in the part you didn't see is a self-inflicted credibility hit. Same applies symmetrically: bot comments can be wrong, but the burden of proof is "I read the source comment AND the relevant code, here's the disagreement," not "the user's paraphrase didn't include this so the bot must be hallucinating."
# By PR number (current repo)
gh pr diff <number>
# Get PR metadata
gh pr view <number> --json title,body,additions,deletions,changedFiles,baseRefName,headRefName
src/ or app code → logic reviewtests/ → test quality review*.toml, *.yml, *.env*) → secrets + config reviewdef f(x=[]))except: / empty catch clauses<decision_logic>
gh not authenticated → Show gh auth login instructionssrc/, app/, core logic; skip lock/generated files<output_format>
## PR Review: <title> (#<number>)
**Branch**: `<head>` → `<base>`
**Changes**: +<additions> / -<deletions> across <N> files
---
### CRITICAL Issues
(Security or correctness blockers — must fix before merge)
- `path/to/file:42` — **Issue**. Remediation.
---
### Warnings
(Should fix — code quality, missing tests, non-critical bugs)
- `path/to/file:17` — **Issue**. Remediation.
---
### Suggestions
(Nice to have — style, naming, minor improvements)
- `path/to/file:88` — Suggestion.
---
### Looks Good
- Items that passed review
---
### Summary
| Dimension | Status |
|-----------|--------|
| Correctness | PASS / WARN / FAIL |
| Security | PASS / WARN / FAIL |
| Best Practices | PASS / WARN / FAIL |
| Test Coverage | PASS / WARN / FAIL |
| Code Quality | PASS / WARN / FAIL |
**Verdict**: APPROVE / REQUEST CHANGES / NEEDS DISCUSSION
</output_format>
<success_criteria>
<self_review_first> Before presenting any non-trivial change as "complete", run the project-specific lens below on your own diff. Apply review-grade cleanups during the initial implementation, not after. Common easy wins:
make_* fixture in conftest, not a private _make_* per file.PBAT-NNNNN: …)? Strip the prefix on the first pass; ticket history belongs in PR/git, not in code.Verify reviewer suggestions at the cited lines, don't reason from memory. Before agreeing OR disagreeing with any reviewer suggestion (Baz, Copilot, human), open the file at the cited lines and read the actual code. For dependency / cross-repo claims, also grep -rEn '...' ~/PycharmProjects/ neighboring repos — the bot's named file may not be the only relevant one. "Probably looks like X" reasoning produces both false confirmations (apply a fix that wasn't needed) and false denials (dismiss a real bug because the imagined code looked fine).
Reproduce CI checks locally on CI's runtime version before declaring ready. Read .github/workflows/*.yml for the runtime version (python-version: 3.11, etc.) and the install footprint (requirements.txt? tests/requirements.txt? extras?). Match all of it locally — don't trust the IDE's long-lived venv. Run every CI step (lint + tests + build), not just the one wired in the IDE. When CI is delegated to a reusable workflow you don't own (uses: <org>/<repo>/.github/workflows/X.yml@<ref>), the upstream's hard-coded paths/filenames are part of the contract — read the upstream once before renaming or splitting any file it consumes. A locally sensible rename of tests/requirements.txt or pytest.ini will fail CI in a way that looks unrelated to the rename. The "CI tells me what flake8 could've told me 30s earlier" loop is avoidable.
The "I'll let pr-review catch it" loop is avoidable churn.
Tech-lead self-review checklist — run this against your own diff BEFORE declaring code done. If you skip it, the reviewer (human or bot) does it for you AND you eat reviewer churn.
~/.claude/rules/base-conventions/RULE.md §"Code Clarity" (the getattr/hasattr bullet) — no getattr/hasattr/setattr on types you wrote.Exception: or bare except: → narrow it. Swallowing without logging → don't.TypeAlias over a nested Callable[..., Callable[..., Awaitable[Any]]] is harder to parse than the inline annotation it replaces; collapse it to the simpler form (Optional[Callable]) unless the precise generic fixes a real ambiguity or reduces duplication.<project_specific_lens> block below to your own diff.scalar_one_or_none() with .first(), removing a swallowed except:, adding a customer_id filter, swapping .isdigit() for .isdecimal()), the file usually has 2-3 mirror copies of the same broken code. Same rule applies to shared-surface renames — if a test fixture changes its return shape (renamed dict key, added/removed attribute, changed argument name), grep every consumer of the fixture in the SAME commit; a partial fixture rename produces confusing failures far from the root cause. Run grep -n <primitive-or-fixture-name> <scope> before declaring done — the same primitive misused once is almost always misused 2-3 times, and calling sites of a bug class (or a renamed surface) are siblings, not strangers.publish + publish_batch, get + list, sync + async), check that retries, logging, auth, and metrics apply to BOTH or NEITHER. Asymmetric cross-cutting concerns leak through the un-wrapped path in production — fix the symmetry before declaring done.
</self_review_first><duplication_classification> When a finding suggests extracting a shared helper, classify it BEFORE applying:
Don't auto-apply extract findings without that classification. A 2-call-site abstraction over genuinely different signatures is thin and awkward; revisit when a third lands. </duplication_classification>
<project_specific_lens>
The 5 generic dimensions above catch language-level issues. The following lens — distilled from recurring review feedback in SensiAI codebases — catches architectural and project-conventions issues that the generic pass misses. Apply BOTH layers; treat each principle below as a first-class review dimension.
api → logic/handler → mapper → db. Each layer has a single responsibility:
logic/X.py — don't reach across domains directly; call the other domain's logic. ("use agencies logic — domain separation!!!!!!")See the canonical rule in ~/.claude/skills/testing/SKILL.md <pytest_principles> "Real objects for domain types". Project quote: "use only real object and not MagicMock — in all tests" / "Always use real objects, mock only external resources (db, clients etc)". Flag MagicMock(id=..., ams_id=...) style mocks for domain types as CHANGES_REQUESTED.
Other testing rules:
local_stack/ do NOT use pytest.ini (that's unit-test only). Don't conflate the two.add_* / assert_and_commit_data helpers in local_stack/conftest.py; don't reinvent.Before approving any new HTTP client, secrets handler, SQS publisher, postgres session, redis client, or AMS-API caller — grep for the equivalent Sensi package and demand its use:
sensi-cloud → secrets manager, SQS (use the singleton pattern)sensi-postgres → DB sessions (multi-client via client_name="X"; envvars are auto-resolved by config)sensi-ams-api-client → all AMS API calls (don't write raw requests.get(...) to AMS endpoints)sensi-logger → structured logging with logger.contextualize(...) for request scopesensi-redis → async redissensi-ams-db-python → customers DB ORM (sensi_ams_db_orm.models.*)
Re-implementing what these provide is a CHANGES_REQUESTED. ("highly recommend to use sensi-ams-api-client pkg to avoid issues" / "please use sensi-cloud pkg" / "use singleton as other claude components")DEFAULT or an explicit backfill. Otherwise existing rows break and rollback breaks dependent services.BEGIN/COMMIT — run_db.sh wraps each migration in its own transaction.description column for consistency.models.py: ORM regenerated and committed (both models.py and prisma/schema.prisma).os.environ.get(...) scattered through code. All env vars in app/core/config.py (or app/util/settings.py) as a pydantic Settings class — UPPER_SNAKE_CASE.*_RR_DB_* for read-replica clients, mirroring ams-api-service).op://<env>-platform/<service>/<key>), never plaintext.local_stack/ runs against actual containers (postgres, redis, localstack, customers_postgres). Wire the customers DB through ams-db-schema package + a separate seed SQL.--exit-code-from <test> is mandatory on the CI docker compose up so failures propagate.assert_and_commit_data); test_run_db_script style tests need every table populated.--ignore=tests/test_*.py line in the service Dockerfile means CI doesn't run those tests. If you add an ignore (because the test imports a module not in the service image), you MUST add a compensating GHA step in the same PR that installs the right deps and runs the ignored tests. "Test exists but CI doesn't run it" ≡ "test doesn't exist." Cross-check: is the new workflow a required check on the PR?try/except wrapping code already wrapped at api/middleware level → redundant, remove.Region, ReferredBy, Administrator, model fields, helper functions, params, imports.try/except ("map should never fail — therefore no need try/catch").# TODO to delete IS the signal to delete.customer_ redundantly. ("if the repository is customers_device so all funcs in it should not be with 'customer' name in them")shift ≠ visit; visit can exist without shift. Don't conflate.schedule ≠ clock — schedule data and time-tracking data are different concepts.*_id (e.g., category_id, not category).~/.claude/rules/base-conventions/RULE.md §"Code Clarity" for the canonical no-reflection rule.bo_id but the value is a customer id, use a pydantic alias and name the field customer_id.Field(...) patterns for constraints (min_length, ge, etc.) — don't validate in code.customer_id query param. Missing/invalid → 422.*_RR_DB_* env-var convention).customer_id and status=1 (soft-delete guard) when querying customer-scoped tables — these are pure filters, not optional params.execute_list_query / shared helpers; don't reinvent them.UPDATE ... WHERE id = ?); don't fetch + update unless you need the row.0.0.0, branch ref, +pbat.NNNNN) → mandatory reminder comment: "update to real version after dep PR merges to master".pytz is fine unpinned.uv.lock / yarn.lock — yes, commit them. But check: was a lock file committed by accident in a place that shouldn't have one?isinstance(result, Response) check when no handler returns Response; a Settings field that's never read (only projected back to env via setdefault); base64-wrapping JSON bytes "in case we need non-JSON later". Each speculative branch costs latency on every request and is dead code until a real caller appears. Revisit when one does.handle_unknown_emails, send_slack_replay_for_invalid_agency_status) is encouraged for readability.try/except blocks that wrap a long function should usually be split — error handling per case is more specific.getattr / isinstance on known ORM properties is a code smell; remove and call the property directly.https) and hostname allow-list at the boundary.requests.get inside an async function → asyncio.to_thread(...).GRAPHQL internal object for known constants — don't hard-code values that scraping resolves.requests and httpx.AsyncClient for the same flow — pick one.443793523615.dkr.ecr.eu-west-1.amazonaws.com/...).<service>-service, not web).--abort-on-container-exit --exit-code-from <test> is mandatory in CI.containerPort must match the Dockerfile EXPOSE and the uvicorn --port.CLAUDE.md / docs/ updated when behavior changed..env, model files, lock files that shouldn't exist.models.py + prisma/schema.prisma committed when schema changed.# TODO files.finally — closing connections, releasing locks, removing temp files, pushing metrics that must fire even on error. "Move to finally" / "do it in finally" recurs.@staticmethod is suspect when the method clearly belongs to instance state; flag "Why is it static? you don't use them as static methods" cases./health returning a probe every 30s should NOT do real DB work. Either return 200 (alive) or do SELECT 1 only. A real query per probe is a CHANGES_REQUESTED.return None → null + application/json breaks them. When reviewing a new webhook, check the third-party's expected response shape.None, ask "what does the code do when it is None? when empty? when whitespace?". Don't accept a single guard at one layer if the field flows through several.open(...) always with encoding="utf-8". open without close (or without with) is a leak.print → logging. logging.exception("…") (not logging.error) when inside except so the traceback is captured. Don't catch + silently ignore.except Exception (or bare except:) is "too broad" and gets flagged. Catch the actual error class and let unknown ones propagate.for + .append(). For dataframes, nunique(), set_index(), to_csv(index=False) are project idioms.--continuous-label), accessed in code with underscores (args.continuous_label). For tri-state options use choices=[...], not three booleans. Boolean flags are bare --flag, not --flag true.Optional[X] not Union[X, None]. Be consistent: all Path or all str for path-typed args, not mixed. Literal["a", "b"] for mode strings. Don't cast when you can simply annotate the return.black, isort, flake8 (and pylint where the project uses it). An isort miss on imports is a small but consistent flag.is True / is False / is None — explicit comparisons for None; for booleans only when the value can also be a non-bool truthy/falsy and the distinction matters.set[T] for unique-unordered; insertion-ordered dict[T, None] or accumulate-via-if x not in lst for unique-ordered). Don't carry a list[T] and clean up with sorted(set(field)) at the end — end-of-function cleanup hides the invariant and makes "is this dedup'd at this point?" un-greppable. Reserve sorted(set(x)) only for boundaries where you can't change the type (external API responses).</project_specific_lens>
Promotes recurring feedback into the right skill, then guides /compact at phase boundaries.
Testing guidance for pytest, Jest/Vitest, Go, and TDD. Use when writing tests or improving coverage.
Methodical debugging with evidence and hypothesis testing. Use when troubleshooting fails or root cause is unclear.
Create new skills, commands, hooks, or subagents. Use when adding capabilities to Claude Code or Cursor.
PostgreSQL patterns for queries, schema, indexing, security. Use when writing SQL, designing schema, or adding indexes.
Orchestrates the Ralph pipeline (spec-interview → PRD → execute). Use for features needing autonomous implementation.