aboutsummaryrefslogtreecommitdiff
path: root/docs/design/2026-07-21-code-scan-findings.org
diff options
context:
space:
mode:
authorCraig Jennings <c@cjennings.net>2026-07-21 06:42:54 -0500
committerCraig Jennings <c@cjennings.net>2026-07-21 06:42:54 -0500
commit24f8c5aac8873837824ca6d61e566e3e24c96ad5 (patch)
tree40336f31bf714ab8fc39596ce5324dda035467fc /docs/design/2026-07-21-code-scan-findings.org
parente201ba696ed27e407d934321a5f4c5874ea4fee5 (diff)
downloadorg-drill-24f8c5aac8873837824ca6d61e566e3e24c96ad5.tar.gz
org-drill-24f8c5aac8873837824ca6d61e566e3e24c96ad5.zip
docs: reflow spec tables, fix docstrings, add code-scan findings report
The two design-spec tables now follow the org table standard. I shortened three docstring first lines past the 80-column byte-compile warning and corrected the two visible-cloze face docstrings, which carried the hidden-cloze wording verbatim. The new findings report (docs/design/2026-07-21-code-scan-findings.org) records a full scan of org-drill.el: 22 graded bugs and 5 refactor groups with file:line evidence and fix directions. The gitignore gains the task-archive path so the archive stays untracked like the todo file.
Diffstat (limited to 'docs/design/2026-07-21-code-scan-findings.org')
-rw-r--r--docs/design/2026-07-21-code-scan-findings.org141
1 files changed, 141 insertions, 0 deletions
diff --git a/docs/design/2026-07-21-code-scan-findings.org b/docs/design/2026-07-21-code-scan-findings.org
new file mode 100644
index 0000000..85b7ea6
--- /dev/null
+++ b/docs/design/2026-07-21-code-scan-findings.org
@@ -0,0 +1,141 @@
+#+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.