salesforce-code-review

Reviews Salesforce changes and delivers severity-ranked findings with concrete fixes and a merge verdict — Apex bulkification and governor limits, sharing and CRUD/FLS enforcement, SOQL and DML in loops, trigger framework and automation ordering, Flow bulk safety and fault paths, Lightning Web Component patterns under Lightning Web Security, SLDS 2 styling, test quality beyond coverage percentage, and package and metadata hygiene.

Download .zip Raw Source

When agents use itUse this whenever the user asks to review Salesforce metadata, a pull request or a diff containing Apex classes, triggers, Flows, Lightning Web Components or permission sets, asks "is this safe to deploy", "anything wrong with this trigger", "will this hit a governor limit", or pastes Apex or LWC and asks for feedback. This skill owns the verdict for Salesforce changes; load the stack skills for idioms only.

Install

Copilot (VS Code, Visual Studio, Copilot CLI and github.com) reads skills from the repository — commit them so the whole team gets them.

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

Uses the open skills CLI. Works with Claude Code, Codex, Cursor, Copilot, Gemini CLI, OpenCode, Windsurf and 60+ others — it asks which agent to install into.

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

Installs the salesforce-quality-skills plugin, which bundles all Salesforce / Quality skills and keeps them updated.

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

Use Download .zip above, then upload it under Customize → Skills → Upload skill. Team and Enterprise admins can sync this repo as a plugin marketplace instead.

Installs into .agents/skills/, which Codex reads.

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

Installs into Cursor's skills folder.

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

Installs into Gemini CLI's skills folder.

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

Any tool with rules, instructions or custom prompts: use Copy SKILL.md above and paste it in. It is plain Markdown.

Commands use your normal git sign-in to GitHub, so they work while the repository is private. Node.js 20+ required.

Try it — example prompts

Prompts this skill is tested against, and what a good answer includes.

Review this trigger handler: it loops over Trigger.new and queries the related Account inside the loop.
[blocker] SOQL inside a loop hits the 100 SOQL query limit at 200 records collect ids into a Set, query once into a Map before the loop verdict: request changes finding given as file:line
Is this @AuraEnabled method safe? It's `public with sharing class` and queries Contact fields.
with sharing enforces record access only, not field or object permissions [blocker] missing WITH USER_MODE on the query it is an entry point — validate inputs and do not trust an id parameter check whether it is reachable by a guest user
We added a record-triggered Flow on Opportunity. The PR only touches the Flow. Anything to check?
inventory the existing automation on Opportunity, including Apex triggers ordering between record-triggered Flows is not guaranteed check for recursion where the Flow and a trigger update each other no Get/Update elements inside a loop fault connectors on every element that can fail before-save for same-record updates
Our test class has 92% coverage. Good to deploy?
coverage is not correctness — read the assertions needs a bulk case at 200 records needs a negative case and a restricted-user case with System.runAs no SeeAllData=true asserts after Test.stopTest() for async work
The diff includes a permission set change and package.xml. Anything there?
read permission set diffs line by line View All / Modify All / Author Apex are blockers until justified field-level security changes reviewed individually package.xml checked for unintended or destructive metadata no secrets in custom metadata or settings

Skill contents

Salesforce Code Review

Targets: Salesforce Spring '26 · Code Analyzer v5 · Lightning Web Security · Verified: 2026-09 against https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_gov_limits.htm and https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/guide/

Salesforce code fails differently from ordinary server code. It runs inside hard governor limits that are per-transaction, so a change that passes every test at one record breaks at 200. It runs in system mode by default, so access enforcement is something the author had to remember rather than something the platform did. And automation composes invisibly — a new record-triggered Flow can break an Apex trigger nobody touched. Review for the 200-record transaction and the least-privileged user, because that is where this platform actually fails.

1. Decide first

