npx skills add AGCO-Global/org-skills --skill backend-code-review -a github-copilot
# or with the org installer (adds .github/skills/backend-code-review):
npx -y github:AGCO-Global/org-skills add skill backend-code-review

npx skills add AGCO-Global/org-skills --skill backend-code-review
# user-level instead of project-level:
npx skills add AGCO-Global/org-skills --skill backend-code-review -g

/plugin marketplace add AGCO-Global/org-skills
/plugin install backend-shared-skills@org-skills
# or just this skill, in this repository:
npx skills add AGCO-Global/org-skills --skill backend-code-review -a claude-code

npx skills add AGCO-Global/org-skills --skill backend-code-review -a codex

npx skills add AGCO-Global/org-skills --skill backend-code-review -a cursor

npx skills add AGCO-Global/org-skills --skill backend-code-review -a gemini-cli

[blocker] OrderController.cs:42 — GET /orders/{id} loads by id only; any authenticated user can read any order (IDOR).
Why it matters: cross-tenant data exposure.
Fix: repo.GetAsync(id, currentUser.TenantId) → 404 when not found or not owned.

## Findings
| # | Severity | Location | Issue | Why it matters | Fix |
|---|---|---|---|---|---|
| 1 | [blocker] | src/orders/repo.ts:88 | Balance read-modify-write without lock | Lost updates under concurrent payments | Version column + conditional UPDATE, retry on conflict |
| 2 | [major] | src/client/pay.ts:17 | No timeout on payment API call | Thread/socket exhaustion when provider hangs | 2 s timeout, retry 2x with jittered backoff |

## Verdict
Request changes | Approve with minor changes | Approve
Blockers: n  Majors: n  Minors: n
Top 3 to address: 1. … 2. … 3. …
Rollout notes: migration order, flags, rollback.
What's good: <one or two lines>

---
name: backend-code-review
description: Perform a staff-engineer-level code review of back-end changes in any stack (.NET, Java/Spring, Node.js/TypeScript, Python, Go) covering correctness, concurrency, transactions, security, data migrations, API contracts, resilience, performance, observability, tests and rollout safety, and deliver a severity-ranked findings table with concrete fixes and a merge verdict. Use this whenever the user asks to review a PR, diff, service, endpoint, handler, repository, migration, message consumer or background job; asks "is this safe to merge", "what's wrong with this", "any issues here", or pastes server-side code and asks for feedback; or wants a pre-merge check for race conditions, N+1 queries, IDOR, missing authorization, unsafe migrations or retry storms. Also use it for self-review before opening a PR, and apply it when reviewing infrastructure-adjacent code such as SQL scripts, queue consumers and scheduled jobs.
metadata:
  technology: Backend (general)
  type: code-review
---

# Backend Code Review

Back-end bugs corrupt data, leak data, or take production down, and they usually surface under concurrency, partial failure or scale that the tests never exercised. Review every change as if it will run on N instances at once, be retried, see malicious input, and need to be rolled back halfway through a deploy.

## 1. Before reading the diff

- Read the PR description or ticket. If the intent isn't stated, ask for it in one sentence; correctness can't be judged without the goal.
- Identify the stack and apply its specific skill (e.g. `dotnet-development`) for idioms. This skill covers what every backend shares.
- Map the blast radius: public API? schema change? shared library? message contract? hot path? Review the riskiest files first.
- More than ~400 changed lines of non-generated code: ask for a split (e.g. migration, then code, then cleanup) before a line-by-line review.

## 2. Severity scale (use these labels verbatim)

| Label | Meaning | Blocks merge? |
|---|---|---|
| **[blocker]** | Data loss/corruption, security hole, broken contract for existing clients, unsafe migration, outage risk | Yes |
| **[major]** | Likely bug under realistic load or failure, missing authZ check on a secondary path, no timeout, N+1 on a hot path, missing test for new behaviour | Yes, unless deferred with a ticket |
| **[minor]** | Readability, naming, small duplication, weak log message | No |
| **[nit]** | Style not caught by tooling | No |
| **[question]** | Need the author's intent before judging | — |
| **[praise]** | Something worth copying | — |

## 3. Checklist — run every section, report only what you find

### Correctness
- Edge cases: empty collections, nulls, zero, negative, max values, duplicate input, Unicode, very large payloads.
- **Concurrency:** check-then-act across requests (`if !exists then insert`) needs a unique constraint, `INSERT ... ON CONFLICT`, or a lock. Lost updates need optimistic concurrency (version column) or `SELECT ... FOR UPDATE`. Shared mutable state in singletons/static fields is a race.
- **Idempotency:** consumers, webhooks, retried jobs and `POST` with retries must tolerate duplicates (dedup key, upsert, idempotency-key table).
- **Transactions:** boundaries wrap the full invariant; no remote calls (HTTP, queue publish) inside a DB transaction. Writing to the DB and publishing an event needs an **outbox**, not two independent writes.
- **Time:** store and compare in UTC; convert at the edge; use injectable clocks; DST and month-end arithmetic via date libraries, not `+ 86400`.
- **Money:** decimal types (`decimal`, `BigDecimal`, `Decimal`) or integer minor units with currency; explicit rounding mode; never `float`/`double`.
- Error handling: specific exceptions, no empty `catch`, no swallowed failures that return success.

