--- name: elisp-review description: > Review Emacs Lisp (elisp) packages for bugs, style/convention violations, and optimisation opportunities. Use this skill whenever the user asks to review, audit, check, or analyse an elisp or Emacs Lisp package, file, or set of files. Trigger on phrases like "review my package", "check my elisp", "audit this emacs package", "look for bugs in my lisp", "optimise my elisp", or any time the user shares .el files and wants feedback. Always use this skill when .el files are involved and improvement is the goal — even if the user just says "what do you think of this?" about an elisp file. --- # Elisp Package Review Skill You are performing a structured code review of an Emacs Lisp package. Your goal is to produce a **prioritised action plan** the author can work through at their own pace. Do NOT rewrite code unless explicitly asked — flag issues and explain them clearly. --- ## Phase 1: Orientation Before diving into issues, briefly orient yourself: 1. Read every `.el` file in the package. 2. Identify the package's purpose, entry points, public API, and key data flows. 3. Note the Emacs version target and any declared dependencies (`Package-Requires`). 4. Check whether `lexical-binding` is enabled in every file. State a one-paragraph summary of what the package does before listing any issues. --- ## Phase 2: Review Dimensions Work through each dimension below in order. Collect ALL findings before presenting — do not present dimension-by-dimension. ### 1. Correctness & Bugs Look for logic errors and runtime hazards: - **Unguarded `car`/`cdr`** on potentially-nil values — use `when`, `if-let`, or `and` to guard. - **Off-by-one errors** in list/string indexing. - **Mutation of shared structure** — accidental aliasing via `setcar`/`setcdr`. - **Process/buffer leaks** — processes or temp buffers created but never cleaned up; missing `unwind-protect`. - **Async race conditions** — timer or sentinel callbacks that assume buffer/ process state that may have changed. - **Wrong equality predicate** — `eq` vs `equal` vs `string=` misuse. - **Incorrect use of `mapcar` vs `mapc`** — using `mapcar` when return value is discarded (wasteful allocation). - **`save-excursion` / `save-restriction` misuse** — forgetting to restore state after buffer modifications. - **Hook not removed on cleanup** — hooks added in init but never removed in teardown/disable path. - **Advice not removed** — `advice-add` without corresponding `advice-remove` in disable/unload path. - **Missing `require`** — symbols used from libraries not explicitly required. - **Circular or redundant `require`** — files requiring each other or requiring things already guaranteed by dependencies. ### 2. Emacs Lisp Style & Conventions Check against established community conventions: **File header:** - `;;; package-name.el --- Short description -*- lexical-binding: t -*-` - `;;; Commentary:` section present and informative. - `;;; Code:` marker present. - `;;; package-name.el ends here` footer present. - `Package-Version`, `Package-Requires`, `Author`, `Keywords`, `URL` headers present and accurate. **Naming:** - All public symbols prefixed with `package-name-` (or agreed namespace). - Internal/private symbols prefixed with `package-name--` (double dash). - Constants use `defconst` not `defvar`. - Booleans named with `-p` suffix (`package-name-verbose-p`). **Docstrings:** - Every `defun`, `defvar`, `defcustom`, `defface`, `define-minor-mode` has a docstring. - First line of docstring is a complete sentence ending in `.` and ≤80 chars. - Interactive commands document their argument in the first line if applicable. - `defcustom` docstrings describe valid values. **Customisation:** - User-facing variables use `defcustom`, not `defvar`. - `defcustom` has correct `:type`, `:group`, and `:safe` where appropriate. - A `defgroup` exists for the package. **Functions:** - Prefer `cl-lib` over deprecated `cl` package (`cl-loop`, `cl-destructuring-bind`, etc.). - Prefer `seq-` functions over manual recursion for sequence operations. - Avoid `flet`/`labels` — use `cl-flet`/`cl-labels`. - Avoid `lexical-let` — unnecessary with `lexical-binding: t`. - `interactive` spec is correct and uses modern forms (e.g. `(interactive "r")` not deprecated forms). - Functions that modify buffers use `with-current-buffer` rather than relying on implicit current buffer. **Control flow:** - Prefer `when`/`unless` over `(if ... nil)` / `(if ... t)`. - Prefer `cond` over deeply nested `if`. - Prefer `pcase` over complex `cond` matching on structure. - Avoid `(not (not x))` — use `(and x t)` or just trust truthiness. ### 3. Performance & Optimisation - **Repeated `buffer-substring` / `buffer-string`** in tight loops — cache the result. - **`re-search-forward` in loops without `narrow-to-region`** — can be O(n²); consider reorganising. - **`append` in loops** — quadratic; prefer `push` + `nreverse`. - **`length` on a list to check emptiness** — use `null` or `consp` instead. - **Uncompiled lambdas in hot paths** — prefer named functions or ensure byte compilation. - **Large `defconst` data** — consider lazy initialisation if not always needed. - **`sit-for 0` / `redisplay` in loops** — usually a sign of a design smell; flag and explain. - **Synchronous process calls blocking UI** — prefer async with sentinels or `make-process`. - **Unnecessary `with-temp-buffer`** — if only string operations are needed, avoid buffer allocation. - **Timer granularity** — timers firing too frequently (< 0.1s) without clear need. - **`font-lock-add-keywords`** called repeatedly — should be called once, not on every mode activation. ### 4. Autoloads & Load-Time Cost - All entry-point commands and public functions the user calls directly should have `;;;###autoload` cookies. - No expensive computation at top level (i.e. outside any function) — this runs at load time. - `defvar` / `defcustom` at top level is fine; `defun` bodies running at load time are not. - `require` at top level is acceptable but flag heavy requires that could be deferred with `with-eval-after-load` or `autoload`. ### 5. Compatibility & Portability - Flag use of functions introduced after the declared minimum Emacs version in `Package-Requires`. - Flag OS-specific code paths without appropriate guards (`system-type` checks). - Flag hard-coded paths. - Flag any use of `(require 'cl)` — must use `(require 'cl-lib)`. ### 6. Error Handling & Robustness - `condition-case` used where failures are plausible (network, file I/O, subprocess). - Error messages are user-readable (not raw Lisp objects). - `user-error` used for user-facing mistakes (not `error`), so Edebug doesn't trap them. - `unwind-protect` used wherever resources (buffers, processes, overlays) are allocated. --- ## Phase 3: Output Format Present findings as a **structured action plan** using the following format. Group by severity. Within each group, order by file then by approximate line number. ``` ## Package Review: ### Summary --- ### 🔴 Critical — Fix Before Release Issues that will cause errors, data loss, or broken behaviour. #### C1. **File:** `foo.el` **~Line:** 42 **Issue:** **Suggestion:** #### C2. ... --- ### 🟠 Important — Strong Recommendation Style violations, missing conventions, or meaningful inefficiencies. #### I1. ... --- ### 🟡 Minor — Worth Addressing Small style issues, minor optimisations, nitpicks. #### M1. ... --- ### 💡 Optimisation Opportunities Performance improvements worth considering, ordered by estimated impact. #### O1. ... --- ### ✅ Strengths Brief list of things done well — keep this honest and specific. ``` --- ## Phase 4: Closing Note After the plan, add a short paragraph: > "This is a plan for you to action at your own pace — not all items need to be > addressed. Prioritise 🔴 Critical items first. Feel free to ask me to elaborate > on any specific finding or to help implement a fix." --- ## Review Principles - **Flag, don't fix.** Explain the problem and point in a direction. The author decides what to do. - **Be specific.** Always cite the file and approximate line number. - **Be proportionate.** A one-file utility and a major package deserve different levels of rigour — calibrate accordingly. - **No ERT / testing review.** Do not comment on presence or absence of tests. - **Respect intent.** If a pattern looks unusual but is clearly deliberate, note it as a question rather than a violation.