fix(agenda): stop the frame's catch-all from swallowing input machinery

The default-deny [t] binding intercepted far more than Org commands. switch-frame events hit the deny handler, so every focus change into or out of the frame spammed the read-only message and broke frame switching. Mouse wheel, mouse-1 clicks, and the help prefix were dead. And S-<f8>/C-M-<f8> were never bound in the mode map, so the frame's own toggle couldn't close it from inside.

I punched the machinery through the catch-all (an explicit nil shadows the [t] default in the same map, so those events fall through to their global bindings) and bound the two frame controls per the spec's allowlist.

Two redo-lifecycle bugs rode along, both from org-agenda-mode's kill-all-local-variables: the buffer-local kill-buffer-hook was stripped on the first refresh tick (killing the buffer would orphan the frame), and the failure banner was held in a buffer-local var while its overlay survives erase-buffer, so a banner could never be removed after a later success. The finalize re-enable now re-adds the hook, and the banner is tagged with an overlay property and found by scanning.

2 files changed, 107 insertions, 21 deletions

diff --git a/modules/org-agenda-frame.el b/modules/org-agenda-frame.el
index 9ac02e4d..8c3659f5 100644
--- a/modules/org-agenda-frame.el
+++ b/modules/org-agenda-frame.el
@@ -235,6 +235,20 @@ The default binding for every key not on the allowlist."
     (define-key map (kbd "Q") #'cj/--agenda-frame-close)
     (define-key map (kbd "x") #'cj/--agenda-frame-close)
     (define-key map (kbd "r") #'cj/--agenda-frame-safe-redo)
+    (define-key map (kbd "S-<f8>") #'cj/agenda-frame-toggle)
+    (define-key map (kbd "C-M-<f8>") #'cj/org-agenda-refresh-files)
+    ;; (d) Input machinery punched through the catch-all.  An explicit nil
+    ;; shadows the [t] default in this map, so these fall through to their
+    ;; global bindings.  Without the punches, every frame-focus change
+    ;; (switch-frame), every wheel scroll, and every mouse click hits the
+    ;; deny handler -- message spam and broken frame switching.
+    (dolist (key (list [switch-frame]
+                       [wheel-up] [wheel-down] [wheel-left] [wheel-right]
+                       [double-wheel-up] [double-wheel-down]
+                       [triple-wheel-up] [triple-wheel-down]
+                       [mouse-1] [down-mouse-1] [drag-mouse-1]
+                       (kbd "C-h")))
+      (define-key map key nil))
     ;; View-changers get the distinct fixed-view message, not the read-only one.
     (dolist (key '("w" "d" "y" "f" "b" "j" "g"))
       (define-key map (kbd key) #'cj/--agenda-frame-denied-fixed-view))
@@ -262,10 +276,15 @@ Added to `org-agenda-finalize-hook'.  `org-agenda-redo' rebuilds through
 minor mode; this reinstates it whenever the just-built buffer is displayed
 in the frame carrying the `cj/agenda-frame' marker (a frame parameter, which
 survives the buffer reset).  Ordinary agenda builds in working frames are
-left untouched."
+left untouched.
+
+The same reset also strips the buffer-local `kill-buffer-hook' installed at
+spawn, so it is re-added here too -- otherwise, after the first refresh
+tick, killing the buffer would no longer delete the frame."
   (let ((frame (cj/--agenda-frame)))
     (when (and frame (get-buffer-window (current-buffer) frame))
-      (cj/agenda-frame-mode 1))))
+      (cj/agenda-frame-mode 1)
+      (add-hook 'kill-buffer-hook #'cj/--agenda-frame-on-kill-buffer nil t))))
 
 ;;; Frame lifecycle — spawn, raise, delete, toggle, cleanup
 
@@ -392,10 +411,12 @@ Non-interactive by design in Phase 1: reachable only from ERT, never from
 (defconst cj/--agenda-frame-fail-count-parameter 'cj/agenda-frame-fail-count
   "Frame parameter holding the consecutive-failure count for the refresh timer.")
 
-(defvar-local cj/--agenda-frame-failure-overlay nil
-  "Overlay showing the refresh-failed notice in the dedicated buffer.
-An overlay, not inserted text, so the org text properties (org-redo-cmd,
-markers) stay intact and consecutive failures never accumulate a header.")
+(defconst cj/--agenda-frame-overlay-property 'cj/agenda-frame-failure
+  "Overlay property tagging the refresh-failed banner.
+The banner is found by scanning for this property, never held in a
+buffer-local variable: `org-agenda-redo' runs `kill-all-local-variables',
+which would wipe the variable while the overlay object survives
+`erase-buffer' -- leaving a banner nothing could ever remove.")
 
 (defun cj/--agenda-frame-seconds-to-next-mark (time period)
   "Return seconds from TIME to the next wall-clock multiple of PERIOD.
@@ -526,22 +547,28 @@ Reports only on the first failure of a run (the 0 -> 1 transition)."
   "Reset FRAME's consecutive-failure count (the next tick reports again)."
   (set-frame-parameter frame cj/--agenda-frame-fail-count-parameter 0))
 
+(defun cj/--agenda-frame-failure-overlays (buffer)
+  "Return the refresh-failed banner overlays in BUFFER (normally 0 or 1)."
+  (with-current-buffer buffer
+    (seq-filter (lambda (o) (overlay-get o cj/--agenda-frame-overlay-property))
+                (overlays-in (point-min) (point-max)))))
+
 (defun cj/--agenda-frame-show-failure-overlay (buffer)
-  "Show the refresh-failed notice as an overlay at the top of BUFFER."
+  "Show the refresh-failed notice as an overlay at the top of BUFFER.
+Idempotent: an existing banner is reused, so consecutive failures never
+stack a second one."
   (with-current-buffer buffer
-    (unless (overlayp cj/--agenda-frame-failure-overlay)
-      (setq cj/--agenda-frame-failure-overlay
-            (make-overlay (point-min) (point-min))))
-    (overlay-put cj/--agenda-frame-failure-overlay 'before-string
-                 (propertize "Agenda frame: refresh failed (C-M-<f8> to force-rescan)\n"
-                             'face 'warning))))
+    (let ((overlay (or (car (cj/--agenda-frame-failure-overlays buffer))
+                       (make-overlay (point-min) (point-min)))))
+      (overlay-put overlay cj/--agenda-frame-overlay-property t)
+      (overlay-put overlay 'before-string
+                   (propertize "Agenda frame: refresh failed (C-M-<f8> to force-rescan)\n"
+                               'face 'warning)))))
 
 (defun cj/--agenda-frame-remove-overlay (buffer)
-  "Remove the refresh-failed overlay from BUFFER, if present."
-  (with-current-buffer buffer
-    (when (overlayp cj/--agenda-frame-failure-overlay)
-      (delete-overlay cj/--agenda-frame-failure-overlay)
-      (setq cj/--agenda-frame-failure-overlay nil))))
+  "Remove the refresh-failed banner from BUFFER, if present."
+  (when (buffer-live-p buffer)
+    (mapc #'delete-overlay (cj/--agenda-frame-failure-overlays buffer))))
 
 ;; -- The refresh itself ------------------------------------------------------
 
diff --git a/tests/test-org-agenda-frame.el b/tests/test-org-agenda-frame.el
index 48ac1594..0b1062a1 100644
--- a/tests/test-org-agenda-frame.el
+++ b/tests/test-org-agenda-frame.el
@@ -223,6 +223,63 @@ not caught by the read-only catch-all."
     (should (eq (lookup-key cj/agenda-frame-mode-map (kbd key))
                 'cj/--agenda-frame-denied-fixed-view))))
 
+(ert-deftest test-org-agenda-frame-map-frame-controls-bound ()
+  "Normal: the frame's own controls work from inside the frame.
+S-<f8> must close/toggle and C-M-<f8> must force-rescan; unbound, the
+catch-all denies them and the frame can't be closed by its own key."
+  (should (eq (lookup-key cj/agenda-frame-mode-map (kbd "S-<f8>"))
+              'cj/agenda-frame-toggle))
+  (should (eq (lookup-key cj/agenda-frame-mode-map (kbd "C-M-<f8>"))
+              'cj/org-agenda-refresh-files)))
+
+(ert-deftest test-org-agenda-frame-map-machinery-punched-through ()
+  "Boundary: input machinery is punched through the [t] catch-all.
+switch-frame events, mouse-wheel scrolling, mouse-1 clicks, and the help
+prefix must fall through to their global bindings (an explicit nil shadows
+the default in this map); otherwise every frame-focus change and every
+scroll spams the deny message."
+  (dolist (key (list [switch-frame]
+                     [wheel-up] [wheel-down] [wheel-left] [wheel-right]
+                     [double-wheel-up] [double-wheel-down]
+                     [triple-wheel-up] [triple-wheel-down]
+                     [mouse-1] [down-mouse-1] [drag-mouse-1]
+                     (kbd "C-h")))
+    ;; accept-default t: a punched key returns nil (falls through to the
+    ;; global map); an unpunched key returns the catch-all deny handler.
+    (should (null (lookup-key cj/agenda-frame-mode-map key t)))))
+
+(ert-deftest test-org-agenda-frame-map-unpunched-still-denied ()
+  "Normal: an ordinary unbound key still hits the catch-all after the punches."
+  (should (eq (lookup-key cj/agenda-frame-mode-map (kbd "t") t)
+              'cj/--agenda-frame-denied-readonly)))
+
+(ert-deftest test-org-agenda-frame-maybe-enable-readds-kill-buffer-hook ()
+  "Normal: the finalize re-enable also re-adds the buffer-local kill hook.
+org-agenda-redo's kill-all-local-variables strips the hook installed at
+spawn; without the re-add, killing the buffer after the first refresh tick
+orphans the frame."
+  (with-temp-buffer
+    (cl-letf (((symbol-function 'cj/--agenda-frame) (lambda () 'agenda))
+              ((symbol-function 'get-buffer-window) (lambda (_b _f) 'win)))
+      (cj/--agenda-frame-maybe-enable-mode)
+      (should (memq 'cj/--agenda-frame-on-kill-buffer
+                    (buffer-local-value 'kill-buffer-hook (current-buffer)))))))
+
+(ert-deftest test-org-agenda-frame-overlay-removable-after-local-var-wipe ()
+  "Error: the failure overlay is found by property, not a buffer-local var.
+kill-all-local-variables (every redo) wipes buffer-local vars while the
+overlay object survives erase-buffer, so a var-held overlay could never be
+removed after a later success -- the failure banner would stick forever."
+  (with-temp-buffer
+    (insert "x\n")
+    (cj/--agenda-frame-show-failure-overlay (current-buffer))
+    ;; Simulate the org-agenda-mode reset between failure and success.
+    (kill-all-local-variables)
+    (cj/--agenda-frame-remove-overlay (current-buffer))
+    ;; The visible banner (a before-string overlay) must be gone.
+    (should (= 0 (seq-count (lambda (o) (overlay-get o 'before-string))
+                            (overlays-in (point-min) (point-max)))))))
+
 (ert-deftest test-org-agenda-frame-map-mutation-keys-not-explicitly-bound ()
   "Boundary: a mutation key (t = org-agenda-todo) is not explicitly bound, so the
 [t] catch-all denies it as read-only."
@@ -594,7 +651,8 @@ re-enables the policy, shows one overlay, and reports once."
                      (lambda (fmt &rest a) (push (apply #'format fmt a) msgs))))
             (cj/--agenda-frame-do-redo 'af (current-buffer) nil)
             (should cj/agenda-frame-mode)                         ; policy re-enabled
-            (should (overlayp cj/--agenda-frame-failure-overlay)) ; overlay shown
+            (should (= 1 (length (cj/--agenda-frame-failure-overlays
+                                  (current-buffer)))))            ; overlay shown
             (should (string-match-p "good agenda content" (buffer-string))) ; restored
             (should-not (string-match-p "PARTIAL" (buffer-string)))
             (should (= 1 (seq-count (lambda (m) (string-match-p "refresh failed" m))
@@ -683,9 +741,10 @@ Guards against restoring by raw line number after the item moved."
     (unwind-protect
         (progn
           (cj/--agenda-frame-show-failure-overlay (current-buffer))
-          (let ((first cj/--agenda-frame-failure-overlay))
+          (let ((first (car (cj/--agenda-frame-failure-overlays (current-buffer)))))
             (cj/--agenda-frame-show-failure-overlay (current-buffer))
-            (should (eq cj/--agenda-frame-failure-overlay first))
+            (should (equal (cj/--agenda-frame-failure-overlays (current-buffer))
+                           (list first)))
             (should (= 1 (seq-count (lambda (o) (overlay-get o 'before-string))
                                     (overlays-in (point-min) (point-max)))))))
       (cj/--agenda-frame-remove-overlay (current-buffer)))))