### Security
- **Injection:** parameterised queries only; no string-built SQL, shell commands, LDAP or template expressions from input.
- **AuthN/AuthZ on every path:** each new endpoint, handler, GraphQL resolver and admin route declares its policy. Object-level checks prevent **IDOR** (`WHERE id = @id AND tenant_id = @tenant`, not just `WHERE id = @id`).
- Input validation at the boundary: type, length, range, allow-lists; reject unknown fields on sensitive DTOs (mass assignment).
- **SSRF:** user-supplied URLs fetched server-side need scheme/host allow-lists and blocking of private/metadata IP ranges after DNS resolution.
- **Deserialization:** no polymorphic/typed deserialization of untrusted data (`BinaryFormatter`, Java native serialization, `pickle`, YAML `load`).
- Secrets from a secret store or environment, never in code, config files or logs. Crypto via vetted libraries; passwords hashed with Argon2/bcrypt/scrypt.
- Logs and errors free of PII, tokens and stack traces returned to clients.

### Data and migrations
- **Expand/contract:** add nullable column / new table → deploy code writing both → backfill → switch reads → drop old in a later release. Never rename or drop in the same deploy that stops using it.
- Large tables: no blocking `ALTER` or index build (`CREATE INDEX CONCURRENTLY`, online DDL); batch backfills with throttling; set lock timeouts.
- New query patterns have supporting indexes; new foreign keys are indexed; unique constraints enforce business uniqueness.
- Migrations are forward-only and re-runnable, or have a tested down path.

### API contracts
- No breaking changes without a new version: removing/renaming fields, tightening validation, changing types, status codes or enum semantics.
- Errors use the service's standard shape (e.g. RFC 9457 Problem Details); no leaked internals.
- List endpoints paginate with a server-enforced maximum page size.
- Event/message schemas evolve additively; consumers tolerate unknown fields.

### Resilience
- Every outbound call has a **timeout** (connect and total); no library defaults of "infinite".
- Retries only for idempotent operations, with exponential backoff + **jitter** and a cap; no retries stacked at several layers.
- Circuit breaker or bulkhead around flaky dependencies; graceful degradation (cached/partial response) when a non-critical dependency is down.
- Queue consumers: bounded concurrency, poison-message handling (dead-letter queue), visibility timeout longer than processing time.

### Performance
- **N+1:** queries inside loops or lazy-loaded relations in serialisation; batch with `IN`, joins or data loaders.
- Unbounded queries (`SELECT` without `LIMIT`, loading whole tables into memory); missing pagination or streaming for exports.
- Sync/blocking I/O on async request threads (`.Result`, `Thread.sleep`, sync file/HTTP calls in Node's event loop).
- Caching: key includes tenant/user where relevant; invalidation or TTL defined; stampede protection on hot keys.
- Allocation in hot loops, regex compiled per call, unbounded in-memory collections or caches.

### Observability
- Structured logs (key-value/JSON) at the right level; one log per failure, not per retry layer.
- Metrics for new behaviour (rate, errors, duration); traces propagate context across HTTP and messaging (W3C `traceparent`).
- Correlation/request ID flows through logs, outbound calls and messages.

### Tests
- New behaviour and bug fixes have tests that fail without the change; assertions on outcomes, not mocks called.
- Integration tests against a real database/broker (e.g. containers) for queries, migrations and transactions; unit tests for pure logic.
- Concurrency, idempotency and failure paths (timeout, 5xx, duplicate message) covered where the change touches them.

### Dependencies and operability
- New dependency: maintained, licence compatible (flag GPL/AGPL in proprietary services), no known CVEs, not duplicating an existing one.
- Config validated at startup; no environment-specific values hardcoded.
- Risky behaviour behind a **feature flag**; rollout plan (canary/percentage) and rollback path stated; can the previous version run against the new schema?

## 4. Example finding

```
[blocker] OrderController.cs:42 — GET /orders/{id} loads by id only; any authenticated user can read any order (IDOR).
Why it matters: cross-tenant data exposure.
Fix: repo.GetAsync(id, currentUser.TenantId) → 404 when not found or not owned.
```

## 5. Deliverable format

```
## Findings
| # | Severity | Location | Issue | Why it matters | Fix |
|---|---|---|---|---|---|
| 1 | [blocker] | src/orders/repo.ts:88 | Balance read-modify-write without lock | Lost updates under concurrent payments | Version column + conditional UPDATE, retry on conflict |
| 2 | [major] | src/client/pay.ts:17 | No timeout on payment API call | Thread/socket exhaustion when provider hangs | 2 s timeout, retry 2x with jittered backoff |

## Verdict
Request changes | Approve with minor changes | Approve
Blockers: n  Majors: n  Minors: n
Top 3 to address: 1. … 2. … 3. …
Rollout notes: migration order, flags, rollback.
What's good: <one or two lines>
```

Order rows by severity. Any `[blocker]`, or a `[major]` without an agreed follow-up ticket, means **Request changes**. In self-review mode, also draft the PR description: intent, approach, migration/rollout plan, testing done, risks.

## Anti-patterns to reject

- Nitpicking naming while missing a race condition, IDOR or unsafe migration.
- "Consider using X" with no reason; always state the impact.
- Approving schema changes without checking the deploy order and old-code compatibility.
- Accepting "it's internal" as a reason to skip authZ, validation or timeouts.
- Asking for retries without idempotency, or for caching without an invalidation story.
- Line-by-line review of a 2 000-line PR instead of asking for a split.
- "LGTM" on new behaviour with no tests, metrics or rollback plan.