Question	Default	Change when
What to read first	The trigger/automation inventory for the objects touched	A pure LWC change with no Apex
Bulk assumption	Every entry point receives 200 records	Never assume a single-record path
Access assumption	The running user is the least privileged one who can reach this	Only where the elevation is deliberate and stated
Static analysis first	Run Code Analyzer v5 and triage its output before reading	Skip only when you cannot run it
Coverage	90 %+ is the org bar; read the assertions as well as the percentage	90 % with no assertions is still a failing test suite
Verdict scope	Blockers and majors block; minors do not	Agreed team exceptions, recorded

2. Before reading the diff

List the automation on every object touched — triggers, record-triggered Flows, Process Builder remnants, workflow rules, validation rules. Order is not guaranteed between Flows, and a new one can re-enter an existing trigger.
Run Code Analyzer v5 and read the Graph Engine findings, which catch CRUD/FLS violations across call paths:
```
sf code-analyzer run --workspace force-app --view detail
```
Identify the entry points — @AuraEnabled methods, Apex REST, invocable methods, triggers, scheduled jobs. Each is a boundary where access and input validation must be enforced.
Check what is guest-reachable. An @AuraEnabled method on a public site is an unauthenticated endpoint.

3. Severity scale

Label	Meaning	Blocks merge?
[blocker]	Data exposure, data loss, SOQL/DML in a loop on a bulk path, governor limit at 200 records, missing sharing or CRUD/FLS on a user-facing path, injection	Yes
[major]	Likely bug at volume, recursion, missing fault path, test with no meaningful assertion, hard-coded id, accessibility failure on a primary flow	Yes, unless deferred with a ticket
[minor]	Maintainability, non-idiomatic platform usage, duplication	No
[nit]	Style the tooling does not catch	No
[question]	Intent needed before judging	—
[praise]	Worth copying	—

4. The findings that matter most

SOQL or DML inside a loop — the first thing to look for, and still the most common blocker. Collect into a map, query once, DML once.
Non-bulk-safe logic — Trigger.new[0], a single-record assumption, or a method taking an Id where the caller has a list.
Missing access enforcement — with sharing covers records only; queries need WITH USER_MODE and DML needs AccessLevel.USER_MODE. WITH SECURITY_ENFORCED in new code is superseded and should be replaced, never combined.
Injection — string-concatenated dynamic SOQL. Database.queryWithBinds with bind variables.
Recursion — a trigger updating its own object with no static guard, or a Flow and a trigger updating each other.
Automation ordering — a before-save Flow and an Apex trigger both writing the same field; ordering between record-triggered Flows is not guaranteed.
Flow bulk safety — a Get/Update element inside a loop is the Flow equivalent of SOQL in a loop; fault paths missing on every element that can fail.
Hard-coded ids — record type ids, queue ids, profile ids. They differ per org and break on deploy.
Tests that assert nothing — coverage is not correctness. Look for a negative case, a bulk case (200 records) and a restricted-user case.
LWC — Apex called imperatively without error handling, DOM access that Lightning Web Security restricts, third-party script from a CDN rather than a static resource, SLDS 2 styling hooks versus overridden internals.

The full per-area checklist: references/review-checklist.md.

5. Reviewing metadata you cannot deploy

Much of a Salesforce diff is XML. What to check by reading:

package.xml / deployment scope — is anything destructive included, and does the change set carry metadata nobody intended?
Permission sets and profiles — a diff that grants "View All" or "Modify All" is a blocker until justified. Check field-level security changes line by line.
Flow XML — look for elements inside loops, missing fault connectors, and the start element's entry criteria.
Custom metadata and settings — secrets do not belong here; they are visible to anyone who can view setup.

6. Deliverable format

