fix: code-review findings on overnight-jul5

- digest queue no longer dropped on failed dispatch (retry next tick)
- collapsed reminders marked fired/rescheduled only after digest delivers
- /tools step-up gate skipped when WebAuthn is not configured (was 403 forever)
- passkey credential store rolls back memory on persist failure
- auth_test fake updated for TickTrace (branch build break)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ASstMtsZWLSRcD1Tq8T68Q

AGENTS.md

@@ -0,0 +1,6 @@
+# Maven — Agent Context
+
+## Vikunja
+This repo maps to **Maven** (project ID: 2) in Vikunja.
+Feature work, bugs, deployment tasks all go here.
+MCP endpoint: `http://localhost:9100/mcp` (or `http://192.168.1.104:9100/mcp` from workpc)

SESSION-05-07-2026.md

@@ -78,8 +78,8 @@ Notes:
 | # | Task | Commit | Status |
 |---|------|--------|--------|
 | 17 | **In-process auth gate for /tools** — local PasskeySession check on POST, returns 403 if unasserted | 5afff00 | done |
-| 18 | **Digest / notification history UI** — section on `/dash` showing batched notifications | — | pending |
-| 19 | **Rule trace page** — new /trace route showing predicate eval results per rule per tick | — | pending (depends on #15 backend, can now build)
+| 18 | **Digest / notification history UI** — /notifications page with recent nudge history | 5c34fb1 | done |
+| 19 | **Rule trace page** — /trace route showing predicate eval results per rule per tick | 85013f7 | done |
 | 20 | **Command history page** — new `/history` route showing recent facts/commands | f8ba396 | done |
 | 21 | **PWA icons** — SVG icon + manifest.json icons array | 00a3bba | done |
 | 22 | **Language unification** — bilingual cheatsheet with RU/EN toggle in nav + ?lang= param | c225ba3 | done |

cmd/mavend/tick.go

@@ -300,7 +300,9 @@ func (t *tickLoop) flushDigest(ctx context.Context, now time.Time, state loop.St
 	}
 	t.cachePhrase(pn)
 	if _, err := t.dispatcher.DispatchNudge(ctx, pn, now); err != nil {
+		// keep the queue — the next tick's maybeFlush re-attempts.
 		log.Printf("tick: dispatch digest: %v", err)
+		return
 	}
 	t.digestQ = nil
 }

cmd/mavweb/credentials.go

@@ -51,7 +51,14 @@ func (cs *credentialStore) Save(id string, publicKey []byte) error {
 		return fmt.Errorf("credential already exists")
 	}
 	cs.creds[id] = localCred{PublicKey: publicKey}
-	return cs.persist()
+	if err := cs.persist(); err != nil {
+		// roll back so memory matches disk — otherwise the credential looks
+		// registered until restart, then silently vanishes, and a retry hits
+		// "already exists".
+		delete(cs.creds, id)
+		return err
+	}
+	return nil
 }
 
 func (cs *credentialStore) Lookup(id string) (publicKey []byte, signCount int64, err error) {
@@ -67,11 +74,16 @@ func (cs *credentialStore) Lookup(id string) (publicKey []byte, signCount int64,
 func (cs *credentialStore) UpdateSignCount(id string, count int64) error {
 	cs.mu.Lock()
 	defer cs.mu.Unlock()
-	cred, ok := cs.creds[id]
+	prev, ok := cs.creds[id]
 	if !ok {
 		return fmt.Errorf("credential not found")
 	}
+	cred := prev
 	cred.SignCount = count
 	cs.creds[id] = cred
-	return cs.persist()
+	if err := cs.persist(); err != nil {
+		cs.creds[id] = prev // keep memory consistent with disk
+		return err
+	}
+	return nil
 }

cmd/mavweb/handlers_test.go

@@ -298,10 +298,10 @@ func TestHandleTools_POST_Disable_CoreError_502(t *testing.T) {
 func TestEnableTool_NoInProcessAuthGate(t *testing.T) {
 	core := &fakeCore{}
 	rr := httptest.NewRecorder()
-	// No passkey session (nil) — no step-up, gate rejects.
+	// Session exists (WebAuthn wired) but was never asserted — gate rejects.
 	handleTools(rr, postForm("enable", url.Values{
 		"name": {"svc"}, "cmd": {"systemctl restart"},
-	}), core, nil)
+	}), core, webauthn.NewPasskeySession(5*time.Minute))
 	if rr.Code != http.StatusForbidden {
 		t.Fatalf("status = %d, want %d (expected auth gate to reject)", rr.Code, http.StatusForbidden)
 	}
@@ -310,6 +310,23 @@ func TestEnableTool_NoInProcessAuthGate(t *testing.T) {
 	}
 }
 
