From 68877f04c2ceb569ae5cd74b8303b84b36ced1c5 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Tue, 5 May 2026 12:20:29 -0500 Subject: fix: validate numeric defcustoms at customize-time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six numeric settings are declared as integers but were read straight into arithmetic and timer math. A bad value (string, negative number, nil where nil isn't supported) used to slip past the defcustom and surface as a timer error or `arith-error' deep in a callback, instead of as a configuration problem at the moment the user set it. I added `chime--validate-integer-setting' as a small shared helper and wired a `:set' on each of the affected defcustoms: - `chime-modeline-lookahead-minutes' — integer >= 0 (0 disables) - `chime-tooltip-lookahead-hours' — integer >= 1 - `chime-modeline-tooltip-max-events' — integer >= 1 or nil (show all) - `chime-day-wide-advance-notice' — integer >= 0 or nil (same-day only) - `chime-max-consecutive-failures' — integer >= 0 (0 disables warnings) - `chime-validation-max-retries' — integer >= 0 (0 = fail immediately) The constraints follow each docstring's stated intent. The helper signals `user-error', so `customize-set-variable' surfaces it as a config problem rather than a generic error trace. Tests: 22 cases in `tests/test-chime-numeric-defcustom-setters.el' — five direct on the helper plus each defcustom's accept/reject paths through `customize-set-variable'. --- chime.el | 48 ++++++- tests/test-chime-numeric-defcustom-setters.el | 186 ++++++++++++++++++++++++++ 2 files changed, 228 insertions(+), 6 deletions(-) create mode 100644 tests/test-chime-numeric-defcustom-setters.el diff --git a/chime.el b/chime.el index 3dafc48..ee08f69 100644 --- a/chime.el +++ b/chime.el @@ -78,6 +78,24 @@ "Chime customization options." :group 'org) +(defun chime--validate-integer-setting (symbol value min allow-nil) + "Reject bad integer values for SYMBOL at customize time. +VALUE is what the user is trying to set. MIN is the inclusive floor. +When ALLOW-NIL is non-nil, nil is accepted; otherwise nil fails like any +other non-integer. Returns VALUE on success so the caller can chain into +`set-default'. The error is a `user-error' so `customize-set-variable' +surfaces it as a configuration problem rather than a generic error." + (cond + ((and allow-nil (null value)) value) + ((not (integerp value)) + (user-error "%s must be %s, got: %S" + symbol + (if allow-nil "nil or an integer" "an integer") + value)) + ((< value min) + (user-error "%s must be >= %d, got: %d" symbol min value)) + (t value))) + (defcustom chime-alert-intervals '((10 . medium) (0 . high)) "Alert intervals with severity levels for upcoming events. Each element is a cons cell (MINUTES . SEVERITY) where: @@ -311,7 +329,10 @@ Example: With value 1 and alert times \\='(\"08:00\"), you'll get: :package-version '(chime . "0.6.0") :group 'chime :type '(choice (const :tag "Same day only" nil) - (integer :tag "Days in advance"))) + (integer :tag "Days in advance")) + :set (lambda (symbol value) + (chime--validate-integer-setting symbol value 0 t) + (set-default symbol value))) (defcustom chime-tooltip-show-all-day-events t "Whether to show all-day events in the tooltip. @@ -350,7 +371,10 @@ Set to 0 to disable modeline display. This setting only takes effect when `chime-enable-modeline' is non-nil." :package-version '(chime . "0.6.0") :group 'chime - :type '(integer :tag "Minutes")) + :type '(integer :tag "Minutes") + :set (lambda (symbol value) + (chime--validate-integer-setting symbol value 0 nil) + (set-default symbol value))) (defcustom chime-modeline-format " ⏰ %s" "Format string for modeline display. @@ -388,7 +412,10 @@ Note: larger values increase the `org-agenda-list' span in the async subprocess, which may slow event checks for large org collections." :package-version '(chime . "0.6.0") :group 'chime - :type '(integer :tag "Hours")) + :type '(integer :tag "Hours") + :set (lambda (symbol value) + (chime--validate-integer-setting symbol value 1 nil) + (set-default symbol value))) (defcustom chime-modeline-tooltip-max-events 5 "Maximum number of events to show in modeline tooltip. @@ -396,7 +423,10 @@ Set to nil to show all events within tooltip lookahead window." :package-version '(chime . "0.6.0") :group 'chime :type '(choice (integer :tag "Maximum events") - (const :tag "Show all" nil))) + (const :tag "Show all" nil)) + :set (lambda (symbol value) + (chime--validate-integer-setting symbol value 1 t) + (set-default symbol value))) (defcustom chime-modeline-no-events-text " ⏰" "Text to display in modeline when no events are within lookahead window. @@ -536,7 +566,10 @@ via `display-warning'. The counter resets on any successful check. Set to 0 to disable failure warnings." :package-version '(chime . "0.6.0") :group 'chime - :type 'integer) + :type 'integer + :set (lambda (symbol value) + (chime--validate-integer-setting symbol value 0 nil) + (set-default symbol value))) (defcustom chime-debug nil "Enable debug functions for troubleshooting chime behavior. @@ -616,7 +649,10 @@ Set to 0 to show errors immediately without retrying. Default is 3 retries (with 30-60s check intervals, this gives ~1.5-3 minutes for org-agenda-files to be populated)." :type 'integer - :group 'chime) + :group 'chime + :set (lambda (symbol value) + (chime--validate-integer-setting symbol value 0 nil) + (set-default symbol value))) (defvar chime-modeline-string nil "Modeline string showing next upcoming event.") diff --git a/tests/test-chime-numeric-defcustom-setters.el b/tests/test-chime-numeric-defcustom-setters.el new file mode 100644 index 0000000..03d7810 --- /dev/null +++ b/tests/test-chime-numeric-defcustom-setters.el @@ -0,0 +1,186 @@ +;;; test-chime-numeric-defcustom-setters.el --- Setter validation for numeric defcustoms -*- lexical-binding: t; -*- + +;; Copyright (C) 2026 Craig Jennings + +;; Author: Craig Jennings + +;; This program is free software: you can redistribute it and/or modify +;; it under the terms of the GNU General Public License as published by +;; the Free Software Foundation, either version 3 of the License, or +;; (at your option) any later version. + +;; This program is distributed in the hope that it will be useful, +;; but WITHOUT ANY WARRANTY; without even the implied warranty of +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;; GNU General Public License for more details. + +;; You should have received a copy of the GNU General Public License +;; along with this program. If not, see . + +;;; Commentary: + +;; The numeric defcustoms feed arithmetic (timer math, lookahead windows, +;; counters) and a bad value reaches the wrong layer if it slips past the +;; setter — the user gets an `arith-error' or a confusing timer failure +;; instead of a configuration error. These tests pin down the contract: +;; bad values raise `user-error' at customize-time; valid values land. + +;;; Code: + +(require 'test-bootstrap (expand-file-name "test-bootstrap.el")) + +;;;; chime--validate-integer-setting helper + +(ert-deftest test-chime-validate-integer-setting-accepts-valid-non-negative () + "Normal: 0 passes when MIN is 0." + (should (eq 0 (chime--validate-integer-setting + 'fake-symbol 0 0 nil)))) + +(ert-deftest test-chime-validate-integer-setting-rejects-non-integer () + "Error: a string raises `user-error' naming the symbol." + (let ((err (should-error (chime--validate-integer-setting + 'fake-symbol "ten" 0 nil) + :type 'user-error))) + (should (string-match-p "fake-symbol" (cadr err))))) + +(ert-deftest test-chime-validate-integer-setting-rejects-below-min () + "Error: a value below MIN raises `user-error'." + (should-error (chime--validate-integer-setting + 'fake-symbol 0 1 nil) + :type 'user-error)) + +(ert-deftest test-chime-validate-integer-setting-allows-nil-when-permitted () + "Boundary: nil passes when ALLOW-NIL is non-nil." + (should (null (chime--validate-integer-setting + 'fake-symbol nil 0 t)))) + +(ert-deftest test-chime-validate-integer-setting-rejects-nil-when-not-permitted () + "Error: nil fails when ALLOW-NIL is nil." + (should-error (chime--validate-integer-setting + 'fake-symbol nil 0 nil) + :type 'user-error)) + +;;;; chime-modeline-lookahead-minutes — integer >= 0 + +(ert-deftest test-chime-modeline-lookahead-minutes-accepts-zero () + "Normal: 0 is valid (disables modeline display)." + (let ((chime-modeline-lookahead-minutes 120)) + (customize-set-variable 'chime-modeline-lookahead-minutes 0) + (should (= 0 chime-modeline-lookahead-minutes)))) + +(ert-deftest test-chime-modeline-lookahead-minutes-rejects-negative () + "Error: negative integer raises." + (let ((chime-modeline-lookahead-minutes 120)) + (should-error (customize-set-variable + 'chime-modeline-lookahead-minutes -5) + :type 'user-error))) + +(ert-deftest test-chime-modeline-lookahead-minutes-rejects-non-integer () + "Error: string raises." + (let ((chime-modeline-lookahead-minutes 120)) + (should-error (customize-set-variable + 'chime-modeline-lookahead-minutes "many") + :type 'user-error))) + +;;;; chime-tooltip-lookahead-hours — integer >= 1 + +(ert-deftest test-chime-tooltip-lookahead-hours-accepts-positive () + "Normal: 1 is the floor." + (let ((chime-tooltip-lookahead-hours 168)) + (customize-set-variable 'chime-tooltip-lookahead-hours 1) + (should (= 1 chime-tooltip-lookahead-hours)))) + +(ert-deftest test-chime-tooltip-lookahead-hours-rejects-zero () + "Error: 0 hours of lookahead would never find anything." + (let ((chime-tooltip-lookahead-hours 168)) + (should-error (customize-set-variable + 'chime-tooltip-lookahead-hours 0) + :type 'user-error))) + +(ert-deftest test-chime-tooltip-lookahead-hours-rejects-negative () + "Error: negative integer raises." + (let ((chime-tooltip-lookahead-hours 168)) + (should-error (customize-set-variable + 'chime-tooltip-lookahead-hours -1) + :type 'user-error))) + +;;;; chime-modeline-tooltip-max-events — nil or integer >= 1 + +(ert-deftest test-chime-modeline-tooltip-max-events-accepts-nil () + "Boundary: nil means show all (per docstring)." + (let ((chime-modeline-tooltip-max-events 5)) + (customize-set-variable 'chime-modeline-tooltip-max-events nil) + (should (null chime-modeline-tooltip-max-events)))) + +(ert-deftest test-chime-modeline-tooltip-max-events-accepts-positive () + "Normal: 1 is the floor when set." + (let ((chime-modeline-tooltip-max-events 5)) + (customize-set-variable 'chime-modeline-tooltip-max-events 1) + (should (= 1 chime-modeline-tooltip-max-events)))) + +(ert-deftest test-chime-modeline-tooltip-max-events-rejects-zero () + "Error: 0 events makes the tooltip empty by config rather than nil." + (let ((chime-modeline-tooltip-max-events 5)) + (should-error (customize-set-variable + 'chime-modeline-tooltip-max-events 0) + :type 'user-error))) + +;;;; chime-day-wide-advance-notice — nil or integer >= 0 + +(ert-deftest test-chime-day-wide-advance-notice-accepts-nil () + "Boundary: nil means same-day only (per docstring)." + (let ((chime-day-wide-advance-notice 1)) + (customize-set-variable 'chime-day-wide-advance-notice nil) + (should (null chime-day-wide-advance-notice)))) + +(ert-deftest test-chime-day-wide-advance-notice-accepts-zero () + "Boundary: 0 is the floor (same-day only) when set." + (let ((chime-day-wide-advance-notice nil)) + (customize-set-variable 'chime-day-wide-advance-notice 0) + (should (= 0 chime-day-wide-advance-notice)))) + +(ert-deftest test-chime-day-wide-advance-notice-rejects-negative () + "Error: negative-day advance notice is meaningless." + (let ((chime-day-wide-advance-notice nil)) + (should-error (customize-set-variable + 'chime-day-wide-advance-notice -1) + :type 'user-error))) + +;;;; chime-max-consecutive-failures — integer >= 0 + +(ert-deftest test-chime-max-consecutive-failures-accepts-zero () + "Normal: 0 disables failure warnings (per docstring)." + (let ((chime-max-consecutive-failures 5)) + (customize-set-variable 'chime-max-consecutive-failures 0) + (should (= 0 chime-max-consecutive-failures)))) + +(ert-deftest test-chime-max-consecutive-failures-rejects-negative () + "Error: negative threshold is meaningless." + (let ((chime-max-consecutive-failures 5)) + (should-error (customize-set-variable + 'chime-max-consecutive-failures -1) + :type 'user-error))) + +;;;; chime-validation-max-retries — integer >= 0 + +(ert-deftest test-chime-validation-max-retries-accepts-zero () + "Normal: 0 means show errors immediately without retrying (per docstring)." + (let ((chime-validation-max-retries 3)) + (customize-set-variable 'chime-validation-max-retries 0) + (should (= 0 chime-validation-max-retries)))) + +(ert-deftest test-chime-validation-max-retries-accepts-positive () + "Normal: positive integer is valid." + (let ((chime-validation-max-retries 3)) + (customize-set-variable 'chime-validation-max-retries 5) + (should (= 5 chime-validation-max-retries)))) + +(ert-deftest test-chime-validation-max-retries-rejects-negative () + "Error: negative retry count is meaningless." + (let ((chime-validation-max-retries 3)) + (should-error (customize-set-variable + 'chime-validation-max-retries -1) + :type 'user-error))) + +(provide 'test-chime-numeric-defcustom-setters) +;;; test-chime-numeric-defcustom-setters.el ends here -- cgit v1.2.3