## Findings
| Severity | File:line | Issue | Why it matters | Fix |
|---|---|---|---|---|
| [blocker] | classes/OrderService.cls:42 | SOQL inside a for loop over Trigger.new | Hits 101 SOQL queries at 200 records | Collect ids, one query into a Map before the loop |
| [blocker] | classes/OrderController.cls:18 | @AuraEnabled method queries without WITH USER_MODE | Returns fields the running user cannot read | Add WITH USER_MODE; class declares with sharing |
| [major]   | flows/Order_After_Save.flow-meta.xml:64 | Update element has no fault connector | Failures are silent; the record is left half-processed | Add a fault path that logs and surfaces the error |

## Summary
Verdict: Approve | Approve with minor changes | Request changes
Blockers: <n>  Majors: <n>  Minors: <n>
Top 3 to address: 1. … 2. … 3. …
Not reviewed: <metadata or files skipped, and why>
What's good: <genuine, specific praise>

Checklist

Every entry point was checked against a 200-record transaction.
No SOQL, DML or callout inside a loop on a bulk path.
Every user-facing class declares sharing; queries use WITH USER_MODE, DML uses AccessLevel.USER_MODE.
No WITH SECURITY_ENFORCED added, and never combined with USER_MODE.
Dynamic SOQL uses bind variables.
Automation inventory for each object touched was reviewed for ordering and recursion.
Flows have fault paths and no data elements inside loops.
No hard-coded ids.
Tests assert behaviour, and include a bulk case and a restricted-user case.
Permission set and profile changes were read line by line.
Guest-reachable entry points validate input and enforce access.
Code Analyzer v5 findings were triaged, including Graph Engine results.
The verdict states what was not reviewed.

Anti-patterns

Anti-pattern	Why it hurts	Fix
Approving because tests pass at 75%	Below the org's 90 % bar, and coverage is not correctness anyway	Hold to 90 %+; read the assertions; require a bulk and a negative case
Reviewing Apex without the automation inventory	A Flow you never opened re-enters the trigger	List all automation on the object first
Treating `with sharing` as full enforcement	Fields and objects are still system mode	Require `USER_MODE` on queries and DML
Ignoring the XML in the diff	Permission and Flow changes are where access quietly widens	Read permission sets, Flow XML and `package.xml`
Line-by-line review of a 2,000-line diff	Attention runs out before the blockers appear	Ask for a split; review by risk area
Style nits without the blockers	The author fixes the nits and ships the data exposure	Lead with blockers; let the linter own style
Running `sf scanner run`	Retired August 2025	`sf code-analyzer run`

Go deeper

references/review-checklist.md — the full per-area checks: Apex, triggers and automation, Flows, LWC, tests, permissions and packaging, with the failure each one prevents.
Sibling skills: apex-development and lwc-development for idioms; salesforce-security for org-level posture; apex-testing and salesforce-frontend-testing for writing the tests this review asks for; salesforce-org-conventions for the org's naming and mandated patterns.

References

Deeper material the agent loads only when needed.

review-checklist.md

---
name: salesforce-code-review
description: >
  Reviews Salesforce changes and delivers severity-ranked findings with concrete fixes and a merge verdict — Apex bulkification and governor limits, sharing and CRUD/FLS enforcement, SOQL and DML in loops, trigger framework and automation ordering, Flow bulk safety and fault paths, Lightning Web Component patterns under Lightning Web Security, SLDS 2 styling, test quality beyond coverage percentage, and package and metadata hygiene. Use this whenever the user asks to review Salesforce metadata, a pull request or a diff containing Apex classes, triggers, Flows, Lightning Web Components or permission sets, asks "is this safe to deploy", "anything wrong with this trigger", "will this hit a governor limit", or pastes Apex or LWC and asks for feedback. This skill owns the verdict for Salesforce changes; load the stack skills for idioms only.
metadata:
  technology: Salesforce
  type: code-review
---

# Salesforce Code Review
> **Targets:** Salesforce Spring '26 · Code Analyzer v5 · Lightning Web Security · **Verified:** 2026-09 against https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_gov_limits.htm and https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/guide/

