path: root/docs/design/2026-07-21-code-scan-findings.org

#+TITLE: Code Scan Findings — org-drill.el
#+AUTHOR: Craig Jennings
#+DATE: 2026-07-21

* About

Full-file bug and refactoring scan of =org-drill.el= (6003 lines), run 2026-07-21 during the overnight sentry session. Method: mechanical metrics in the main thread (function lengths via sexp parsing, parameter counts, literal duplication, dead-code grep), then four scoped read-only review agents over line regions 1-1380, 1380-2440, 2440-3660, 3660-6003, each required to quote verbatim code for every claim. The highest-impact claims were re-verified against the source by hand before this report was written. Line numbers refer to the tree at commit =26091e7= (sentry/2026-07-21-ratio).

Severity grading uses the severity × frequency matrix in the global todo-format rule. Both ERT suites (unit + integration) pass at scan time; the bugs below live on paths the suites don't exercise (interactive flows, legacy data formats, default-config live use).

* Bugs

** [A] BUG-1 — Statistics dashboard scans its own buffer, not the deck (VERIFIED)
=org-drill.el:5730-5735= (=org-drill-statistics--render=). The dashboard body — overview counts, attention tables, forecast, cards CSV rows, all of which call =org-drill-map-entries= — is computed inside =with-current-buffer buffer= (the =*Org Drill Statistics*= buffer). With the default =org-drill-scope= of ='file=, =org-drill-current-scope= returns nil ("current buffer"), so every card scan traverses the empty dashboard buffer. Result with default config: Total cards 0, all attention tables empty, forecast all zeros, empty cards.csv — regardless of deck contents. Bonus crash: cycling scope to ='directory= calls =(file-name-directory (buffer-file-name))= with nil in the file-less dashboard buffer → wrong-type-argument.
Fix direction: capture the originating deck buffer (or resolve the scope to a concrete file list) in =org-drill-statistics= before rendering, and run all card traversals in that context.
Grading: Major severity (new feature non-functional in default config, no data loss) × every dashboard user, every time = P1 = [#A].

** [B] BUG-2 — copy-entry fallback pastes a destination subtree instead of the source card (VERIFIED)
=org-drill.el:3888-3901= (=org-drill-copy-entry-to-other-buffer=). =org-copy-subtree= runs before =switch-to-buffer dest=; when =org-find-olp= fails (path only partially exists in DEST), the recursive fallback re-executes the whole function while DEST is current — re-copying whatever subtree point sits on in DEST. DEST gains a stripped "imported" copy of one of its own subtrees; the source card is silently lost.
Trigger: =org-drill-merge-buffers= where SRC has a new item whose outline path only partially exists in DEST.
Fix direction: copy once up front and pass the text (or keep SRC current); use =with-current-buffer= instead of =switch-to-buffer=.
Grading: Critical severity (deck corruption + silent card loss) × rare edge = P2 = [#B].

** [B] BUG-3 — Dashboard card links use an unregistered link type (VERIFIED)
=org-drill.el:5280-5281= emits =[[org-drill-card:POS][...]]= links, but no =org-link-set-parameters= call exists anywhere in the package (grep confirmed). RET (bound to =org-open-at-point=) treats the type as a fuzzy link and signals "No match" — dashboard navigation never works. A bare POS also cannot identify the buffer under multi-file scopes.
Fix direction: register the type with a :follow handler and encode buffer/file identity alongside POS.
Grading: Major × every dashboard user attempting navigation = P2 = [#B]. Fix alongside BUG-1.

** [B] BUG-4 — Leitner rating 1 on a box-0 card writes box -1; card vanishes (VERIFIED)
=org-drill.el:4495-4501= (=org-drill-leitner-rebox=). Box 0 is a valid state (capture accepts 0-5; rebox defaults an absent property to 0), but the demotion floors at box 1 only when current-box = 1, so box 0 yields "-1", which matches neither the unboxed nor the boxed branch of =org-drill-map-leitner-capture= — the card silently leaves the system. Related: the prompt says "0 - Completely forgot. (Back to Zero)" while the ?0 branch writes box "1".
Fix direction: clamp the demotion at the minimum box; reconcile the ?0 branch with its prompt.
Grading: Critical (silent card loss) × rare edge = P2 = [#B].

** [C] BUG-5 — Tags key at the answer prompt reveals the answer (VERIFIED)
=org-drill.el:2096=: =(while (memq ch '(nil org-drill--tags-key)))= quotes the symbol, but =ch= is a character, so the memq never matches the tags key; after =org-set-tags-command= runs, control falls through to =(t t)= and reveals the answer instead of re-prompting. Contrast the correct =(list ...)= form at 1800-1804.
Fix: =(memq ch (list nil org-drill--tags-key))=.
Grading: Major (tag-editing at prompt broken, card prematurely revealed) × some users sometimes = P3 = [#C].

** [C] BUG-6 — "No longer due" skip leaves current-item set; session ends in error (VERIFIED shape)
=org-drill.el:3163=: the not-due skip branch (unlike the 'skip route at 3120) leaves =(oref session current-item)= pointing at the skipped marker, so =org-drill-entries-pending-p= stays true with empty queues; the next iteration pops nil and hits "Unexpectedly ran out of pending drill items". Trigger: last card of a session fails for the 16th time → tagged leech → under default =org-drill-leech-method= 'skip it is no longer due → session aborts with an error; the stats record and SM5 persist are skipped.
Fix: clear current-item in the skip branch (or treat a nil pop as clean exit).
Grading: Major (session record lost, error instead of report) × rare edge = P3 = [#C].

** [C] BUG-7 — store-item-data crashes on nil meanq/ease from legacy cards (VERIFIED shape)
=org-drill.el:1373-1378=: =org-drill-round-float= is applied to =meanq= / =ease= slots that =org-drill-get-item-data= can legitimately leave nil (legacy LEARN_DATA path at 1331; property path at 1352-1353 when DRILL_AVERAGE_QUALITY / DRILL_EASE are absent) → wrong-type-argument. Reachable via =org-drill--copy-scheduling-to-marker= (get → store directly).
Fix: guard nil in the store (skip or default), or never emit nil for rated cards in get.
Grading: Major (crash) × rare (legacy-format cards) = P3 = [#C].

** [C] BUG-8 — Empty-string org-drill-match crashes every session (VERIFIED)
=org-drill.el:999-1003=: =(elt org-drill-match 0)= on ="\"\""= signals args-out-of-range; the defcustom's string widget makes "" easy to set via customize. Every command routed through =org-drill-map-entries= then errors.
Fix: require non-zero length before the elt (treat "" as nil).
Grading: Major (session can't start) × rare = P3 = [#C].

** [C] BUG-9 — SM2 e-factor floor clamps input, not output (VERIFIED)
=org-drill.el:1441-1443=: canonical SM2 clamps after the update; here the check runs before, so returned EF can dip below 1.3, and at exactly 1.3 the quality argument is ignored next call — a hard card's ratings stop mattering near the floor.
Fix: compute the update, then =(max 1.3 result)=.
Grading: Minor (scheduling drift near floor) × some users sometimes = P3 = [#C].

** [C] BUG-10 — Weight clamping diverges between smart-reschedule and the hypothetical path
=org-drill.el:1685-1689= vs =1734-1738=: smart-reschedule clamps the whole weighted sum at 1.0; the hypothetical-next-review-date form clamps only the delta/weight quotient (forcing ≥1 day growth). Since =org-drill-reschedule= always passes numeric days-ahead, the hypothetical formula is what actually schedules interactively — the two entry points schedule the same card differently (e.g. last 10, computed 13, weight 4 → 10.75 vs 11).
Fix: extract one shared weight-adjust helper; pick one clamp semantics.
Grading: Minor × some (weighted cards) = P3 = [#C].

** [C] BUG-11 — Early/late-repetition adjustment is inert in the shipped call graph
=org-drill.el:1674-1680= (+ 1726-1730): sm5's delta-days argument is never passed by either caller, and simple8's delta-days-adjusted interval is discarded whenever days-ahead is numeric — which it is for both in-tree callers. Setting =org-drill-adjust-intervals-for-early-and-late-repetitions-p= changes nothing interactively.
Fix: thread delta-days through the hypothetical path (or compute the adjustment where days-ahead is derived).
Grading: Major (documented option does nothing) × some users sometimes = P3 = [#C].

** [C] BUG-12 — org-drill-leitner-completed never resets (VERIFIED)
Defvar at 4316, incremented at 4525, read at 4335/4387 — no reset anywhere. It accumulates for the life of the Emacs process, so =org-drill-sm-or-leitner='s dispatch (pending > completed) increasingly favors Leitner over due SM sessions.
Fix: reset to 0 at the start of each Leitner session.
Grading: Major (wrong session dispatch) × some (Leitner users, long-lived daemon) = P3 = [#C].

** [C] BUG-13 — Cloze overlays leak on C-g in hide-n / hide-nth
=org-drill.el:2759= (and 2782-2784): cleanup sits in a =prog1= tail, skipped on nonlocal exit; the surrounding macros' unwind handlers delete only the hidden-text overlay category, not cloze overlays. C-g at the prompt of any multicloze card leaves [...] masks in the org buffer permanently.
Fix: unwind-protect around prompt + cleanup — falls out of the shared presenter helper (REF-2).
Grading: Minor × some = P3 = [#C].

** [C] BUG-14 — replace-entry-text-multi assert too weak; overlays can run past point-max
=org-drill.el:2494=: layout consumes 2 chars per replacement (needs ≥ 2N-1) but the assert requires only N; a short body at buffer end → args-out-of-range mid-presentation; mid-buffer, the overlay spills past the next heading. Odd-offset characters between one-char overlays also show through.
Fix: strengthen the assert to 2N-1 and clamp overlay ends to p-max.
Grading: Major (crash) × rare = P3 = [#C].

** [C] BUG-15 — show1-lastmore's uncommon branch never reveals the first piece
=org-drill.el:2849=: passes force-hide-first, guaranteeing the first piece stays hidden — but the comment says "show any item except the last" and the docstring says "any random piece". Behavior matches neither.
Fix: plain =(org-drill-present-multicloze-hide-n session -1)= for "any random piece" (or implement except-last); fix the comment.
Grading: Minor × some = P3 = [#C].

** [C] BUG-16 — "All time" CSV export truncates daily.csv to 90 days
=org-drill.el:5983-5984= (clamp at 5927): the "all time" preset maps days to nil, which --daily-rows replaces with =org-drill-statistics-trend-days= (90); daily.csv silently drops older days while sessions.csv covers the whole log.
Fix: derive days from the oldest record when the range is unbounded (or document the floor).
Grading: Minor × some = P3 = [#C].

** [C] BUG-17 — Trends renders as level-1 heading, swallowing later sections
=org-drill.el:5527-5529=: "* Trends" among "** ..." siblings — TAB-collapsing Trends hides Distribution, Attention, and Forecast.
Fix: "** Trends".
Grading: Cosmetic × every dashboard user = P3 = [#C].

** [D] BUG-18 — Merge leaves point moved in DEST
=org-drill.el:3977-3981=: the strip maphash has no save-excursion; after merge, point in DEST sits on the last unmatched entry. Fix: wrap in save-excursion. Cosmetic × some = P4 = [#D].

** [D] BUG-19 — free-markers call in prepare-fresh-session is a no-op
=org-drill.el:3511= runs on the brand-new empty session (constructed at 3634); the previous session's queue markers are never explicitly freed (GC-bounded impact). Fix: free the old session's markers before reassigning, or drop the call. Minor internal × every = [#D].

** [D] BUG-20 — "Drill session finished!" on quit, doubled on completion
=org-drill.el:3653=: unconditional after =org-drill-entries=, including quit/edit exits; on normal completion the same message + sit-for repeats in --show-end-message. Fix: delete; let show-end-message own it. Cosmetic × most = P4 = [#D].

** [D] BUG-21 — Hidden-cloze display string baked at load time
=org-drill.el:279-284=: the "[...]" display string is computed once at load from the delimiter defcustoms, while the cloze regexp is recomputed per buffer — customized delimiters mismatch the mask. Fix: compute at overlay creation. Minor × rare = P4 = [#D].

** [D] BUG-22 — card-type/card-tags alist :type specs don't match value shape
=org-drill.el:414-416= (and 446-447): values are lists, spec says =function= — customize edits produce malformed entries. Fix: correct the :type. Minor × rare = P4 = [#D].

* Refactoring opportunities

** [C] REF-1 — Scheduler duplication: meanq update triplicated, normalize/assert prologue duplicated
=org-drill.el:1503-1506= (verbatim at 1402-1405, 1608-1610); n/ef defaulting + quality asserts duplicated across sm2/sm5. Extract =org-drill--update-meanq= + a shared state-normalize prologue. Pairs naturally with fixing BUG-9/10/11. Also: SM5 failure path writes an OF-matrix entry keyed by a next-ef the card never adopts (1520-1528) — decide and document, or move the write into the success branch. SM2's second-repetition noise table (1420-1427) carries unreachable arms given the failure boundary.

** [C] REF-2 — Presenter boilerplate: one shared present-and-prompt helper
The latex/inline-images/hide-drawers/prompt/re-hide closing sequence is near-verbatim in six presenters (2573-2578, 2615-2620, 2643-2648, 2660-2665, 2755-2761, 2778-2784). Extract one helper with an unwind-protect — which also fixes BUG-13. The weight-validation cond is additionally triplicated across firstmore/lastmore/firstless (2817-2831, 2841-2855, 2866-2880) — extract a 'fallback/'uncommon/'common dispatcher.

** [C] REF-3 — Statistics attention pipeline duplicated
=org-drill-statistics--render-attention= (5316-5399) re-implements the filter+sort+cap+row pipeline of the three selectors (5131-5195), which production code never calls (tests only); comparators exist in two copies each. Drive both from one (predicate, comparator) table. Also delete: dead =count= in org-drill-leitner (4354-4364), vestigial let-bindings in --render-all (5679-5681), inert same-file declare-function block (5596-5600).

** [C] REF-4 — org-drill-get-item-data duplication
The seven-line property-path constructor appears verbatim twice (1335-1341, 1347-1353) and the virgin-state constructor twice (1343-1345, 1355-1357); the inner if re-implements the outer cond's arms. Extract =org-drill--card-state-from-properties= + a virgin-state helper. Pairs with fixing BUG-7.

** [D] REF-5 — Small cleanups batch
- =org-drill-pop-random= macro evaluates PLACE five times (967-972) — bind once via macroexp-let2.
- =(or default nil)= → =default= in the DRILL_AVERAGE_QUALITY getter (1201-1204).
- Dead =again-entries= arm in entries-pending-p (3003) — outer or already covers it.
- DRILL_* property-name literals repeat 4-9× each — consider named constants.
- =org-drill-with-replaced-entry-text-multi= has zero in-tree callers but is public API for custom card-type authors — keep, do not remove.

* Metrics summary

Longest defuns (true sexp lengths): render-attention 75, org-drill 71, simple8 69, --read-rating-key 66 — all under the 100-line critical band; ten functions in the 51-100 "split recommended" band, most either schedulers (REF-1) or renderers (REF-3). No functions with 5+ positional parameters remain (card-state refactor). Byte-compile is warning-free as of =26091e7=.

* Verification status

VERIFIED = re-checked by hand against the source during consolidation. VERIFIED shape = quoted code confirmed present and mechanism plausible; the full trigger chain still wants a reproduction test. Unmarked = agent-reported with verbatim quotes that matched spot-checks; write the failing ERT reproduction first per the project's TDD convention before fixing any of them.