diff options
| -rw-r--r-- | docs/design/2026-07-10-ledger-config-audit.org | 124 | ||||
| -rw-r--r-- | tests/test-ledger-config.el | 70 |
2 files changed, 194 insertions, 0 deletions
diff --git a/docs/design/2026-07-10-ledger-config-audit.org b/docs/design/2026-07-10-ledger-config-audit.org new file mode 100644 index 00000000..15db988f --- /dev/null +++ b/docs/design/2026-07-10-ledger-config-audit.org @@ -0,0 +1,124 @@ +#+TITLE: Ledger config audit — correctness and guardrail gaps +#+AUTHOR: Craig Jennings +#+DATE: 2026-07-10 + +* Scope + +The correctness half of the ledger guardrail work. It audits +=modules/ledger-config.el= (71 lines, no test file before today) and reports what's +wrong and what's missing. It designs no guardrails, because those choices are +Craig's. + +Characterization tests for the behavior that exists now landed alongside this note +at =tests/test-ledger-config.el=. + +The stakes set the bar. This is a financial file, and Craig's stated goal for the +follow-on work is "enough guardrails that it's hard to make a costly mistake." + +* Findings + +** F1. The linting doesn't run. Nothing checks unbalanced transactions. + +The module's own commentary says it provides "flycheck linting." + +=flycheck-ledger= is loaded (=ledger-config.el:59-60=) and registers the =ledger= +checker. But =flycheck-mode= is never enabled in a ledger buffer. +=flycheck-config.el:47= hooks =flycheck-mode= to exactly two modes: + +#+begin_src elisp +:hook ((sh-mode emacs-lisp-mode) . flycheck-mode) +#+end_src + +There is no =global-flycheck-mode= anywhere in the config. Verified against a real +=ledger-mode= buffer in the running daemon: =major-mode= is =ledger-mode=, +=flycheck-checkers= contains =ledger=, and =flycheck-mode= is =nil=. + +So an unbalanced transaction, a malformed date, and a typo'd account name all +produce no warning. The guardrail Craig believes he has is not connected. + +This is the finding that matters. Everything below is smaller. + +** F2. Every save silently reorders the entire file. + +=cj/ledger--clean-before-save= runs =ledger-mode-clean-buffer= on +=before-save-hook= (=ledger-config.el:32-40=), with =cj/ledger-clean-on-save= +defaulting to =t=. + +=ledger-mode-clean-buffer= (=ledger-mode.el:226=) is not a formatter. Its own +docstring says "Indent, remove multiple line feeds and sort the buffer," and its +body calls, in order: =untabify=, =ledger-sort-buffer=, =ledger-post-align-postings=, +=ledger-mode-remove-extra-lines=. + +=ledger-sort-buffer= (=ledger-sort.el:106=) sorts the whole buffer by date, from +=point-min= to =point-max= unless the file carries explicit sort markers. + +So every =C-x C-s= rewrites the transaction order of a financial file. The task +asked whether clean-on-save "ever reorders or rewrites in a surprising way." It +reorders, every time, by design of the upstream function. + +Whether that's wanted is Craig's call. It is at minimum undocumented: the module +commentary calls it "clean-on-save," which reads as whitespace tidying. + +** F3. The demoted error hides a partial rewrite, not just a message. + +The clean is wrapped in =with-demoted-errors= so "a malformed buffer still saves" +(=ledger-config.el:34-37=). + +The comment is accurate about the save. It is misleading about the buffer. The +operations inside =ledger-mode-clean-buffer= run in sequence and mutate as they go. +An error raised by =ledger-post-align-postings= or by the =search-forward= that +restores point (=ledger-mode.el:239=) happens *after* =ledger-sort-buffer= has +already reordered everything. Nothing rolls back. + +The result is a file that saved, in a state neither the user nor the cleaner +intended, with a message in the echo area that scrolls away. + +** F4. Reconcile has no confirmation, and clears whole transactions. + +=ledger-clear-whole-transactions= is =t= (=ledger-config.el:43=), so a reconcile +marks entire transactions cleared rather than individual postings. Combined with no +confirmation step anywhere in the module, a stray keypress in a reconcile buffer +mutates the ledger file. + +This is upstream behavior, not a bug in this module. It is named here because the +follow-on guardrail work asked about "reconcile safety" and this is what reconcile +safety currently amounts to. + +* What is not a problem + +Worth recording so the guardrail work doesn't chase it. + +- *company-ledger's global backend is correctly scoped.* =ledger-config.el:68= does + =(add-to-list 'company-backends 'company-ledger)= globally, which looks like it + would offer ledger completions everywhere. It doesn't: the backend's own =prefix= + command (=company-ledger.el:110-111=) returns nil unless the buffer is + =beancount-mode= or derives from =ledger-mode=. +- *The reports pass =--strict=.* All five entries in =ledger-reports= + (=ledger-config.el:48-52=) use =--strict=, so a report over a file with an + undeclared account errors rather than silently inventing one. This is a real + guardrail and it is on. +- *The missing-binary check is at the right place.* =cj/executable-find-or-warn= + runs at =:config= (=ledger-config.el:54=), so a missing =ledger= CLI warns at load + rather than failing cryptically inside a report. +- *The clean-on-save hook is installed buffer-locally* (=ledger-config.el:40=), not + globally. Pinned by a test. + +* Gaps, as a list + +1. No linting reaches ledger buffers (F1). +2. No confirmation before a whole-buffer rewrite on save (F2). +3. No rollback when that rewrite fails partway (F3). +4. No confirmation before reconcile mutates the file (F4). +5. No validation that a save leaves the file balanced. +6. No test coverage before today. + +* What this note deliberately does not do + +It proposes no fix. Whether clean-on-save should prompt, become opt-in, or be +replaced by a check-only lint is a preference call about Craig's own accounting +workflow, and so is the shape of a reconcile confirmation. Those belong to the +ledger guardrail UX task. + +The one finding that isn't a preference call is F1: turning flycheck on in ledger +buffers restores a guardrail the module already claims to have. That's a defect, +not a design choice. diff --git a/tests/test-ledger-config.el b/tests/test-ledger-config.el new file mode 100644 index 00000000..5224c184 --- /dev/null +++ b/tests/test-ledger-config.el @@ -0,0 +1,70 @@ +;;; test-ledger-config.el --- Characterization tests for ledger-config -*- lexical-binding: t; -*- + +;;; Commentary: +;; Captures the behavior ledger-config.el has today, before any guardrail work +;; changes it. See docs/design/2026-07-10-ledger-config-audit.org for the audit +;; these tests pin. +;; +;; The clean-on-save helpers are defined in the `use-package' `:preface', which +;; use-package emits unconditionally, so they exist under `make test' even though +;; ledger-mode itself never loads there (no `package-initialize' in the test run). +;; +;; `ledger-mode-clean-buffer' is stubbed: it is the boundary this config delegates +;; to, and it rewrites the whole buffer. What these tests pin is whether our hook +;; calls it, not what it does. + +;;; Code: + +(require 'ert) +(require 'cl-lib) + +(add-to-list 'load-path (expand-file-name "modules" user-emacs-directory)) +(require 'ledger-config) + +(ert-deftest test-ledger-config-clean-before-save-cleans-when-enabled () + "Normal: with `cj/ledger-clean-on-save' set, the save hook cleans the buffer." + (let ((called 0) + (cj/ledger-clean-on-save t)) + (cl-letf (((symbol-function 'ledger-mode-clean-buffer) + (lambda (&rest _) (setq called (1+ called))))) + (cj/ledger--clean-before-save)) + (should (= 1 called)))) + +(ert-deftest test-ledger-config-clean-before-save-skips-when-disabled () + "Boundary: with `cj/ledger-clean-on-save' nil, the save hook does nothing." + (let ((called 0) + (cj/ledger-clean-on-save nil)) + (cl-letf (((symbol-function 'ledger-mode-clean-buffer) + (lambda (&rest _) (setq called (1+ called))))) + (cj/ledger--clean-before-save)) + (should (= 0 called)))) + +(ert-deftest test-ledger-config-clean-before-save-demotes-errors () + "Error: a failing clean does not signal, so the file still saves. +This is the current contract. It also means a clean that fails partway +leaves the buffer in whatever state it reached, because nothing rolls back." + (let ((cj/ledger-clean-on-save t) + (inhibit-message t)) + (cl-letf (((symbol-function 'ledger-mode-clean-buffer) + (lambda (&rest _) (error "boom")))) + (should (progn (cj/ledger--clean-before-save) t))))) + +(ert-deftest test-ledger-config-enable-clean-on-save-is-buffer-local () + "Normal: the hook installs buffer-locally, not globally." + (let ((global-before (default-value 'before-save-hook))) + (with-temp-buffer + (cj/ledger--enable-clean-on-save) + (should (memq #'cj/ledger--clean-before-save before-save-hook)) + (should-not (memq #'cj/ledger--clean-before-save + (default-value 'before-save-hook)))) + (should (equal global-before (default-value 'before-save-hook))))) + +(ert-deftest test-ledger-config-clean-on-save-defaults-on () + "Normal: clean-on-save ships enabled. +Pinned because the audit questions whether a whole-buffer sort belongs on +every save of a financial file. If that default flips, this test should +fail and be updated deliberately." + (should (eq t (default-value 'cj/ledger-clean-on-save)))) + +(provide 'test-ledger-config) +;;; test-ledger-config.el ends here |