Salesforce code fails differently from ordinary server code. It runs inside hard governor limits that are per-transaction, so a change that passes every test at one record breaks at 200. It runs in system mode by default, so access enforcement is something the author had to remember rather than something the platform did. And automation composes invisibly — a new record-triggered Flow can break an Apex trigger nobody touched. Review for the 200-record transaction and the least-privileged user, because that is where this platform actually fails.

## 1. Decide first

| Question | Default | Change when |
|---|---|---|
| What to read first | The trigger/automation inventory for the objects touched | A pure LWC change with no Apex |
| Bulk assumption | Every entry point receives 200 records | Never assume a single-record path |
| Access assumption | The running user is the least privileged one who can reach this | Only where the elevation is deliberate and stated |
| Static analysis first | Run Code Analyzer v5 and triage its output before reading | Skip only when you cannot run it |
| Coverage | 90 %+ is the org bar; read the assertions as well as the percentage | 90 % with no assertions is still a failing test suite |
| Verdict scope | Blockers and majors block; minors do not | Agreed team exceptions, recorded |

## 2. Before reading the diff

1. **List the automation on every object touched** — triggers, record-triggered Flows, Process Builder remnants, workflow rules, validation rules. Order is not guaranteed between Flows, and a new one can re-enter an existing trigger.
2. **Run Code Analyzer v5** and read the Graph Engine findings, which catch CRUD/FLS violations across call paths:
   ```bash
   sf code-analyzer run --workspace force-app --view detail
   ```
3. **Identify the entry points** — `@AuraEnabled` methods, Apex REST, invocable methods, triggers, scheduled jobs. Each is a boundary where access and input validation must be enforced.
4. **Check what is guest-reachable.** An `@AuraEnabled` method on a public site is an unauthenticated endpoint.

## 3. Severity scale

| Label | Meaning | Blocks merge? |
|---|---|---|
| **[blocker]** | Data exposure, data loss, SOQL/DML in a loop on a bulk path, governor limit at 200 records, missing sharing or CRUD/FLS on a user-facing path, injection | Yes |
| **[major]** | Likely bug at volume, recursion, missing fault path, test with no meaningful assertion, hard-coded id, accessibility failure on a primary flow | Yes, unless deferred with a ticket |
| **[minor]** | Maintainability, non-idiomatic platform usage, duplication | No |
| **[nit]** | Style the tooling does not catch | No |
| **[question]** | Intent needed before judging | — |
| **[praise]** | Worth copying | — |

## 4. The findings that matter most

- **SOQL or DML inside a loop** — the first thing to look for, and still the most common blocker. Collect into a map, query once, DML once.
- **Non-bulk-safe logic** — `Trigger.new[0]`, a single-record assumption, or a method taking an `Id` where the caller has a list.
- **Missing access enforcement** — `with sharing` covers records only; queries need `WITH USER_MODE` and DML needs `AccessLevel.USER_MODE`. `WITH SECURITY_ENFORCED` in new code is superseded and should be replaced, never combined.
- **Injection** — string-concatenated dynamic SOQL. `Database.queryWithBinds` with bind variables.
- **Recursion** — a trigger updating its own object with no static guard, or a Flow and a trigger updating each other.
- **Automation ordering** — a before-save Flow and an Apex trigger both writing the same field; ordering between record-triggered Flows is not guaranteed.
- **Flow bulk safety** — a Get/Update element inside a loop is the Flow equivalent of SOQL in a loop; fault paths missing on every element that can fail.
- **Hard-coded ids** — record type ids, queue ids, profile ids. They differ per org and break on deploy.
- **Tests that assert nothing** — coverage is not correctness. Look for a negative case, a bulk case (200 records) and a restricted-user case.
- **LWC** — Apex called imperatively without error handling, DOM access that Lightning Web Security restricts, third-party script from a CDN rather than a static resource, SLDS 2 styling hooks versus overridden internals.