+// TestEnableTool_NoWebAuthnConfigured verifies that when WebAuthn is not
+// wired at all (nil session — no way to ever assert), the step-up gate is
+// not applied and /tools falls back to its transport-level auth.
+func TestEnableTool_NoWebAuthnConfigured(t *testing.T) {
+	core := &fakeCore{}
+	rr := httptest.NewRecorder()
+	handleTools(rr, postForm("enable", url.Values{
+		"name": {"svc"}, "cmd": {"systemctl restart"},
+	}), core, nil)
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status = %d, want 200; body=%s", rr.Code, rr.Body.String())
+	}
+	if core.gotEnableName != "svc" {
+		t.Errorf("name = %q, want svc", core.gotEnableName)
+	}
+}
+
 // TestEnableTool_WithAuthGate_RequiresStepUp verifies that a POST /tools with
 // an asserted passkey session proceeds past the auth gate to core.EnableTool.
 func TestEnableTool_WithAuthGate_RequiresStepUp(t *testing.T) {

cmd/mavweb/main.go

@@ -154,9 +154,15 @@ func main() {
 	// Passkey registration + assertion are the step-up mechanism for
 	// AuthStepUp actions (tool enable). Without -webauthn-origin, these
 	// endpoints return 503 and step-up is unavailable (FloorSession).
-	stepUpSession := webauthn.NewPasskeySession(5 * time.Minute)
+	// stepUpSession stays nil unless the passkey endpoints are wired — it can
+	// only ever be asserted via AssertFinish, so gating POST /tools on it
+	// without those endpoints would make tool enable/disable permanently 403.
+	// Without WebAuthn configured, /tools falls back to the transport-level
+	// auth it sits behind (wg+nginx+auth), same as before step-up existed.
+	var stepUpSession *webauthn.PasskeySession
 
 	if *pkOrigin != "" && *pkRPID != "" && core != nil {
+		stepUpSession = webauthn.NewPasskeySession(5 * time.Minute)
 		pk, err := newPasskeyHandle(webauthn.Config{
 			Origin: *pkOrigin,
 			RPID:   *pkRPID,
@@ -501,7 +507,9 @@ func handleTools(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, sessi
 	ctx := r.Context()
 	var msg string
 	if r.Method == http.MethodPost {
-		if !session.IsStepUp() {
+		// nil session ⇒ WebAuthn not configured; step-up gate not applicable
+		// (see wiring in main — asserting would be impossible, not just unmet).
+		if session != nil && !session.IsStepUp() {
 			http.Error(w, "step-up required: assert a passkey first", http.StatusForbidden)
 			return
 		}

internal/auth/auth_test.go

@@ -339,6 +339,9 @@ func (r *recordingAPI) CreateReminder(_ context.Context, _ time.Time, _, _ strin
 	return 1, nil
 }
 func (r *recordingAPI) MarkReminder(_ context.Context, _ int64, _ string) error { return nil }
+func (r *recordingAPI) TickTrace(_ context.Context) (ipc.TickTrace, error) {
+	return ipc.TickTrace{}, nil
+}
 func (r *recordingAPI) RecordNudge(_ context.Context, _, _, _ string, _ time.Time) (int64, error) {
 	return 1, nil
 }

internal/delivery/dispatcher.go

@@ -185,24 +185,37 @@ func (d *Dispatcher) DispatchReminder(ctx context.Context, pr PhrasedReminder, n
 		out = append(out, Dispatch{Sendable: s})
 	}
 	if d.cfg.Reminders != nil && len(out) > 0 {
-		// ID=0 is a synthetic digest reminder; it's not in the DB so
-		// MarkReminder/RescheduleReminder would fail. The originals
-		// were already marked fired by collapseReminders in gather.go.
-		if rd.Reminder.ID != 0 {
-			if rd.Reminder.Cron != "" {
-				if err := d.cfg.Reminders.RescheduleReminder(ctx, rd.Reminder.ID, now); err != nil {
-					return out, fmt.Errorf("reschedule reminder %d: %w", rd.Reminder.ID, err)
-				}
-			} else {
-				if err := d.cfg.Reminders.MarkReminder(ctx, rd.Reminder.ID, "fired"); err != nil {
-					return out, fmt.Errorf("mark reminder fired: %w", err)
+		// ID=0 is a synthetic digest reminder; it's not in the DB. Complete
+		// the collapsed originals it stands in for instead — only now, after
+		// a successful send, so a failed digest leaves them all pending.
+		if rd.Reminder.ID == 0 {
+			for _, orig := range rd.Reminder.Collapsed {
+				if err := d.completeReminder(ctx, orig, now); err != nil {
+					return out, err
 				}
 			}
+		} else if err := d.completeReminder(ctx, rd.Reminder, now); err != nil {
+			return out, err
 		}
 	}
 	return out, nil
 }
 
+// completeReminder — post-delivery bookkeeping for one reminder: recurring
+// (cron set) reschedules, one-shot marks fired.
+func (d *Dispatcher) completeReminder(ctx context.Context, r store.Reminder, now time.Time) error {
+	if r.Cron != "" {
+		if err := d.cfg.Reminders.RescheduleReminder(ctx, r.ID, now); err != nil {
+			return fmt.Errorf("reschedule reminder %d: %w", r.ID, err)
+		}
+		return nil
+	}
+	if err := d.cfg.Reminders.MarkReminder(ctx, r.ID, "fired"); err != nil {
+		return fmt.Errorf("mark reminder %d fired: %w", r.ID, err)
+	}
+	return nil
+}
+
 // RepeatUnacked — the daemon calls this each tick to re-send un-acked sev4
 // telegram nudges. `keys` = rule names with un-acked telegram sends (the
 // daemon queries the nudges table for pending-outcome sev4+telegram rows —

internal/loop/gather.go

@@ -12,7 +12,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"log"
 	"time"
 
 	"github.com/kami/maven/internal/store"
@@ -143,10 +142,7 @@ func (g *Gatherer) GatherState(ctx context.Context, now time.Time) (State, []sto
 	if err != nil {
 		return State{}, nil, err
 	}
-	due, err = collapseReminders(ctx, g.store, due)
-	if err != nil {
-		return State{}, nil, err
-	}
+	due = collapseReminders(due)
 
 	s := State{
 		Now:           now,
@@ -204,12 +200,14 @@ func readFact(ctx context.Context, s *store.Store, key string) (store.Fact, bool
 
 // collapseReminders — when multiple reminders are due at once (e.g. after
 // the daemon was offline), collapse them into a single digest reminder to
-// avoid a burst of individual notifications. All original reminders are
-// marked as fired; the synthetic digest (ID=0) is what gets dispatched.
+// avoid a burst of individual notifications. The synthetic digest (ID=0)
+// carries the originals in Collapsed; the dispatcher completes them (mark
+// fired / reschedule) only after the digest actually delivers, preserving
+// the "failed send leaves the reminder pending" invariant.
 // When 0 or 1 reminders are due, returns them unchanged.
-func collapseReminders(ctx context.Context, s *store.Store, due []store.Reminder) ([]store.Reminder, error) {
+func collapseReminders(due []store.Reminder) []store.Reminder {
 	if len(due) <= 1 {
-		return due, nil
+		return due
 	}
 	// Build a summary payload.
 	var items []string
@@ -235,21 +233,13 @@ func collapseReminders(ctx context.Context, s *store.Store, due []store.Reminder
 		"items": items,
 	})
 
-	// Mark originals as fired so they won't re-fire on next tick.
-	for _, r := range due {
-		if err := s.MarkReminder(ctx, r.ID, "fired"); err != nil {
-			// Log and continue — one failure shouldn't block the digest.
-			// The next tick will re-gather and re-attempt.
-			log.Printf("gather: mark reminder %d fired: %v", r.ID, err)
-		}
-	}
-
 	// Return a single synthetic digest reminder. ID=0 signals "digest" to
-	// the dispatcher (which skips MarkReminder for ID=0).
+	// the dispatcher, which completes the Collapsed originals on success.
 	return []store.Reminder{{
 		ID:        0,
 		FireTs:    earliest,
 		Payload:   string(digestPayload),
 		Status:    "pending",
-	}}, nil
+		Collapsed: due,
+	}}
 }

internal/store/reminders.go

@@ -19,6 +19,11 @@ type Reminder struct {
 	Payload   string       // raw json
 	Status    string       // pending | fired | cancelled
 	Cron      string       // cron expression, empty for one-shot
+
+	// Collapsed — set only on a synthetic digest reminder (ID=0): the original
+	// due reminders it stands in for. Not persisted. The dispatcher completes
+	// (marks fired / reschedules) each of these after the digest delivers.
+	Collapsed []Reminder
 }
 
 var (