# Quality Code Review > Review code for any Frappe application — a checklist distilled from years of engineering practice on correctness, security, performance, concurrency, readability, API design, and testing. Use this when reviewing a diff, a PR, or a piece of code for quality and security, or when you want a reviewer's checklist grounded in hard-won Frappe/ERPNext lessons. - Skill: `frappe/quality-code-review` (Agent Skill) - Install (CLI): `npx skillmds@latest add frappe/quality-code-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/frappe/quality-code-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: frappe (https://skillmd.com/u/frappe) - Updated: 2026-09-09 - Page: https://skillmd.com/skills/frappe/quality-code-review --- # Quality Code Review A reviewer's checklist for Frappe applications. Protect **correctness, security, and the future maintainer**, in that order of consequence. The sections below run in that order — spend most attention on §1 and §2. Prefer a root-cause fix over a workaround, and say *why* a finding matters (what breaks, for whom). --- ## 1. Correctness & stability (highest consequence) The worst bug is **silent stateful corruption** — wrong ledgers/stock posted with no error. Treat stateful and legal/accounting/compliance code as **"failure is not an option"** code. - **Fail early and loudly.** Use assertions for internal invariants: `assert total_credit == total_debit`. (Assertions are for invariants the code guarantees — not user-facing validation.) - **Picture how it breaks.** For every change ask: *Where can this break? How will someone misuse this?* Write fool-proof code. Extensions and overrides should especially consider handling all sorts of failure modes. - **No partial commits.** A stray `frappe.db.commit()` / `db.rollback()` mid-transaction ends the transaction and exposes partial state — flag every one. Submitting/saving with validation bypassed (docs posted with no SL/GL entry when validation fails) is a critical bug. - **Don't sacrifice atomicity for convenience** (e.g. adding `autocommit` to fix bootstrapping makes transactions non-atomic). Autocommit belongs only on schema creation. - **Preserve invariants over UX.** "Compromise UX, but guarantee correctness." Don't degrade working code to accommodate broken code. - **Validate the issue before fixing it.** Sometimes the correct fix is "don't fix this". Identify the root causes first. - **Watch for destructive DB APIs with empty/`None` filters.** `set_value("Site", None, ...)` / `db.delete` with no filter updates/deletes *every* row. These must error, not silently operate on the whole table. Flag any `set_value`/`delete`/`get_value` where the name/filter could be `None` or empty- or attacker-controlled. - **Check the types in a condition actually match.** A comparison between mismatched types (string vs `datetime`, string vs int) silently never matches or is always true — cast explicitly (`cint`/`flt`) at the boundary. - **Question the return shape.** Before indexing a result, ask whether it can be `None` / `[None, None]`; watch `as_dict=1` (list of dicts) vs scalar confusion. - **Don't silently change long-standing semantics.** Behavior callers have relied on for a long time is a contract — altering it is a breaking change in disguise, even when no signature changed. ## 2. Security Avoiding a vulnerability is far easier than fixing one safely. Audit security-critical code (auth, authorization, permissions, user management) especially hard. **Injection** - NEVER build SQL by string concatenation/f-strings. Use the ORM or query builder. If raw SQL is unavoidable, use parameter substitution (`frappe.db.sql("... where name = %s", (user,))`) — never interpolate yourself. Better still, **avoid introducing new raw SQL at all**: beyond injection risk it ties code to one database, and the framework aims to stay DB-agnostic (Postgres support). Prefer `frappe.qb`. - **Type confusion is an injection vector even with the ORM.** Frappe accepts complex types, so a parameter expected to be a string can arrive as a filter list: `{"key": ["!=", ""]}` passed to `db.get_value` bypasses a secret-key check. **Validate input *types* at trust boundaries** — explicit `isinstance(key, str)`. Audit every `@frappe.whitelist` method for this. - Never `eval`/`exec` anything yourself. `safe_eval`/`safe_exec` only, in limited volume, and "safe_exec is not magic." Never accept a client-supplied method path to execute. **Sandboxing & trust boundaries** - Sandboxed execution (RestrictedPython/`safe_exec`) is not reliably safe — assume escapes exist. Security toggles must live at the **right trust boundary**: server-script enablement is a *bench*-level config, never site-level (a tenant could enable it and take over the whole server). - Prefer **allowlists over blocklists** — blocklists are bypassable. Don't expose everything by default. **Access control** - "Think 10 times before `allow_guest=True`" — it is not a shortcut around real authn/authz. Web pages must apply permissions *before* reading/sharing data. Prefer `get_list`/`get_all` over hand-rolled queries. - **Scope relaxations precisely.** Verify a rate-limit/permission exception targets exactly the intended principal — not, say, all non-guest users. **Path traversal / filesystem** - Prefer the File doctype API. If user input enters a path, ensure it can't traverse (`/../../`) outside the site folder. **Crypto / secrets** - Never roll your own crypto; reuse existing implementations. Verify authenticity of guest/webhook requests (HMAC). - Signed/one-time URLs: use a truly secret signing value; expire by **both** time and first use; **validate using the URL alone, not merged form data** (Frappe merges URL + form data → replay attacks with one valid signature). - Store secrets in password fields; never plain text; never leak secrets in logs or error messages. **XSS & the rest of OWASP** - Don't inject user input into the DOM. Treat XSS as critical even when it looks trivial — HTML/JS injection usually leads to account hijack. - Don't fix XSS by sanitizing and throwing away special characters. Prefer escaping right before injecting values in DOM. ## 3. Performance is correctness The cheapest time to fix performance is at review; slow code merged sits undetected for years. Performance is a feature (Doherty threshold; humans perceive ~100ms). - **Budgets:** common reads < 100ms; reads < 1s; most writes < 5s; **never** exceed ~10s. P99 of a frequent read-only request should be ~1s. A slow synchronous request blocks a worker (head-of-line blocking). - **Complexity rule:** a frequently-called endpoint must do O(1) or O(log N) work — a large constant factor at worst, never O(N). Counting rows is O(N), not O(1); `COUNT(*)` over a large/filtered table is expensive. Bound unbounded scans (e.g. last 3 months, a "1000+" sentinel) rather than scanning everything. - **Indexes are code.** Flag any `WHERE`/join/filter on an unindexed column. Indexes (and custom indexes) must be **committed in code**, not applied ad-hoc — they get lost on migration otherwise. Form loads that pull comments/versions/assignments need *all* those queries indexed; one unindexed query makes everything sluggish. - **No DB calls in loops.** "Don't write validations that call db in LOOPS." Flag N+1 patterns. Cache stable values (UOM, docstatus, status) instead of re-querying. This is acceptable in background jobs, but never in requests. - **The Remove → Reduce → Reuse ladder** for slow code you can't fix: remove it, invoke it less, or memoize. Pick the right cache scope ( `@redis_cache`, `@request_cache`, `@site_cache` — the last balloons memory if overused). DO NOT hand-roll caches in `frappe.local` or `frappe.flags`: "you'll just be creating brand-new cache-invalidation bugs." Don't cache trivially cheap work. - **Memory:** don't stuff junk into shared module-level files / `__init__.py` / class-level state — it stays resident forever. Remove unused module-level imports (move into the function that uses them). Watch for leaks. - **Reorder conditionals so the DB call is last.** In a boolean expression, put cheap in-memory checks first so short-circuiting can skip the query entirely. - **Aggregate in SQL, not Python.** Use `SUM()`/`COUNT()` in the query instead of fetching all rows to reduce them in memory; push filters into the subquery so they apply *before* the join. - **Don't fetch a whole doc for one value.** Use `get_value`/`get_single_value`/ `set_value` for a single column instead of `get_doc().save()`; use `frappe.delete_doc` instead of `get_doc().delete()` (which fetches the doc only to delete it). - **No MyISAM tables in hot paths.** Reading a MyISAM table takes an implicit table-level lock — never touch one in a request path. - **Move long work to a background queue.** Long-running work belongs in `enqueue(..., queue="long")`, not a synchronous request that blocks a worker. ## 4. Concurrency - **Check-then-act is a race.** `if not frappe.db.exists(...): insert()` — two workers both see "not exists" and both insert. Prefer a **DB-level unique constraint**; "outsource integrity to the database." - **Locking footguns:** `SELECT ... FOR UPDATE` on an unindexed query locks every scanned row (and gaps) — always ensure the filter uses an index, or you lock the whole table. Locking a parent but not its children yields a "mutant" doc. - **Global mutable state / class attributes are global in Python** — a shared engine/class attribute leaking query state across concurrent requests produces garbage. Make query-building **stateless**. Don't do "weird shit with `frappe.local`" — `local` is for variables, not static state. ## 5. Readability & maintainability ~50% of dev time is spent reading code; rotten code eventually forces a rewrite. - **Keep functions pure when they can be pure** — easy to read and test. - **Don't pass mutable objects around to be filled in** ("assembly" code) — return new values. Passing a mutable to be mutated forces a reader to open two files to understand one thing. - A function that mutates its input must be named appropriately. - **Prefer the boring construct.** While that functional map-reduce one-liner looks beautiful, please just write a 4-line for-loop. Favor debuggable code over clever code. - **Consistency over personal style.** A codebase shouldn't be a hodge-podge of 10 styles. Match the surrounding formatting/naming/import conventions; flag a change that breaks them. - **Good taste:** restructure so the edge case becomes the common case, removing special-case branches. Ask: can this be simpler? Less code? Is it over-indented if-else soup? - **Prefer extending shared components over copy-paste divergence.** 3–4 forked implementations of one thing → slow long-term velocity. Avoid tight coupling across modules; integrate through clear, documented public APIs. - Document **public** modules/classes/functions with docstrings; **prefer type annotations over describing types in prose** ("type hints are 10x better"); type checkers find non-obvious bugs. - Docstrings should only mention important things. Keep them short and to the point. Don't explain what's trivially understood from function name. Focus on "why". - **Split unrelated changes** into separate commits/PRs — keeps review focused and `git blame`/reverts clean. ## 6. API design & backward compatibility - **Principle of least astonishment:** an API's name + signature should convey ~90% of intent; users shouldn't be surprised by behavior. - **Reject loose/overloaded parameters** that accept many disjoint types (string/dict/list/None). Prefer separate single-purpose functions. Beware implicit fallbacks; use explicit variants. "APIs whose correct use depends on tribal knowledge are a liability." - **Build for extension, not override.** Provide hooks; never monkey-patch core at runtime ("inexcusably horrible" — breaks future fixes) and never copy a whole core file to change a few lines (fixes won't propagate). - **Backward compatibility is an obligation** for mature/public APIs. Follow semver; **minor versions = zero breaking changes**. Breaking changes include: removing public functions/fields, reordering args, new mandatory args, changed business logic, moved/renamed files (broken imports), bumped shared deps. Renaming without keeping the old name as an alias is an *unnecessary* break. Every breaking change ships a deprecation warning + docs. - **Watch for schema breaking-change footguns:** adding mandatory fields to existing sites, making long-lived fields unique (needs a data patch), changing field types without patches, removing fields. - **Schema changes that silently skip existing sites need a data patch.** Single doctypes don't sync new-field defaults to existing sites, and a field-type change (e.g. text→int) doesn't convert existing values — both need an explicit patch, tested against a populated site. - **New parameters go last as keyword args with safe defaults** (`None`, not `""`) so existing positional callers don't break. When renaming, keep the old name as a shim: `def old_name(...): return new_name(...)`. - **Patch hygiene.** Data patches must be **idempotent** (safe to re-run), **correctly ordered** (run after the field/doctype they read exists), and live in the **right app** (a framework change is patched in the framework, not the downstream app). - **A modified existing test is a red flag.** If making a change pass required editing an existing test's assertions, you've likely broken a real workflow — justify it explicitly rather than bending the test. ## 7. Testing - **Each PR needs decent test coverage** — patch coverage on the diff, not just project coverage. (Frappe target: 85% covered lines in the diff.) Tests should capture the most-used business scenarios. - **Regression test every fix.** A bug fix without a test that would have caught it invites the regression back. For extreme-consequence (stateful/compliance) code, go beyond examples — property-based testing (Hypothesis). - **Flag missing migration/data-patch coverage.** Schema changes and data patches are the highest-risk, least-tested area; a change that alters fields or migrates data needs a patch tested against a realistic, populated site (empty tables always "migrate" successfully even when the change is invalid). - **Tests must be deterministic and independent.** No `random` (flaky); no reliance on state left by other tests (order-dependence); use `freeze_time` for time-dependent logic. ## 8. Error messages, logging & observability - **Error message quality is a legitimate review item.** Titles must be specific and Google-able (never "Message"/"Error"). Reference field names as fields. State *what changed*: the row, the field, and before→after values (`1 → 2`). The user must know "qty changed from what to what?" - **Surface failures to the affected party** — "a broken email setup is the user's problem only if they know it's broken." - **Log things.** Preserve tracebacks/exception context (orders of magnitude easier debugging). Log destructive/admin actions with attributable identity (who, when, from where), persisted outside ephemeral containers.