aboutsummaryrefslogtreecommitdiff
path: root/docs/design/2026-07-10-ledger-config-audit.org
diff options
context:
space:
mode:
Diffstat (limited to 'docs/design/2026-07-10-ledger-config-audit.org')
-rw-r--r--docs/design/2026-07-10-ledger-config-audit.org146
1 files changed, 146 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..ff969d78
--- /dev/null
+++ b/docs/design/2026-07-10-ledger-config-audit.org
@@ -0,0 +1,146 @@
+#+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.
+
+FIXED 2026-07-10. =flycheck-config.el= now hooks =flycheck-mode= to =ledger-mode=.
+Verified end to end on an unbalanced fixture: flycheck reports "Transaction does not
+balance" with the =$10.00= remainder. The finding is kept below as the record of
+what was wrong.
+
+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.
+
+*Accepted 2026-07-10, on inspection of the sort key.* The reordering is
+chronological and nothing else. =ledger-sort-startkey= (=ledger-sort.el:62=) builds
+its key from the first ten characters of a transaction's opening line, the ISO date,
+through =ledger-parse-iso-date= and =float-time=. Payee, amount, and account never
+enter the key. =sort-subr= uses Emacs's stable sort, so transactions sharing a date
+keep the order they were typed in.
+
+The scope is bounded on request: =ledger-sort-buffer= narrows to the region between
+=; Ledger-mode: Start sort= and =; Ledger-mode: End sort= marker comments when the
+file carries them, and only falls back to the whole buffer when it doesn't.
+
+So a save sorts by date, stably, over a region the file can choose. Craig accepts
+that. The finding stays recorded because "clean-on-save" still reads as whitespace
+tidying, and the next person to meet this function should know it sorts.
+
+** 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).+ Fixed 2026-07-10.
+2. No confirmation before the date-sort on save (F2). Accepted: the sort is
+ chronological and stable.
+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 designs no guardrails. Whether a clean that fails should roll back, and what
+shape a reconcile confirmation takes, are preference calls about Craig's own
+accounting workflow. Those belong to the ledger guardrail UX task.
+
+The one finding that wasn't a preference call was F1: turning flycheck on in ledger
+buffers restored a guardrail the module already claimed to have. That was a defect,
+not a design choice, and it is fixed.
+
+F2 was resolved by reading the sort key rather than by changing code. What remains
+for the guardrail task is F3 (no rollback when a clean fails partway) and F4 (no
+confirmation before reconcile), plus the open question of whether a save should
+verify the file still balances.