--- 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: Majors: Minors: Top 3 to address: 1. … 2. … 3. … Not reviewed: What's good: ``` ## 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.