The full per-area checklist: `references/review-checklist.md`.

## 5. Reviewing metadata you cannot deploy

Much of a Salesforce diff is XML. What to check by reading:

- **`package.xml` / deployment scope** — is anything destructive included, and does the change set carry metadata nobody intended?
- **Permission sets and profiles** — a diff that grants "View All" or "Modify All" is a blocker until justified. Check field-level security changes line by line.
- **Flow XML** — look for elements inside loops, missing fault connectors, and the start element's entry criteria.
- **Custom metadata and settings** — secrets do not belong here; they are visible to anyone who can view setup.

## 6. Deliverable format

```
## Findings
| Severity | File:line | Issue | Why it matters | Fix |
|---|---|---|---|---|
| [blocker] | classes/OrderService.cls:42 | SOQL inside a for loop over Trigger.new | Hits 101 SOQL queries at 200 records | Collect ids, one query into a Map before the loop |
| [blocker] | classes/OrderController.cls:18 | @AuraEnabled method queries without WITH USER_MODE | Returns fields the running user cannot read | Add WITH USER_MODE; class declares with sharing |
| [major]   | flows/Order_After_Save.flow-meta.xml:64 | Update element has no fault connector | Failures are silent; the record is left half-processed | Add a fault path that logs and surfaces the error |

## Summary
Verdict: Approve | Approve with minor changes | Request changes
Blockers: <n>  Majors: <n>  Minors: <n>
Top 3 to address: 1. … 2. … 3. …
Not reviewed: <metadata or files skipped, and why>
What's good: <genuine, specific praise>
```

## Checklist

- [ ] Every entry point was checked against a 200-record transaction.
- [ ] No SOQL, DML or callout inside a loop on a bulk path.
- [ ] Every user-facing class declares sharing; queries use `WITH USER_MODE`, DML uses `AccessLevel.USER_MODE`.
- [ ] No `WITH SECURITY_ENFORCED` added, and never combined with `USER_MODE`.
- [ ] Dynamic SOQL uses bind variables.
- [ ] Automation inventory for each object touched was reviewed for ordering and recursion.
- [ ] Flows have fault paths and no data elements inside loops.
- [ ] No hard-coded ids.
- [ ] Tests assert behaviour, and include a bulk case and a restricted-user case.
- [ ] Permission set and profile changes were read line by line.
- [ ] Guest-reachable entry points validate input and enforce access.
- [ ] Code Analyzer v5 findings were triaged, including Graph Engine results.
- [ ] The verdict states what was not reviewed.

## Anti-patterns

| Anti-pattern | Why it hurts | Fix |
|---|---|---|
| Approving because tests pass at 75% | Below the org's 90 % bar, and coverage is not correctness anyway | Hold to 90 %+; read the assertions; require a bulk and a negative case |
| Reviewing Apex without the automation inventory | A Flow you never opened re-enters the trigger | List all automation on the object first |
| Treating `with sharing` as full enforcement | Fields and objects are still system mode | Require `USER_MODE` on queries and DML |
| Ignoring the XML in the diff | Permission and Flow changes are where access quietly widens | Read permission sets, Flow XML and `package.xml` |
| Line-by-line review of a 2,000-line diff | Attention runs out before the blockers appear | Ask for a split; review by risk area |
| Style nits without the blockers | The author fixes the nits and ships the data exposure | Lead with blockers; let the linter own style |
| Running `sf scanner run` | Retired August 2025 | `sf code-analyzer run` |

## Go deeper

- `references/review-checklist.md` — the full per-area checks: Apex, triggers and automation, Flows, LWC, tests, permissions and packaging, with the failure each one prevents.
- Sibling skills: `apex-development` and `lwc-development` for idioms; `salesforce-security` for org-level posture; `apex-testing` and `salesforce-frontend-testing` for writing the tests this review asks for; `salesforce-org-conventions` for the org's naming and mandated patterns.