From 35c6ff5a71e2cdf1336a4f767331b82cf14f0969 Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 13 Aug 2026 02:50:59 +0400 Subject: [PATCH] Make delivery and integration failures explicit Persist reminder presentations and retry state, atomically complete collapsed deliveries, fall back across away reaches, and block permanent failures visibly (V-715, V-678). Fail closed when enabled integrations lack credentials and keep remote arms explicitly dark (V-691). Give mavweb one sanitized, request-correlated error contract (V-689). Owner explicitly requested direct commits to master. --- .gitignore | 4 + JOURNAL.md | 188 +++++++ Makefile | 7 +- cmd/mavend/main.go | 30 +- cmd/mavend/ntfy_wiring_test.go | 35 ++ cmd/mavend/telegramintake.go | 2 +- cmd/mavend/tick.go | 136 ++++- cmd/mavend/tick_reminder_delivery_test.go | 170 +++++++ cmd/mavend/voicewire.go | 8 +- cmd/mavweb/ambient.go | 30 +- cmd/mavweb/ambient_test.go | 12 + cmd/mavweb/chat.go | 35 +- cmd/mavweb/ecosystem.go | 16 +- cmd/mavweb/events_test.go | 9 +- cmd/mavweb/facts.go | 30 +- cmd/mavweb/main.go | 24 +- cmd/mavweb/models.go | 39 +- cmd/mavweb/models_test.go | 21 + cmd/mavweb/notifications.go | 11 +- cmd/mavweb/pages.go | 39 +- cmd/mavweb/problems.go | 118 +++++ cmd/mavweb/problems_test.go | 135 +++++ cmd/mavweb/reminders.go | 20 +- cmd/mavweb/reminders.html | 2 +- cmd/mavweb/reminders_test.go | 13 + cmd/mavweb/routines.go | 34 +- cmd/mavweb/shell.go | 10 +- cmd/mavweb/tasks.go | 73 ++- cmd/mavweb/tasks_test.go | 18 + cmd/mavweb/tools.go | 37 +- cmd/mavweb/voiceproxy.go | 32 +- cmd/mavweb/webauthn.go | 32 +- deploy/README.md | 5 + deploy/cw2/serve.py | 19 +- deploy/cw2/test_serve.py | 24 + deploy/mavend.json | 11 +- deploy/telegram.env.example | 32 +- docker-compose.yml | 12 +- docs/caveats/CLAUDE.md | 9 +- docs/caveats/config.md | 11 - docs/caveats/storage.md | 8 - docs/caveats/transport.md | 12 - docs/deployment.md | 17 +- docs/offload.md | 7 +- docs/operations.md | 20 +- internal/config/config.go | 36 +- internal/config/config_test.go | 120 ++++- internal/config/deployconfig_test.go | 65 ++- internal/config/workstation.go | 91 +++- internal/delivery/channel.go | 18 +- internal/delivery/dispatcher.go | 189 ++++--- internal/delivery/dispatcher_test.go | 257 +++++++++- internal/delivery/durability_test.go | 4 +- internal/delivery/ntfysink/ntfysink.go | 52 +- internal/delivery/ntfysink/ntfysink_test.go | 77 ++- .../delivery/telegramsink/telegramsink.go | 49 +- .../telegramsink/telegramsink_test.go | 19 +- internal/ipc/api.go | 36 +- internal/ipc/maperr_test.go | 5 +- internal/ipc/storeapi.go | 21 +- internal/loop/gather.go | 65 ++- internal/loop/gather_test.go | 33 ++ internal/store/delivery.go | 33 +- internal/store/delivery_test.go | 8 +- internal/store/migrations.go | 23 + internal/store/reminders.go | 476 +++++++++++++++++- internal/store/reminders_delivery_test.go | 417 +++++++++++++++ 67 files changed, 3174 insertions(+), 477 deletions(-) create mode 100644 JOURNAL.md create mode 100644 cmd/mavend/ntfy_wiring_test.go create mode 100644 cmd/mavend/tick_reminder_delivery_test.go create mode 100644 cmd/mavweb/problems.go create mode 100644 cmd/mavweb/problems_test.go create mode 100644 deploy/cw2/test_serve.py delete mode 100644 docs/caveats/transport.md create mode 100644 internal/store/reminders_delivery_test.go diff --git a/.gitignore b/.gitignore index 30023b1..0033865 100644 --- a/.gitignore +++ b/.gitignore @@ -54,6 +54,10 @@ opencode.json # Test coverage output coverage.out +# Python service/test bytecode. +__pycache__/ +*.py[cod] + # Agent worktrees and local agent state. The workflow itself is tracked: the # hooks, the skills and the prose dictionary are how a session behaves, so they # get reviewed like code. Everything else under .claude/ is scratch. diff --git a/JOURNAL.md b/JOURNAL.md new file mode 100644 index 0000000..473fe3f --- /dev/null +++ b/JOURNAL.md @@ -0,0 +1,188 @@ +# Maven completion journal + +This journal tracks the autonomous completion goal started on 2026-08-13. It +is an operational index, not a substitute for living subsystem documentation, +dated evaluations, Vikunja tasks, or focused caveat entries. + +## 2026-08-13 — baseline and backlog reconstruction + +Goal: make Maven usable end to end with every current and planned feature +wired, tested, and polished. Completion requires clean automated gates and +successful sessions across local, degraded-ecosystem, and integrated modes. + +Initial observations: + +- `HEAD` is `2cf8b7e`, identical to both local and remote `master`, while the + checked-out branch is the stale `task/704-...` branch. +- The worktree already contained staged documentation/evaluation changes, + staged transcript deletions, an unstaged `deploy/mavwaked.service` change, + and untracked `deploy/asoundrc`. These are pre-existing work and are being + preserved and validated before any commit. +- The repository has no prior goal journal. Durable subsystem facts continue + to belong under `docs/`; unresolved limits continue to belong under + `docs/caveats/` with a task and revisit trigger. + +Work streams started: + +- Vikunja project 2: inventory every open task and recover acceptance criteria. +- Repository: compare feature plans, caveats, routing/ecosystem contracts, and + implementation. +- Verification: run build, race tests, simulator, analyzer gates, and inspect + skipped hardware/model evaluations separately. +- Runtime: exercise the web, IPC, voice, model, and sibling-service paths with + real local dependencies where available and explicit degraded-mode probes + otherwise. + +References: `docs/workflow.md`, `docs/qa.md`, `docs/ecosystem.md`, +`docs/routing.md`, `docs/caveats/CLAUDE.md`, and Vikunja Maven project 2. + +### Backlog correction + +The first Vikunja page was accidentally read without a `done: false` filter and +mixed closed history into the working set. Re-querying all pages strictly open +produced 127 records: 89 implementation-open, 20 shipped-but-QA-open, 12 +external/owner gates, and 6 duplicate or stale-open records. Closed tasks are +used only as commit provenance; they are not work to redo. V-704 was the only +open hanging task and was closed after its measured correction landed. + +### Model-aware baseline + +The ordinary `make test` passed but does not set `MAVEN_ONNX_LIB`, so model-aware +tests can self-skip. The explicit ONNX boundary gate exposed V-702/V-703: the +held-out `я рассказывал тебе про байкал?` was the sole miss at 28/29. A +three-neighbour class score fixed the semantic collision without adding a word +pattern or copying the held-out sentence. Boundary is now 29/29 and the adjacent +topic gate remains 43/43. Measurement: +`docs/evals/2026-08-13-personal-boundary-neighbourhood.md`. + +That narrow result was not accepted as the completion gate. A second agent +wrote a balanced 72-case RU/EN matrix across remembered speech, possession, +narrative, proper nouns, personal preambles, and advice/current questions. It +contains no production seeds and no Baikal paraphrase. The top-three candidate +scores only 61/72 (84.7%); top-two reaches 62/72, one-neighbour 56/72, and a +whole-class centroid 54/72. V-702 therefore remains open while a principled +classifier is developed against the independent matrix. The 29/29 measurement +describes the narrow regression set, not general boundary quality. + +### Live delivery incident + +The five-service compose stack was running, but a due reminder was being +re-phrased and retried through ntfy every tick. The sink returned HTTP 403 each +time. Only secret names were inspected: the configured ntfy and workstation +token variables were absent from the deployed environment file; no secret +values were read into this journal. The durable outbox records each failed +attempt, but the retry path has no backoff or alternate channel and spends the +resident model again before every failure. This is active V-651 behavior, with +the repeated-phrasing shape related to V-687. + +V-715 now owns the incident acceptance criteria. In the working tree, phrases, +collapsed-group identity, attempts, and next-attempt time are durable; definite +failure backs off from one minute to a capped hour; retries and restarts reuse +the exact phrase; and away delivery tries ntfy then Telegram, stopping at the +first success. The committed deployment explicitly disables the uncredentialed +ntfy block. Independent review added a real occurrence key for collapsed +bundles, suppresses crash-ambiguous attempts from automatic replay, classifies +HTTP 401/403 as permanent, blocks permanently unreachable reminders visibly, +and commits the successful outbox result plus every collapsed original in one +SQLite transaction. The store, delivery, IPC, loop, config, and mavweb race +suites pass. A live rebuild and one-time delivery of the existing backlog are +still required before V-715 is closed. + +### Explicit integration enablement + +V-691's deployment boundary was audited against every `${VAR}` reference. The +canonical `deploy/telegram.env.example` now names Telegram, ntfy, workstation +model, workstation STT, Home Assistant, ambient, CW2, and database-key inputs. +Enabled Telegram, ntfy, ambient, non-loopback workstation model, and +non-loopback workstation STT paths refuse missing credentials; each arm has an +explicit disabled state. The live config disables the currently uncredentialed +ntfy and workstation-model arms while retaining the separately credentialed +STT arm. CW2 also refuses a non-loopback bind without its token. + +Focused Go race suites, the Python CW2 startup contract, deploy-config drift +test, secret-expanded config validation (values not printed), and +`docker compose config --quiet` pass. The workpc is unreachable from this host, +so installing the updated CW2 script there remains an external deployment step; +the affected model arm is explicitly dark rather than ambiguously half-live. + +### Traceable web failures + +V-689 gives every mavweb response a server-generated request ID and routes +every handler failure through one sanitized problem envelope. Stable error +codes and the request ID reach the browser; the wrapped internal error reaches +only the server log beside the same ID. Degraded inline panels use stable public +text rather than backend paths or tokens, and direct ecosystem reads propagate +the web request ID as their correlation ID. An AST guard prevents new production +handlers from bypassing the contract with `http.Error`. The full mavweb race +suite passes, including disclosure, untrusted-ID, log-join, and propagation +tests. + +### Bounded external responses + +Three audit defects were repaired and committed directly to `master`: + +- V-608 (`d7e8804`): llama completion responses are capped at 1 MiB, including + the LAN workstation seam. +- V-675 (`459fe7a`): remote STT requires nonblank text and an explicit finite + confidence in `[0,1]`, caps JSON at 64 KiB, and falls back to mavsttd on a + malformed HTTP 200. +- V-676 (`7d0250a`): Open-Meteo geocoding and forecasts are bounded, required + fields are nullable/validated, and coordinates/weather values are range + checked so `{}` cannot become plausible zero-degree weather. + +Each focused race suite passed and each task was closed only after the commit. + +### Transport shutdown + +V-679 (`de61b75`) adds the listener's `done` channel to TCP `Accept`. A +concurrent-close test holds a silent peer in handshake and proves an in-flight +accept returns `net.ErrClosed`; the race test passed twenty consecutive runs. + +V-688 (`80b6068`, caveat retirement `a0e6643`) bounds the browser push-to-talk +body at ten minutes of mono PCM and configures header, idle, and read limits on +the web server. The unused `/ws` handler was removed instead of retaining a +second unauthenticated streaming transport with no browser caller. Focused race +tests prove the exact-size request succeeds and an oversized request returns +HTTP 413. + +### Conversation continuity + +V-542 (`da9114b`) repairs the five-turn monitor conversation without changing +single-turn intent classification. Exact user utterances are now persisted +separately from normalized intent slots and retained in chronological order. +An anaphoric query with live transcript context reaches the chat path, while +non-anaphoric sources are unchanged and acts stay fail-closed. An explicit +conversation opener extends the session lifetime through later fact/query/chat +routes without suppressing the grounded fact write. + +The deterministic scenario now names the monitor in all four contextual +replies, proves that the original raw turn reached `PhraseChat` four times, +stores the fact once, and produces zero unsolicited sends across five turns and +one tick. Focused race tests passed for `cmd/mavend`, `internal/dialogue`, +`internal/router`, and `internal/lexicon`. Measurement: +`docs/evals/2026-08-13-conversation-continuity.md`. + +### Personal-data boundary + +V-702 replaces the narrow nearest-neighbour privacy gate with a frozen, +class-balanced logistic head over multilingual-e5-small. It introduces no +lexical exception and leaves the decision threshold at 0.5. Historical +regressions score 29/29 and the balanced 72-case RU/EN fixture scores 72/72. + +The first 24-case challenge found one private-configuration miss. That result +was treated as model-selection data rather than advertised as independent +proof. Shrinkage LDA and an LDA/logistic ensemble repaired it but regressed the +72-case gate, so both were rejected. Increasing the logistic L2 coefficient +from 0.0001 to 0.0003 repairs the miss while improving four-fold corpus +cross-validation from 97/104 to 99/104 and whole-shape holdout from 91/104 to +92/104. A fresh 24-case challenge written only after that head was frozen scores +24/24 at minimum signed probability margin +0.1718. The original challenge is +also 24/24 but its +0.0001 edge remains documented as a regression, not fresh +evidence. Full measurement: +`docs/evals/2026-08-13-personal-boundary-linear-head.md`. + +During the audit, running multiple ONNX-backed tests in one `go test` process +showed that only the first initializes; later tests self-skip because the +runtime is process-global. All V-702 figures were therefore rerun in separate +processes. V-716 tracks fixing that harness gap rather than hiding it in this +feature. diff --git a/Makefile b/Makefile index 90ed983..03f9551 100644 --- a/Makefile +++ b/Makefile @@ -16,7 +16,7 @@ PIPER_BIN := $(shell pwd)/deps/piper/piper PIPER_MODEL := $(shell pwd)/models/tts/ru_RU-irina-medium.onnx PIPER_ESPEAK := $(shell pwd)/deps/piper/espeak-ng-data -.PHONY: t audit simulate stt-fixtures test-stt-golden all build build-stt build-tts build-daemon build-client build-waked build-web build-poll build-caldav clean test fmt-check vet run-stt run-tts run-web download-embedder deps-go deps-sentinel deps-vuln vuln deps-lint lint deadcode analyze tidy eval-router eval-reach eval-recall eval-phrasing eval-models build-gpud +.PHONY: t audit simulate stt-fixtures test-stt-golden test-cw2-config all build build-stt build-tts build-daemon build-client build-waked build-web build-poll build-caldav clean test fmt-check vet run-stt run-tts run-web download-embedder deps-go deps-sentinel deps-vuln vuln deps-lint lint deadcode analyze tidy eval-router eval-reach eval-recall eval-phrasing eval-models build-gpud all: build @@ -188,7 +188,10 @@ simulate: CGO_CFLAGS="$(CGO_CFLAGS)" CGO_LDFLAGS="$(CGO_LDFLAGS)" LD_LIBRARY_PATH="$(shell pwd)/deps/lib" \ $(GO) test -v -count=1 -run TestSimulator ./cmd/mavend/ -test: fmt-check vet +test-cw2-config: + python3 -m unittest discover -s deploy/cw2 -p 'test_*.py' + +test: fmt-check vet test-cw2-config CGO_CFLAGS="$(CGO_CFLAGS)" CGO_LDFLAGS="$(CGO_LDFLAGS)" LD_LIBRARY_PATH="$(shell pwd)/deps/lib" \ $(GO) test -race -coverprofile=coverage.out ./internal/... ./cmd/... diff --git a/cmd/mavend/main.go b/cmd/mavend/main.go index 75c4f76..9c6005a 100644 --- a/cmd/mavend/main.go +++ b/cmd/mavend/main.go @@ -559,7 +559,7 @@ func run(args []string) error { func personaFacts(cfg *config.Config) persona.Facts { f := persona.Facts{ // Telegram lives outside the voice block, so it counts either way. - Telegram: cfg.Telegram != nil && cfg.Telegram.BotToken != "" && cfg.Telegram.ChatID != "", + Telegram: cfg.Telegram != nil && !cfg.Telegram.Disabled && cfg.Telegram.BotToken != "" && cfg.Telegram.ChatID != "", } if cfg.Voice == nil { return f @@ -678,16 +678,12 @@ func wireGatherer(st *store.Store, cfg *config.Config, rules []loop.Rule) *loop. // reconciled to "unknown" here, before the tick loop resumes sending, so // nothing auto-resends into that ambiguity. func wireDispatcher(st *store.Store, cfg *config.Config, voiceW *voiceWiring) (*delivery.Dispatcher, error) { - var ntfy delivery.Sink - if cfg.Ntfy != nil { - s, err := ntfysink.New(*cfg.Ntfy) - if err != nil { - return nil, fmt.Errorf("wire ntfy sink: %w", err) - } - ntfy = s + ntfy, err := wireNtfySink(cfg.Ntfy) + if err != nil { + return nil, err } var telegram delivery.Sink - if cfg.Telegram != nil { + if cfg.Telegram != nil && !cfg.Telegram.Disabled { s, err := telegramsink.New(*cfg.Telegram) if err != nil { return nil, fmt.Errorf("wire telegram sink: %w", err) @@ -712,6 +708,22 @@ func wireDispatcher(st *store.Store, cfg *config.Config, voiceW *voiceWiring) (* }), nil } +// wireNtfySink keeps an optional reach optional without ever turning a missing +// secret into anonymous publishing. A block is live unless it says disabled; +// therefore an expanded-empty token in a live block fails startup instead of +// spending days in a permanent 403 retry loop. Disabled is an explicit +// operator choice and lets another away reach take over. +func wireNtfySink(cfg *ntfysink.Config) (delivery.Sink, error) { + if cfg == nil || cfg.Disabled { + return nil, nil + } + sink, err := ntfysink.New(*cfg) + if err != nil { + return nil, fmt.Errorf("wire ntfy sink: %w", err) + } + return sink, nil +} + // wireTickLoop reads the loop's three intervals and its schedules out of the // config, so the two boot paths cannot disagree about them. func wireTickLoop(st *store.Store, gatherer *loop.Gatherer, dispatcher *delivery.Dispatcher, phr phraser.Phraser, rules []loop.Rule, cfg *config.Config) *tickLoop { diff --git a/cmd/mavend/ntfy_wiring_test.go b/cmd/mavend/ntfy_wiring_test.go new file mode 100644 index 0000000..f01caed --- /dev/null +++ b/cmd/mavend/ntfy_wiring_test.go @@ -0,0 +1,35 @@ +package main + +import ( + "testing" + + "github.com/kami/maven/internal/delivery/ntfysink" +) + +func TestWireNtfySinkRejectsMissingCredentialWhenEnabled(t *testing.T) { + _, err := wireNtfySink(&ntfysink.Config{ + BaseURL: "https://ntfy.example", Topic: "maven", + }) + if err == nil { + t.Fatal("expanded-empty credential did not fail an enabled reach") + } +} + +func TestWireNtfySinkLeavesExplicitlyDisabledReachDark(t *testing.T) { + sink, err := wireNtfySink(&ntfysink.Config{ + Disabled: true, BaseURL: "https://ntfy.example", Topic: "maven", + }) + if err != nil { + t.Fatalf("wireNtfySink: %v", err) + } + if sink != nil { + t.Fatal("disabled reach built a live sink") + } +} + +func TestWireNtfySinkRejectsMalformedEnabledConfig(t *testing.T) { + _, err := wireNtfySink(&ntfysink.Config{Token: "token", Topic: "maven"}) + if err == nil { + t.Fatal("malformed enabled config did not fail wiring") + } +} diff --git a/cmd/mavend/telegramintake.go b/cmd/mavend/telegramintake.go index ed63bba..99f4ff9 100644 --- a/cmd/mavend/telegramintake.go +++ b/cmd/mavend/telegramintake.go @@ -25,7 +25,7 @@ import ( // already failed the boot in wireDispatcher for the same config, so a second // hard failure would only lose that message. func wireTelegramIntake(ctx context.Context, wg *sync.WaitGroup, api ipc.CoreAPI, cfg *config.Config) { - if cfg == nil || cfg.Telegram == nil || !cfg.Telegram.Intake || api == nil { + if cfg == nil || cfg.Telegram == nil || cfg.Telegram.Disabled || !cfg.Telegram.Intake || api == nil { return } sink, err := telegramsink.New(*cfg.Telegram) diff --git a/cmd/mavend/tick.go b/cmd/mavend/tick.go index 76b0911..8b61c97 100644 --- a/cmd/mavend/tick.go +++ b/cmd/mavend/tick.go @@ -10,11 +10,13 @@ package main import ( "context" + "crypto/sha256" "errors" "fmt" "log" "os" "path/filepath" + "sort" "sync" "time" @@ -226,18 +228,11 @@ func (t *tickLoop) tick(ctx context.Context, now time.Time) { // detectPatterns below for how idempotence and dismissal are respected. t.detectPatterns(ctx, now, state) - // reminders: gate-bypassing class. fired once, marked after a successful - // delivery. a failed send leaves the reminder pending — the next tick - // re-gathers and re-attempts. + // reminders: gate-bypassing class. The presentation and retry clock live on + // the reminder occurrence, so a transport outage neither spends the model + // every tick nor changes what the reminder says after a restart. for _, d := range loop.RemindDecisions(state, due) { - pr, err := t.phraser.PhraseReminder(ctx, d) - if err != nil { - log.Printf("tick: phrase reminder %d: %v", d.Reminder.ID, err) - continue - } - if _, err := t.dispatcher.DispatchReminder(ctx, pr, now); err != nil { - log.Printf("tick: dispatch reminder %d: %v", d.Reminder.ID, err) - } + t.deliverReminder(ctx, d, now) } // sev4-away repeats: re-send un-acked telegram nudges per repeatInterval. @@ -263,6 +258,125 @@ func (t *tickLoop) tick(ctx context.Context, now time.Time) { } } +// deliverReminder advances one due reminder (or collapsed bundle) through the +// durable delivery state. A phrase is cached before the first external send; +// every definite failure advances the persisted bounded backoff. +func (t *tickLoop) deliverReminder(ctx context.Context, d loop.ReminderDecision, now time.Time) { + originals := reminderOriginals(d.Reminder) + pr, cached := cachedReminderPhrase(d, originals) + if !cached { + var err error + pr, err = t.phraser.PhraseReminder(ctx, d) + if err == nil && pr.Body == "" { + err = errors.New("phraser returned an empty reminder body") + } + if err != nil { + log.Printf("tick: phrase reminder %d: %v", d.Reminder.ID, err) + t.scheduleReminderRetry(ctx, originals, now) + return + } + if pr.Mood == "" { + pr.Mood = "neutral" + } + group := reminderDeliveryGroup(originals) + if err := t.store.CacheReminderPhrase( + ctx, originals, group, pr.Body, pr.Summary, pr.Mood, + ); err != nil { + // A cancellation or another completion can win while phrasing. Do + // not send a presentation that no longer owns every original. + log.Printf("tick: cache reminder %d phrase: %v", d.Reminder.ID, err) + return + } + // The store now owns the phrase, but this tick's value predates that + // write. Stamp the exact persisted occurrence identity onto the value + // handed to the dispatcher so its outbox row can suppress an ambiguous + // crash for both a real reminder and a synthetic collapsed bundle. + for i := range originals { + originals[i].DeliveryGroup = group + originals[i].PhraseBody = pr.Body + originals[i].PhraseSummary = pr.Summary + originals[i].PhraseMood = pr.Mood + } + if d.Reminder.ID == 0 { + d.Reminder.Collapsed = originals + } else { + d.Reminder = originals[0] + } + } + // A phraser is not allowed to substitute the reminder decision. In + // particular, the durable group stamped above must reach the outbox. + pr.Decision = d + + if _, err := t.dispatcher.DispatchReminder(ctx, pr, now); err != nil { + log.Printf("tick: dispatch reminder %d: %v", d.Reminder.ID, err) + t.scheduleReminderRetry(ctx, originals, now) + } +} + +func (t *tickLoop) scheduleReminderRetry(ctx context.Context, originals []store.Reminder, now time.Time) { + if err := t.store.ScheduleReminderRetry(ctx, originals, now); err != nil { + log.Printf("tick: schedule reminder retry: %v", err) + } +} + +// reminderOriginals converts the synthetic ID=0 bundle back to real store +// rows. Keeping this in one helper makes it impossible to accidentally persist +// retry state against reminder zero. +func reminderOriginals(r store.Reminder) []store.Reminder { + if r.ID == 0 { + return append([]store.Reminder(nil), r.Collapsed...) + } + return []store.Reminder{r} +} + +// cachedReminderPhrase reconstructs a PhrasedReminder only when every original +// agrees on one persisted group and presentation. That agreement is what lets +// a collapsed bundle survive a restart without being re-phrased. +func cachedReminderPhrase(d loop.ReminderDecision, originals []store.Reminder) (delivery.PhrasedReminder, bool) { + if len(originals) == 0 || !originals[0].HasDeliveryPhrase() { + return delivery.PhrasedReminder{}, false + } + first := originals[0] + for _, r := range originals[1:] { + if !r.HasDeliveryPhrase() || + r.DeliveryGroup != first.DeliveryGroup || + r.PhraseBody != first.PhraseBody || + r.PhraseSummary != first.PhraseSummary || + r.PhraseMood != first.PhraseMood { + return delivery.PhrasedReminder{}, false + } + } + mood := first.PhraseMood + if mood == "" { + mood = "neutral" + } + return delivery.PhrasedReminder{ + Decision: d, + Body: first.PhraseBody, + Summary: first.PhraseSummary, + Mood: mood, + }, true +} + +// reminderDeliveryGroup deterministically names one occurrence or collapsed +// set. The next-fire instant is part of the identity so a recurring reminder's +// later occurrence can never inherit the previous occurrence's phrase. +func reminderDeliveryGroup(originals []store.Reminder) string { + ordered := append([]store.Reminder(nil), originals...) + sort.Slice(ordered, func(i, j int) bool { + if ordered[i].ID == ordered[j].ID { + return ordered[i].NextFireTs.Before(ordered[j].NextFireTs) + } + return ordered[i].ID < ordered[j].ID + }) + h := sha256.New() + for _, r := range ordered { + _, _ = fmt.Fprintf(h, "%d:%d;", r.ID, r.NextFireTs.UnixMilli()) + } + sum := h.Sum(nil) + return fmt.Sprintf("reminder:%x", sum[:12]) +} + // savePresence writes back the bucket GatherState just resolved. // // It lives here and not in GatherState because that method holds a read-only diff --git a/cmd/mavend/tick_reminder_delivery_test.go b/cmd/mavend/tick_reminder_delivery_test.go new file mode 100644 index 0000000..e87edfd --- /dev/null +++ b/cmd/mavend/tick_reminder_delivery_test.go @@ -0,0 +1,170 @@ +package main + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/kami/maven/internal/delivery" + "github.com/kami/maven/internal/loop" + "github.com/kami/maven/internal/phraser" + "github.com/kami/maven/internal/store" +) + +type reminderCountingPhraser struct { + phraser.Phraser + calls int + body string + summary string + mood string +} + +func (p *reminderCountingPhraser) PhraseReminder(_ context.Context, d loop.ReminderDecision) (delivery.PhrasedReminder, error) { + p.calls++ + return delivery.PhrasedReminder{ + Decision: d, + Body: p.body, + Summary: p.summary, + Mood: p.mood, + }, nil +} + +type reminderFailSink struct { + sends int +} + +func (s *reminderFailSink) Send(_ context.Context, _ delivery.Sendable) error { + s.sends++ + return errors.New("transport unavailable") +} + +func newReminderDeliveryLoop(t *testing.T, st *store.Store, sink delivery.Sink, p phraser.Phraser) *tickLoop { + t.Helper() + rules := loop.DefaultRules() + return newTickLoop( + st, + loop.NewGatherer(st, rules), + delivery.NewDispatcher(delivery.Config{ + Voice: sink, Ntfy: sink, Telegram: sink, + Nudges: st, Reminders: st, Outbox: st, + }), + p, + rules, + time.Second, + 5*time.Minute, + 0, + nil, nil, nil, nil, + ) +} + +func TestTickReminderRetryUsesPersistedPhraseAfterRestart(t *testing.T) { + st := newTestStore(t) + ctx := context.Background() + now := refNow() + if _, err := st.CreateReminder(ctx, now.Add(-time.Minute), `{"text":"позвонить маме"}`, ""); err != nil { + t.Fatal(err) + } + + fail := &reminderFailSink{} + firstPhraser := &reminderCountingPhraser{ + Phraser: phraser.NewStub(), body: "Не забудь позвонить маме.", + summary: "Позвонить маме", mood: "warm", + } + tl := newReminderDeliveryLoop(t, st, fail, firstPhraser) + tl.tick(ctx, now) + if firstPhraser.calls != 1 { + t.Fatalf("first tick phrased %d times, want 1", firstPhraser.calls) + } + rows, err := st.ListReminders(ctx, 1) + if err != nil || len(rows) != 1 { + t.Fatalf("list = %d, err=%v", len(rows), err) + } + if !rows[0].HasDeliveryPhrase() || rows[0].DeliveryAttempts != 1 { + t.Fatalf("failed delivery state was not persisted: %+v", rows[0]) + } + if want := now.Add(store.ReminderRetryBase); !rows[0].NextAttemptTs.Equal(want) { + t.Fatalf("next attempt = %s, want %s", rows[0].NextAttemptTs, want) + } + + // A normal tick inside the wait does no transport work and no model work. + sendsAfterFirst := fail.sends + tl.tick(ctx, now.Add(30*time.Second)) + if firstPhraser.calls != 1 || fail.sends != sendsAfterFirst { + t.Fatalf("retry wait did work: phrase calls=%d, sends=%d (was %d)", firstPhraser.calls, fail.sends, sendsAfterFirst) + } + + // Constructing a new loop is the daemon-restart boundary. Its phraser would + // say something different if called; the stored phrase must win instead. + success := &fakeSink{} + afterRestart := &reminderCountingPhraser{ + Phraser: phraser.NewStub(), body: "WRONG NEW PHRASE", summary: "WRONG", mood: "neutral", + } + restarted := newReminderDeliveryLoop(t, st, success, afterRestart) + restarted.tick(ctx, now.Add(store.ReminderRetryBase)) + if afterRestart.calls != 0 { + t.Fatalf("restart re-phrased the reminder %d times", afterRestart.calls) + } + if len(success.sends) != 1 { + t.Fatalf("retry sends = %d, want 1", len(success.sends)) + } + if got := success.sends[0].Body; got != "Позвонить маме" { + t.Fatalf("away retry body = %q, want persisted summary", got) + } + rows, err = st.ListReminders(ctx, 1) + if err != nil || rows[0].Status != store.ReminderFired { + t.Fatalf("successful retry did not fire reminder: rows=%+v err=%v", rows, err) + } +} + +func TestTickCollapsedReminderRetriesOnePhraseAndCompletesOriginals(t *testing.T) { + st := newTestStore(t) + ctx := context.Background() + now := refNow() + for _, text := range []string{"полить цветы", "записаться к врачу"} { + if _, err := st.CreateReminder(ctx, now.Add(-time.Minute), text, ""); err != nil { + t.Fatal(err) + } + } + + fail := &reminderFailSink{} + firstPhraser := &reminderCountingPhraser{ + Phraser: phraser.NewStub(), body: "У тебя два напоминания.", + summary: "Два напоминания", mood: "neutral", + } + newReminderDeliveryLoop(t, st, fail, firstPhraser).tick(ctx, now) + if firstPhraser.calls != 1 { + t.Fatalf("collapsed bundle phrased %d times, want 1", firstPhraser.calls) + } + rows, err := st.ListReminders(ctx, 10) + if err != nil || len(rows) != 2 { + t.Fatalf("list = %d, err=%v", len(rows), err) + } + for _, r := range rows { + if r.DeliveryGroup == "" || r.DeliveryGroup != rows[0].DeliveryGroup || + r.PhraseBody != "У тебя два напоминания." || r.DeliveryAttempts != 1 { + t.Fatalf("collapsed original lost shared state: %+v", r) + } + } + + success := &fakeSink{} + afterRestart := &reminderCountingPhraser{ + Phraser: phraser.NewStub(), body: "WRONG", summary: "WRONG", mood: "neutral", + } + newReminderDeliveryLoop(t, st, success, afterRestart).tick(ctx, now.Add(store.ReminderRetryBase)) + if afterRestart.calls != 0 { + t.Fatalf("collapsed retry re-phrased %d times", afterRestart.calls) + } + if len(success.sends) != 1 || success.sends[0].ReminderID != 0 { + t.Fatalf("collapsed retry sends = %+v, want one synthetic delivery", success.sends) + } + rows, err = st.ListReminders(ctx, 10) + if err != nil { + t.Fatal(err) + } + for _, r := range rows { + if r.Status != store.ReminderFired { + t.Fatalf("collapsed original %d status = %q, want fired", r.ID, r.Status) + } + } +} diff --git a/cmd/mavend/voicewire.go b/cmd/mavend/voicewire.go index f3e4aa8..6763776 100644 --- a/cmd/mavend/voicewire.go +++ b/cmd/mavend/voicewire.go @@ -377,19 +377,19 @@ func wireVoice(cfg *config.Config, coreAPI ipc.CoreAPI, phr phraser.Phraser, mem // degraded mode, so the seam is nil and the cascade routes with the classifier. func modelSeam(cfg *config.Config, resident *llm.Client) (router.Completer, *llm.Pair) { if resident == nil { - if cfg.Workstation != nil { + if cfg.Workstation != nil && !cfg.Workstation.ModelDisabled { log.Printf("voice: a workstation is configured but there is no resident model to floor it with — ignoring the block") } return nil, nil } - if cfg.Workstation == nil { + if cfg.Workstation == nil || cfg.Workstation.ModelDisabled { return resident, nil } ws := cfg.Workstation remote := llm.New(ws.URL, time.Duration(ws.Timeout)) remote.SetToken(ws.Token) if ws.Token == "" { - log.Printf("voice: no workstation.token — mavgpud refuses an unauthenticated request, so this reads as a card that is always busy") + log.Printf("voice: unauthenticated workstation model endpoint is loopback-only") } pair := llm.NewPair( remote, @@ -434,7 +434,7 @@ func sttSeam(cfg *config.Config, floor stt.Transcriber) (stt.Transcriber, *stt.P ) pair.Start(context.Background()) if s.Token == "" { - log.Print("voice: the workstation transcriber has no token, so anything on the LAN can post audio to it") + log.Print("voice: unauthenticated workstation transcriber endpoint is loopback-only") } log.Printf("voice: workstation transcriber at %s, probed every %s, mavsttd as the floor", s.URL, time.Duration(s.Probe)) diff --git a/cmd/mavweb/ambient.go b/cmd/mavweb/ambient.go index 1d98b48..5d59cce 100644 --- a/cmd/mavweb/ambient.go +++ b/cmd/mavweb/ambient.go @@ -4,6 +4,7 @@ import ( "crypto/subtle" "encoding/json" "errors" + "fmt" "io" "log" "net/http" @@ -51,34 +52,41 @@ type ambientResp struct { // never registered, so it is treated as a hard failure here too. func handleAmbient(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, token string) { if r.Method != http.MethodPost { - http.Error(w, "POST only", http.StatusMethodNotAllowed) + writeProblem(w, r, http.StatusMethodNotAllowed, problemMethodNotAllowed, + "POST only", nil) return } if token == "" { - http.Error(w, "ambient ingest disabled (no -ambient-token)", http.StatusServiceUnavailable) + writeProblem(w, r, http.StatusServiceUnavailable, problemIntegrationOff, + "ambient ingest disabled (no -ambient-token)", nil) return } if !ambientAuthorized(r, token) { - http.Error(w, "unauthorized", http.StatusUnauthorized) + writeProblem(w, r, http.StatusUnauthorized, problemUnauthorized, + "unauthorized", nil) return } if core == nil { - http.Error(w, "ambient ingest disabled (no -core)", http.StatusServiceUnavailable) + writeProblem(w, r, http.StatusServiceUnavailable, problemCoreUnavailable, + "ambient ingest disabled (no -core)", nil) return } var n calendar.Notification body, err := io.ReadAll(io.LimitReader(r.Body, ambientMaxBody)) if err != nil { - http.Error(w, "read failed", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "read failed", fmt.Errorf("read ambient request: %w", err)) return } if err := json.Unmarshal(body, &n); err != nil { - http.Error(w, "bad json", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "bad json", fmt.Errorf("decode ambient request: %w", err)) return } if n.Posted.IsZero() { - writeAmbient(w, http.StatusBadRequest, ambientResp{Reason: "posted_at is required"}) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "posted_at is required", nil) return } @@ -99,8 +107,8 @@ func handleAmbient(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, tok writeAmbient(w, http.StatusOK, ambientResp{Stored: false, Key: key, Reason: "unchanged"}) return } else if err != nil && !errors.Is(err, ipc.ErrNoFact) { - log.Printf("ambient: read %s: %v", key, err) - http.Error(w, "read failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "read failed", fmt.Errorf("read ambient fact %q: %w", key, err)) return } @@ -116,8 +124,8 @@ func handleAmbient(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, tok Source: calendar.SourceAmbient, Confidence: calendar.AmbientConfidence, }); err != nil { - log.Printf("ambient: write %s: %v", key, err) - http.Error(w, "write failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreWriteFailed, + "write failed", fmt.Errorf("write ambient fact %q: %w", key, err)) return } log.Printf("ambient: %s=%s (%s, pkg=%s)", key, val, calendar.SourceAmbient, n.Package) diff --git a/cmd/mavweb/ambient_test.go b/cmd/mavweb/ambient_test.go index f9ed037..87015c1 100644 --- a/cmd/mavweb/ambient_test.go +++ b/cmd/mavweb/ambient_test.go @@ -16,6 +16,18 @@ import ( const ambientTestToken = "s3cret" +func TestValidateAmbientConfig(t *testing.T) { + if err := validateAmbientConfig(false, ""); err != nil { + t.Fatalf("explicitly disabled ambient config: %v", err) + } + if err := validateAmbientConfig(true, ""); err == nil { + t.Fatal("enabled ambient ingest accepted an empty token") + } + if err := validateAmbientConfig(true, ambientTestToken); err != nil { + t.Fatalf("enabled authenticated ambient config: %v", err) + } +} + // ambientCore adds provenance-scoped reads to fakeCore, which the dedupe path // needs. type ambientCore struct { diff --git a/cmd/mavweb/chat.go b/cmd/mavweb/chat.go index 6091f5a..8657f8d 100644 --- a/cmd/mavweb/chat.go +++ b/cmd/mavweb/chat.go @@ -3,7 +3,7 @@ package main import ( _ "embed" "errors" - "log" + "fmt" "net/http" "net/url" "strconv" @@ -47,7 +47,7 @@ var correctionTargets = []router.Intent{ // handleChatPage renders the chat conversation page. func handleChatPage(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { - if !requireCore(w, core, "chat") { + if !requireCore(w, r, core, "chat") { return } msgs := []chatMsg{} @@ -83,13 +83,14 @@ func handleChatPage(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { // denies, which is the point of that flag. func handleChatAPI(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, session *webauthn.PasskeySession, requireStepUp bool) { if r.Method != http.MethodPost { - http.Error(w, "POST only", http.StatusMethodNotAllowed) + writeProblem(w, r, http.StatusMethodNotAllowed, problemMethodNotAllowed, + "POST only", nil) return } - if !requireCore(w, core, "chat") { + if !requireCore(w, r, core, "chat") { return } - if !stepUpGate(w, session, requireStepUp) { + if !stepUpGate(w, r, session, requireStepUp) { return } text := strings.TrimSpace(r.FormValue("text")) @@ -105,8 +106,8 @@ func handleChatAPI(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, ses // which is right for a single-owner box. reply, err := core.Chat(r.Context(), "web", text) if err != nil { - log.Printf("chat api: %v", err) - http.Redirect(w, r, "/chat", http.StatusSeeOther) + writeProblem(w, r, http.StatusBadGateway, problemCoreChangeFailed, + "chat failed", fmt.Errorf("run web chat turn: %w", err)) return } // The claiming query source rides back on the redirect so the page can show @@ -138,36 +139,40 @@ func handleChatAPI(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, ses // id could otherwise mislabel turns he never corrected. func handleCorrectAPI(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, session *webauthn.PasskeySession, requireStepUp bool) { if r.Method != http.MethodPost { - http.Error(w, "POST only", http.StatusMethodNotAllowed) + writeProblem(w, r, http.StatusMethodNotAllowed, problemMethodNotAllowed, + "POST only", nil) return } - if !requireCore(w, core, "correct") { + if !requireCore(w, r, core, "correct") { return } - if !stepUpGate(w, session, requireStepUp) { + if !stepUpGate(w, r, session, requireStepUp) { return } id, err := strconv.ParseInt(strings.TrimSpace(r.FormValue("trace_id")), 10, 64) if err != nil || id <= 0 { - http.Error(w, "trace_id required", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "trace_id required", err) return } shouldBe := strings.TrimSpace(r.FormValue("should_be")) // Only one of the seven, or nothing. Free text here would put an unroutable // label in the one table V-632 fits prototypes from. if shouldBe != "" && !isCorrectionTarget(shouldBe) { - http.Error(w, "should_be must be one of the seven intents", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "should_be must be one of the seven intents", nil) return } if err := core.CorrectTurn(r.Context(), id, shouldBe); err != nil { - log.Printf("correct turn %d: %v", id, err) // A turn past the retention bound is gone, and saying so is different // from saying the write broke. if errors.Is(err, ipc.ErrNoSuchTrace) { - http.Error(w, "that turn is no longer stored", http.StatusNotFound) + writeProblem(w, r, http.StatusNotFound, problemResourceNotFound, + "that turn is no longer stored", fmt.Errorf("correct turn %d: %w", id, err)) return } - http.Error(w, "correction failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreChangeFailed, + "correction failed", fmt.Errorf("correct turn %d: %w", id, err)) return } stamp := shouldBe diff --git a/cmd/mavweb/ecosystem.go b/cmd/mavweb/ecosystem.go index 1bb1439..e867330 100644 --- a/cmd/mavweb/ecosystem.go +++ b/cmd/mavweb/ecosystem.go @@ -33,17 +33,29 @@ func getEco(ctx context.Context, base, path string, out any) string { } req, err := http.NewRequestWithContext(ctx, http.MethodGet, base+path, nil) if err != nil { - return err.Error() + log.Printf("ecosystem panel request_id=%s build %q: %v", requestIDFromContext(ctx), path, err) + return "invalid endpoint" + } + req.Header.Set("Accept", "application/json") + req.Header.Set("X-Requested-By", "mavweb") + // These are direct browser-surface reads rather than an action initiated in + // mavend, so the HTTP request ID is the natural correlation root. Calls that + // pass through core mint their action correlation inside mavend instead. + if id := requestIDFromContext(ctx); id != "" { + req.Header.Set("X-Correlation-ID", id) } resp, err := ecoClient.Do(req) if err != nil { + log.Printf("ecosystem panel request_id=%s GET %s: %v", requestIDFromContext(ctx), path, err) return "unreachable" } defer resp.Body.Close() if resp.StatusCode != 200 { + log.Printf("ecosystem panel request_id=%s GET %s: HTTP %d", requestIDFromContext(ctx), path, resp.StatusCode) return fmt.Sprintf("http %d", resp.StatusCode) } if err := json.NewDecoder(resp.Body).Decode(out); err != nil { + log.Printf("ecosystem panel request_id=%s decode %s: %v", requestIDFromContext(ctx), path, err) return "bad json" } return "" @@ -111,7 +123,7 @@ func handleEcosystem(w http.ResponseWriter, r *http.Request, urls ecoURLs, core if core == nil { d.Calls.Err = "not configured" } else if rows, err := core.RecentEcosystemTraces(ctx, 50); err != nil { - log.Printf("ecosystem traces: %v", err) + log.Printf("ecosystem traces request_id=%s: %v", requestIDFromContext(ctx), err) d.Calls.Err = "core read failed" } else { d.Calls.Rows = rows diff --git a/cmd/mavweb/events_test.go b/cmd/mavweb/events_test.go index 0e280b2..3a40e4e 100644 --- a/cmd/mavweb/events_test.go +++ b/cmd/mavweb/events_test.go @@ -75,8 +75,13 @@ func TestEventsPageReportsAReadFailure(t *testing.T) { t.Fatalf("status = %d, want 200 with the error rendered", w.Code) } body := w.Body.String() - if !strings.Contains(body, "journal unavailable") || !strings.Contains(body, "core is down") { - t.Errorf("page did not report the read failure: %s", body) + if !strings.Contains(body, "intake journal unavailable") || + !strings.Contains(body, string(problemCoreReadFailed)) || + !strings.Contains(body, "request ") { + t.Errorf("page did not report a traceable, sanitized read failure: %s", body) + } + if strings.Contains(body, "core is down") { + t.Errorf("page disclosed the internal read error: %s", body) } if strings.Contains(body, "nothing has arrived yet") { t.Error("a failed read rendered as an empty journal") diff --git a/cmd/mavweb/facts.go b/cmd/mavweb/facts.go index 73a847a..525edef 100644 --- a/cmd/mavweb/facts.go +++ b/cmd/mavweb/facts.go @@ -3,6 +3,7 @@ package main import ( "encoding/json" "errors" + "fmt" "log" "net/http" "strings" @@ -32,16 +33,18 @@ var presenceSignals = map[string]string{ // is a marker. Only allowlisted keys are accepted (see presenceSignals). func handleSignal(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { if r.Method != http.MethodPost { - http.Error(w, "POST only", http.StatusMethodNotAllowed) + writeProblem(w, r, http.StatusMethodNotAllowed, problemMethodNotAllowed, + "POST only", nil) return } - if !requireCore(w, core, "presence ingest") { + if !requireCore(w, r, core, "presence ingest") { return } key := r.URL.Query().Get("key") source, ok := presenceSignals[key] if !ok { - http.Error(w, "unknown signal key", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "unknown signal key", nil) return } // kind=env: an observation about the device/surface, NOT a self-fact — a @@ -56,8 +59,8 @@ func handleSignal(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { Source: source, Confidence: 1.0, }); err != nil { - log.Printf("signal %s: %v", key, err) - http.Error(w, "write failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreWriteFailed, + "write failed", fmt.Errorf("write presence signal %q: %w", key, err)) return } w.WriteHeader(http.StatusNoContent) @@ -65,29 +68,32 @@ func handleSignal(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { func handleRevert(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, session *webauthn.PasskeySession, requireStepUp bool) { if r.Method != http.MethodPost { - http.Error(w, "POST only", http.StatusMethodNotAllowed) + writeProblem(w, r, http.StatusMethodNotAllowed, problemMethodNotAllowed, + "POST only", nil) return } - if !requireCore(w, core, "revert") { + if !requireCore(w, r, core, "revert") { return } - if !stepUpGate(w, session, requireStepUp) { + if !stepUpGate(w, r, session, requireStepUp) { return } key := strings.TrimSpace(r.FormValue("key")) if key == "" { - http.Error(w, "key required", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "key required", nil) return } newID, err := core.RevertFact(r.Context(), key) if err != nil { - log.Printf("revert %q: %v", key, err) if errors.Is(err, ipc.ErrNoFact) { - http.Error(w, "no fact to revert", http.StatusNotFound) + writeProblem(w, r, http.StatusNotFound, problemResourceNotFound, + "no fact to revert", fmt.Errorf("revert fact %q: %w", key, err)) return } - http.Error(w, "revert failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreChangeFailed, + "revert failed", fmt.Errorf("revert fact %q: %w", key, err)) return } log.Printf("reverted fact for key=%s, new_id=%d", key, newID) diff --git a/cmd/mavweb/main.go b/cmd/mavweb/main.go index e199a5b..9b01610 100644 --- a/cmd/mavweb/main.go +++ b/cmd/mavweb/main.go @@ -48,9 +48,14 @@ func main() { hexisURL := flag.String("hexis", "", "Hexis base URL for the /ecosystem panel (empty = not configured)") // Shared secret for POST /api/ambient, the notification-relay ingest that // reads the work calendar as a signal instead of holding a work credential - // (see ambient.go). Empty ⇒ the route is not registered at all. - ambientToken := flag.String("ambient-token", "", "shared secret for POST /api/ambient notification ingest (empty = ingest disabled, route not registered)") + // (see ambient.go). Enabling and authenticating are separate on purpose: an + // expanded-empty secret cannot silently turn a live integration off. + ambientEnabled := flag.Bool("ambient-enabled", false, "enable POST /api/ambient notification ingest (requires -ambient-token)") + ambientToken := flag.String("ambient-token", "", "shared secret for POST /api/ambient notification ingest") flag.Parse() + if err := validateAmbientConfig(*ambientEnabled, *ambientToken); err != nil { + log.Fatal(err) + } var core ipc.CoreAPI // swapConn — a second connection, for /models and nothing else. A model swap @@ -129,9 +134,9 @@ func main() { w.Write([]byte(*ntfyWS)) }) mux.HandleFunc("/api/signal", corePage(handleSignal)) - // Off unless configured: no token, no route — an unconfigured ingest is not - // a 503 waiting to be probed, it does not exist. - if *ambientToken != "" { + // Off unless explicitly enabled: a dark ingest has no route at all, while an + // enabled ingest with no token was rejected before the server was built. + if *ambientEnabled { mux.HandleFunc("/api/ambient", func(w http.ResponseWriter, r *http.Request) { handleAmbient(w, r, core, *ambientToken) }) @@ -251,6 +256,13 @@ func main() { } } +func validateAmbientConfig(enabled bool, token string) error { + if enabled && token == "" { + return errors.New("mavweb: ambient ingest is enabled but -ambient-token is empty") + } + return nil +} + // logUnguardedSurfaces names, at startup, what step-up would have covered had // WebAuthn been configured. One surface per line: these are read in a terminal // at the moment someone is deciding whether the box is safe to expose. @@ -288,7 +300,7 @@ const ( func mavwebHTTPServer(addr string, handler http.Handler) *http.Server { return &http.Server{ Addr: addr, - Handler: handler, + Handler: withRequestID(handler), ReadHeaderTimeout: mavwebReadHeaderTimeout, ReadTimeout: mavwebReadTimeout, IdleTimeout: mavwebIdleTimeout, diff --git a/cmd/mavweb/models.go b/cmd/mavweb/models.go index 2b38bcd..2c3b77d 100644 --- a/cmd/mavweb/models.go +++ b/cmd/mavweb/models.go @@ -4,6 +4,7 @@ import ( "context" _ "embed" "errors" + "fmt" "log" "net/http" "strconv" @@ -57,7 +58,8 @@ type modelsPage struct { // the reply. On its own connection the swap only blocks the swap. func handleModels(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, swapConn modelController, session *webauthn.PasskeySession, requireStepUp bool) { if core == nil { - http.Error(w, "models disabled (no -core)", http.StatusServiceUnavailable) + writeProblem(w, r, http.StatusServiceUnavailable, problemCoreUnavailable, + "models disabled (no -core)", nil) return } mc, ok := swapConn, swapConn != nil @@ -65,7 +67,8 @@ func handleModels(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, swap mc, ok = core.(modelController) } if !ok { - http.Error(w, "models unavailable: core connection does not support model swap", http.StatusServiceUnavailable) + writeProblem(w, r, http.StatusServiceUnavailable, problemModelsUnavailable, + "models unavailable: core connection does not support model swap", nil) return } ctx := r.Context() @@ -73,12 +76,14 @@ func handleModels(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, swap if r.Method == http.MethodPost { if !stepUpOK(session, requireStepUp) { - http.Error(w, "step-up required: assert a passkey first", http.StatusForbidden) + writeProblem(w, r, http.StatusForbidden, problemStepUpRequired, + "step-up required: assert a passkey first", nil) return } path := strings.TrimSpace(r.FormValue("model_path")) if path == "" { - http.Error(w, "model_path required", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "model_path required", nil) return } // Only the path comes off the form. n_ctx and n_gpu_layers are load @@ -93,20 +98,26 @@ func handleModels(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, swap page.Msg = "loaded " + res.Model + " (" + strconv.FormatInt(res.TookMs, 10) + "ms)" log.Printf("models: swapped to %s (%s) in %dms", res.ModelPath, res.Model, res.TookMs) case errors.Is(err, ipc.ErrForbidden): - http.Error(w, "refused: that model is not in phraser.swap_models, or step-up was not asserted", http.StatusForbidden) + writeProblem(w, r, http.StatusForbidden, problemModelsForbidden, + "refused: that model is not in phraser.swap_models, or step-up was not asserted", + fmt.Errorf("swap model %q: %w", path, err)) return case errors.Is(err, ipc.ErrUnknownMethod): - http.Error(w, "swap not configured on this core", http.StatusServiceUnavailable) + writeProblem(w, r, http.StatusServiceUnavailable, problemModelsUnavailable, + "swap not configured on this core", fmt.Errorf("swap model %q: %w", path, err)) return case res.NoBackend: - page.Err = "swap failed AND the rollback failed — no model is loaded. She is answering from templates and routing on the classifier. Try loading a model again; a restart is not needed." - log.Printf("models: swap to %s failed and the rollback failed, no model loaded: %v", path, err) + page.Err = inlineProblem(r, problemModelsUnavailable, + "swap failed AND the rollback failed — no model is loaded. She is answering from templates and routing on the classifier. Try loading a model again; a restart is not needed.", + fmt.Errorf("swap model %q and rollback: %w", path, err)) case res.RolledBack: - page.Err = "swap failed, rolled back to " + res.Model + " — she is still answering, with the old model" - log.Printf("models: swap to %s failed, rolled back: %v", path, err) + page.Err = inlineProblem(r, problemModelsUnavailable, + "swap failed, rolled back to "+res.Model+" — she is still answering, with the old model", + fmt.Errorf("swap model %q, rolled back to %q: %w", path, res.Model, err)) default: - page.Err = "swap failed: " + err.Error() - log.Printf("models: swap to %s failed: %v", path, err) + page.Err = inlineProblem(r, problemModelsUnavailable, + "swap failed; the current model state is shown below", + fmt.Errorf("swap model %q: %w", path, err)) } } @@ -115,8 +126,8 @@ func handleModels(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, swap if errors.Is(err, ipc.ErrUnknownMethod) { page.Off = true } else { - log.Printf("models: status: %v", err) - http.Error(w, "core read failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "core read failed", fmt.Errorf("read model status: %w", err)) return } } diff --git a/cmd/mavweb/models_test.go b/cmd/mavweb/models_test.go index 7810b5f..3882643 100644 --- a/cmd/mavweb/models_test.go +++ b/cmd/mavweb/models_test.go @@ -149,6 +149,27 @@ type errBrokenModel struct{} func (errBrokenModel) Error() string { return "llm: server did not start" } +type errPrivateModel struct{} + +func (errPrivateModel) Error() string { return "exec /private/llama-server: token rejected" } + +func TestModels_SwapFailureIsSanitizedAndTraceable(t *testing.T) { + core := &fakeModelCore{ + swapErr: errPrivateModel{}, + status: ipc.ModelStatusResp{Model: "qwen3", ModelPath: "/m/old.gguf"}, + } + w := modelsPOST(t, core, nil, false, "/m/cpt.gguf") + body := w.Body.String() + for _, want := range []string{"swap failed", string(problemModelsUnavailable), "request "} { + if !strings.Contains(body, want) { + t.Errorf("sanitized model error missing %q:\n%s", want, body) + } + } + if strings.Contains(body, "/private/llama-server") || strings.Contains(body, "token rejected") { + t.Errorf("model page disclosed the backend error:\n%s", body) + } +} + func TestModels_TotalFailureDoesNotSaySheIsStillAnswering(t *testing.T) { // The load failed and so did the rollback: nothing is loaded. The page used // to branch on RolledBack first and render "rolled back to — she is still diff --git a/cmd/mavweb/notifications.go b/cmd/mavweb/notifications.go index 97d68e5..8234a42 100644 --- a/cmd/mavweb/notifications.go +++ b/cmd/mavweb/notifications.go @@ -2,7 +2,7 @@ package main import ( _ "embed" - "log" + "fmt" "net/http" "strconv" @@ -48,14 +48,14 @@ func deliveryRows(as []ipc.DeliveryAttempt) []deliveryRow { } func handleNotifications(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { - if !requireCore(w, core, "notifications") { + if !requireCore(w, r, core, "notifications") { return } ctx := r.Context() nudges, err := core.RecentNudges(ctx, 50) if err != nil { - log.Printf("notifications: %v", err) - http.Error(w, "notifications error: "+err.Error(), http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "notifications unavailable", fmt.Errorf("read recent nudges: %w", err)) return } // The outbox, on the page that already answers "what did she send". @@ -66,7 +66,8 @@ func handleNotifications(w http.ResponseWriter, r *http.Request, core ipc.CoreAP if err != nil { // The nudge list is still worth showing, so this is a note on the page // rather than a dead page. - log.Printf("notifications: delivery attempts: %v", err) + logProblem(r, http.StatusOK, problemCoreReadFailed, + "delivery attempts unavailable", fmt.Errorf("read delivery attempts: %w", err)) } renderPage(w, notificationsTmpl, map[string]any{ "Nudges": nudges, diff --git a/cmd/mavweb/pages.go b/cmd/mavweb/pages.go index 1fa5688..54ae644 100644 --- a/cmd/mavweb/pages.go +++ b/cmd/mavweb/pages.go @@ -3,8 +3,8 @@ package main import ( "cmp" _ "embed" + "fmt" "html/template" - "log" "net/http" "strings" "time" @@ -73,7 +73,7 @@ var voiceTmpl = parsePage("voice", voiceHTML, nil) var ecosystemTmpl = parsePage("ecosystem", ecosystemHTML, nil) func handleDash(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { - if !requireCore(w, core, "dash") { + if !requireCore(w, r, core, "dash") { return } ctx := r.Context() @@ -82,8 +82,8 @@ func handleDash(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { nudges, err3 := core.RecentNudges(ctx, 50) notes, err4 := core.RecentNotes(ctx, 50) if err := cmp.Or(err1, err2, err3, err4); err != nil { - log.Printf("dash: %v", err) - http.Error(w, "core read failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "core read failed", fmt.Errorf("read dashboard: %w", err)) return } renderPage(w, dashTmpl, struct { @@ -95,13 +95,13 @@ func handleDash(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { } func handleHistory(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { - if !requireCore(w, core, "history") { + if !requireCore(w, r, core, "history") { return } facts, err := core.RecentFacts(r.Context(), 200) if err != nil { - log.Printf("history: %v", err) - http.Error(w, "core read failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "core read failed", fmt.Errorf("read fact history: %w", err)) return } renderPage(w, historyTmpl, struct { @@ -110,13 +110,13 @@ func handleHistory(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { } func handleTrace(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { - if !requireCore(w, core, "trace") { + if !requireCore(w, r, core, "trace") { return } trace, err := core.TickTrace(r.Context()) if err != nil { - log.Printf("trace: %v", err) - http.Error(w, "core read failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "core read failed", fmt.Errorf("read tick trace: %w", err)) return } // The turn records share this page rather than getting one of their own @@ -127,7 +127,8 @@ func handleTrace(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { // deploy. turns, err := core.TurnDecisions(r.Context(), 25) if err != nil { - log.Printf("trace: turn decisions: %v", err) + logProblem(r, http.StatusOK, problemCoreReadFailed, + "turn decisions unavailable", fmt.Errorf("read turn decisions: %w", err)) } renderPage(w, traceTmpl, traceData{Tick: trace, Turns: turns}) } @@ -149,14 +150,14 @@ type morningView struct { } func handleMorning(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { - if !requireCore(w, core, "morning") { + if !requireCore(w, r, core, "morning") { return } ctx := r.Context() status, err := core.MorningStatus(ctx) if err != nil { - log.Printf("morning: %v", err) - http.Error(w, "core read failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "core read failed", fmt.Errorf("read morning status: %w", err)) return } view := morningView{Routines: status} @@ -165,8 +166,8 @@ func handleMorning(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { // down with it — the page degrades to what it had before. plan, err := core.DayPlan(ctx) if err != nil { - log.Printf("morning: day plan: %v", err) - view.PlanErr = err.Error() + view.PlanErr = inlineProblem(r, problemCoreReadFailed, + "day plan unavailable", fmt.Errorf("read day plan: %w", err)) } else { view.Plan = &plan } @@ -186,14 +187,14 @@ type eventsView struct { const eventsPageLimit = 200 func handleEvents(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { - if !requireCore(w, core, "intake journal") { + if !requireCore(w, r, core, "intake journal") { return } var view eventsView evs, err := core.RecentEvents(r.Context(), eventsPageLimit) if err != nil { - log.Printf("events: %v", err) - view.Err = err.Error() + view.Err = inlineProblem(r, problemCoreReadFailed, + "intake journal unavailable", fmt.Errorf("read intake journal: %w", err)) } else { view.Events = evs } diff --git a/cmd/mavweb/problems.go b/cmd/mavweb/problems.go new file mode 100644 index 0000000..c947fc8 --- /dev/null +++ b/cmd/mavweb/problems.go @@ -0,0 +1,118 @@ +package main + +import ( + "context" + "crypto/rand" + "encoding/json" + "errors" + "fmt" + "log" + "net/http" + "os" +) + +// problemCode is the stable, low-cardinality name a client can key on. The +// request ID identifies one occurrence; the code identifies the class of +// failure without exposing the wrapped implementation error. +type problemCode string + +const ( + problemMethodNotAllowed problemCode = "request.method_not_allowed" + problemInvalidRequest problemCode = "request.invalid" + problemPayloadTooLarge problemCode = "request.payload_too_large" + problemUnauthorized problemCode = "auth.unauthorized" + problemStepUpRequired problemCode = "auth.step_up_required" + problemResourceNotFound problemCode = "resource.not_found" + problemIntegrationOff problemCode = "integration.disabled" + problemCoreUnavailable problemCode = "core.unavailable" + problemCoreReadFailed problemCode = "core.read_failed" + problemCoreWriteFailed problemCode = "core.write_failed" + problemCoreChangeFailed problemCode = "core.change_failed" + problemToolsChange problemCode = "tools.change_failed" + problemRoutinesChange problemCode = "routines.change_failed" + problemModelsUnavailable problemCode = "models.unavailable" + problemModelsForbidden problemCode = "models.forbidden" + problemWebAuthnBegin problemCode = "webauthn.begin_failed" + problemWebAuthnFinish problemCode = "webauthn.finish_failed" + problemWebAuthnStepUp problemCode = "webauthn.step_up_failed" + problemVoiceUnavailable problemCode = "voice.unavailable" + problemVoiceTransport problemCode = "voice.transport_failed" + problemVoiceResponse problemCode = "voice.response_failed" +) + +type requestIDKey struct{} + +// problemLogger is separate from the package-wide logger so the contract test +// can capture exactly one problem line without redirecting unrelated output. +var problemLogger = log.New(os.Stderr, "", log.LstdFlags) + +// problemResponse is the one non-success envelope returned by mavweb. Error is +// deliberately a public message, never err.Error(). Code is stable across +// occurrences; request_id joins this answer to the full server-side log line. +type problemResponse struct { + Error string `json:"error"` + Code problemCode `json:"code"` + RequestID string `json:"request_id"` +} + +// withRequestID mints the request identifier at the HTTP boundary. A caller's +// X-Request-ID is ignored: accepting it would let an untrusted client forge a +// link to another request's logs. The generated ID is also returned on success, +// which lets an operator start from any surprising response, not errors alone. +func withRequestID(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + id := rand.Text() + w.Header().Set("X-Request-ID", id) + ctx := context.WithValue(r.Context(), requestIDKey{}, id) + next.ServeHTTP(w, r.WithContext(ctx)) + }) +} + +func requestIDFromContext(ctx context.Context) string { + id, _ := ctx.Value(requestIDKey{}).(string) + return id +} + +func problemRequestID(r *http.Request) string { + id := requestIDFromContext(r.Context()) + if id == "" { + id = rand.Text() + } + return id +} + +func logProblem(r *http.Request, status int, code problemCode, public string, err error) string { + id := problemRequestID(r) + if err == nil { + err = errors.New(public) + } + problemLogger.Printf("mavweb problem request_id=%s code=%s status=%d method=%s path=%q: %v", + id, code, status, r.Method, r.URL.Path, err) + return id +} + +// inlineProblem preserves a useful partial page when one panel fails, while +// applying the same disclosure and correlation rules as an HTTP problem. +func inlineProblem(r *http.Request, code problemCode, public string, err error) string { + id := logProblem(r, http.StatusOK, code, public, err) + return fmt.Sprintf("%s (code %s, request %s)", public, code, id) +} + +// writeProblem is the only mavweb HTTP error writer. The wrapped error is +// logged in full and only the explicit public message, stable code and request +// ID cross the HTTP boundary. +func writeProblem(w http.ResponseWriter, r *http.Request, status int, code problemCode, public string, err error) { + // Unit-level handlers and embedders may call a handler without installing + // the server middleware. They still get the same traceable contract. + id := logProblem(r, status, code, public, err) + w.Header().Set("X-Request-ID", id) + + w.Header().Set("Content-Type", "application/problem+json; charset=utf-8") + w.Header().Set("Cache-Control", "no-store") + w.WriteHeader(status) + // A failed client connection leaves nowhere useful to report an encoder + // error; the full problem is already in the server log before this write. + _ = json.NewEncoder(w).Encode(problemResponse{ + Error: public, Code: code, RequestID: id, + }) +} diff --git a/cmd/mavweb/problems_test.go b/cmd/mavweb/problems_test.go new file mode 100644 index 0000000..cf86344 --- /dev/null +++ b/cmd/mavweb/problems_test.go @@ -0,0 +1,135 @@ +package main + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "go/ast" + "go/parser" + "go/token" + "io" + "log" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestWriteProblemSanitizesAndCorrelates(t *testing.T) { + var logs bytes.Buffer + old := problemLogger + problemLogger = logForTest(&logs) + t.Cleanup(func() { problemLogger = old }) + + const id = "TESTREQUESTID" + r := httptest.NewRequest(http.MethodPost, "/tools", nil) + r = r.WithContext(context.WithValue(r.Context(), requestIDKey{}, id)) + w := httptest.NewRecorder() + internal := errors.New("dial unix /run/private/mavend.sock: bearer secret-token") + writeProblem(w, r, http.StatusBadGateway, problemToolsChange, "enable failed", internal) + + if w.Code != http.StatusBadGateway { + t.Fatalf("status = %d, want 502", w.Code) + } + if got := w.Header().Get("X-Request-ID"); got != id { + t.Fatalf("X-Request-ID = %q, want %q", got, id) + } + if got := w.Header().Get("Content-Type"); !strings.HasPrefix(got, "application/problem+json") { + t.Fatalf("Content-Type = %q", got) + } + var got problemResponse + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil { + t.Fatalf("decode problem: %v", err) + } + if got.Error != "enable failed" || got.Code != problemToolsChange || got.RequestID != id { + t.Fatalf("problem = %+v", got) + } + if strings.Contains(w.Body.String(), "private") || strings.Contains(w.Body.String(), "secret-token") { + t.Fatalf("HTTP response disclosed the wrapped error: %s", w.Body.String()) + } + for _, want := range []string{id, string(problemToolsChange), internal.Error()} { + if !strings.Contains(logs.String(), want) { + t.Errorf("server log missing %q: %s", want, logs.String()) + } + } +} + +func TestRequestIDMiddlewareMintsAndIgnoresCallerID(t *testing.T) { + var seen string + h := withRequestID(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + seen = requestIDFromContext(r.Context()) + w.WriteHeader(http.StatusNoContent) + })) + r := httptest.NewRequest(http.MethodGet, "/api/ping", nil) + r.Header.Set("X-Request-ID", "caller-chosen") + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + + if seen == "" || seen == "caller-chosen" { + t.Fatalf("request ID = %q; want a server-generated value", seen) + } + if got := w.Header().Get("X-Request-ID"); got != seen { + t.Fatalf("response request ID = %q, context ID = %q", got, seen) + } +} + +func TestEcosystemPanelPropagatesRequestID(t *testing.T) { + const id = "WEBREQUESTCORRELATION" + var correlation, requester string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + correlation = r.Header.Get("X-Correlation-ID") + requester = r.Header.Get("X-Requested-By") + _, _ = io.WriteString(w, `[]`) + })) + defer srv.Close() + ctx := context.WithValue(context.Background(), requestIDKey{}, id) + var rows []ecoEntity + if got := getEco(ctx, srv.URL, "/entities", &rows); got != "" { + t.Fatalf("getEco error = %q", got) + } + if correlation != id || requester != "mavweb" { + t.Fatalf("correlation = %q, requester = %q", correlation, requester) + } +} + +// The contract is architectural, not a convention people must remember. Keep +// a syntax-level guard so a new handler cannot bypass writeProblem by adding +// another http.Error call. +func TestProductionHandlersUseOneProblemWriter(t *testing.T) { + entries, err := os.ReadDir(".") + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + name := entry.Name() + if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + file, err := parser.ParseFile(token.NewFileSet(), filepath.Clean(name), nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Error" { + return true + } + pkg, ok := sel.X.(*ast.Ident) + if ok && pkg.Name == "http" { + t.Errorf("%s contains http.Error; use writeProblem", name) + } + return true + }) + } +} + +func logForTest(w io.Writer) *log.Logger { + return log.New(w, "", 0) +} diff --git a/cmd/mavweb/reminders.go b/cmd/mavweb/reminders.go index b953ac6..6536680 100644 --- a/cmd/mavweb/reminders.go +++ b/cmd/mavweb/reminders.go @@ -3,7 +3,7 @@ package main import ( _ "embed" "encoding/json" - "log" + "fmt" "net/http" "strings" @@ -26,6 +26,7 @@ type reminderRow struct { Created string Fires string Status string + Detail string Text string } @@ -50,10 +51,19 @@ func reminderText(payload string) string { func reminderRows(rs []ipc.Reminder) []reminderRow { out := make([]reminderRow, 0, len(rs)) for _, r := range rs { + status := r.Status + detail := "" + if !r.DeliveryBlockedTs.IsZero() { + status = "blocked" + detail = r.DeliveryBlockedError + } else if r.DeliveryAttempts > 0 && !r.NextAttemptTs.IsZero() { + detail = "retry " + r.NextAttemptTs.Local().Format("02 Jan 15:04") + } out = append(out, reminderRow{ Created: r.CreatedTs.Local().Format("02 Jan 15:04"), Fires: r.FireTs.Local().Format("02 Jan 15:04"), - Status: r.Status, + Status: status, + Detail: detail, Text: reminderText(r.Payload), }) } @@ -61,13 +71,13 @@ func reminderRows(rs []ipc.Reminder) []reminderRow { } func handleReminders(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { - if !requireCore(w, core, "reminders") { + if !requireCore(w, r, core, "reminders") { return } reminders, err := core.ListReminders(r.Context(), 50) if err != nil { - log.Printf("reminders: %v", err) - http.Error(w, "reminders error: "+err.Error(), http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "reminders unavailable", fmt.Errorf("list reminders: %w", err)) return } renderPage(w, remindersTmpl, map[string]any{"Reminders": reminderRows(reminders)}) diff --git a/cmd/mavweb/reminders.html b/cmd/mavweb/reminders.html index e28ef22..7bdbb20 100644 --- a/cmd/mavweb/reminders.html +++ b/cmd/mavweb/reminders.html @@ -5,7 +5,7 @@ {{range .Reminders}} {{.Created}} {{.Fires}} -{{.Status}} +{{.Status}}{{if .Detail}}
{{.Detail}}
{{end}} {{.Text}} {{end}} {{else}}
diff --git a/cmd/mavweb/reminders_test.go b/cmd/mavweb/reminders_test.go index 0fa4384..3e5e16b 100644 --- a/cmd/mavweb/reminders_test.go +++ b/cmd/mavweb/reminders_test.go @@ -31,6 +31,19 @@ func TestReminderRowsUnwrapAndLocalise(t *testing.T) { } } +func TestReminderRowsExposeBlockedDelivery(t *testing.T) { + blocked := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + rows := reminderRows([]ipc.Reminder{{ + Status: "pending", + Payload: `{"text":"позвонить врачу"}`, + DeliveryBlockedTs: blocked, + DeliveryBlockedError: "ntfy credentials rejected", + }}) + if len(rows) != 1 || rows[0].Status != "blocked" || rows[0].Detail != "ntfy credentials rejected" { + t.Fatalf("blocked reminder is not visible: %+v", rows) + } +} + // A payload that is not the envelope is his own words, so it is shown as it is. func TestReminderTextKeepsPlainPayload(t *testing.T) { for _, tc := range []struct{ in, want string }{ diff --git a/cmd/mavweb/routines.go b/cmd/mavweb/routines.go index 1ca6db3..22f13b9 100644 --- a/cmd/mavweb/routines.go +++ b/cmd/mavweb/routines.go @@ -5,7 +5,6 @@ import ( _ "embed" "errors" "fmt" - "log" "net/http" "strconv" "strings" @@ -41,7 +40,7 @@ type routineView struct { // gated — it only ever removes a reason to speak, so the worst a weaker caller // can do is make maven quieter. func handleRoutines(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, session *webauthn.PasskeySession, requireStepUp bool) { - if !requireCore(w, core, "routines") { + if !requireCore(w, r, core, "routines") { return } ctx := r.Context() @@ -54,8 +53,8 @@ func handleRoutines(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, se } proposed, err := core.ListProposedRoutines(ctx) if err != nil { - log.Printf("routines: list: %v", err) - http.Error(w, "routines error: "+err.Error(), http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "routines unavailable", fmt.Errorf("list proposed routines: %w", err)) return } renderPage(w, routinesTmpl, struct { @@ -76,44 +75,45 @@ func applyRoutinePost(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, // already the step-up-gated surface for this table, and a second gated // surface is a second thing to get wrong. if action == "seed" { - if !stepUpGate(w, session, requireStepUp) { + if !stepUpGate(w, r, session, requireStepUp) { return "", false } out, err := seedRoutineEvent(ctx, core, r) if err != nil { - log.Printf("routines: seed: %v", err) - http.Error(w, "seed failed: "+err.Error(), http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemRoutinesChange, + "seed failed", fmt.Errorf("seed routine event: %w", err)) return "", false } return out, true } - idStr := r.FormValue("id") - var rid int64 - if n, _ := fmt.Sscanf(idStr, "%d", &rid); n != 1 { - http.Error(w, "invalid id", http.StatusBadRequest) + rid, err := strconv.ParseInt(strings.TrimSpace(r.FormValue("id")), 10, 64) + if err != nil || rid <= 0 { + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "invalid id", err) return "", false } switch action { case "accept": - if !stepUpGate(w, session, requireStepUp) { + if !stepUpGate(w, r, session, requireStepUp) { return "", false } if err := acceptRoutine(ctx, core, rid); err != nil { - log.Printf("routines: accept %d: %v", rid, err) - http.Error(w, "accept failed: "+err.Error(), http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemRoutinesChange, + "accept failed", fmt.Errorf("accept routine %d: %w", rid, err)) return "", false } return "accepted routine — maven will remind you", true case "dismiss": if err := core.DismissProposedRoutine(ctx, rid); err != nil { - log.Printf("routines: dismiss %d: %v", rid, err) - http.Error(w, "dismiss failed: "+err.Error(), http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemRoutinesChange, + "dismiss failed", fmt.Errorf("dismiss routine %d: %w", rid, err)) return "", false } return "dismissed routine", true default: - http.Error(w, "unknown action", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "unknown action", nil) return "", false } } diff --git a/cmd/mavweb/shell.go b/cmd/mavweb/shell.go index 3e18a6e..4b19457 100644 --- a/cmd/mavweb/shell.go +++ b/cmd/mavweb/shell.go @@ -159,9 +159,10 @@ func renderPage(w http.ResponseWriter, t *template.Template, data any) { // requireCore answers whether the surface has a core to read. mavweb runs // without -core (voice-only), and every page that needs mavend says so with a // 503 naming itself rather than a blank error. -func requireCore(w http.ResponseWriter, core ipc.CoreAPI, surface string) bool { +func requireCore(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, surface string) bool { if core == nil { - http.Error(w, surface+" disabled (no -core)", http.StatusServiceUnavailable) + writeProblem(w, r, http.StatusServiceUnavailable, problemCoreUnavailable, + surface+" disabled (no -core)", nil) return false } return true @@ -169,11 +170,12 @@ func requireCore(w http.ResponseWriter, core ipc.CoreAPI, surface string) bool { // stepUpGate reports whether the caller may proceed through the AuthStepUp // gate, writing the 403 itself when it may not. See stepUpOK for the policy. -func stepUpGate(w http.ResponseWriter, session *webauthn.PasskeySession, requireStepUp bool) bool { +func stepUpGate(w http.ResponseWriter, r *http.Request, session *webauthn.PasskeySession, requireStepUp bool) bool { if stepUpOK(session, requireStepUp) { return true } - http.Error(w, "step-up required: assert a passkey first", http.StatusForbidden) + writeProblem(w, r, http.StatusForbidden, problemStepUpRequired, + "step-up required: assert a passkey first", nil) return false } diff --git a/cmd/mavweb/tasks.go b/cmd/mavweb/tasks.go index 8122228..fb8ec7a 100644 --- a/cmd/mavweb/tasks.go +++ b/cmd/mavweb/tasks.go @@ -5,7 +5,6 @@ import ( _ "embed" "errors" "fmt" - "log" "net/http" "strconv" "strings" @@ -88,7 +87,7 @@ func rowOf(t ipc.Task) taskRow { // from something she read into work he owns. That review step is why derived // tasks are captured as candidates in the first place. func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { - if !requireCore(w, core, "tasks") { + if !requireCore(w, r, core, "tasks") { return } ctx := r.Context() @@ -97,15 +96,14 @@ func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { var err error msg, err = applyTaskPost(ctx, core, r) if err != nil { - log.Printf("tasks: %v", err) - errMsg = err.Error() + errMsg = inlineProblem(r, problemCoreChangeFailed, taskPublicMessage(err), err) } } all, err := core.ListTasks(ctx, "") if err != nil { - log.Printf("tasks: list: %v", err) - http.Error(w, "tasks error: "+err.Error(), http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "tasks unavailable", fmt.Errorf("list tasks: %w", err)) return } // Live rows are ordered by the same ranker the spoken list uses, so the page @@ -162,6 +160,39 @@ func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { }{msg, errMsg, tasks.Stalls(live, now()), cands, open, resolved, resolvedTotal > len(resolved)}) } +// taskInputError is a form/domain refusal safe to show back to the owner. IPC, +// storage and transport errors never use this type and therefore receive the +// generic task failure text plus a request reference. +type taskInputError struct{ message string } + +func (e *taskInputError) Error() string { return e.message } + +func taskInput(message string) error { return &taskInputError{message: message} } + +func taskInputf(format string, args ...any) error { + return &taskInputError{message: fmt.Sprintf(format, args...)} +} + +type taskPartialError struct { + public string + err error +} + +func (e *taskPartialError) Error() string { return e.public + ": " + e.err.Error() } +func (e *taskPartialError) Unwrap() error { return e.err } + +func taskPublicMessage(err error) string { + var input *taskInputError + if errors.As(err, &input) { + return input.message + } + var partial *taskPartialError + if errors.As(err, &partial) { + return partial.public + } + return "task update failed" +} + // applyTaskPost performs one write and returns the message to show. A bad // request returns an error, which the page renders inline rather than as a // bare 400 — this is a form surface, not an API. @@ -170,7 +201,7 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri if action == "add" { text := strings.TrimSpace(r.FormValue("text")) if text == "" { - return "", errors.New("empty task text") + return "", taskInput("empty task text") } req := ipc.CaptureTaskReq{Text: text, Source: "tap:web", Status: "open", Ts: now()} wgt, err := formWeight(r) @@ -198,7 +229,7 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri id, err := strconv.ParseInt(r.FormValue("id"), 10, 64) if err != nil { - return "", errors.New("invalid id") + return "", taskInput("invalid id") } if action == "promote" { @@ -214,7 +245,7 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri // is not editable here: that ladder is one-way and has its own buttons. text := strings.TrimSpace(r.FormValue("text")) if text == "" { - return "", errors.New("empty task text") + return "", taskInput("empty task text") } wgt, err := formWeight(r) if err != nil { @@ -230,9 +261,9 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri case errors.Is(err, ipc.ErrTaskDuplicate): // Naming the collision instead of merging: two live rows carry two // provenances, and picking one is not the page's call. - return "", errors.New("another open task already says this — drop one of the two") + return "", taskInput("another open task already says this — drop one of the two") case errors.Is(err, ipc.ErrTaskResolved): - return "", errors.New("a resolved task keeps the text it was finished under") + return "", taskInput("a resolved task keeps the text it was finished under") default: return "", err } @@ -247,7 +278,7 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri case "drop": status, msg = "dropped", "dropped task" default: - return "", fmt.Errorf("unknown action %q", action) + return "", taskInputf("unknown action %q", action) } if err := core.SetTaskStatus(ctx, id, status, now(), "tap:web"); err != nil { return "", statusWriteErr(err) @@ -257,7 +288,7 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri // errNoDoneWhen — the refusal has to name what is missing, or the button looks // broken. The field it asks for arrives with the intake form (Vikunja #511). -var errNoDoneWhen = errors.New("write a definition of done before confirming this candidate") +var errNoDoneWhen = taskInput("write a definition of done before confirming this candidate") // statusWriteErr translates a SetTaskStatus failure into what the page says. func statusWriteErr(err error) error { @@ -281,11 +312,11 @@ func statusWriteErr(err error) error { func promoteCandidate(ctx context.Context, core ipc.CoreAPI, r *http.Request, id int64) (string, error) { doneWhen := strings.TrimSpace(r.FormValue("done_when")) if doneWhen == "" { - return "", errors.New("write a definition of done — what has to be true for this to be finished") + return "", taskInput("write a definition of done — what has to be true for this to be finished") } text := strings.TrimSpace(r.FormValue("text")) if text == "" { - return "", errors.New("empty task text") + return "", taskInput("empty task text") } due, err := formDue(r, now()) if err != nil { @@ -326,7 +357,7 @@ func promoteCandidate(ctx context.Context, core ipc.CoreAPI, r *http.Request, id if _, err := core.CreateReminder(ctx, fire, text, ""); err != nil { // The task IS promoted; only the reminder failed. Saying "confirmed" // and nothing else would leave him expecting a nudge that will not come. - return "", fmt.Errorf("confirmed, but the reminder did not save: %w", err) + return "", &taskPartialError{public: "task confirmed, but the reminder did not save", err: err} } return "confirmed, and maven will remind you that morning", nil } @@ -343,15 +374,15 @@ func resolveBlocker(ctx context.Context, core ipc.CoreAPI, field string) (string ref, err := core.ResolveEntity(ctx, name, []string{"person"}) switch { case errors.Is(err, ipc.ErrNotImplemented): - return "", errors.New("no identity service here, so blocked-on cannot be stored — leave it empty") + return "", taskInput("no identity service here, so blocked-on cannot be stored — leave it empty") case errors.Is(err, ipc.ErrNoEntity): - return "", fmt.Errorf("nexus does not know %q", name) + return "", taskInputf("nexus does not know %q", name) case err != nil: return "", fmt.Errorf("resolving %q: %w", name, err) case ref.Ambiguous: // Asking, not picking: a task blocked on the wrong person is a // mistake nobody can see afterwards. - return "", fmt.Errorf("%q matches %s — say which", name, strings.Join(ref.Candidates, ", ")) + return "", taskInputf("%q matches %s — say which", name, strings.Join(ref.Candidates, ", ")) } return ref.ID, nil } @@ -366,7 +397,7 @@ func formWeight(r *http.Request) (int, error) { } wgt, err := strconv.Atoi(v) if err != nil || wgt < 0 { - return 0, fmt.Errorf("bad weight %q", v) + return 0, taskInputf("bad weight %q", v) } if wgt > tasks.MaxWeight { wgt = tasks.MaxWeight @@ -383,7 +414,7 @@ func formDue(r *http.Request, now time.Time) (*time.Time, error) { } due, err := time.ParseInLocation("2006-01-02", d, now.Location()) if err != nil { - return nil, fmt.Errorf("bad due date %q", d) + return nil, taskInputf("bad due date %q", d) } return &due, nil } diff --git a/cmd/mavweb/tasks_test.go b/cmd/mavweb/tasks_test.go index 3f86f1c..b2fe6c1 100644 --- a/cmd/mavweb/tasks_test.go +++ b/cmd/mavweb/tasks_test.go @@ -191,6 +191,24 @@ func TestHandleTasksRejectsBadPost(t *testing.T) { } } +func TestHandleTasksSanitizesCoreWriteFailure(t *testing.T) { + core := &fakeTaskCore{captureErr: fmt.Errorf("sqlite /private/maven.db: key material rejected")} + form := url.Values{"action": {"add"}, "text": {"что-то"}} + req := httptest.NewRequest(http.MethodPost, "/tasks", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + handleTasks(rec, req, core) + body := rec.Body.String() + for _, want := range []string{"task update failed", string(problemCoreChangeFailed), "request "} { + if !strings.Contains(body, want) { + t.Errorf("sanitized task error missing %q: %s", want, body) + } + } + if strings.Contains(body, "/private/maven.db") || strings.Contains(body, "key material") { + t.Errorf("task page disclosed the core error: %s", body) + } +} + func TestHandleTasksNoCore(t *testing.T) { rec := httptest.NewRecorder() handleTasks(rec, httptest.NewRequest(http.MethodGet, "/tasks", nil), nil) diff --git a/cmd/mavweb/tools.go b/cmd/mavweb/tools.go index 5a445e9..6a96e52 100644 --- a/cmd/mavweb/tools.go +++ b/cmd/mavweb/tools.go @@ -3,8 +3,8 @@ package main import ( "cmp" _ "embed" + "fmt" "html/template" - "log" "net/http" "strings" "time" @@ -32,13 +32,13 @@ var toolsTmpl = parsePage("tools", toolsHTML, template.FuncMap{ // shell-word parsing; the box owner controls this input, quote a wrapper script // if an arg needs spaces). func handleTools(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, session *webauthn.PasskeySession, requireStepUp bool) { - if !requireCore(w, core, "tools") { + if !requireCore(w, r, core, "tools") { return } ctx := r.Context() var msg string if r.Method == http.MethodPost { - if !stepUpGate(w, session, requireStepUp) { + if !stepUpGate(w, r, session, requireStepUp) { return } action := r.FormValue("action") @@ -49,54 +49,59 @@ func handleTools(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, sessi cmd := strings.Fields(r.FormValue("cmd")) destructive := r.FormValue("destructive") != "" if name == "" || len(cmd) == 0 { - http.Error(w, "name and cmd required", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "name and cmd required", nil) return } if err := core.EnableTool(ctx, name, cmd, destructive, scope, time.Now()); err != nil { - log.Printf("tools: enable %q: %v", name, err) - http.Error(w, "enable failed: "+err.Error(), http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemToolsChange, + "enable failed", fmt.Errorf("enable tool %q: %w", name, err)) return } msg = "enabled " + name case "disable": if name == "" { - http.Error(w, "name required", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "name required", nil) return } if err := core.DisableTool(ctx, name); err != nil { - log.Printf("tools: disable %q: %v", name, err) - http.Error(w, "disable failed: "+err.Error(), http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemToolsChange, + "disable failed", fmt.Errorf("disable tool %q: %w", name, err)) return } msg = "disabled " + name case "dismiss": if name == "" { - http.Error(w, "name required", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "name required", nil) return } if err := core.DeleteTool(ctx, name); err != nil { - log.Printf("tools: dismiss %q: %v", name, err) - http.Error(w, "dismiss failed: "+err.Error(), http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemToolsChange, + "dismiss failed", fmt.Errorf("dismiss tool %q: %w", name, err)) return } msg = "dismissed " + name default: - http.Error(w, "unknown action", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "unknown action", nil) return } } proposed, err1 := core.ListTools(ctx, "proposed") enabled, err2 := core.ListTools(ctx, "enabled") if err := cmp.Or(err1, err2); err != nil { - log.Printf("tools: %v", err) - http.Error(w, "core read failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemCoreReadFailed, + "core read failed", fmt.Errorf("list tools: %w", err)) return } // MCP is off by default and an older core may not know the method at all, // so a failure here renders an empty section rather than breaking the page. servers, err := core.MCPServers(ctx) if err != nil { - log.Printf("tools: mcp servers: %v", err) + logProblem(r, http.StatusOK, problemCoreReadFailed, + "MCP server status unavailable", fmt.Errorf("read MCP server status: %w", err)) servers = nil } // Enabled rows are shown grouped by capability domain (Vikunja #452). A diff --git a/cmd/mavweb/voiceproxy.go b/cmd/mavweb/voiceproxy.go index 9cb8964..1fd2531 100644 --- a/cmd/mavweb/voiceproxy.go +++ b/cmd/mavweb/voiceproxy.go @@ -46,24 +46,28 @@ func pushToTalk(pcm []byte) voice.Request { func handlePTT(w http.ResponseWriter, r *http.Request, voiceAddr string, session *webauthn.PasskeySession, requireStepUp bool) { if r.Method != http.MethodPost { - http.Error(w, "POST only", http.StatusMethodNotAllowed) + writeProblem(w, r, http.StatusMethodNotAllowed, problemMethodNotAllowed, + "POST only", nil) return } - if !stepUpGate(w, session, requireStepUp) { + if !stepUpGate(w, r, session, requireStepUp) { return } body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, maxPTTAudioBytes)) if err != nil { var tooLarge *http.MaxBytesError if errors.As(err, &tooLarge) { - http.Error(w, "audio exceeds the ten-minute PTT limit", http.StatusRequestEntityTooLarge) + writeProblem(w, r, http.StatusRequestEntityTooLarge, problemPayloadTooLarge, + "audio exceeds the ten-minute PTT limit", err) return } - http.Error(w, "read audio", http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "read audio", fmt.Errorf("read PTT audio: %w", err)) return } if len(body) < 4 { - http.Error(w, "too short", 400) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "audio too short", nil) return } @@ -72,36 +76,38 @@ func handlePTT(w http.ResponseWriter, r *http.Request, voiceAddr string, session var d net.Dialer tc, err := d.DialContext(r.Context(), "tcp", voiceAddr) if err != nil { - log.Printf("ptt dial voice: %v", err) - http.Error(w, "voice unavailable", http.StatusServiceUnavailable) + writeProblem(w, r, http.StatusServiceUnavailable, problemVoiceUnavailable, + "voice unavailable", fmt.Errorf("dial voice service: %w", err)) return } defer tc.Close() req := pushToTalk(body) if err := writeFrame(tc, &req); err != nil { - log.Printf("ptt write: %v", err) - http.Error(w, err.Error(), 500) + writeProblem(w, r, http.StatusBadGateway, problemVoiceTransport, + "voice request failed", fmt.Errorf("write voice request: %w", err)) return } for { resp, push, err := readOneFrame(tc) if err != nil { - log.Printf("ptt read: %v", err) - http.Error(w, err.Error(), 500) + writeProblem(w, r, http.StatusBadGateway, problemVoiceTransport, + "voice response failed", fmt.Errorf("read voice response: %w", err)) return } if push != nil { continue } if resp.Error != nil { - http.Error(w, resp.Error.Message, 500) + writeProblem(w, r, http.StatusBadGateway, problemVoiceResponse, + "voice turn failed", fmt.Errorf("voice RPC error: %s", resp.Error.Message)) return } var pttResp voice.PushToTalkResp if err := json.Unmarshal(resp.Result, &pttResp); err != nil { - http.Error(w, err.Error(), 500) + writeProblem(w, r, http.StatusBadGateway, problemVoiceResponse, + "voice response failed", fmt.Errorf("decode voice response: %w", err)) return } w.Header().Set("Content-Type", "audio/l16;rate=16000;channels=1") diff --git a/cmd/mavweb/webauthn.go b/cmd/mavweb/webauthn.go index f1ef4a7..1233144 100644 --- a/cmd/mavweb/webauthn.go +++ b/cmd/mavweb/webauthn.go @@ -92,8 +92,8 @@ var passkeyTmpl = parsePage("passkey", passkeyPageHTML, nil) func (h *PasskeyHandle) RegisterBegin(w http.ResponseWriter, r *http.Request) { opts, challenge, err := h.rp.CreationOptions([]byte("maven-user"), "maven user") if err != nil { - log.Printf("webauthn: register begin: %v", err) - http.Error(w, err.Error(), http.StatusInternalServerError) + writeProblem(w, r, http.StatusInternalServerError, problemWebAuthnBegin, + "passkey registration could not start", fmt.Errorf("webauthn register begin: %w", err)) return } w.Header().Set("Content-Type", "application/json") @@ -102,7 +102,8 @@ func (h *PasskeyHandle) RegisterBegin(w http.ResponseWriter, r *http.Request) { func (h *PasskeyHandle) RegisterFinish(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { - http.Error(w, "POST only", http.StatusMethodNotAllowed) + writeProblem(w, r, http.StatusMethodNotAllowed, problemMethodNotAllowed, + "POST only", nil) return } var body struct { @@ -110,7 +111,8 @@ func (h *PasskeyHandle) RegisterFinish(w http.ResponseWriter, r *http.Request) { Credential map[string]any `json:"credential"` } if err := json.NewDecoder(r.Body).Decode(&body); err != nil { - http.Error(w, "bad request: "+err.Error(), http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "invalid registration request", fmt.Errorf("decode webauthn registration: %w", err)) return } save := func(id string, publicKey []byte, _ []byte, _ string) error { @@ -118,8 +120,8 @@ func (h *PasskeyHandle) RegisterFinish(w http.ResponseWriter, r *http.Request) { } credID, err := h.rp.FinishRegistration(save, body.Challenge, body.Credential) if err != nil { - log.Printf("webauthn: register finish: %v", err) - http.Error(w, err.Error(), http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemWebAuthnFinish, + "passkey registration failed", fmt.Errorf("webauthn register finish: %w", err)) return } log.Printf("webauthn: registered credential %s", credID) @@ -140,8 +142,8 @@ func (h *PasskeyHandle) RegisterFinish(w http.ResponseWriter, r *http.Request) { func (h *PasskeyHandle) AssertBegin(w http.ResponseWriter, r *http.Request) { opts, challenge, err := h.rp.AssertionOptions() if err != nil { - log.Printf("webauthn: assert begin: %v", err) - http.Error(w, err.Error(), http.StatusInternalServerError) + writeProblem(w, r, http.StatusInternalServerError, problemWebAuthnBegin, + "passkey assertion could not start", fmt.Errorf("webauthn assert begin: %w", err)) return } w.Header().Set("Content-Type", "application/json") @@ -150,7 +152,8 @@ func (h *PasskeyHandle) AssertBegin(w http.ResponseWriter, r *http.Request) { func (h *PasskeyHandle) AssertFinish(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { - http.Error(w, "POST only", http.StatusMethodNotAllowed) + writeProblem(w, r, http.StatusMethodNotAllowed, problemMethodNotAllowed, + "POST only", nil) return } var body struct { @@ -176,7 +179,8 @@ func (h *PasskeyHandle) AssertFinish(w http.ResponseWriter, r *http.Request) { Explicit bool `json:"explicit"` } if err := json.NewDecoder(r.Body).Decode(&body); err != nil { - http.Error(w, "bad request: "+err.Error(), http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemInvalidRequest, + "invalid assertion request", fmt.Errorf("decode webauthn assertion: %w", err)) return } @@ -189,8 +193,8 @@ func (h *PasskeyHandle) AssertFinish(w http.ResponseWriter, r *http.Request) { credID, err := h.rp.FinishAssertion(lookup, update, body.Challenge, body.Credential) if err != nil { - log.Printf("webauthn: assert finish: %v", err) - http.Error(w, err.Error(), http.StatusBadRequest) + writeProblem(w, r, http.StatusBadRequest, problemWebAuthnFinish, + "passkey assertion failed", fmt.Errorf("webauthn assert finish: %w", err)) return } @@ -201,8 +205,8 @@ func (h *PasskeyHandle) AssertFinish(w http.ResponseWriter, r *http.Request) { ctx, cancel := context.WithTimeout(r.Context(), 5*time.Second) defer cancel() if err := h.assertFn.AssertStepUp(ctx); err != nil { - log.Printf("webauthn: assert step-up: %v", err) - http.Error(w, "step-up assertion failed", http.StatusBadGateway) + writeProblem(w, r, http.StatusBadGateway, problemWebAuthnStepUp, + "step-up assertion failed", fmt.Errorf("assert step-up in core: %w", err)) return } } diff --git a/deploy/README.md b/deploy/README.md index 1d3f9ac..0c64634 100644 --- a/deploy/README.md +++ b/deploy/README.md @@ -11,6 +11,11 @@ dir and read-only models. cp deploy/db_key.env.example deploy/db_key.env printf 'MAVEN_DB_KEY=%s\n' "$(openssl rand 32 | base64 -w0)" > deploy/db_key.env +# 1b. review the complete secret inventory. Copy homesrv values to +# deploy/telegram.env, ambient enable/token values to .env, and workpc values +# to the destinations named in the example. Live blocks reject empty secrets. +cp deploy/telegram.env.example deploy/telegram.env + # 2. build + start docker compose build docker compose up -d diff --git a/deploy/cw2/serve.py b/deploy/cw2/serve.py index 89da08c..c13e54c 100644 --- a/deploy/cw2/serve.py +++ b/deploy/cw2/serve.py @@ -20,6 +20,7 @@ every stutter on the way there. """ import hmac +import ipaddress import json import logging import os @@ -71,6 +72,18 @@ def authorised(headers): return hmac.compare_digest(got, "Bearer " + TOKEN) +def validate_listener_auth(host, token): + """Refuse a network listener without its bearer boundary.""" + try: + loopback = ipaddress.ip_address(host).is_loopback + except ValueError: + loopback = host.casefold() == "localhost" + if not loopback and not token.strip(): + raise ValueError( + f"CW2_TOKEN is required while CW2_HOST={host!r} is reachable from the network" + ) + + class Handler(BaseHTTPRequestHandler): protocol_version = "HTTP/1.1" @@ -142,8 +155,10 @@ class Handler(BaseHTTPRequestHandler): def main(): - if not TOKEN: - log.warning("no CW2_TOKEN set: anything on the LAN can post audio here") + try: + validate_listener_auth(HOST, TOKEN) + except ValueError as exc: + raise SystemExit(str(exc)) from exc # Bind before loading, so a restart answers 503 rather than refusing the # connection. Both make Maven fall back, but only one of them says why. srv = ThreadingHTTPServer((HOST, PORT), Handler) diff --git a/deploy/cw2/test_serve.py b/deploy/cw2/test_serve.py new file mode 100644 index 0000000..d6493e7 --- /dev/null +++ b/deploy/cw2/test_serve.py @@ -0,0 +1,24 @@ +import unittest + +import serve + + +class ListenerAuthTest(unittest.TestCase): + def test_network_listener_requires_token(self): + for host in ("0.0.0.0", "192.168.1.105", "::"): + for token in ("", " "): + with self.subTest(host=host, token=token): + with self.assertRaises(ValueError): + serve.validate_listener_auth(host, token) + + def test_loopback_listener_may_be_explicitly_unauthenticated(self): + for host in ("127.0.0.1", "::1", "localhost"): + with self.subTest(host=host): + serve.validate_listener_auth(host, "") + + def test_network_listener_accepts_token(self): + serve.validate_listener_auth("0.0.0.0", "secret") + + +if __name__ == "__main__": + unittest.main() diff --git a/deploy/mavend.json b/deploy/mavend.json index 349d163..8262054 100644 --- a/deploy/mavend.json +++ b/deploy/mavend.json @@ -31,14 +31,14 @@ "rule — three things in series that have each failed once, and when they do", "a sev4 nudge has nowhere to go. ntfy shares none of them: it is reached", "directly, no relay.", - "It is not only a spare. The routing table sends sev3-away and away", - "reminders here and NOWHERE else, so with this block absent those two", - "routes hit a nil sink and vanish without a log or an outbox row.", + "It is the preferred away reach. Reminders fall back to Telegram in order,", + "and every missing reach is recorded in the outbox rather than disappearing.", "The credential is an ntfy access token, scoped write-only to this one", "topic, so a popped sink can push to it and cannot read it back. Set it in", "deploy/telegram.env beside the telegram secrets; that file is gitignored." ], "ntfy": { + "disabled": true, "base_url": "https://ntfy.kvmx.ru", "topic": "maven", "token": "${NTFY_TOKEN}" @@ -76,7 +76,9 @@ "resident model without saying so. Deleting this block restores exactly", "the behaviour homesrv had before it existed.", "Addressed by LAN address, not container name: mavgpud runs on another", - "machine and there is no shared docker network to name it on." + "machine and there is no shared docker network to name it on.", + "model_disabled keeps only this model arm dark until MAVEN_GPU_TOKEN is", + "provisioned; the independently authenticated STT arm below stays live." ], "//workstation.stt": [ "CrisperWhisper 2.0 turbo on the same machine, a second service on port", @@ -93,6 +95,7 @@ "The token is what stops anything on the LAN posting audio to that port." ], "workstation": { + "model_disabled": true, "url": "http://192.168.1.105:8080", "token": "${MAVEN_GPU_TOKEN}", "probe": "15s", diff --git a/deploy/telegram.env.example b/deploy/telegram.env.example index 3717da7..439584f 100644 --- a/deploy/telegram.env.example +++ b/deploy/telegram.env.example @@ -16,6 +16,34 @@ NTFY_TOKEN= # the card. Any long random string; mint one with: # openssl rand -hex 32 # The same value goes in a file on workpc, named by token_file in -# deploy/mavgpud.json. Unset here and every workstation turn falls back to the -# resident model, because mavgpud answers 401 and Maven reads that as down. +# deploy/mavgpud.json. Empty is valid only while workstation.model_disabled is +# true; enabling the LAN model arm without it fails Maven's config validation. MAVEN_GPU_TOKEN= + +# Bearer token used by Maven when it sends audio to the workstation's +# CrisperWhisper service. The matching CW2_TOKEN is installed on workpc; these +# two values must be identical. Leaving both unset is only safe while that +# remote STT service is bound to loopback or its STT block is disabled. +MAVEN_STT_TOKEN= + +# Home Assistant is currently explicitly disabled in deploy/mavend.json. Keep +# this name in the canonical inventory so enabling that block cannot turn into +# a valid-looking configuration with an expanded-empty token. +HA_TOKEN= + +# docker compose reads these from the repository-root `.env`, before it starts +# containers; they are listed here even though their destination differs so +# this remains the complete deployment inventory. Copy both lines to `.env`. +# Enabling with an empty token fails startup; false is the explicit dark state. +# MAVEN_AMBIENT_ENABLED=false +# MAVEN_AMBIENT_TOKEN= + +# Workstation-side counterpart to MAVEN_STT_TOKEN. Install this line in the +# EnvironmentFile named by deploy/mavgpud.service (currently +# %h/Programs/cw2-service/cw2.env), not in homesrv's telegram.env. +# CW2_TOKEN= + +# The encrypted database key has its own narrowly mounted file, +# deploy/db_key.env, and its own generator in deploy/README.md. It is named +# here so an operator auditing this one inventory sees every deployed secret. +# MAVEN_DB_KEY= diff --git a/docker-compose.yml b/docker-compose.yml index 9d3f9d6..0d499db 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -97,15 +97,17 @@ services: <<: *image # voice.bind is 0.0.0.0:9100 in deploy/mavend.json so mavweb can reach it # cross-container. Verified 2026-07-06. - # -ambient-token turns on POST /api/ambient (Vikunja #126): the phone posts - # notification text, mavweb keeps only a meeting time. Empty ⇒ no route at - # all, which is what a missing MAVEN_AMBIENT_TOKEN gives. The value comes - # from the gitignored .env docker compose reads for interpolation, NOT from - # an env_file — flags are interpolated before any service env exists. + # -ambient-enabled turns on POST /api/ambient (Vikunja #126): the phone posts + # notification text, mavweb keeps only a meeting time. The explicit enabled + # flag prevents an unset token from looking like an intentional dark state. + # Both values come from the gitignored .env docker compose reads for + # interpolation, NOT from an env_file — flags are interpolated before any + # service env exists. # Weakness worth naming: mavweb takes this as a flag, so it is visible in # `ps` inside this container, unlike the zenmoney and IMAP secrets which are # read from files. command: ["mavweb", "-addr", ":9201", "-voice", "mavend:9100", "-core", "/run/maven/mavend.sock", + "-ambient-enabled", "${MAVEN_AMBIENT_ENABLED:-false}", "-ambient-token", "${MAVEN_AMBIENT_TOKEN:-}", "-nexus", "http://nexus:9740", "-praxis", "http://praxis:8989", "-hexis", "http://hexis:9741"] depends_on: [mavend] diff --git a/docs/caveats/CLAUDE.md b/docs/caveats/CLAUDE.md index a024518..a56d9fd 100644 --- a/docs/caveats/CLAUDE.md +++ b/docs/caveats/CLAUDE.md @@ -22,27 +22,26 @@ caveat is the pointer between them plus the trigger. Every entry below came from the 2026-08-10 deep audit (`docs/evals/2026-08-10-repo-audit.md`), except the last, which came from wiring -the gate the audit asked for. Nine of the twenty findings are fixed and have no +the gate the audit asked for. Twelve of the twenty findings are fixed and have no entry. The unauthenticated mavgpud proxy was V-673. The 20 reachable advisories in the toolchain and `x/text` were V-682. The missing analyzers were V-694, and what they now report is the baseline entry under V-701. The invalid STT and weather responses were V-675/V-676, the stuck TCP close was V-679, and the unbounded PTT path was V-688. The two unguarded invariants were V-692 and V-693, and their guards are described in -`docs/routing.md`. +`docs/routing.md`. The complete secret inventory and fail-closed integration +validation were V-691. The atomic reminder transition was V-678. The traceable, +sanitized mavweb problem contract was V-689. | limit | severity | | --- | --- | | [Anyone past the proxy can enroll a passkey](security.md#enrollment) | high | | [Passkey credentials are rewritten in place](security.md#credentials) | medium | | [Dialogue persistence errors are swallowed](storage.md#dialogue) | medium | -| [The reminder transition is a lost update](storage.md#reminders) | medium | | [A recall miss scans two whole tables](storage.md#recall) | medium | -| [mavweb errors cannot be traced](transport.md#errors) | medium | | [Fact enrichment is a 20-call serial waterfall](workers.md#enrichment) | medium | | [A suppressed nudge is phrased anyway](workers.md#nudges) | medium | | [Committed absolute paths pin the build to this box](config.md#paths) | medium | -| [The env example omits deployed variables](config.md#secrets) | medium | | [The analyzers pass against a baseline, not zero](dependencies.md#baseline) | medium | | [Domain packages depend on store and IPC types](layering.md#dtos) | low | | [Eleven symbols are unreachable](layering.md#deadcode) | low | diff --git a/docs/caveats/config.md b/docs/caveats/config.md index 41ccd5b..8fdd67e 100644 --- a/docs/caveats/config.md +++ b/docs/caveats/config.md @@ -8,14 +8,3 @@ workstation model and Python paths. Vendoring hides the `go.mod` problem for an ordinary build. `-mod=mod`, `go mod tidy` and a fresh checkout all fail. Revisit when: anyone clones this repo elsewhere, or a `tidy` is needed. Workaround: build only from this checkout, with the vendor directory. - -## The env example omits deployed variables [#691] {#secrets} - -Costs: a fresh deploy can lose remote speech-to-text or ambient authentication -and run on fallback behaviour with an apparently valid config. Three variables -are referenced and undocumented: `MAVEN_STT_TOKEN`, `MAVEN_AMBIENT_TOKEN` and -`CW2_TOKEN`. V-673 added `MAVEN_GPU_TOKEN` to the example. -It is not silent. The loader logs which variables were unset and says whatever -they configure is off. What is missing is a startup failure. -Revisit when: the box is redeployed from scratch, or a new secret is added. -Workaround: read that log line at startup. diff --git a/docs/caveats/storage.md b/docs/caveats/storage.md index 3f139dc..eab4d06 100644 --- a/docs/caveats/storage.md +++ b/docs/caveats/storage.md @@ -11,14 +11,6 @@ Revisit when: a restart is reported as losing context. Sooner if a turn starts reading dialogue rows back to him. Workaround: none. The failure is invisible from outside. -## The reminder transition is a lost update [#678] {#reminders} - -Costs: a concurrent fire and cancel both succeed and the last writer wins. -Medium today because cancellation has no surface. High the moment V-622 adds -one, and V-622 does not describe this invariant. -Revisit when: V-622 starts, whichever comes first. -Workaround: none, but the window is small while nothing can cancel. - ## A recall miss scans two whole tables [#681] {#recall} Costs: every missed recall reads all of `memory_vectors` and then decodes and diff --git a/docs/caveats/transport.md b/docs/caveats/transport.md deleted file mode 100644 index 911cad3..0000000 --- a/docs/caveats/transport.md +++ /dev/null @@ -1,12 +0,0 @@ -# Transport - -The HTTP and socket seams. What a client can do to them, and what a shutdown -can do to us. - -## mavweb errors cannot be traced [#689] {#errors} - -Costs: some handlers return the raw internal error, which discloses internals. -Others return a generic one with no identifier, which cannot be joined to its -log line. There is no request-id middleware to join them. -Revisit when: a reported UI failure cannot be found in the log. -Workaround: read the log by timestamp. diff --git a/docs/deployment.md b/docs/deployment.md index 3b0ed05..58a7ba3 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -78,7 +78,10 @@ vocabulary size. CW2's 51897 tokens shift seven special token ids. So CW2 is its own transformers service on port 8081 (`deploy/cw2/serve.py`). `stt.HTTPTranscriber` posts raw PCM to it with a bearer token, because audio is the most sensitive thing that crosses this seam. The switch is `workstation.stt` -in `deploy/mavend.json`, and deleting the block sends every utterance to mavsttd. +in `deploy/mavend.json`. A LAN URL with an empty token fails config validation; +`workstation.stt.disabled: true` is the STT-only dark state; +`workstation.disabled: true` disables both remote arms. The model has its own +`model_disabled` switch because CW2 can remain live without the model proxy. **mavgpud runs that service as a second child.** This is not an optimisation. CW2 is a ROCm process on the same card, so it registers on the KFD like any @@ -208,6 +211,18 @@ rule. ## Web UI conventions +Every HTTP response carries a server-generated `X-Request-ID`. Error responses +use `application/problem+json` with three fields: a sanitized `error`, a stable +low-cardinality `code`, and that `request_id`. `writeProblem` is the only error +writer: it records the full wrapped error beside the same ID in the mavweb log, +while implementation paths, transport messages and credentials never reach the +browser. A page that can usefully degrade instead of failing keeps HTTP 200 and +renders the same code/request reference beside its sanitized panel error. + +The `/ecosystem` panel uses its HTTP request ID as `X-Correlation-ID` on its +direct Nexus, Praxis and Hexis reads. Calls routed through mavend retain +mavend's action-scoped correlation contract instead. + Server-rendered pages share `cmd/mavweb/static/ui.css` (served at `/ui.css`) and the shell partial in `cmd/mavweb/shell.html`. A page opens with `{{template "shellTop" ""}}` and closes with `{{template "shellBottom"}}`, diff --git a/docs/offload.md b/docs/offload.md index f51fc1c..ee5f19e 100644 --- a/docs/offload.md +++ b/docs/offload.md @@ -113,8 +113,11 @@ read `/slots`, which returns other callers' prompts. mavgpud now reads network without one. Downgrading to loopback instead would look safe and take the model arm down. Maven sends the same token from `workstation.token`, on the completion and on the `/health` probe alike. An unsigned probe answers 401, -which Pair reads as a busy card, so a missing token degrades to the resident -model rather than breaking a turn. The proxy also +which Pair reads as a busy card, but Maven now rejects a live LAN workstation +block with a missing token at startup: fallback must describe a temporarily +unavailable machine, not hide a broken secret source. `model_disabled: true` +is the explicit resident-only state and does not take CW2 down with it. The +proxy also allowlists the five paths Maven calls, so a leaked token buys the model API and not llama-server's admin surface. diff --git a/docs/operations.md b/docs/operations.md index 7b9dd63..4440ba4 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -67,14 +67,30 @@ Config path: `~/.config/maven/mavend.json`. Full example with all options. Omit the `embedder` block entirely to use the deterministic HashEmbedder floor (no ML, no ONNX runtime dependency). Useful for testing or low-resource setups. -`${NTFY_TOKEN}` and the `${TELEGRAM_*}` vars are expanded from `deploy/telegram.env`, which is gitignored. Copy `deploy/telegram.env.example` and fill it in. Mint a scoped token rather than reusing an admin one. It needs write access to the `maven` topic and nothing else: +`deploy/telegram.env.example` is the canonical inventory for every deployed +secret, including values whose destination is the root `.env`, the workpc CW2 +environment file, or `deploy/db_key.env`. Copy values only to the destination +named beside them; never commit the populated files. Maven expands the homesrv +sink, workstation, and Home Assistant variables from `deploy/telegram.env`. + +Every written integration block is either explicitly disabled or live. A live +Telegram, ntfy, LAN workstation model/STT, Home Assistant, ambient, CW2, or +encrypted-database configuration with an empty credential fails startup. This +keeps a missing env file from quietly becoming fallback behavior. The deployed +ntfy is currently `disabled: true`, and the workstation model arm is +`model_disabled: true`; the separately credentialed CW2 STT arm remains live. +Remove a dark-state flag only after provisioning that arm's credential. + +Mint a scoped ntfy token rather than reusing an admin one. It needs write access +to the `maven` topic and nothing else: ```sh ntfy access maven maven write-only ntfy token add --expires=never maven ``` -Deleting the `ntfy` block turns the reach off, and that is not a no-op. The routing table sends sev3-away nudges and away reminders to ntfy and nowhere else. With no sink wired they hit a nil and vanish, leaving no log line and no `delivery_attempts` row (V-649). +`disabled: true` keeps a documented ntfy block dark. When enabled, reminders +try ntfy and fall through to Telegram; delivery attempts record each reach. ## mavsttd — STT worker (optional, remote whisper.cpp) diff --git a/internal/config/config.go b/internal/config/config.go index 3f5e928..41d9e8a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -332,12 +332,11 @@ func Load(path string) (*Config, error) { // file committed to git. expanded, missing := expandEnv(string(b)) if len(missing) > 0 { - // An unset variable expands to "", which every block reads as "not - // configured" and none of them complains about. That is the intended - // behaviour and it stays: CI parses this same file with no secrets - // present. What was missing is the line telling the operator which - // capability he just turned off by forgetting an env file. - log.Printf("config: %s references unset environment variables %v — those settings are empty, so whatever they configure is off", path, missing) + // Expansion happens before typed validation. A disabled block may carry + // empty placeholders; an enabled block that needs one of these values is + // rejected below. Log the names too so a failed deploy says which secret + // source was absent without ever printing a value. + log.Printf("config: %s references unset environment variables %v — expanded them to empty; enabled integrations will reject missing credentials", path, missing) } var c Config if err := json.Unmarshal([]byte(expanded), &c); err != nil { @@ -459,6 +458,12 @@ func (c *Config) validate() error { if err := c.validateTelegram(); err != nil { return err } + if err := c.validateNtfy(); err != nil { + return err + } + if err := c.validateWorkstation(); err != nil { + return err + } return nil } @@ -468,16 +473,23 @@ func (c *Config) validate() error { // answers nothing — the failure is invisible from the chat. Same shape as // validateNetScan: fail the config rather than the turn. func (c *Config) validateTelegram() error { - if c.Telegram == nil || !c.Telegram.Intake { + if c.Telegram == nil || c.Telegram.Disabled { return nil } - // An unset ${TELEGRAM_*} expands to empty, and the daemon already reads an - // empty token or chat id as telegram not being wired at all. Validating a - // block that wires nothing would fail a box that merely has no bot. - if c.Telegram.BotToken == "" || c.Telegram.ChatID == "" { + if err := telegramsink.Validate(*c.Telegram); err != nil { + return err + } + if c.Telegram.Intake { + return telegramsink.ValidateIntakeChatID(c.Telegram.ChatID) + } + return nil +} + +func (c *Config) validateNtfy() error { + if c.Ntfy == nil { return nil } - return telegramsink.ValidateIntakeChatID(c.Telegram.ChatID) + return ntfysink.Validate(*c.Ntfy) } // DBEncryptionKey resolves the at-rest encryption key: DBKeyEnv (if set) wins diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 75750e1..7008e47 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -435,10 +435,21 @@ func TestNormaliseDropsAddresslessWorkstation(t *testing.T) { } } +func TestModelDisabledKeepsLiveSTT(t *testing.T) { + p := writeConfig(t, `{"workstation":{"model_disabled":true,"stt":{"url":"http://192.168.1.105:8081/transcribe","token":"secret"}}}`) + c, err := Load(p) + if err != nil { + t.Fatalf("Load: %v", err) + } + if c.Workstation == nil || c.Workstation.Stt == nil || !c.Workstation.ModelDisabled { + t.Fatalf("model-only dark state did not preserve STT: %+v", c.Workstation) + } +} + // The health endpoint defaults to the supervisor's, not llama-server's: mavgpud // answers 503 while the card is held, and that refusal is the whole signal. func TestNormaliseFillsWorkstationDefaults(t *testing.T) { - c := &Config{Workstation: &WorkstationConfig{URL: "http://192.168.1.105:8080/"}} + c := &Config{Workstation: &WorkstationConfig{URL: "http://192.168.1.105:8080/", Token: "secret"}} c.applyDefaults() if c.Workstation == nil { t.Fatal("dropped a usable workstation block") @@ -460,6 +471,7 @@ func TestNormaliseKeepsExplicitWorkstationHealth(t *testing.T) { c := &Config{Workstation: &WorkstationConfig{ URL: "http://192.168.1.105:8080", Health: "http://192.168.1.105:9000/ready", + Token: "secret", }} c.applyDefaults() if got, want := c.Workstation.Health, "http://192.168.1.105:9000/ready"; got != want { @@ -467,6 +479,112 @@ func TestNormaliseKeepsExplicitWorkstationHealth(t *testing.T) { } } +func TestWorkstationLANEndpointsRequireSecrets(t *testing.T) { + for _, tc := range []struct { + name string + body string + }{ + {"model", `{"workstation":{"url":"http://192.168.1.105:8080"}}`}, + {"stt", `{"workstation":{"url":"http://127.0.0.1:8080","stt":{"url":"http://192.168.1.105:8081/transcribe"}}}`}, + } { + t.Run(tc.name, func(t *testing.T) { + if _, err := Load(writeConfig(t, tc.body)); err == nil { + t.Fatal("Load accepted an enabled LAN endpoint with no secret") + } + }) + } +} + +func TestWorkstationDisabledAllowsEmptySecrets(t *testing.T) { + p := writeConfig(t, `{"workstation":{"disabled":true,"url":"http://192.168.1.105:8080","stt":{"url":"http://192.168.1.105:8081/transcribe"}}}`) + c, err := Load(p) + if err != nil { + t.Fatalf("Load: %v", err) + } + if c.Workstation != nil { + t.Fatalf("disabled workstation survived normalisation: %+v", c.Workstation) + } +} + +func TestWorkstationArmsMayBeDisabledIndependently(t *testing.T) { + modelOff := writeConfig(t, `{"workstation":{"model_disabled":true,"url":"http://192.168.1.105:8080","stt":{"url":"http://127.0.0.1:8081/transcribe"}}}`) + if _, err := Load(modelOff); err != nil { + t.Fatalf("Load model-disabled config: %v", err) + } + + sttOff := writeConfig(t, `{"workstation":{"url":"http://127.0.0.1:8080","stt":{"disabled":true,"url":"http://192.168.1.105:8081/transcribe"}}}`) + c, err := Load(sttOff) + if err != nil { + t.Fatalf("Load STT-disabled config: %v", err) + } + if c.Workstation == nil || c.Workstation.Stt != nil { + t.Fatalf("STT-only dark state changed model arm: %+v", c.Workstation) + } +} + +func TestWorkstationLoopbackMayRunWithoutSecret(t *testing.T) { + p := writeConfig(t, `{"workstation":{"url":"http://127.0.0.1:8080","stt":{"url":"http://localhost:8081/transcribe"}}}`) + if _, err := Load(p); err != nil { + t.Fatalf("Load loopback development endpoints: %v", err) + } +} + +func TestTelegramLiveBlockRequiresBothSecrets(t *testing.T) { + for _, body := range []string{ + `{"telegram":{"chat_id":"42"}}`, + `{"telegram":{"bot_token":"token"}}`, + } { + if _, err := Load(writeConfig(t, body)); err == nil { + t.Fatalf("Load accepted enabled Telegram block: %s", body) + } + } +} + +func TestTelegramDisabledAllowsEmptySecrets(t *testing.T) { + if _, err := Load(writeConfig(t, `{"telegram":{"disabled":true}}`)); err != nil { + t.Fatalf("Load disabled Telegram: %v", err) + } +} + +func TestNtfyLiveBlockRequiresSecret(t *testing.T) { + p := writeConfig(t, `{"ntfy":{"base_url":"https://ntfy.example","topic":"maven"}}`) + if _, err := Load(p); err == nil { + t.Fatal("Load accepted enabled ntfy block with no credential") + } +} + +func TestNtfyDisabledAllowsEmptySecret(t *testing.T) { + p := writeConfig(t, `{"ntfy":{"disabled":true,"base_url":"https://ntfy.example","topic":"maven"}}`) + if _, err := Load(p); err != nil { + t.Fatalf("Load disabled ntfy: %v", err) + } +} + +func TestSmartHomeLiveBlockRequiresSecret(t *testing.T) { + p := writeConfig(t, `{"smarthome":{"enabled":true,"provider":"homeassistant","url":"http://192.168.1.50:8123"}}`) + if _, err := Load(p); err == nil { + t.Fatal("Load accepted enabled Home Assistant block with no token") + } +} + +func TestSmartHomeDisabledAllowsEmptySecret(t *testing.T) { + p := writeConfig(t, `{"smarthome":{"enabled":false,"provider":"homeassistant","url":"http://192.168.1.50:8123"}}`) + if _, err := Load(p); err != nil { + t.Fatalf("Load disabled Home Assistant: %v", err) + } +} + +func TestConfiguredDBKeyEnvCannotBeEmpty(t *testing.T) { + t.Setenv("MAVEN_TEST_DB_KEY", "") + c := &Config{DBKeyEnv: "MAVEN_TEST_DB_KEY"} + if _, err := c.DBEncryptionKey(); err == nil { + t.Fatal("configured empty database key did not fail startup resolution") + } + if key, err := (&Config{}).DBEncryptionKey(); err != nil || key != nil { + t.Fatalf("explicit plaintext config = (%v, %v), want (nil, nil)", key, err) + } +} + func TestTelegramIntakeRefusesNamedChat(t *testing.T) { // The push half accepts an @channelusername and the intake half cannot use // one, so a box with both boots clean and answers nothing. Refuse the diff --git a/internal/config/deployconfig_test.go b/internal/config/deployconfig_test.go index e888ba7..090bf55 100644 --- a/internal/config/deployconfig_test.go +++ b/internal/config/deployconfig_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "strings" "testing" ) @@ -14,14 +15,20 @@ import ( // a config change and a code change have to agree here or the suite is red. // // The ${VAR} expansions come from a gitignored deploy/telegram.env that is not -// present in CI. An unset var expands to the empty string, which is exactly the -// "not configured" state every block already has to handle, so the parse is -// still meaningful without the secrets. +// present in CI. Live blocks reject empty secrets; this test supplies inert +// Telegram values and verifies that every credential-less block is explicitly +// disabled. func TestDeployConfigLoads(t *testing.T) { path := filepath.Join("..", "..", "deploy", "mavend.json") if _, err := os.Stat(path); err != nil { t.Skipf("no deploy config at %s: %v", path, err) } + // Live blocks fail on expanded-empty credentials. CI provides inert values + // so this test exercises the committed shape; explicitly disabled blocks + // (ntfy, workstation and Home Assistant) require none. + t.Setenv("TELEGRAM_BOT_TOKEN", "test-token") + t.Setenv("TELEGRAM_CHAT_ID", "-1001234567890") + t.Setenv("MAVEN_STT_TOKEN", "test-token") cfg, err := Load(path) if err != nil { t.Fatalf("Load(%s): %v", path, err) @@ -46,11 +53,10 @@ func TestDeployConfigLoads(t *testing.T) { t.Error("router threshold did not get its default") } - // The second reach (V-649). Deleting this block is how you turn ntfy off, - // so its absence has to be loud: sev3-away nudges and away reminders route - // to ntfy and to nothing else, and a nil sink drops them with no log and no - // outbox row. The token is a ${VAR} that CI cannot resolve, so this checks - // the wiring and not the credential. + // The second reach (V-649). The token is a ${VAR} that CI cannot resolve, so + // the committed deployment makes the dark state explicit. Removing disabled + // without provisioning a credential makes runtime wiring fail startup; it + // can never silently publish anonymously. if cfg.Ntfy == nil { t.Fatal("deploy config has no ntfy block — sev3-away and away reminders " + "would have nowhere to land, and would vanish silently rather than fail") @@ -58,4 +64,47 @@ func TestDeployConfigLoads(t *testing.T) { if cfg.Ntfy.BaseURL == "" || cfg.Ntfy.Topic == "" { t.Errorf("ntfy block is incomplete: base_url=%q topic=%q", cfg.Ntfy.BaseURL, cfg.Ntfy.Topic) } + if !cfg.Ntfy.Disabled { + t.Fatal("deploy ntfy reach has no checked-in credential and must remain explicitly disabled") + } + if cfg.Workstation == nil || !cfg.Workstation.ModelDisabled || cfg.Workstation.Stt == nil { + t.Fatalf("deploy must disable only its uncredentialed model arm and retain authenticated STT: %+v", cfg.Workstation) + } +} + +func TestCanonicalDeployEnvExampleNamesEverySecret(t *testing.T) { + path := filepath.Join("..", "..", "deploy", "telegram.env.example") + b, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + example := string(b) + references := map[string]bool{ + // These two live outside ${...}: DBKeyEnv names an environment variable + // as JSON data, while the separately deployed Python child reads its own + // environment directly. + "CW2_TOKEN": true, + "MAVEN_DB_KEY": true, + } + for _, source := range []string{ + filepath.Join("..", "..", "deploy", "mavend.json"), + filepath.Join("..", "..", "docker-compose.yml"), + } { + raw, err := os.ReadFile(source) + if err != nil { + t.Fatalf("read deploy source %s: %v", source, err) + } + _ = os.Expand(string(raw), func(expr string) string { + // Compose supports ${NAME:-default}; os.Expand deliberately hands the + // full braced expression to this callback. + name, _, _ := strings.Cut(expr, ":-") + references[name] = true + return "" + }) + } + for name := range references { + if !strings.Contains(example, name+"=") { + t.Errorf("canonical deploy env example omits %s", name) + } + } } diff --git a/internal/config/workstation.go b/internal/config/workstation.go index 6b34d2a..c6f153f 100644 --- a/internal/config/workstation.go +++ b/internal/config/workstation.go @@ -1,6 +1,8 @@ package config import ( + "fmt" + "net" "net/url" "strings" "time" @@ -18,6 +20,13 @@ import ( // Only the prompt crosses the LAN, and the workstation is not "the box". The // rules in CLAUDE.md about what may leave still apply. type WorkstationConfig struct { + // Disabled keeps both written workpc arms explicitly dark. ModelDisabled is + // the narrower switch: CW2 STT may remain live while the large-model + // supervisor has no provisioned client token. + Disabled bool `json:"disabled,omitempty"` + + ModelDisabled bool `json:"model_disabled,omitempty"` + // URL — where mavgpud listens, e.g. "http://192.168.1.105:8080". Empty ⇒ // the whole block is normalised to nil and nothing probes anything. URL string `json:"url,omitempty"` @@ -30,8 +39,8 @@ type WorkstationConfig struct { // Token — the bearer credential mavgpud requires, expanded from the // environment like every other secret here. It is what stops anything on // the LAN spending the card, so a URL that is not loopback needs one. - // Wrong or missing reads as a workstation that is down, and Maven falls - // back to the resident model. + // A missing token on a LAN URL fails config validation. Loopback development + // endpoints may omit it. Token string `json:"token,omitempty"` // Probe — how often admission is re-checked. 0 ⇒ DefaultWorkstationProbe. @@ -60,6 +69,10 @@ type WorkstationConfig struct { // the ggml-small.bin homesrv loads // (docs/evals/2026-08-09-crisperwhisper2-russian-wer.md). type WorkstationSttConfig struct { + // Disabled keeps the written CW2 endpoint dark without also disabling the + // independently authenticated model supervisor. + Disabled bool `json:"disabled,omitempty"` + // URL — the transcribe endpoint, e.g. // "http://192.168.1.105:8081/transcribe". Empty ⇒ the block is normalised // to nil and mavsttd takes every turn. @@ -97,29 +110,85 @@ const ( // model: an unconfigured workstation is the default deploy and must be // indistinguishable from today. func (c *Config) normaliseWorkstation() { - if c.Workstation != nil && strings.TrimSpace(c.Workstation.URL) == "" { + if c.Workstation != nil && c.Workstation.Disabled { c.Workstation = nil } if c.Workstation == nil { return } w := c.Workstation - if strings.TrimSpace(w.Health) == "" { - w.Health = strings.TrimRight(w.URL, "/") + "/health" + // Preserve the historical empty-block meaning. A live STT sub-block makes + // the parent non-empty; in that case omitting the model URL is an error + // unless model_disabled states the operator's intent. + if !w.ModelDisabled && strings.TrimSpace(w.URL) == "" && + (w.Stt == nil || w.Stt.Disabled || strings.TrimSpace(w.Stt.URL) == "") { + c.Workstation = nil + return } - if w.Probe <= 0 { - w.Probe = Duration(DefaultWorkstationProbe) - } - if w.Timeout <= 0 { - w.Timeout = Duration(DefaultWorkstationTimeout) + if !w.ModelDisabled { + if strings.TrimSpace(w.Health) == "" { + w.Health = strings.TrimRight(w.URL, "/") + "/health" + } + if w.Probe <= 0 { + w.Probe = Duration(DefaultWorkstationProbe) + } + if w.Timeout <= 0 { + w.Timeout = Duration(DefaultWorkstationTimeout) + } } normaliseWorkstationStt(w) + if w.ModelDisabled && w.Stt == nil { + c.Workstation = nil + } +} + +// validateWorkstation rejects a live LAN endpoint without its bearer secret. +// Loopback remains useful for local development without manufacturing a secret; +// malformed or non-HTTP endpoints are rejected before any probe starts. +func (c *Config) validateWorkstation() error { + if c.Workstation == nil { + return nil + } + w := c.Workstation + if !w.ModelDisabled { + if err := validateWorkstationEndpoint("workstation.url", w.URL, w.Token); err != nil { + return err + } + if err := validateWorkstationEndpoint("workstation.health", w.Health, w.Token); err != nil { + return err + } + } + if w.Stt != nil { + if err := validateWorkstationEndpoint("workstation.stt.url", w.Stt.URL, w.Stt.Token); err != nil { + return err + } + if err := validateWorkstationEndpoint("workstation.stt.health", w.Stt.Health, w.Stt.Token); err != nil { + return err + } + } + return nil +} + +func validateWorkstationEndpoint(name, raw, token string) error { + u, err := url.Parse(raw) + if err != nil || u.Host == "" || (u.Scheme != "http" && u.Scheme != "https") { + return fmt.Errorf("%s must be an absolute http(s) URL", name) + } + host := strings.TrimSpace(u.Hostname()) + loopback := strings.EqualFold(host, "localhost") + if ip := net.ParseIP(host); ip != nil { + loopback = ip.IsLoopback() + } + if !loopback && strings.TrimSpace(token) == "" { + return fmt.Errorf("%s is not loopback, so its token is required while enabled", name) + } + return nil } // normaliseWorkstationStt applies the speech-to-text block's defaults. No // address, no remote: mavsttd then takes every utterance, which is today. func normaliseWorkstationStt(w *WorkstationConfig) { - if w.Stt != nil && strings.TrimSpace(w.Stt.URL) == "" { + if w.Stt != nil && (w.Stt.Disabled || strings.TrimSpace(w.Stt.URL) == "") { w.Stt = nil } if w.Stt == nil { diff --git a/internal/delivery/channel.go b/internal/delivery/channel.go index 8da5d20..ce7e891 100644 --- a/internal/delivery/channel.go +++ b/internal/delivery/channel.go @@ -20,7 +20,8 @@ // // - reminders are a SEPARATE class — two delivery paths. reminders bypass // the restraint gate ("wake me 7" fires in quiet hours; that's the point). -// snooze still applies. voice when present, ntfy when away. fire once. +// snooze still applies. voice when present; when away, try ntfy then +// telegram as alternatives and stop after the first success. fire once. // // Architecture mirrors the loop's gather/pure split: the routing table is a // PURE function of (severity, presence); the Dispatcher holds the impure Sinks @@ -43,6 +44,12 @@ import ( // to import the voice package. var ErrVoiceNoSession = errors.New("delivery: voice: no live session") +// ErrPermanent is the class of transport failures that waiting cannot repair: +// a revoked credential or an endpoint that refuses this sender. Dispatchers +// may still try a different reach for the same message, but the failed reach +// must not be put on an automatic retry clock until its configuration changes. +var ErrPermanent = errors.New("delivery: permanent failure") + // Channel — one delivery transport. Drop is an explicit no-op (the routing // table chose to suppress, which is a decision, not a failure — "a missed // water nudge is noise"). a nil Sink for a wired channel is a daemon config @@ -91,8 +98,11 @@ func ChannelsFor(sev loop.Severity, presence store.Bucket) []Channel { // ChannelsForReminder — reminders are a SEPARATE class that bypasses the gate. // "wake me 7" fires in quiet hours; that's the point. presence still routes -// reachability: voice when present, ntfy when away. fires once — no repeat -// (repeat-til-ack is a sev4 ops-hard behavior, not a reminder behavior). +// reachability: voice when present, then an ordered ntfy→telegram alternative +// chain when away. The dispatcher stops after the first successful alternative, +// so a reminder still fires once rather than being broadcast on both channels. +// There is no repeat (repeat-til-ack is a sev4 ops-hard behavior, not a reminder +// behavior). // // reminders don't carry a Severity — they're user-stated future intent, not // loop-derived insistence. the routing is presence-only: reachability without @@ -100,7 +110,7 @@ func ChannelsFor(sev loop.Severity, presence store.Bucket) []Channel { // per-reminder override, not a table entry. func ChannelsForReminder(presence store.Bucket) []Channel { if presence == store.Away { - return []Channel{ChannelNtfy} + return []Channel{ChannelNtfy, ChannelTelegram} } return []Channel{ChannelVoice} } diff --git a/internal/delivery/dispatcher.go b/internal/delivery/dispatcher.go index 085e362..832903d 100644 --- a/internal/delivery/dispatcher.go +++ b/internal/delivery/dispatcher.go @@ -22,13 +22,25 @@ type NudgeRecorder interface { RecordNudge(ctx context.Context, rule, channel, message string, ts time.Time) (int64, error) } -// ReminderCompleter — the seam the store implements. For one-shot reminders: -// pending → fired after successful delivery. For recurring reminders (with -// cron): reschedule after successful delivery. A failed send does NOT mark or -// reschedule it (it stays pending; the next tick re-delivers). +// ReminderCompleter — the seam the store implements. Every original represented +// by one external delivery is completed in one transaction. That matters for a +// collapsed catch-up bundle: partially firing the originals would make the +// next tick repeat a presentation that the user already received. type ReminderCompleter interface { - MarkReminder(ctx context.Context, id int64, status string) error - RescheduleReminder(ctx context.Context, id int64, now time.Time) error + CompleteReminderDelivery(ctx context.Context, originals []store.Reminder, now time.Time) error +} + +// DurableReminderCompleter closes the successful outbox attempt and advances +// every reminder occurrence in one local transaction. The external send and +// local commit cannot be one transaction, but the local half must be: a crash +// between `attempt=sent` and `reminder=fired` otherwise strands the reminder in +// a permanently suppressed state. +type DurableReminderCompleter interface { + CompleteSuccessfulReminderAttempt(ctx context.Context, attemptID int64, originals []store.Reminder, now time.Time) error +} + +type ReminderBlocker interface { + BlockReminderDelivery(ctx context.Context, originals []store.Reminder, now time.Time, reason string) error } // Outbox — the durable delivery ledger. Begin is recorded BEFORE the external @@ -39,7 +51,7 @@ type ReminderCompleter interface { // disabled (existing send/record behavior, unchanged — test scenarios that // don't care about crash recovery). type Outbox interface { - BeginDeliveryAttempt(ctx context.Context, kind, rule string, reminderID int64, channel, bodyHash string, now time.Time) (int64, error) + BeginDeliveryAttempt(ctx context.Context, kind, rule string, reminderID int64, deliveryGroup, channel, bodyHash string, now time.Time) (int64, error) CompleteDeliveryAttempt(ctx context.Context, id int64, status string, now time.Time) error } @@ -57,11 +69,11 @@ func bodyHash(channel Channel, body string) string { // on one attempt shouldn't block a nudge/reminder actually reaching the user // — but it does mean this attempt can't be reconciled after a crash, so it's // logged. Returns 0 (no-op id) when unrecorded. -func (d *Dispatcher) beginOutbox(ctx context.Context, kind, rule string, reminderID int64, channel Channel, body string, now time.Time) int64 { +func (d *Dispatcher) beginOutbox(ctx context.Context, kind, rule string, reminderID int64, deliveryGroup string, channel Channel, body string, now time.Time) int64 { if d.cfg.Outbox == nil { return 0 } - id, err := d.cfg.Outbox.BeginDeliveryAttempt(ctx, kind, rule, reminderID, string(channel), bodyHash(channel, body), now) + id, err := d.cfg.Outbox.BeginDeliveryAttempt(ctx, kind, rule, reminderID, deliveryGroup, string(channel), bodyHash(channel, body), now) if err != nil { log.Printf("dispatcher: outbox begin failed (send proceeds untracked): %v", err) return 0 @@ -69,16 +81,37 @@ func (d *Dispatcher) beginOutbox(ctx context.Context, kind, rule string, reminde return id } +// beginReminderOutbox is stricter than the nudge helper above. A reminder may +// be retried indefinitely, so sending it without the durable attempt row would +// reopen an unobservable duplicate window after a crash. A configured but +// unhealthy outbox therefore blocks this transport attempt; a deliberately nil +// outbox still supports small isolated test/development wiring. +func (d *Dispatcher) beginReminderOutbox(ctx context.Context, reminderID int64, deliveryGroup string, channel Channel, body string, now time.Time) (int64, error) { + if d.cfg.Outbox == nil { + return 0, nil + } + id, err := d.cfg.Outbox.BeginDeliveryAttempt( + ctx, "reminder", "", reminderID, deliveryGroup, + string(channel), bodyHash(channel, body), now, + ) + if err != nil { + return 0, fmt.Errorf("begin reminder delivery attempt: %w", err) + } + return id, nil +} + // completeOutbox records the sink's outcome for a prior beginOutbox call. // id==0 means either tracking is disabled or the begin failed — nothing to // complete either way. -func (d *Dispatcher) completeOutbox(ctx context.Context, id int64, status string, now time.Time) { +func (d *Dispatcher) completeOutbox(ctx context.Context, id int64, status string, now time.Time) error { if id == 0 || d.cfg.Outbox == nil { - return + return nil } if err := d.cfg.Outbox.CompleteDeliveryAttempt(ctx, id, status, now); err != nil { log.Printf("dispatcher: outbox complete failed: %v", err) + return err } + return nil } // PhrasedNudge — the phraser module's output for a nudge. the phraser (the @@ -155,7 +188,7 @@ func (d *Dispatcher) DispatchNudge(ctx context.Context, pn PhrasedNudge, now tim // the same afterwards. no nudges row: that table feeds the // ignored_rate signal, and a nudge nobody could see must not // count as ignored. - id := d.beginOutbox(ctx, "nudge", c.Rule.Name, 0, ch, pn.Summary, now) + id := d.beginOutbox(ctx, "nudge", c.Rule.Name, 0, "", ch, pn.Summary, now) d.completeOutbox(ctx, id, store.DeliveryDropped, now) log.Printf("dispatcher: dropped %s (sev%d, presence=%s) — routing table suppressed it", c.Rule.Name, c.Severity, c.State.Presence) @@ -176,7 +209,7 @@ func (d *Dispatcher) DispatchNudge(ctx context.Context, pn PhrasedNudge, now tim if sink == nil { continue } - attemptID := d.beginOutbox(ctx, "nudge", c.Rule.Name, 0, ch, messageForChannel(s), now) + attemptID := d.beginOutbox(ctx, "nudge", c.Rule.Name, 0, "", ch, messageForChannel(s), now) if err := safeSend(ctx, sink, s); err != nil { if errors.Is(err, ErrSinkPanicked) { // one broken sink must not eat the other channels for this @@ -226,14 +259,17 @@ func (d *Dispatcher) DispatchNudge(ctx context.Context, pn PhrasedNudge, now tim } // DispatchReminder — routes a phrased reminder. reminders bypass the gate and -// fire once (pending → fired after successful delivery). voice when present, -// ntfy when away. no repeat (reminders fire once). marks the reminder fired -// only if at least one channel succeeded — a failed send leaves it pending -// for the next tick to re-deliver. +// fire once (pending → fired after successful delivery). Voice is preferred +// when present; if it has no live session, delivery falls back to the ordered +// away alternatives. Away delivery tries ntfy, then telegram, and stops after +// the first success. A failed or unwired alternative falls through to the next +// one. If every selected alternative fails, the reminder stays pending and an +// error is returned for the tick's retry path. func (d *Dispatcher) DispatchReminder(ctx context.Context, pr PhrasedReminder, now time.Time) ([]Dispatch, error) { rd := pr.Decision channels := ChannelsForReminder(rd.State.Presence) - var out []Dispatch + var failures []error + allPermanent := true for i := 0; i < len(channels); i++ { ch := channels[i] s := Sendable{ @@ -245,62 +281,103 @@ func (d *Dispatcher) DispatchReminder(ctx context.Context, pr PhrasedReminder, n Ts: now, } s = minimalForAway(s) - sink := d.sinkFor(ch) - if sink == nil { + reminderID, deliveryGroup := reminderDeliveryIdentity(rd.Reminder) + attemptID, err := d.beginReminderOutbox(ctx, reminderID, deliveryGroup, ch, messageForChannel(s), now) + if err != nil { + allPermanent = false + failures = append(failures, err) + log.Printf("dispatcher: reminder %d delivery via %s withheld: %v", rd.Reminder.ID, ch, err) + continue + } + sink := d.sinkFor(ch) + if sink == nil { + d.completeOutbox(ctx, attemptID, store.DeliveryFailed, now) + err := fmt.Errorf("%s sink is not configured", ch) + failures = append(failures, err) + log.Printf("dispatcher: reminder %d delivery via %s failed: %v", rd.Reminder.ID, ch, err) + if ch == ChannelVoice { + channels = ChannelsForReminder(store.Away) + failures = nil + allPermanent = true + i = -1 + } continue } - attemptID := d.beginOutbox(ctx, "reminder", "", rd.Reminder.ID, ch, messageForChannel(s), now) if err := safeSend(ctx, sink, s); err != nil { - if errors.Is(err, ErrSinkPanicked) { - d.completeOutbox(ctx, attemptID, store.DeliveryFailed, now) - continue + d.completeOutbox(ctx, attemptID, store.DeliveryFailed, now) + failures = append(failures, fmt.Errorf("send %s: %w", ch, err)) + if !errors.Is(err, ErrPermanent) { + allPermanent = false } if errors.Is(err, ErrVoiceNoSession) { - // presence guess was wrong — reroute reminder to the away - // channel (ntfy). voice is the only present channel, so nothing - // has been sent yet. - d.completeOutbox(ctx, attemptID, store.DeliveryFailed, now) + // Presence was stale. Voice is the only present alternative, so + // nothing has been sent and it is safe to start the away chain. log.Printf("dispatcher: no live voice session for reminder %d, rerouting to away channels", rd.Reminder.ID) channels = ChannelsForReminder(store.Away) + failures = nil + allPermanent = true i = -1 continue } - d.completeOutbox(ctx, attemptID, store.DeliveryFailed, now) - return out, fmt.Errorf("send %s: %w", ch, err) + log.Printf("dispatcher: reminder %d delivery via %s failed: %v", rd.Reminder.ID, ch, err) + continue } - d.completeOutbox(ctx, attemptID, store.DeliverySent, now) - 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. Complete - // the collapsed originals it stands in for instead — only now, after - // a successful send, so a failed digest leaves them all pending. + out := []Dispatch{{Sendable: s}} + originals := []store.Reminder{rd.Reminder} if rd.Reminder.ID == 0 { - for _, orig := range rd.Reminder.Collapsed { - if err := d.completeReminder(ctx, orig, now); err != nil { - return out, err - } + originals = rd.Reminder.Collapsed + } + if durable, ok := d.cfg.Reminders.(DurableReminderCompleter); ok && attemptID != 0 { + if err := durable.CompleteSuccessfulReminderAttempt(ctx, attemptID, originals, now); err != nil { + return out, fmt.Errorf("commit successful reminder delivery: %w", err) + } + return out, nil + } + if err := d.completeOutbox(ctx, attemptID, store.DeliverySent, now); err != nil { + // The external sink accepted the reminder, but its durable outcome is + // ambiguous. Do not complete the reminder row: startup reconciliation + // will mark the attempt unknown and DueReminders will hold the exact + // occurrence for operator resolution rather than sending a duplicate. + return out, fmt.Errorf("record successful reminder delivery: %w", err) + } + if d.cfg.Reminders != nil { + if err := d.cfg.Reminders.CompleteReminderDelivery(ctx, originals, now); err != nil { + return out, fmt.Errorf("complete reminder delivery: %w", err) + } + } + return out, nil + } + if len(failures) == 0 { + failures = append(failures, errors.New("no delivery alternatives selected")) + allPermanent = false + } + joined := errors.Join(failures...) + if allPermanent && d.cfg.Reminders != nil { + originals := []store.Reminder{rd.Reminder} + if rd.Reminder.ID == 0 { + originals = rd.Reminder.Collapsed + } + if blocker, ok := d.cfg.Reminders.(ReminderBlocker); ok { + if err := blocker.BlockReminderDelivery(ctx, originals, now, joined.Error()); err != nil { + return nil, fmt.Errorf("block permanently undeliverable reminder %d: %w", rd.Reminder.ID, err) } - } else if err := d.completeReminder(ctx, rd.Reminder, now); err != nil { - return out, err } } - return out, nil + return nil, fmt.Errorf("deliver reminder %d: %w", rd.Reminder.ID, joined) } -// 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 +// reminderDeliveryIdentity gives the outbox both a human-readable real row id +// and the exact occurrence key used for crash suppression. A collapsed digest +// has synthetic ID zero, so its first original is the representative; the +// shared delivery group still identifies every original atomically. +func reminderDeliveryIdentity(r store.Reminder) (int64, string) { + if r.ID != 0 { + return r.ID, r.DeliveryGroup } - if err := d.cfg.Reminders.MarkReminder(ctx, r.ID, "fired"); err != nil { - return fmt.Errorf("mark reminder %d fired: %w", r.ID, err) + if len(r.Collapsed) == 0 { + return 0, "" } - return nil + return r.Collapsed[0].ID, r.Collapsed[0].DeliveryGroup } // RepeatUnacked — the daemon calls this each tick to re-send un-acked sev4 @@ -340,7 +417,7 @@ func (d *Dispatcher) RepeatUnacked(ctx context.Context, keys []string, now time. Ts: now, } s = minimalForAway(s) - attemptID := d.beginOutbox(ctx, "nudge", key, 0, ChannelTelegram, messageForChannel(s), now) + attemptID := d.beginOutbox(ctx, "nudge", key, 0, "", ChannelTelegram, messageForChannel(s), now) if err := safeSend(ctx, d.cfg.Telegram, s); err != nil { d.completeOutbox(ctx, attemptID, store.DeliveryFailed, now) if errors.Is(err, ErrSinkPanicked) { diff --git a/internal/delivery/dispatcher_test.go b/internal/delivery/dispatcher_test.go index bf1f776..83b7bf9 100644 --- a/internal/delivery/dispatcher_test.go +++ b/internal/delivery/dispatcher_test.go @@ -3,6 +3,7 @@ package delivery import ( "context" "errors" + "fmt" "testing" "time" @@ -61,9 +62,22 @@ type fakeReminderCompleter struct { status string } rescheduled []int64 + blocked []int64 + blockReason string err error } +func (f *fakeReminderCompleter) BlockReminderDelivery(_ context.Context, originals []store.Reminder, _ time.Time, reason string) error { + if f.err != nil { + return f.err + } + for _, r := range originals { + f.blocked = append(f.blocked, r.ID) + } + f.blockReason = reason + return nil +} + func (f *fakeReminderCompleter) MarkReminder(_ context.Context, id int64, status string) error { if f.err != nil { return f.err @@ -83,6 +97,23 @@ func (f *fakeReminderCompleter) RescheduleReminder(_ context.Context, id int64, return nil } +func (f *fakeReminderCompleter) CompleteReminderDelivery(_ context.Context, originals []store.Reminder, _ time.Time) error { + if f.err != nil { + return f.err + } + for _, r := range originals { + if r.Cron != "" { + f.rescheduled = append(f.rescheduled, r.ID) + continue + } + f.marked = append(f.marked, struct { + id int64 + status string + }{r.ID, store.ReminderFired}) + } + return nil +} + type fakeAck struct { acked map[string]bool lastSent map[string]time.Time @@ -152,10 +183,10 @@ func TestChannelsForReminderPresentVoice(t *testing.T) { } } -func TestChannelsForReminderAwayNtfy(t *testing.T) { +func TestChannelsForReminderAwayAlternatives(t *testing.T) { got := ChannelsForReminder(store.Away) - if len(got) != 1 || got[0] != ChannelNtfy { - t.Fatalf("reminder away: want [ntfy], got %v", got) + if len(got) != 2 || got[0] != ChannelNtfy || got[1] != ChannelTelegram { + t.Fatalf("reminder away: want [ntfy telegram], got %v", got) } } @@ -353,8 +384,9 @@ func TestDispatchReminderPresentVoice(t *testing.T) { func TestDispatchReminderAwayNtfy(t *testing.T) { ntfy := &fakeSink{} + telegram := &fakeSink{} rc := &fakeReminderCompleter{} - d := NewDispatcher(Config{Ntfy: ntfy, Reminders: rc}) + d := NewDispatcher(Config{Ntfy: ntfy, Telegram: telegram, Reminders: rc}) rd := loop.ReminderDecision{ Reminder: store.Reminder{ID: 7, Status: "pending"}, @@ -369,6 +401,9 @@ func TestDispatchReminderAwayNtfy(t *testing.T) { if len(out) != 1 || out[0].Sendable.Channel != ChannelNtfy { t.Fatalf("want 1 ntfy, got %+v", out) } + if len(telegram.sends) != 0 { + t.Fatalf("ntfy succeeded; telegram must not receive a duplicate, got %d sends", len(telegram.sends)) + } // away channel gets summary, not body if ntfy.sends[0].Summary != "wake up" { t.Fatalf("ntfy summary: want 'wake up', got %q", ntfy.sends[0].Summary) @@ -378,6 +413,203 @@ func TestDispatchReminderAwayNtfy(t *testing.T) { } } +func TestDispatchReminderAwayNtfyFailureFallsBackToTelegram(t *testing.T) { + ntfy := &fakeSink{err: errors.New("ntfy unavailable")} + telegram := &fakeSink{} + rc := &fakeReminderCompleter{} + ob := &fakeOutbox{} + d := NewDispatcher(Config{Ntfy: ntfy, Telegram: telegram, Reminders: rc, Outbox: ob}) + + out, err := d.DispatchReminder(context.Background(), PhrasedReminder{ + Decision: loop.ReminderDecision{ + Reminder: store.Reminder{ID: 8, Status: "pending"}, + State: loop.State{Now: refNow(), Presence: store.Away}, + }, + Body: "full medication detail", Summary: "take medication", + }, refNow()) + if err != nil { + t.Fatalf("dispatch: %v", err) + } + if len(out) != 1 || out[0].Sendable.Channel != ChannelTelegram || len(telegram.sends) != 1 { + t.Fatalf("want one telegram fallback, got out=%+v sends=%d", out, len(telegram.sends)) + } + if got := messageForChannel(telegram.sends[0]); got != "take medication" { + t.Fatalf("telegram fallback must retain the minimal away body, got %q", got) + } + if len(rc.marked) != 1 || rc.marked[0].id != 8 || rc.marked[0].status != "fired" { + t.Fatalf("successful fallback must complete the reminder, got %+v", rc.marked) + } + if len(ob.attempts) != 2 || ob.attempts[0].channel != "ntfy" || ob.attempts[0].status != store.DeliveryFailed || + ob.attempts[1].channel != "telegram" || ob.attempts[1].status != store.DeliverySent { + t.Fatalf("want ntfy failed then telegram sent attempts, got %+v", ob.attempts) + } +} + +func TestDispatchReminderAwayNilNtfyFallsBackVisibly(t *testing.T) { + telegram := &fakeSink{} + rc := &fakeReminderCompleter{} + ob := &fakeOutbox{} + d := NewDispatcher(Config{Telegram: telegram, Reminders: rc, Outbox: ob}) + + out, err := d.DispatchReminder(context.Background(), PhrasedReminder{ + Decision: loop.ReminderDecision{ + Reminder: store.Reminder{ID: 9, Status: "pending"}, + State: loop.State{Now: refNow(), Presence: store.Away}, + }, + Body: "full detail", Summary: "short reminder", + }, refNow()) + if err != nil { + t.Fatalf("dispatch: %v", err) + } + if len(out) != 1 || out[0].Sendable.Channel != ChannelTelegram || len(telegram.sends) != 1 { + t.Fatalf("want one telegram fallback, got out=%+v sends=%d", out, len(telegram.sends)) + } + if len(ob.attempts) != 2 || ob.attempts[0].channel != "ntfy" || ob.attempts[0].status != store.DeliveryFailed || + ob.attempts[1].channel != "telegram" || ob.attempts[1].status != store.DeliverySent { + t.Fatalf("nil ntfy must leave a failed row before telegram succeeds, got %+v", ob.attempts) + } + if len(rc.marked) != 1 || rc.marked[0].id != 9 { + t.Fatalf("successful fallback must complete the reminder, got %+v", rc.marked) + } +} + +func TestDispatchReminderAwayAllAlternativesFailStaysPending(t *testing.T) { + ntfy := &fakeSink{err: errors.New("ntfy unavailable")} + telegram := &fakeSink{err: errors.New("telegram unavailable")} + rc := &fakeReminderCompleter{} + ob := &fakeOutbox{} + d := NewDispatcher(Config{Ntfy: ntfy, Telegram: telegram, Reminders: rc, Outbox: ob}) + + out, err := d.DispatchReminder(context.Background(), PhrasedReminder{ + Decision: loop.ReminderDecision{ + Reminder: store.Reminder{ID: 10, Status: "pending"}, + State: loop.State{Now: refNow(), Presence: store.Away}, + }, + Body: "full detail", Summary: "short reminder", + }, refNow()) + if err == nil { + t.Fatal("all alternatives failed: want an error") + } + if len(out) != 0 || len(rc.marked) != 0 || len(rc.rescheduled) != 0 { + t.Fatalf("failed reminder must stay pending, got out=%+v marked=%+v rescheduled=%+v", out, rc.marked, rc.rescheduled) + } + if len(ob.attempts) != 2 || ob.attempts[0].status != store.DeliveryFailed || ob.attempts[1].status != store.DeliveryFailed { + t.Fatalf("want two failed attempts, got %+v", ob.attempts) + } +} + +func TestDispatchReminderAllPermanentAlternativesBlockGroup(t *testing.T) { + permanent := fmt.Errorf("%w: credentials rejected", ErrPermanent) + ntfy := &fakeSink{err: permanent} + telegram := &fakeSink{err: permanent} + rc := &fakeReminderCompleter{} + d := NewDispatcher(Config{Ntfy: ntfy, Telegram: telegram, Reminders: rc}) + now := refNow() + originals := []store.Reminder{ + {ID: 21, Status: store.ReminderPending, NextFireTs: now, DeliveryGroup: "reminder:permanent"}, + {ID: 22, Status: store.ReminderPending, NextFireTs: now, DeliveryGroup: "reminder:permanent"}, + } + + _, err := d.DispatchReminder(context.Background(), PhrasedReminder{ + Decision: loop.ReminderDecision{ + Reminder: store.Reminder{ID: 0, Status: store.ReminderPending, Collapsed: originals}, + State: loop.State{Now: now, Presence: store.Away}, + }, Body: "details", Summary: "two reminders", + }, now) + if err == nil || !errors.Is(err, ErrPermanent) { + t.Fatalf("permanent alternatives error = %v", err) + } + if len(ntfy.sends) != 0 || len(telegram.sends) != 0 { + // fakeSink records only successes, so this also asserts neither was + // mistaken for a successful delivery. + t.Fatalf("permanent failure produced successful sends: ntfy=%d telegram=%d", len(ntfy.sends), len(telegram.sends)) + } + if len(rc.blocked) != 2 || rc.blocked[0] != 21 || rc.blocked[1] != 22 || rc.blockReason == "" { + t.Fatalf("permanent bundle was not durably blocked: ids=%v reason=%q", rc.blocked, rc.blockReason) + } +} + +func TestDispatchReminderWithUnrecordableOutboxDoesNotSend(t *testing.T) { + telegram := &fakeSink{} + rc := &fakeReminderCompleter{} + ob := &fakeOutbox{beginErr: errors.New("database unavailable")} + d := NewDispatcher(Config{Telegram: telegram, Reminders: rc, Outbox: ob}) + + out, err := d.DispatchReminder(context.Background(), PhrasedReminder{ + Decision: loop.ReminderDecision{ + Reminder: store.Reminder{ID: 11, Status: "pending", DeliveryGroup: "reminder:11"}, + State: loop.State{Now: refNow(), Presence: store.Away}, + }, + Body: "full detail", Summary: "short reminder", + }, refNow()) + if err == nil { + t.Fatal("unrecordable reminder attempt must be withheld") + } + if len(out) != 0 || len(telegram.sends) != 0 || len(rc.marked) != 0 { + t.Fatalf("unrecordable reminder escaped durable boundary: out=%+v sends=%d completed=%+v", out, len(telegram.sends), rc.marked) + } +} + +func TestDispatchReminderOutboxCompletionAmbiguityDoesNotCompleteOccurrence(t *testing.T) { + telegram := &fakeSink{} + rc := &fakeReminderCompleter{} + ob := &fakeOutbox{completeErr: errors.New("database unavailable after send")} + d := NewDispatcher(Config{Telegram: telegram, Reminders: rc, Outbox: ob}) + + out, err := d.DispatchReminder(context.Background(), PhrasedReminder{ + Decision: loop.ReminderDecision{ + Reminder: store.Reminder{ID: 12, Status: store.ReminderPending, DeliveryGroup: "reminder:12"}, + State: loop.State{Now: refNow(), Presence: store.Away}, + }, + Body: "full detail", Summary: "short reminder", + }, refNow()) + if err == nil { + t.Fatal("ambiguous outbox completion must surface an error") + } + if len(out) != 1 || out[0].Sendable.Channel != ChannelTelegram || len(telegram.sends) != 1 { + t.Fatalf("the accepted external send must be reported once: out=%+v sends=%d", out, len(telegram.sends)) + } + if len(rc.marked) != 0 || len(rc.rescheduled) != 0 { + t.Fatalf("ambiguous accepted send completed the occurrence: marked=%+v rescheduled=%+v", rc.marked, rc.rescheduled) + } + if len(ob.attempts) != 2 || ob.attempts[1].status != store.DeliveryPending { + t.Fatalf("accepted send should remain pending for startup reconciliation: %+v", ob.attempts) + } +} + +func TestDispatchReminderCollapsedOutboxUsesRealIDAndSharedGroup(t *testing.T) { + telegram := &fakeSink{} + rc := &fakeReminderCompleter{} + ob := &fakeOutbox{} + d := NewDispatcher(Config{Telegram: telegram, Reminders: rc, Outbox: ob}) + now := refNow() + originals := []store.Reminder{ + {ID: 81, Status: store.ReminderPending, NextFireTs: now, DeliveryGroup: "reminder:bundle"}, + {ID: 82, Status: store.ReminderPending, NextFireTs: now, DeliveryGroup: "reminder:bundle"}, + } + + if _, err := d.DispatchReminder(context.Background(), PhrasedReminder{ + Decision: loop.ReminderDecision{ + Reminder: store.Reminder{ID: 0, Status: store.ReminderPending, Collapsed: originals}, + State: loop.State{Now: now, Presence: store.Away}, + }, + Body: "two reminders", Summary: "two reminders", + }, now); err != nil { + t.Fatalf("dispatch: %v", err) + } + if len(ob.attempts) != 2 { + t.Fatalf("attempts=%d, want nil-ntfy plus telegram", len(ob.attempts)) + } + for _, a := range ob.attempts { + if a.reminderID != 81 || a.deliveryGroup != "reminder:bundle" { + t.Fatalf("synthetic ID escaped into outbox: %+v", a) + } + } + if len(rc.marked) != 2 || rc.marked[0].id != 81 || rc.marked[1].id != 82 { + t.Fatalf("collapsed completion did not cover originals: %+v", rc.marked) + } +} + func TestDispatchReminderFailedSendNotMarkedFired(t *testing.T) { // a failed send must not mark the reminder fired — it stays pending for // the next tick to re-deliver. same instinct as "record after success." @@ -486,11 +718,12 @@ func TestDispatchNudgeVoiceNoSessionSev2Drops(t *testing.T) { } func TestDispatchReminderVoiceNoSessionRoutesNtfy(t *testing.T) { - // present reminder → [voice]. voice has no session → away = ntfy. + // present reminder → [voice]. voice has no session → away starts at ntfy. voice := &fakeSink{err: ErrVoiceNoSession} ntfy := &fakeSink{} + telegram := &fakeSink{} rc := &fakeReminderCompleter{} - d := NewDispatcher(Config{Voice: voice, Ntfy: ntfy, Reminders: rc}) + d := NewDispatcher(Config{Voice: voice, Ntfy: ntfy, Telegram: telegram, Reminders: rc}) rd := loop.ReminderDecision{ Reminder: store.Reminder{ID: 99, Status: "pending"}, @@ -505,6 +738,9 @@ func TestDispatchReminderVoiceNoSessionRoutesNtfy(t *testing.T) { if len(ntfy.sends) != 1 || len(out) != 1 || out[0].Sendable.Channel != ChannelNtfy { t.Fatalf("reminder voice-no-session: want 1 ntfy, got ntfy=%d out=%+v", len(ntfy.sends), out) } + if len(telegram.sends) != 0 { + t.Fatalf("successful ntfy fallback must stop before telegram, got %d sends", len(telegram.sends)) + } if len(rc.marked) != 1 || rc.marked[0].status != "fired" { t.Fatalf("rerouted reminder must be marked fired: %+v", rc.marked) } @@ -641,6 +877,7 @@ func TestDispatchRecurringReminderReschedules(t *testing.T) { type outboxAttempt struct { kind, rule string reminderID int64 + deliveryGroup string channel, hash string status string begunAt, doneAt time.Time @@ -659,13 +896,13 @@ type fakeOutbox struct { completeErr error } -func (f *fakeOutbox) BeginDeliveryAttempt(_ context.Context, kind, rule string, reminderID int64, channel, hash string, now time.Time) (int64, error) { +func (f *fakeOutbox) BeginDeliveryAttempt(_ context.Context, kind, rule string, reminderID int64, deliveryGroup, channel, hash string, now time.Time) (int64, error) { if f.beginErr != nil { return 0, f.beginErr } f.nextID++ f.attempts = append(f.attempts, &outboxAttempt{ - kind: kind, rule: rule, reminderID: reminderID, channel: channel, hash: hash, + kind: kind, rule: rule, reminderID: reminderID, deliveryGroup: deliveryGroup, channel: channel, hash: hash, status: "pending", begunAt: now, }) return f.nextID, nil @@ -985,8 +1222,8 @@ func TestPanicInReminderSinkResolvesTheAttempt(t *testing.T) { }, Body: "звонок", Summary: "звонок", }, refNow()) - if err != nil { - t.Fatalf("a panicking sink must not fail the dispatch: %v", err) + if err == nil { + t.Fatal("a panicking sole reminder sink must report delivery failure") } if len(out) != 0 { t.Fatalf("nothing was delivered, want no dispatches, got %+v", out) diff --git a/internal/delivery/durability_test.go b/internal/delivery/durability_test.go index 5f3e00b..0d265e1 100644 --- a/internal/delivery/durability_test.go +++ b/internal/delivery/durability_test.go @@ -122,7 +122,7 @@ func TestCrashBetweenBeginAndCompleteBecomesUnknown(t *testing.T) { sink := &fakeSink{} // the crash: intent recorded, no completion. - id, err := st.BeginDeliveryAttempt(ctx, "nudge", "disk_low", 0, "telegram", "hash", refNow()) + id, err := st.BeginDeliveryAttempt(ctx, "nudge", "disk_low", 0, "", "telegram", "hash", refNow()) if err != nil { t.Fatalf("begin: %v", err) } @@ -172,7 +172,7 @@ func TestUnknownIsNeverResolvedToSentOrFailed(t *testing.T) { st := openTestStore(t) ctx := context.Background() - id, err := st.BeginDeliveryAttempt(ctx, "nudge", "disk_low", 0, "telegram", "hash", refNow()) + id, err := st.BeginDeliveryAttempt(ctx, "nudge", "disk_low", 0, "", "telegram", "hash", refNow()) if err != nil { t.Fatalf("begin: %v", err) } diff --git a/internal/delivery/ntfysink/ntfysink.go b/internal/delivery/ntfysink/ntfysink.go index 5942951..b0a4c66 100644 --- a/internal/delivery/ntfysink/ntfysink.go +++ b/internal/delivery/ntfysink/ntfysink.go @@ -37,6 +37,12 @@ import ( // the credential lives in the daemon's config (or a systemd credential), // never in the binary. type Config struct { + // Disabled keeps a written endpoint explicitly dark. This is distinct from + // an expanded-empty credential: the latter is a configuration error, while + // this flag records an operator decision to use another delivery reach until + // credentials are provisioned. + Disabled bool `json:"disabled,omitempty"` + // BaseURL — the ntfy server, no trailing path. Required. BaseURL string `json:"base_url"` @@ -71,23 +77,45 @@ type Sink struct { hc *http.Client } -// New validates the config and builds the sink. BaseURL and Topic are -// required; auth is optional (but deny-all servers reject unauthed publishes). -func New(cfg Config) (*Sink, error) { - if cfg.BaseURL == "" { - return nil, fmt.Errorf("ntfysink: BaseURL is required") +// Validate checks one configuration without constructing a client. A disabled +// block is the only state in which credentials may be empty. Maven's ntfy +// reach is private, and accepting an accidental anonymous configuration turns +// a missing environment variable into an endless 403 retry loop. +func Validate(cfg Config) error { + if cfg.Disabled { + return nil + } + if strings.TrimSpace(cfg.BaseURL) == "" { + return fmt.Errorf("ntfysink: BaseURL is required while enabled") } if _, err := url.Parse(cfg.BaseURL); err != nil { - return nil, fmt.Errorf("ntfysink: bad BaseURL: %w", err) + return fmt.Errorf("ntfysink: bad BaseURL: %w", err) } - if cfg.Topic == "" { - return nil, fmt.Errorf("ntfysink: Topic is required") + if strings.TrimSpace(cfg.Topic) == "" { + return fmt.Errorf("ntfysink: Topic is required while enabled") } // Refuse rather than pick. Two credentials configured means someone // intended one of them, and guessing which would send the other nowhere // and leave a working config that is not the one they wrote. - if cfg.Token != "" && cfg.Username != "" { - return nil, fmt.Errorf("ntfysink: set Token or Username, not both") + if strings.TrimSpace(cfg.Token) != "" && (strings.TrimSpace(cfg.Username) != "" || cfg.Password != "") { + return fmt.Errorf("ntfysink: set Token or Username/Password, not both") + } + if strings.TrimSpace(cfg.Token) == "" { + if strings.TrimSpace(cfg.Username) == "" || cfg.Password == "" { + return fmt.Errorf("ntfysink: Token or Username/Password is required while enabled") + } + } + return nil +} + +// New validates the config and builds the sink. Disabled configs belong at the +// daemon wiring boundary and cannot accidentally become live sinks. +func New(cfg Config) (*Sink, error) { + if cfg.Disabled { + return nil, fmt.Errorf("ntfysink: config is disabled") + } + if err := Validate(cfg); err != nil { + return nil, err } to := cfg.Timeout if to == 0 { @@ -127,6 +155,10 @@ func (s *Sink) Send(ctx context.Context, d delivery.Sendable) error { defer resp.Body.Close() if resp.StatusCode/100 != 2 { rb, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) + if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { + return fmt.Errorf("%w: ntfysink: credentials rejected (%d): %s", + delivery.ErrPermanent, resp.StatusCode, strings.TrimSpace(string(rb))) + } return fmt.Errorf("ntfysink: ntfy returned %d: %s", resp.StatusCode, strings.TrimSpace(string(rb))) } return nil diff --git a/internal/delivery/ntfysink/ntfysink_test.go b/internal/delivery/ntfysink/ntfysink_test.go index f7a0172..5fb1991 100644 --- a/internal/delivery/ntfysink/ntfysink_test.go +++ b/internal/delivery/ntfysink/ntfysink_test.go @@ -2,6 +2,7 @@ package ntfysink import ( "context" + "errors" "io" "net/http" "net/http/httptest" @@ -81,6 +82,10 @@ func reminderSendable(summary string) delivery.Sendable { } } +func tokenConfig(baseURL string) Config { + return Config{BaseURL: baseURL, Topic: "maven", Token: "scoped-write-token"} +} + // ----------------------------- config --------------------------------------- func TestNewRejectsEmptyBaseURL(t *testing.T) { @@ -98,7 +103,7 @@ func TestNewRejectsEmptyTopic(t *testing.T) { } func TestNewDefaultTimeout(t *testing.T) { - s, err := New(Config{BaseURL: "http://localhost:8085", Topic: "maven"}) + s, err := New(tokenConfig("http://localhost:8085")) if err != nil { t.Fatalf("New: %v", err) } @@ -107,6 +112,26 @@ func TestNewDefaultTimeout(t *testing.T) { } } +func TestNewRejectsAnonymousPublishing(t *testing.T) { + _, err := New(Config{BaseURL: "http://localhost:8085", Topic: "maven"}) + if err == nil { + t.Fatal("New accepted a topic with no credential") + } + if !strings.Contains(err.Error(), "required while enabled") { + t.Fatalf("error should identify the enabled reach: %v", err) + } +} + +func TestValidateAllowsEmptyCredentialOnlyWhenDisabled(t *testing.T) { + cfg := Config{Disabled: true, BaseURL: "http://localhost:8085", Topic: "maven"} + if err := Validate(cfg); err != nil { + t.Fatalf("Validate disabled config: %v", err) + } + if _, err := New(cfg); err == nil { + t.Fatal("New built a live sink from a disabled config") + } +} + // ----------------------------- send shape ---------------------------------- func TestSendPostsToTopicPath(t *testing.T) { @@ -114,7 +139,7 @@ func TestSendPostsToTopicPath(t *testing.T) { srv := httptest.NewServer(rs.handler()) defer srv.Close() - sink, err := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, err := New(tokenConfig(srv.URL)) if err != nil { t.Fatalf("New: %v", err) } @@ -137,7 +162,7 @@ func TestSendBodyIsSummaryNotFullBody(t *testing.T) { srv := httptest.NewServer(rs.handler()) defer srv.Close() - sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, _ := New(tokenConfig(srv.URL)) if err := sink.Send(context.Background(), nudgeSendable(loop.Sev3, "cert expiring soon")); err != nil { t.Fatalf("Send: %v", err) } @@ -155,7 +180,7 @@ func TestSendNeverSendsTheBodyWhenSummaryEmpty(t *testing.T) { defer srv.Close() s := nudgeSendable(loop.Sev3, "") - sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, _ := New(tokenConfig(srv.URL)) if err := sink.Send(context.Background(), s); err != nil { t.Fatalf("Send: %v", err) } @@ -173,7 +198,7 @@ func TestSendNeverSendsAnEmptyMessage(t *testing.T) { srv := httptest.NewServer(rs.handler()) defer srv.Close() - sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, _ := New(tokenConfig(srv.URL)) s := nudgeSendable(loop.Sev3, "") s.Body = "" s.RuleName = "" @@ -209,18 +234,18 @@ func TestSendSetsBasicAuth(t *testing.T) { } } -func TestSendNoAuthWhenUsernameEmpty(t *testing.T) { +func TestTokenConfigSendsBearerAuth(t *testing.T) { rs := newRecordingServer(t, 200, "") srv := httptest.NewServer(rs.handler()) defer srv.Close() - sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, _ := New(tokenConfig(srv.URL)) if err := sink.Send(context.Background(), nudgeSendable(loop.Sev3, "down")); err != nil { t.Fatalf("Send: %v", err) } _, _, _, auth, _, _ := rs.snapshot() - if auth != "" { - t.Fatalf("want no auth header, got %q", auth) + if auth != "Bearer scoped-write-token" { + t.Fatalf("want bearer auth header, got %q", auth) } } @@ -260,7 +285,7 @@ func TestSendTitleIsMaven(t *testing.T) { srv := httptest.NewServer(rs.handler()) defer srv.Close() - sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, _ := New(tokenConfig(srv.URL)) if err := sink.Send(context.Background(), nudgeSendable(loop.Sev3, "down")); err != nil { t.Fatalf("Send: %v", err) } @@ -277,7 +302,7 @@ func TestPrioritySev3IsHigh(t *testing.T) { srv := httptest.NewServer(rs.handler()) defer srv.Close() - sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, _ := New(tokenConfig(srv.URL)) _ = sink.Send(context.Background(), nudgeSendable(loop.Sev3, "cert")) _, _, _, _, _, prio := rs.snapshot() if prio != "4" { @@ -290,7 +315,7 @@ func TestPrioritySev4IsMax(t *testing.T) { srv := httptest.NewServer(rs.handler()) defer srv.Close() - sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, _ := New(tokenConfig(srv.URL)) _ = sink.Send(context.Background(), nudgeSendable(loop.Sev4, "down")) _, _, _, _, _, prio := rs.snapshot() if prio != "5" { @@ -303,7 +328,7 @@ func TestPriorityReminderIsHigh(t *testing.T) { srv := httptest.NewServer(rs.handler()) defer srv.Close() - sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, _ := New(tokenConfig(srv.URL)) _ = sink.Send(context.Background(), reminderSendable("wake up")) _, _, _, _, _, prio := rs.snapshot() if prio != "4" { @@ -318,7 +343,7 @@ func TestSendReturnsErrorOnNon2xx(t *testing.T) { srv := httptest.NewServer(rs.handler()) defer srv.Close() - sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, _ := New(tokenConfig(srv.URL)) err := sink.Send(context.Background(), nudgeSendable(loop.Sev3, "down")) if err == nil { t.Fatal("want error on 403") @@ -326,6 +351,24 @@ func TestSendReturnsErrorOnNon2xx(t *testing.T) { if !strings.Contains(err.Error(), "403") { t.Fatalf("error should mention status 403, got: %v", err) } + if !errors.Is(err, delivery.ErrPermanent) { + t.Fatalf("403 = %v; want delivery.ErrPermanent", err) + } +} + +func TestSendServerFailureRemainsRetryable(t *testing.T) { + rs := newRecordingServer(t, http.StatusServiceUnavailable, `{"error":"temporarily unavailable"}`) + srv := httptest.NewServer(rs.handler()) + defer srv.Close() + + sink, _ := New(tokenConfig(srv.URL)) + err := sink.Send(context.Background(), nudgeSendable(loop.Sev3, "down")) + if err == nil { + t.Fatal("want error on 503") + } + if errors.Is(err, delivery.ErrPermanent) { + t.Fatalf("503 was marked permanent: %v", err) + } } func TestSendContextCancelReturnsError(t *testing.T) { @@ -333,7 +376,7 @@ func TestSendContextCancelReturnsError(t *testing.T) { srv := httptest.NewServer(rs.handler()) defer srv.Close() - sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) + sink, _ := New(tokenConfig(srv.URL)) ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) defer cancel() err := sink.Send(ctx, nudgeSendable(loop.Sev3, "down")) @@ -343,7 +386,9 @@ func TestSendContextCancelReturnsError(t *testing.T) { } func TestSendConnectionRefusedReturnsError(t *testing.T) { - sink, _ := New(Config{BaseURL: "http://127.0.0.1:1", Topic: "maven", Timeout: time.Second}) + cfg := tokenConfig("http://127.0.0.1:1") + cfg.Timeout = time.Second + sink, _ := New(cfg) err := sink.Send(context.Background(), nudgeSendable(loop.Sev3, "down")) if err == nil { t.Fatal("want error on connection refused") diff --git a/internal/delivery/telegramsink/telegramsink.go b/internal/delivery/telegramsink/telegramsink.go index b4016ee..eb1d828 100644 --- a/internal/delivery/telegramsink/telegramsink.go +++ b/internal/delivery/telegramsink/telegramsink.go @@ -51,6 +51,11 @@ const DefaultTimeout = 10 * time.Second // config file; the bot token + chat id live in the daemon's config (or a // systemd credential), never in the binary. type Config struct { + // Disabled keeps a written Telegram block explicitly dark. A present block + // is otherwise live, so an expanded-empty credential is a configuration + // error rather than an implicit opt-out. + Disabled bool `json:"disabled,omitempty"` + // BotToken — the telegram bot token from BotFather. required. sent in the // URL path (the only place telegram accepts it), not in the body. BotToken string `json:"bot_token"` @@ -91,27 +96,47 @@ type Sink struct { base string // resolved BaseURL, no trailing slash } -// New validates the config and builds the sink. BotToken and ChatID are -// required; Proxy and BaseURL are optional. -func New(cfg Config) (*Sink, error) { - if cfg.BotToken == "" { - return nil, fmt.Errorf("telegramsink: BotToken is required") +// Validate checks one configuration without constructing a client. A disabled +// block is valid and intentionally carries no credentials; every live block +// must carry both secrets and valid endpoint URLs. +func Validate(cfg Config) error { + if cfg.Disabled { + return nil } - if cfg.ChatID == "" { - return nil, fmt.Errorf("telegramsink: ChatID is required") + if strings.TrimSpace(cfg.BotToken) == "" { + return fmt.Errorf("telegramsink: BotToken is required while enabled") + } + if strings.TrimSpace(cfg.ChatID) == "" { + return fmt.Errorf("telegramsink: ChatID is required while enabled") } base := cfg.BaseURL if base == "" { base = DefaultBaseURL } if _, err := url.Parse(base); err != nil { - return nil, fmt.Errorf("telegramsink: bad BaseURL: %w", err) + return fmt.Errorf("telegramsink: bad BaseURL: %w", err) } if cfg.Proxy != "" { if _, err := url.Parse(cfg.Proxy); err != nil { - return nil, fmt.Errorf("telegramsink: bad Proxy: %w", err) + return fmt.Errorf("telegramsink: bad Proxy: %w", err) } } + return nil +} + +// New validates the config and builds the sink. Disabled configs belong at the +// wiring boundary and cannot accidentally become live sinks. +func New(cfg Config) (*Sink, error) { + if cfg.Disabled { + return nil, fmt.Errorf("telegramsink: config is disabled") + } + if err := Validate(cfg); err != nil { + return nil, err + } + base := cfg.BaseURL + if base == "" { + base = DefaultBaseURL + } to := cfg.Timeout if to == 0 { to = DefaultTimeout @@ -192,7 +217,11 @@ func (s *Sink) Send(ctx context.Context, d delivery.Sendable) error { var tr telegramResp jsonErr := json.Unmarshal(rb, &tr) if jsonErr == nil && !tr.Ok { - return fmt.Errorf("telegramsink: telegram returned error %d: %s", tr.ErrorCode, strings.TrimSpace(tr.Description)) + err := fmt.Errorf("telegramsink: telegram returned error %d: %s", tr.ErrorCode, strings.TrimSpace(tr.Description)) + if tr.ErrorCode == http.StatusUnauthorized || tr.ErrorCode == http.StatusForbidden { + return fmt.Errorf("%w: %v", delivery.ErrPermanent, err) + } + return err } if resp.StatusCode/100 != 2 { return fmt.Errorf("telegramsink: telegram returned %d: %s", resp.StatusCode, snippet(rb)) diff --git a/internal/delivery/telegramsink/telegramsink_test.go b/internal/delivery/telegramsink/telegramsink_test.go index d806fc4..22fb9eb 100644 --- a/internal/delivery/telegramsink/telegramsink_test.go +++ b/internal/delivery/telegramsink/telegramsink_test.go @@ -106,6 +106,16 @@ func TestNewRejectsEmptyChatID(t *testing.T) { } } +func TestValidateAllowsEmptySecretsOnlyWhenDisabled(t *testing.T) { + cfg := Config{Disabled: true} + if err := Validate(cfg); err != nil { + t.Fatalf("Validate disabled config: %v", err) + } + if _, err := New(cfg); err == nil { + t.Fatal("New built a live sink from a disabled config") + } +} + func TestNewDefaultTimeout(t *testing.T) { s, err := New(Config{BotToken: "123:abc", ChatID: "42"}) if err != nil { @@ -289,6 +299,9 @@ func TestSendReturnsErrorOnTelegramError(t *testing.T) { if !strings.Contains(err.Error(), "401") { t.Fatalf("error should mention error_code 401, got: %v", err) } + if !errors.Is(err, delivery.ErrPermanent) { + t.Fatalf("revoked bot credential must be permanent, got: %v", err) + } } // A relay that is up but cannot reach api.telegram.org answers 200 with a page @@ -525,9 +538,9 @@ func TestSendRoutesThroughProxyWhenConfigured(t *testing.T) { // ----------------------------- reminder same shape -------------------------- func TestReminderSendUsesSamePath(t *testing.T) { - // reminders away route to ntfy, not telegram — but if the daemon ever - // routes a reminder via telegram (per-reminder override), the sink must - // accept KindReminder undamaged. exercises the kind-agnostic contract. + // Telegram is the second away alternative for reminders. If ntfy is + // unavailable and the dispatcher falls through, the sink must accept + // KindReminder undamaged. exercises the kind-agnostic contract. rs := newRecordingServer(t, 200, "") srv := httptest.NewServer(rs.handler()) defer srv.Close() diff --git a/internal/ipc/api.go b/internal/ipc/api.go index db90185..cdf8074 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -63,14 +63,15 @@ type Nudge struct { // DeliveryAttempt — one row of the delivery outbox. Times are formatted by the // reader; Completed is nil while the attempt is still pending. type DeliveryAttempt struct { - ID int64 `json:"id"` - Kind string `json:"kind"` - Rule string `json:"rule,omitempty"` - ReminderID int64 `json:"reminder_id,omitempty"` - Channel string `json:"channel"` - Status string `json:"status"` - Created time.Time `json:"created"` - Completed *time.Time `json:"completed,omitempty"` + ID int64 `json:"id"` + Kind string `json:"kind"` + Rule string `json:"rule,omitempty"` + ReminderID int64 `json:"reminder_id,omitempty"` + DeliveryGroup string `json:"delivery_group,omitempty"` + Channel string `json:"channel"` + Status string `json:"status"` + Created time.Time `json:"created"` + Completed *time.Time `json:"completed,omitempty"` } // Note — a recall/preference item; ranked by embedding cosine on query. @@ -85,13 +86,18 @@ type Note struct { // Reminder — user-stated future intent; fires once or recurring (if cron set). type Reminder struct { - ID int64 `json:"id"` - CreatedTs time.Time `json:"created_ts"` - FireTs time.Time `json:"fire_ts"` - NextFireTs time.Time `json:"next_fire_ts"` - Payload string `json:"payload"` - Status string `json:"status"` // pending|fired|cancelled - Cron string `json:"cron"` + ID int64 `json:"id"` + CreatedTs time.Time `json:"created_ts"` + FireTs time.Time `json:"fire_ts"` + NextFireTs time.Time `json:"next_fire_ts"` + Payload string `json:"payload"` + Status string `json:"status"` // pending|fired|cancelled + Cron string `json:"cron"` + DeliveryGroup string `json:"delivery_group,omitempty"` + DeliveryAttempts int `json:"delivery_attempts,omitempty"` + NextAttemptTs time.Time `json:"next_attempt_ts,omitempty"` + DeliveryBlockedTs time.Time `json:"delivery_blocked_ts,omitempty"` + DeliveryBlockedError string `json:"delivery_blocked_error,omitempty"` } // Presence — the read the phraser / delivery modules need to decide channel diff --git a/internal/ipc/maperr_test.go b/internal/ipc/maperr_test.go index 6ff31fb..e08c384 100644 --- a/internal/ipc/maperr_test.go +++ b/internal/ipc/maperr_test.go @@ -42,8 +42,9 @@ var mapErrPairs = []struct { // and fails TestMapErrCoversEveryStoreSentinel, which is the point: whether a // module can branch on an error is a decision, not a default. var unmappedStoreErrors = map[string]string{ - "ErrKeyLen": "unlock path — the key never crosses CoreAPI", - "ErrDecrypt": "unlock path — the key never crosses CoreAPI", + "ErrKeyLen": "unlock path — the key never crosses CoreAPI", + "ErrDecrypt": "unlock path — the key never crosses CoreAPI", + "ErrReminderPhrase": "daemon-internal delivery-state validation; modules neither cache reminder presentations nor branch on this verdict", "ErrToolCmd": "write-side validation of an allowlist mutation; the caller is the owner at a step-up, not a module branching on the verdict", diff --git a/internal/ipc/storeapi.go b/internal/ipc/storeapi.go index a0fe3f2..5c7aebb 100644 --- a/internal/ipc/storeapi.go +++ b/internal/ipc/storeapi.go @@ -149,7 +149,7 @@ func (a *storeAPI) DeliveryAttempts(ctx context.Context, status string, n int) ( return mapRows(as, err, func(at store.DeliveryAttempt) DeliveryAttempt { out := DeliveryAttempt{ ID: at.ID, Kind: at.Kind, Rule: at.Rule, ReminderID: at.ReminderID, - Channel: at.Channel, Status: at.Status, Created: at.Created, + DeliveryGroup: at.DeliveryGroup, Channel: at.Channel, Status: at.Status, Created: at.Created, } if at.HasComplete { t := at.Completed @@ -352,13 +352,18 @@ func toTool(t store.Tool) Tool { func toReminder(r store.Reminder) Reminder { return Reminder{ - ID: r.ID, - CreatedTs: r.CreatedTs, - FireTs: r.FireTs, - NextFireTs: r.NextFireTs, - Payload: r.Payload, - Status: r.Status, - Cron: r.Cron, + ID: r.ID, + CreatedTs: r.CreatedTs, + FireTs: r.FireTs, + NextFireTs: r.NextFireTs, + Payload: r.Payload, + Status: r.Status, + Cron: r.Cron, + DeliveryGroup: r.DeliveryGroup, + DeliveryAttempts: r.DeliveryAttempts, + NextAttemptTs: r.NextAttemptTs, + DeliveryBlockedTs: r.DeliveryBlockedTs, + DeliveryBlockedError: r.DeliveryBlockedError, } } diff --git a/internal/loop/gather.go b/internal/loop/gather.go index de5db82..2a756c8 100644 --- a/internal/loop/gather.go +++ b/internal/loop/gather.go @@ -12,6 +12,7 @@ import ( "context" "encoding/json" "fmt" + "sort" "time" "github.com/kami/maven/internal/calendar" @@ -258,16 +259,62 @@ 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. 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. +// the daemon was offline), collapse them into digest reminders to avoid a +// burst of individual notifications. A previously phrased delivery group is +// kept intact and separate from newly-due reminders: otherwise one new row +// joining a failed bundle would force the old bundle through the model again. +// The synthetic digest (ID=0) carries the originals in Collapsed; the +// dispatcher completes them only after the digest actually delivers. func collapseReminders(due []store.Reminder) []store.Reminder { if len(due) <= 1 { return due } + + // Every reminder without a delivery group is part of the new bundle for + // this tick. Persisted groups each retain their own bundle identity. + const ungrouped = "\x00" + type reminderGroup struct { + key string + reminders []store.Reminder + } + var groups []reminderGroup + byKey := make(map[string]int) + for _, r := range due { + key := r.DeliveryGroup + if key == "" { + key = ungrouped + } + i, ok := byKey[key] + if !ok { + i = len(groups) + byKey[key] = i + groups = append(groups, reminderGroup{key: key}) + } + groups[i].reminders = append(groups[i].reminders, r) + } + // DueReminders was ordered globally. Grouping can move rows together, so + // restore earliest-first ordering between the resulting deliveries. + sort.SliceStable(groups, func(i, j int) bool { + a := groups[i].reminders[0] + b := groups[j].reminders[0] + if a.NextFireTs.Equal(b.NextFireTs) { + return a.ID < b.ID + } + return a.NextFireTs.Before(b.NextFireTs) + }) + + out := make([]store.Reminder, 0, len(groups)) + for _, group := range groups { + if len(group.reminders) == 1 { + out = append(out, group.reminders[0]) + continue + } + out = append(out, collapseReminderGroup(group.reminders)) + } + return out +} + +func collapseReminderGroup(due []store.Reminder) store.Reminder { // Build a summary payload. var items []string earliest := due[0].FireTs @@ -294,11 +341,11 @@ func collapseReminders(due []store.Reminder) []store.Reminder { // Return a single synthetic digest reminder. ID=0 signals "digest" to // the dispatcher, which completes the Collapsed originals on success. - return []store.Reminder{{ + return store.Reminder{ ID: 0, FireTs: earliest, Payload: string(digestPayload), - Status: "pending", + Status: store.ReminderPending, Collapsed: due, - }} + } } diff --git a/internal/loop/gather_test.go b/internal/loop/gather_test.go index b4bc97c..d839e31 100644 --- a/internal/loop/gather_test.go +++ b/internal/loop/gather_test.go @@ -47,3 +47,36 @@ func TestGatherStateLoadsPrefixFamilies(t *testing.T) { t.Fatal("the rule must fire on a gathered per-monitor fact") } } + +func TestCollapseRemindersKeepsPersistedBundleSeparateFromNewDueRows(t *testing.T) { + now := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + due := []store.Reminder{ + {ID: 1, FireTs: now, NextFireTs: now, Payload: "one", DeliveryGroup: "reminder:old", PhraseBody: "old bundle"}, + {ID: 2, FireTs: now.Add(time.Second), NextFireTs: now.Add(time.Second), Payload: "two", DeliveryGroup: "reminder:old", PhraseBody: "old bundle"}, + {ID: 3, FireTs: now.Add(2 * time.Second), NextFireTs: now.Add(2 * time.Second), Payload: "three"}, + } + + got := collapseReminders(due) + if len(got) != 2 { + t.Fatalf("deliveries = %d, want old bundle plus new reminder: %+v", len(got), got) + } + if got[0].ID != 0 || len(got[0].Collapsed) != 2 || got[0].Collapsed[0].ID != 1 || got[0].Collapsed[1].ID != 2 { + t.Fatalf("old persisted bundle was changed: %+v", got[0]) + } + if got[1].ID != 3 || len(got[1].Collapsed) != 0 { + t.Fatalf("new reminder joined the old phrase: %+v", got[1]) + } +} + +func TestCollapseRemindersMakesOneBundleForNewDueRows(t *testing.T) { + now := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + due := []store.Reminder{ + {ID: 1, FireTs: now, NextFireTs: now, Payload: "one"}, + {ID: 2, FireTs: now, NextFireTs: now, Payload: "two"}, + {ID: 3, FireTs: now, NextFireTs: now, Payload: "three"}, + } + got := collapseReminders(due) + if len(got) != 1 || got[0].ID != 0 || len(got[0].Collapsed) != 3 { + t.Fatalf("new reminders did not collapse together: %+v", got) + } +} diff --git a/internal/store/delivery.go b/internal/store/delivery.go index 4c016c7..710be9c 100644 --- a/internal/store/delivery.go +++ b/internal/store/delivery.go @@ -28,14 +28,16 @@ const ( // send happens, so a crash between "sent externally" and "recorded" leaves a // trace instead of silence. kind is "nudge" or "reminder"; rule is set for // nudges, reminderID for reminders (the other left at its zero value). +// deliveryGroup is the exact persisted reminder occurrence or collapsed +// bundle; it is empty for nudges and legacy reminder attempts. // bodyHash is an opaque caller-computed key (e.g. sha256 of channel+body) — // stored for post-crash operator triage, not enforced as a uniqueness // constraint (a rule/reminder legitimately re-sends across ticks). -func (s *Store) BeginDeliveryAttempt(ctx context.Context, kind, rule string, reminderID int64, channel, bodyHash string, now time.Time) (int64, error) { +func (s *Store) BeginDeliveryAttempt(ctx context.Context, kind, rule string, reminderID int64, deliveryGroup, channel, bodyHash string, now time.Time) (int64, error) { res, err := s.db.ExecContext(ctx, - `INSERT INTO delivery_attempts (kind, rule, reminder_id, channel, body_hash, status, created_ts) - VALUES (?, ?, ?, ?, ?, 'pending', ?)`, - kind, rule, reminderID, channel, bodyHash, now.UnixMilli()) + `INSERT INTO delivery_attempts (kind, rule, reminder_id, delivery_group, channel, body_hash, status, created_ts) + VALUES (?, ?, ?, ?, ?, ?, 'pending', ?)`, + kind, rule, reminderID, deliveryGroup, channel, bodyHash, now.UnixMilli()) if err != nil { return 0, fmt.Errorf("begin delivery attempt: %w", err) } @@ -90,15 +92,16 @@ func (s *Store) ReconcileStaleDeliveryAttempts(ctx context.Context, now time.Tim // DeliveryAttempt — one row of the outbox, as a reader sees it. type DeliveryAttempt struct { - ID int64 - Kind string // nudge|reminder - Rule string // set for nudges - ReminderID int64 // set for reminders - Channel string - Status string // one of the Delivery* constants - Created time.Time - Completed time.Time // zero while pending - HasComplete bool + ID int64 + Kind string // nudge|reminder + Rule string // set for nudges + ReminderID int64 // set for reminders + DeliveryGroup string // exact reminder occurrence/bundle; empty for nudges and legacy rows + Channel string + Status string // one of the Delivery* constants + Created time.Time + Completed time.Time // zero while pending + HasComplete bool } // ListDeliveryAttempts returns recent attempts, newest first. An empty status @@ -117,7 +120,7 @@ func (s *Store) ListDeliveryAttempts(ctx context.Context, status string, limit i if limit <= 0 { limit = 50 } - q := `SELECT id, kind, rule, reminder_id, channel, status, created_ts, completed_ts + q := `SELECT id, kind, rule, reminder_id, delivery_group, channel, status, created_ts, completed_ts FROM delivery_attempts` args := []any{} if status != "" { @@ -138,7 +141,7 @@ func (s *Store) ListDeliveryAttempts(ctx context.Context, status string, limit i var a DeliveryAttempt var created int64 var completed *int64 - if err := rows.Scan(&a.ID, &a.Kind, &a.Rule, &a.ReminderID, &a.Channel, &a.Status, &created, &completed); err != nil { + if err := rows.Scan(&a.ID, &a.Kind, &a.Rule, &a.ReminderID, &a.DeliveryGroup, &a.Channel, &a.Status, &created, &completed); err != nil { return nil, fmt.Errorf("list delivery attempts: scan: %w", err) } a.Created = time.UnixMilli(created) diff --git a/internal/store/delivery_test.go b/internal/store/delivery_test.go index d261448..936b911 100644 --- a/internal/store/delivery_test.go +++ b/internal/store/delivery_test.go @@ -15,7 +15,7 @@ func TestDroppedDeliveryAttemptRoundTrips(t *testing.T) { ctx := context.Background() now := time.Now() - id, err := s.BeginDeliveryAttempt(ctx, "nudge", "water", 0, "drop", "abc123", now) + id, err := s.BeginDeliveryAttempt(ctx, "nudge", "water", 0, "", "drop", "abc123", now) if err != nil { t.Fatalf("BeginDeliveryAttempt: %v", err) } @@ -40,21 +40,21 @@ func TestListDeliveryAttempts(t *testing.T) { s := newTestStore(t) base := time.Date(2026, 8, 4, 9, 0, 0, 0, time.UTC) - sent, err := s.BeginDeliveryAttempt(ctx, "nudge", "water", 0, "telegram", "h1", base) + sent, err := s.BeginDeliveryAttempt(ctx, "nudge", "water", 0, "", "telegram", "h1", base) if err != nil { t.Fatal(err) } if err := s.CompleteDeliveryAttempt(ctx, sent, DeliverySent, base.Add(time.Second)); err != nil { t.Fatal(err) } - dropped, err := s.BeginDeliveryAttempt(ctx, "nudge", "care", 0, "telegram", "h2", base.Add(time.Minute)) + dropped, err := s.BeginDeliveryAttempt(ctx, "nudge", "care", 0, "", "telegram", "h2", base.Add(time.Minute)) if err != nil { t.Fatal(err) } if err := s.CompleteDeliveryAttempt(ctx, dropped, DeliveryDropped, base.Add(time.Minute)); err != nil { t.Fatal(err) } - if _, err := s.BeginDeliveryAttempt(ctx, "reminder", "", 7, "voice", "h3", base.Add(2*time.Minute)); err != nil { + if _, err := s.BeginDeliveryAttempt(ctx, "reminder", "", 7, "reminder:test", "voice", "h3", base.Add(2*time.Minute)); err != nil { t.Fatal(err) } diff --git a/internal/store/migrations.go b/internal/store/migrations.go index 3381064..7862e98 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -351,6 +351,29 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 source TEXT NOT NULL DEFAULT '', encoder_id TEXT NOT NULL DEFAULT '' );`, + // #25 — durable reminder delivery state (V-651). A reminder can remain + // pending for a long time when every reachable transport is unhealthy. The + // phrased text belongs to that delivery occurrence, so retaining it here + // avoids spending the resident model again on every retry and across daemon + // restarts. next_attempt_ts makes transport failures wait on a bounded + // exponential backoff instead of retrying at the tick rate. delivery_group + // keeps the originals of a collapsed catch-up bundle together; a reminder + // that becomes due later must not make the old bundle get re-phrased. + `ALTER TABLE reminders ADD COLUMN delivery_group TEXT NOT NULL DEFAULT ''; + ALTER TABLE reminders ADD COLUMN phrase_body TEXT NOT NULL DEFAULT ''; + ALTER TABLE reminders ADD COLUMN phrase_summary TEXT NOT NULL DEFAULT ''; + ALTER TABLE reminders ADD COLUMN phrase_mood TEXT NOT NULL DEFAULT ''; + ALTER TABLE reminders ADD COLUMN delivery_attempts INTEGER NOT NULL DEFAULT 0 CHECK (delivery_attempts >= 0); + ALTER TABLE reminders ADD COLUMN next_attempt_ts INTEGER; + ALTER TABLE delivery_attempts ADD COLUMN delivery_group TEXT NOT NULL DEFAULT ''; + ALTER TABLE reminders ADD COLUMN delivery_blocked_ts INTEGER; + ALTER TABLE reminders ADD COLUMN delivery_blocked_error TEXT NOT NULL DEFAULT ''; + CREATE INDEX IF NOT EXISTS idx_reminders_due + ON reminders (next_fire_ts, next_attempt_ts) + WHERE status = 'pending'; + CREATE INDEX IF NOT EXISTS idx_delivery_attempts_reminder_group + ON delivery_attempts (delivery_group, status) + WHERE kind = 'reminder' AND delivery_group <> '';`, } // migrate applies every migration with a number greater than the DB's current diff --git a/internal/store/reminders.go b/internal/store/reminders.go index f7b67df..46705f7 100644 --- a/internal/store/reminders.go +++ b/internal/store/reminders.go @@ -22,6 +22,20 @@ type Reminder struct { Status string // pending | fired | cancelled Cron string // cron expression, empty for one-shot + // DeliveryGroup and Phrase* are the durable presentation for this exact + // occurrence. A collapsed catch-up bundle stores the same group and phrase + // on every original reminder, so a retry (including after restart) says the + // same thing without asking the model again. Rescheduling a recurring + // reminder clears them for its next occurrence. + DeliveryGroup string + PhraseBody string + PhraseSummary string + PhraseMood string + DeliveryAttempts int + NextAttemptTs time.Time + DeliveryBlockedTs time.Time + DeliveryBlockedError string + // 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. @@ -62,18 +76,35 @@ const ( ReminderPending = "pending" ReminderFired = "fired" ReminderCancelled = "cancelled" + + // ReminderRetryBase and ReminderRetryMax bound the retry cadence. Attempts + // continue indefinitely because a reminder must not disappear during a + // long transport outage; only the delay stops growing. + ReminderRetryBase = time.Minute + ReminderRetryMax = time.Hour ) var ( ErrReminderNotFound = errors.New("store: reminder not found") ErrReminderState = errors.New("store: reminder not in a mutable state") + ErrReminderPhrase = errors.New("store: reminder delivery phrase invalid") ) +const reminderColumns = `id, created_ts, fire_ts, next_fire_ts, payload, status, cron, + delivery_group, phrase_body, phrase_summary, phrase_mood, delivery_attempts, next_attempt_ts, + delivery_blocked_ts, delivery_blocked_error` + func scanReminder(sc scanner) (Reminder, error) { var r Reminder var created, fire, nextFire int64 var cron *string - if err := sc.Scan(&r.ID, &created, &fire, &nextFire, &r.Payload, &r.Status, &cron); err != nil { + var nextAttempt *int64 + var blocked *int64 + if err := sc.Scan( + &r.ID, &created, &fire, &nextFire, &r.Payload, &r.Status, &cron, + &r.DeliveryGroup, &r.PhraseBody, &r.PhraseSummary, &r.PhraseMood, + &r.DeliveryAttempts, &nextAttempt, &blocked, &r.DeliveryBlockedError, + ); err != nil { return Reminder{}, err } r.CreatedTs = time.UnixMilli(created).UTC() @@ -82,6 +113,12 @@ func scanReminder(sc scanner) (Reminder, error) { if cron != nil { r.Cron = *cron } + if nextAttempt != nil { + r.NextAttemptTs = time.UnixMilli(*nextAttempt).UTC() + } + if blocked != nil { + r.DeliveryBlockedTs = time.UnixMilli(*blocked).UTC() + } return r, nil } @@ -111,11 +148,20 @@ func (s *Store) CreateReminder(ctx context.Context, fire time.Time, payload, cro // DueReminders returns pending reminders with next_fire_ts <= now, oldest first. // This is the predicate input from the loop side: `next_fire_ts <= now AND status='pending'`. func (s *Store) DueReminders(ctx context.Context, now time.Time) ([]Reminder, error) { - rows, err := s.db.QueryContext(ctx, ` - SELECT id, created_ts, fire_ts, next_fire_ts, payload, status, cron + rows, err := s.db.QueryContext(ctx, `SELECT `+reminderColumns+` FROM reminders WHERE status = 'pending' AND next_fire_ts <= ? - ORDER BY next_fire_ts ASC`, now.UnixMilli()) + AND (next_attempt_ts IS NULL OR next_attempt_ts <= ?) + AND delivery_blocked_ts IS NULL + AND NOT EXISTS ( + SELECT 1 + FROM delivery_attempts AS attempt + WHERE attempt.kind = 'reminder' + AND attempt.delivery_group = reminders.delivery_group + AND reminders.delivery_group <> '' + AND attempt.status IN ('pending', 'sent', 'unknown') + ) + ORDER BY next_fire_ts ASC, id ASC`, now.UnixMilli(), now.UnixMilli()) if err != nil { return nil, fmt.Errorf("due reminders: %w", err) } @@ -140,8 +186,7 @@ func (s *Store) DueReminders(ctx context.Context, now time.Time) ([]Reminder, er // for next year stayed on it. Bounding by fire time drops what is out of range // instead of what is old. func (s *Store) PendingReminders(ctx context.Context, from, to time.Time) ([]Reminder, error) { - rows, err := s.db.QueryContext(ctx, ` - SELECT id, created_ts, fire_ts, next_fire_ts, payload, status, cron + rows, err := s.db.QueryContext(ctx, `SELECT `+reminderColumns+` FROM reminders WHERE status = ? AND next_fire_ts >= ? AND next_fire_ts < ? ORDER BY next_fire_ts ASC, id ASC`, @@ -164,29 +209,42 @@ func (s *Store) PendingReminders(ctx context.Context, from, to time.Time) ([]Rem // MarkReminder sets a reminder's status. Only valid transitions: pending→fired, // pending→cancelled. Anything else is a programming error. func (s *Store) MarkReminder(ctx context.Context, id int64, status string) error { - if status != "fired" && status != "cancelled" { + if status != ReminderFired && status != ReminderCancelled { return fmt.Errorf("%w: %s", ErrReminderState, status) } - // pending → fired|cancelled only. + // The old read-then-write transition allowed two callers to both observe + // pending and both report success. Keeping the source state in the UPDATE + // predicate makes pending → fired|cancelled one atomic contest (V-678). + res, err := s.db.ExecContext(ctx, + "UPDATE reminders SET status = ? WHERE id = ? AND status = ?", + status, id, ReminderPending) + if err != nil { + return err + } + n, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("mark reminder: rows affected: %w", err) + } + if n == 1 { + return nil + } + + // Preserve the public distinction between a missing id and a completed + // state without weakening the atomic transition above. var current string - err := s.db.QueryRowContext(ctx, "SELECT status FROM reminders WHERE id = ?", id).Scan(¤t) + err = s.db.QueryRowContext(ctx, "SELECT status FROM reminders WHERE id = ?", id).Scan(¤t) if errors.Is(err, sql.ErrNoRows) { return ErrReminderNotFound } if err != nil { return err } - if current != "pending" { - return fmt.Errorf("%w: currently %s", ErrReminderState, current) - } - _, err = s.db.ExecContext(ctx, "UPDATE reminders SET status = ? WHERE id = ?", status, id) - return err + return fmt.Errorf("%w: currently %s", ErrReminderState, current) } // ListReminders returns the n most recent reminders, newest first. func (s *Store) ListReminders(ctx context.Context, n int) ([]Reminder, error) { - rows, err := s.db.QueryContext(ctx, ` - SELECT id, created_ts, fire_ts, next_fire_ts, payload, status, cron + rows, err := s.db.QueryContext(ctx, `SELECT `+reminderColumns+` FROM reminders ORDER BY created_ts DESC, id DESC LIMIT ?`, n) if err != nil { return nil, fmt.Errorf("list reminders: %w", err) @@ -203,6 +261,352 @@ func (s *Store) ListReminders(ctx context.Context, n int) ([]Reminder, error) { return out, rows.Err() } +// HasDeliveryPhrase reports whether this occurrence already has a durable +// presentation. Summary may intentionally be empty (the delivery boundary has +// a generic privacy-preserving fallback), so Body is the readiness marker. +func (r Reminder) HasDeliveryPhrase() bool { + return r.DeliveryGroup != "" && r.PhraseBody != "" +} + +// ReminderRetryDelay returns the delay after attempt (one-based). It grows +// exponentially from one minute and stays at one hour; retries do not stop. +func ReminderRetryDelay(attempt int) time.Duration { + if attempt <= 1 { + return ReminderRetryBase + } + delay := ReminderRetryBase + for i := 1; i < attempt && delay < ReminderRetryMax; i++ { + if delay >= ReminderRetryMax/2 { + return ReminderRetryMax + } + delay *= 2 + } + if delay > ReminderRetryMax { + return ReminderRetryMax + } + return delay +} + +// CacheReminderPhrase stores one presentation on every original represented +// by a reminder delivery. For a collapsed bundle, originals contains every +// row in Reminder.Collapsed and group is shared across all of them. +// +// The occurrence timestamp and empty-body predicates keep this from attaching +// an old phrase to a recurring reminder's next occurrence or overwriting a +// phrase another delivery already claimed. The transaction prevents a partial +// bundle cache: after a crash either every original can reconstruct the bundle +// or none can. +func (s *Store) CacheReminderPhrase( + ctx context.Context, + originals []Reminder, + group, body, summary, mood string, +) error { + if len(originals) == 0 || group == "" || body == "" { + return ErrReminderPhrase + } + if mood == "" { + mood = "neutral" + } + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("cache reminder phrase: begin: %w", err) + } + defer tx.Rollback() + + seen := make(map[int64]struct{}, len(originals)) + for _, r := range originals { + if r.ID <= 0 || r.NextFireTs.IsZero() { + return ErrReminderPhrase + } + if _, ok := seen[r.ID]; ok { + return ErrReminderPhrase + } + seen[r.ID] = struct{}{} + res, err := tx.ExecContext(ctx, ` + UPDATE reminders + SET delivery_group = ?, phrase_body = ?, phrase_summary = ?, phrase_mood = ? + WHERE id = ? AND status = ? AND next_fire_ts = ? AND phrase_body = ''`, + group, body, summary, mood, r.ID, ReminderPending, r.NextFireTs.UnixMilli()) + if err != nil { + return fmt.Errorf("cache reminder phrase %d: %w", r.ID, err) + } + n, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("cache reminder phrase %d: rows affected: %w", r.ID, err) + } + if n != 1 { + return fmt.Errorf("%w: reminder %d is no longer an unphrased pending occurrence", ErrReminderState, r.ID) + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("cache reminder phrase: commit: %w", err) + } + return nil +} + +// ScheduleReminderRetry moves every still-pending original into a retry wait. +// Terminal rows are skipped: cancellation winning while a send was in flight +// must not be resurrected. A recurring row whose occurrence changed is skipped +// for the same reason. All remaining originals get their own persisted attempt +// count; normally a collapsed bundle keeps those counts in lockstep. +func (s *Store) ScheduleReminderRetry(ctx context.Context, originals []Reminder, now time.Time) error { + if len(originals) == 0 { + return ErrReminderNotFound + } + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("schedule reminder retry: begin: %w", err) + } + defer tx.Rollback() + + seen := make(map[int64]struct{}, len(originals)) + for _, expected := range originals { + if expected.ID <= 0 || expected.NextFireTs.IsZero() { + return ErrReminderNotFound + } + if _, ok := seen[expected.ID]; ok { + continue + } + seen[expected.ID] = struct{}{} + + var status string + var nextFire int64 + var attempts int + var blocked *int64 + err := tx.QueryRowContext(ctx, + `SELECT status, next_fire_ts, delivery_attempts, delivery_blocked_ts FROM reminders WHERE id = ?`, + expected.ID).Scan(&status, &nextFire, &attempts, &blocked) + if errors.Is(err, sql.ErrNoRows) { + return ErrReminderNotFound + } + if err != nil { + return fmt.Errorf("schedule reminder retry %d: read: %w", expected.ID, err) + } + if status != ReminderPending || nextFire != expected.NextFireTs.UnixMilli() || blocked != nil { + continue + } + if expected.DeliveryGroup != "" { + var ambiguous int + if err := tx.QueryRowContext(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM delivery_attempts + WHERE kind = 'reminder' AND delivery_group = ? + AND status IN ('pending', 'sent', 'unknown') + )`, expected.DeliveryGroup).Scan(&ambiguous); err != nil { + return fmt.Errorf("schedule reminder retry %d: inspect outbox: %w", expected.ID, err) + } + if ambiguous != 0 { + // The sink may have accepted this presentation. Leave the reminder + // pending but ineligible for automatic retry; operator resolution + // must not be replaced with a guessed duplicate. + continue + } + } + + attempts++ + nextAttempt := now.Add(ReminderRetryDelay(attempts)).UnixMilli() + res, err := tx.ExecContext(ctx, ` + UPDATE reminders + SET delivery_attempts = ?, next_attempt_ts = ? + WHERE id = ? AND status = ? AND next_fire_ts = ? AND delivery_attempts = ?`, + attempts, nextAttempt, expected.ID, ReminderPending, nextFire, attempts-1) + if err != nil { + return fmt.Errorf("schedule reminder retry %d: %w", expected.ID, err) + } + n, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("schedule reminder retry %d: rows affected: %w", expected.ID, err) + } + if n != 1 { + return fmt.Errorf("%w: reminder %d changed concurrently", ErrReminderState, expected.ID) + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("schedule reminder retry: commit: %w", err) + } + return nil +} + +// BlockReminderDelivery records a permanent transport/configuration refusal on +// every original represented by one presentation. Blocked rows stay pending and +// visible, but never retry automatically. UnblockReminderDelivery is the +// deliberate recovery path after credentials or policy are repaired. +func (s *Store) BlockReminderDelivery(ctx context.Context, originals []Reminder, now time.Time, reason string) error { + if len(originals) == 0 || strings.TrimSpace(reason) == "" { + return ErrReminderState + } + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("block reminder delivery: begin: %w", err) + } + defer tx.Rollback() + seen := make(map[int64]struct{}, len(originals)) + for _, expected := range originals { + if expected.ID <= 0 || expected.NextFireTs.IsZero() { + return ErrReminderNotFound + } + if _, ok := seen[expected.ID]; ok { + continue + } + seen[expected.ID] = struct{}{} + res, err := tx.ExecContext(ctx, ` + UPDATE reminders + SET delivery_blocked_ts = ?, delivery_blocked_error = ?, next_attempt_ts = NULL + WHERE id = ? AND status = ? AND next_fire_ts = ?`, + now.UnixMilli(), reason, expected.ID, ReminderPending, expected.NextFireTs.UnixMilli()) + if err != nil { + return fmt.Errorf("block reminder %d: %w", expected.ID, err) + } + if err := requireOneReminderRow(res, expected.ID); err != nil { + return err + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("block reminder delivery: commit: %w", err) + } + return nil +} + +func (s *Store) UnblockReminderDelivery(ctx context.Context, id int64) error { + res, err := s.db.ExecContext(ctx, ` + UPDATE reminders + SET delivery_blocked_ts = NULL, delivery_blocked_error = '', next_attempt_ts = NULL + WHERE id = ? AND status = ? AND delivery_blocked_ts IS NOT NULL`, id, ReminderPending) + if err != nil { + return fmt.Errorf("unblock reminder %d: %w", id, err) + } + return requireOneReminderRow(res, id) +} + +// CompleteReminderDelivery applies the bookkeeping for every original covered +// by one successful external send in a single transaction. One-shot reminders +// become fired; recurring reminders advance to their next occurrence and shed +// the old presentation/retry state. The all-or-nothing boundary prevents a +// collapsed digest from becoming half-fired and then being repeated. +func (s *Store) CompleteReminderDelivery(ctx context.Context, originals []Reminder, now time.Time) error { + return s.completeReminderDeliveryIn(ctx, 0, originals, now, time.Local) +} + +// CompleteSuccessfulReminderAttempt atomically closes a definitely successful +// external attempt and advances every reminder occurrence it represented. If +// this transaction cannot commit, the attempt remains pending; startup then +// reconciles it to unknown, and the occurrence is held from automatic replay. +func (s *Store) CompleteSuccessfulReminderAttempt(ctx context.Context, attemptID int64, originals []Reminder, now time.Time) error { + if attemptID <= 0 { + return fmt.Errorf("complete reminder delivery: invalid attempt id %d", attemptID) + } + return s.completeReminderDeliveryIn(ctx, attemptID, originals, now, time.Local) +} + +func (s *Store) completeReminderDeliveryIn(ctx context.Context, attemptID int64, originals []Reminder, now time.Time, loc *time.Location) error { + if len(originals) == 0 { + return ErrReminderNotFound + } + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("complete reminder delivery: begin: %w", err) + } + defer tx.Rollback() + if attemptID != 0 { + res, err := tx.ExecContext(ctx, + `UPDATE delivery_attempts SET status = ?, completed_ts = ? WHERE id = ? AND status = ?`, + DeliverySent, now.UnixMilli(), attemptID, DeliveryPending) + if err != nil { + return fmt.Errorf("complete reminder delivery attempt %d: %w", attemptID, err) + } + n, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("complete reminder delivery attempt %d: rows affected: %w", attemptID, err) + } + if n != 1 { + return fmt.Errorf("complete reminder delivery attempt %d: not pending", attemptID) + } + } + + seen := make(map[int64]struct{}, len(originals)) + for _, expected := range originals { + if expected.ID <= 0 || expected.NextFireTs.IsZero() { + return ErrReminderNotFound + } + if _, ok := seen[expected.ID]; ok { + return fmt.Errorf("%w: duplicate reminder %d in one delivery", ErrReminderState, expected.ID) + } + seen[expected.ID] = struct{}{} + + row := tx.QueryRowContext(ctx, `SELECT `+reminderColumns+` FROM reminders WHERE id = ?`, expected.ID) + current, err := scanReminder(row) + if errors.Is(err, sql.ErrNoRows) { + return ErrReminderNotFound + } + if err != nil { + return fmt.Errorf("complete reminder %d: read: %w", expected.ID, err) + } + if current.Status != ReminderPending || !current.NextFireTs.Equal(expected.NextFireTs) { + return fmt.Errorf("%w: reminder %d occurrence changed", ErrReminderState, expected.ID) + } + if expected.DeliveryGroup != "" && current.DeliveryGroup != expected.DeliveryGroup { + return fmt.Errorf("%w: reminder %d delivery group changed", ErrReminderState, expected.ID) + } + + if current.Cron == "" { + if err := completeOneShotReminderTx(ctx, tx, current); err != nil { + return err + } + continue + } + next, err := nextReminderOccurrence(current, now, loc) + if err != nil { + return err + } + if next.IsZero() { + if err := completeOneShotReminderTx(ctx, tx, current); err != nil { + return err + } + continue + } + res, err := tx.ExecContext(ctx, ` + UPDATE reminders + SET next_fire_ts = ?, delivery_group = '', phrase_body = '', + phrase_summary = '', phrase_mood = '', delivery_attempts = 0, + next_attempt_ts = NULL, delivery_blocked_ts = NULL, + delivery_blocked_error = '' + WHERE id = ? AND status = ? AND next_fire_ts = ?`, + next.UnixMilli(), current.ID, ReminderPending, current.NextFireTs.UnixMilli()) + if err != nil { + return fmt.Errorf("complete recurring reminder %d: %w", current.ID, err) + } + if err := requireOneReminderRow(res, current.ID); err != nil { + return err + } + } + + if err := tx.Commit(); err != nil { + return fmt.Errorf("complete reminder delivery: commit: %w", err) + } + return nil +} + +func completeOneShotReminderTx(ctx context.Context, tx *sql.Tx, r Reminder) error { + res, err := tx.ExecContext(ctx, + `UPDATE reminders SET status = ? WHERE id = ? AND status = ? AND next_fire_ts = ?`, + ReminderFired, r.ID, ReminderPending, r.NextFireTs.UnixMilli()) + if err != nil { + return fmt.Errorf("complete reminder %d: %w", r.ID, err) + } + return requireOneReminderRow(res, r.ID) +} + +func requireOneReminderRow(res sql.Result, id int64) error { + n, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("complete reminder %d: rows affected: %w", id, err) + } + if n != 1 { + return fmt.Errorf("%w: reminder %d changed concurrently", ErrReminderState, id) + } + return nil +} + // RescheduleReminder computes the next fire time for a recurring reminder and // updates next_fire_ts. Returns ErrReminderState if the reminder is not // recurring or not pending. If the schedule yields no further fire time at all, @@ -229,8 +633,7 @@ func (s *Store) RescheduleReminder(ctx context.Context, id int64, now time.Time) } func (s *Store) rescheduleReminderIn(ctx context.Context, id int64, now time.Time, loc *time.Location) error { - row := s.db.QueryRowContext(ctx, ` - SELECT id, created_ts, fire_ts, next_fire_ts, payload, status, cron + row := s.db.QueryRowContext(ctx, `SELECT `+reminderColumns+` FROM reminders WHERE id = ?`, id) r, err := scanReminder(row) if err != nil { @@ -243,9 +646,38 @@ func (s *Store) rescheduleReminderIn(ctx context.Context, id int64, now time.Tim return fmt.Errorf("%w: currently %s", ErrReminderState, r.Status) } + next, err := nextReminderOccurrence(r, now, loc) + if err != nil { + return err + } + if next.IsZero() { + return s.MarkReminder(ctx, id, ReminderFired) + } + res, err := s.db.ExecContext(ctx, ` + UPDATE reminders + SET next_fire_ts = ?, delivery_group = '', phrase_body = '', + phrase_summary = '', phrase_mood = '', delivery_attempts = 0, + next_attempt_ts = NULL, delivery_blocked_ts = NULL, + delivery_blocked_error = '' + WHERE id = ? AND status = ? AND next_fire_ts = ?`, + next.UnixMilli(), id, ReminderPending, r.NextFireTs.UnixMilli()) + if err != nil { + return err + } + n, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("reschedule reminder: rows affected: %w", err) + } + if n != 1 { + return fmt.Errorf("%w: reminder %d changed concurrently", ErrReminderState, id) + } + return nil +} + +func nextReminderOccurrence(r Reminder, now time.Time, loc *time.Location) (time.Time, error) { sched, err := cron.ParseStandard(r.Cron) if err != nil { - return fmt.Errorf("parse cron %q: %w", r.Cron, err) + return time.Time{}, fmt.Errorf("parse cron %q: %w", r.Cron, err) } // Next is strictly after the time it is given, so the last fire cannot be // returned again and no fudge minute is needed. The bound stops a schedule @@ -255,9 +687,7 @@ func (s *Store) rescheduleReminderIn(ctx context.Context, id int64, now time.Tim next = sched.Next(next) } if next.IsZero() || !next.After(now) { - _, err = s.db.ExecContext(ctx, "UPDATE reminders SET status = 'fired' WHERE id = ?", id) - return err + return time.Time{}, nil } - _, err = s.db.ExecContext(ctx, "UPDATE reminders SET next_fire_ts = ? WHERE id = ?", next.UnixMilli(), id) - return err + return next, nil } diff --git a/internal/store/reminders_delivery_test.go b/internal/store/reminders_delivery_test.go new file mode 100644 index 0000000..02fa400 --- /dev/null +++ b/internal/store/reminders_delivery_test.go @@ -0,0 +1,417 @@ +package store + +import ( + "context" + "errors" + "path/filepath" + "sync" + "testing" + "time" +) + +func TestReminderPhraseAndRetrySurviveRestart(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "reminders.db") + now := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + + s, err := Open(ctx, path) + if err != nil { + t.Fatal(err) + } + id, err := s.CreateReminder(ctx, now.Add(-time.Minute), `{"text":"позвонить маме"}`, "") + if err != nil { + t.Fatal(err) + } + due, err := s.DueReminders(ctx, now) + if err != nil || len(due) != 1 { + t.Fatalf("initial due reminders = %d, err=%v", len(due), err) + } + if err := s.CacheReminderPhrase(ctx, due, "reminder:one", "Позвони маме.", "Позвони маме", "warm"); err != nil { + t.Fatalf("cache phrase: %v", err) + } + if err := s.ScheduleReminderRetry(ctx, due, now); err != nil { + t.Fatalf("schedule retry: %v", err) + } + if err := s.Close(); err != nil { + t.Fatal(err) + } + + s, err = Open(ctx, path) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = s.Close() }) + if got, err := s.DueReminders(ctx, now.Add(30*time.Second)); err != nil || len(got) != 0 { + t.Fatalf("during retry wait: got %d due, err=%v", len(got), err) + } + due, err = s.DueReminders(ctx, now.Add(ReminderRetryBase)) + if err != nil || len(due) != 1 { + t.Fatalf("at retry time: got %d due, err=%v", len(due), err) + } + r := due[0] + if r.ID != id || r.DeliveryGroup != "reminder:one" || r.PhraseBody != "Позвони маме." || + r.PhraseSummary != "Позвони маме" || r.PhraseMood != "warm" { + t.Fatalf("persisted delivery presentation = %+v", r) + } + if r.DeliveryAttempts != 1 { + t.Fatalf("attempts = %d, want 1", r.DeliveryAttempts) + } + if want := now.Add(ReminderRetryBase); !r.NextAttemptTs.Equal(want) { + t.Fatalf("next attempt = %s, want %s", r.NextAttemptTs, want) + } +} + +func TestReminderRetryDelayIsExponentialAndBounded(t *testing.T) { + wants := []time.Duration{ + time.Minute, 2 * time.Minute, 4 * time.Minute, 8 * time.Minute, + 16 * time.Minute, 32 * time.Minute, time.Hour, time.Hour, time.Hour, + } + for i, want := range wants { + if got := ReminderRetryDelay(i + 1); got != want { + t.Errorf("attempt %d delay = %s, want %s", i+1, got, want) + } + } +} + +func TestCollapsedReminderRetryStateMovesTogether(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + for _, text := range []string{"полить цветы", "позвонить врачу"} { + if _, err := s.CreateReminder(ctx, now.Add(-time.Minute), text, ""); err != nil { + t.Fatal(err) + } + } + originals, err := s.DueReminders(ctx, now) + if err != nil || len(originals) != 2 { + t.Fatalf("due originals = %d, err=%v", len(originals), err) + } + if err := s.CacheReminderPhrase(ctx, originals, "reminder:bundle", "У тебя два напоминания.", "Два напоминания", "neutral"); err != nil { + t.Fatal(err) + } + if err := s.ScheduleReminderRetry(ctx, originals, now); err != nil { + t.Fatal(err) + } + + rows, err := s.ListReminders(ctx, 10) + if err != nil || len(rows) != 2 { + t.Fatalf("list = %d, err=%v", len(rows), err) + } + for _, r := range rows { + if r.DeliveryGroup != "reminder:bundle" || r.PhraseBody != "У тебя два напоминания." || r.DeliveryAttempts != 1 { + t.Errorf("bundle original did not move with group: %+v", r) + } + if !r.NextAttemptTs.Equal(now.Add(time.Minute)) { + t.Errorf("next attempt = %s, want %s", r.NextAttemptTs, now.Add(time.Minute)) + } + } +} + +func TestRecurringReminderClearsDeliveryStateForNextOccurrence(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + loc := time.FixedZone("MSK", 3*60*60) + fire := time.Date(2026, 8, 13, 9, 0, 0, 0, loc) + id, err := s.CreateReminder(ctx, fire, `{"text":"стендап"}`, "0 9 * * *") + if err != nil { + t.Fatal(err) + } + due, err := s.DueReminders(ctx, fire) + if err != nil || len(due) != 1 { + t.Fatalf("due = %d, err=%v", len(due), err) + } + if err := s.CacheReminderPhrase(ctx, due, "reminder:occurrence", "Пора на стендап.", "Стендап", "neutral"); err != nil { + t.Fatal(err) + } + if err := s.ScheduleReminderRetry(ctx, due, fire); err != nil { + t.Fatal(err) + } + if err := s.rescheduleReminderIn(ctx, id, fire.Add(time.Minute), loc); err != nil { + t.Fatalf("reschedule: %v", err) + } + + rows, err := s.ListReminders(ctx, 1) + if err != nil || len(rows) != 1 { + t.Fatalf("list = %d, err=%v", len(rows), err) + } + r := rows[0] + if r.Status != ReminderPending || r.DeliveryGroup != "" || r.PhraseBody != "" || + r.PhraseSummary != "" || r.PhraseMood != "" || r.DeliveryAttempts != 0 || !r.NextAttemptTs.IsZero() { + t.Fatalf("next occurrence retained old delivery state: %+v", r) + } + want := time.Date(2026, 8, 14, 9, 0, 0, 0, loc) + if !r.NextFireTs.Equal(want) { + t.Fatalf("next fire = %s, want %s", r.NextFireTs.In(loc), want) + } +} + +func TestReminderTerminalTransitionHasExactlyOneWinner(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + + for iteration := 0; iteration < 40; iteration++ { + id, err := s.CreateReminder(ctx, now, "race", "") + if err != nil { + t.Fatal(err) + } + start := make(chan struct{}) + var wg sync.WaitGroup + errs := make([]error, 2) + statuses := []string{ReminderFired, ReminderCancelled} + for i := range statuses { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + errs[i] = s.MarkReminder(ctx, id, statuses[i]) + }(i) + } + close(start) + wg.Wait() + + successes := 0 + losers := 0 + winner := "" + for i, err := range errs { + switch { + case err == nil: + successes++ + winner = statuses[i] + case errors.Is(err, ErrReminderState): + losers++ + default: + t.Fatalf("iteration %d transition %s: %v", iteration, statuses[i], err) + } + } + if successes != 1 || losers != 1 { + t.Fatalf("iteration %d: successes=%d losers=%d errors=%v", iteration, successes, losers, errs) + } + var stored string + if err := s.db.QueryRowContext(ctx, `SELECT status FROM reminders WHERE id = ?`, id).Scan(&stored); err != nil { + t.Fatal(err) + } + if stored != winner { + t.Fatalf("iteration %d: stored %q, successful transition %q", iteration, stored, winner) + } + } +} + +func TestAmbiguousCollapsedDeliveryIsNotAutomaticallyRepeated(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + for _, text := range []string{"полить цветы", "позвонить врачу"} { + if _, err := s.CreateReminder(ctx, now.Add(-time.Minute), text, ""); err != nil { + t.Fatal(err) + } + } + originals, err := s.DueReminders(ctx, now) + if err != nil || len(originals) != 2 { + t.Fatalf("due originals = %d, err=%v", len(originals), err) + } + const group = "reminder:ambiguous-bundle" + if err := s.CacheReminderPhrase(ctx, originals, group, "У тебя два напоминания.", "Два напоминания", "neutral"); err != nil { + t.Fatal(err) + } + for i := range originals { + originals[i].DeliveryGroup = group + } + + // Simulate a process dying after Begin and before it can record whether the + // external sink accepted the bundle. Synthetic reminder zero is never put + // in the outbox: a real original is the human-readable representative and + // the group is the occurrence identity shared by both originals. + attemptID, err := s.BeginDeliveryAttempt(ctx, "reminder", "", originals[0].ID, group, "telegram", "hash", now) + if err != nil { + t.Fatal(err) + } + if _, err := s.ReconcileStaleDeliveryAttempts(ctx, now.Add(time.Minute)); err != nil { + t.Fatal(err) + } + if due, err := s.DueReminders(ctx, now.Add(24*time.Hour)); err != nil || len(due) != 0 { + t.Fatalf("ambiguous delivered bundle was eligible for automatic repeat: due=%+v err=%v", due, err) + } + attempts, err := s.ListDeliveryAttempts(ctx, "", 10) + if err != nil || len(attempts) != 1 { + t.Fatalf("attempts=%+v err=%v", attempts, err) + } + if attempts[0].ID != attemptID || attempts[0].Status != DeliveryUnknown || attempts[0].DeliveryGroup != group { + t.Fatalf("ambiguous occurrence lost its durable identity: %+v", attempts[0]) + } + if err := s.ScheduleReminderRetry(ctx, originals, now.Add(time.Minute)); err != nil { + t.Fatal(err) + } + rows, err := s.ListReminders(ctx, 10) + if err != nil || len(rows) != 2 { + t.Fatalf("rows=%+v err=%v", rows, err) + } + for _, r := range rows { + if r.DeliveryAttempts != 0 || !r.NextAttemptTs.IsZero() { + t.Fatalf("ambiguous occurrence was scheduled for automatic retry: %+v", r) + } + } +} + +func TestDefiniteFailedDeliveryRemainsRetryEligible(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + if _, err := s.CreateReminder(ctx, now.Add(-time.Minute), "позвонить врачу", ""); err != nil { + t.Fatal(err) + } + originals, err := s.DueReminders(ctx, now) + if err != nil || len(originals) != 1 { + t.Fatalf("due originals = %d, err=%v", len(originals), err) + } + const group = "reminder:definite-failure" + if err := s.CacheReminderPhrase(ctx, originals, group, "Позвони врачу.", "Позвони врачу", "neutral"); err != nil { + t.Fatal(err) + } + id, err := s.BeginDeliveryAttempt(ctx, "reminder", "", originals[0].ID, group, "telegram", "hash", now) + if err != nil { + t.Fatal(err) + } + if err := s.CompleteDeliveryAttempt(ctx, id, DeliveryFailed, now); err != nil { + t.Fatal(err) + } + if due, err := s.DueReminders(ctx, now); err != nil || len(due) != 1 { + t.Fatalf("definite failure was not retry eligible: due=%+v err=%v", due, err) + } +} + +func TestCollapsedCompletionRollsBackAsOneTransaction(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + for _, text := range []string{"first", "fail-second"} { + if _, err := s.CreateReminder(ctx, now.Add(-time.Minute), text, ""); err != nil { + t.Fatal(err) + } + } + originals, err := s.DueReminders(ctx, now) + if err != nil || len(originals) != 2 { + t.Fatalf("due originals = %d, err=%v", len(originals), err) + } + if _, err := s.db.ExecContext(ctx, ` + CREATE TRIGGER fail_second_reminder_completion + BEFORE UPDATE OF status ON reminders + WHEN OLD.payload = 'fail-second' + BEGIN + SELECT RAISE(ABORT, 'injected second completion failure'); + END`); err != nil { + t.Fatal(err) + } + if err := s.CompleteReminderDelivery(ctx, originals, now); err == nil { + t.Fatal("injected second-row failure did not fail the bundle completion") + } + rows, err := s.ListReminders(ctx, 10) + if err != nil || len(rows) != 2 { + t.Fatalf("rows=%+v err=%v", rows, err) + } + for _, r := range rows { + if r.Status != ReminderPending { + t.Fatalf("bundle completion partially committed reminder %d as %q", r.ID, r.Status) + } + } +} + +func TestSuccessfulAttemptAndCollapsedCompletionCommitAtomically(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + for _, text := range []string{"first", "fail-second"} { + if _, err := s.CreateReminder(ctx, now.Add(-time.Minute), text, ""); err != nil { + t.Fatal(err) + } + } + originals, err := s.DueReminders(ctx, now) + if err != nil || len(originals) != 2 { + t.Fatalf("due originals = %d, err=%v", len(originals), err) + } + const group = "reminder:atomic-success" + if err := s.CacheReminderPhrase(ctx, originals, group, "Two reminders.", "Two reminders", "neutral"); err != nil { + t.Fatal(err) + } + for i := range originals { + originals[i].DeliveryGroup = group + } + attemptID, err := s.BeginDeliveryAttempt(ctx, "reminder", "", originals[0].ID, group, "telegram", "hash", now) + if err != nil { + t.Fatal(err) + } + if _, err := s.db.ExecContext(ctx, ` + CREATE TRIGGER fail_atomic_second_completion + BEFORE UPDATE OF status ON reminders + WHEN OLD.payload = 'fail-second' + BEGIN + SELECT RAISE(ABORT, 'injected second completion failure'); + END`); err != nil { + t.Fatal(err) + } + if err := s.CompleteSuccessfulReminderAttempt(ctx, attemptID, originals, now); err == nil { + t.Fatal("injected reminder completion failure did not fail the local commit") + } + attempts, err := s.ListDeliveryAttempts(ctx, "", 10) + if err != nil || len(attempts) != 1 || attempts[0].Status != DeliveryPending { + t.Fatalf("successful outbox marker committed without reminder state: attempts=%+v err=%v", attempts, err) + } + rows, err := s.ListReminders(ctx, 10) + if err != nil || len(rows) != 2 { + t.Fatalf("rows=%+v err=%v", rows, err) + } + for _, r := range rows { + if r.Status != ReminderPending { + t.Fatalf("atomic rollback partially completed reminder %d as %q", r.ID, r.Status) + } + } + if _, err := s.ReconcileStaleDeliveryAttempts(ctx, now.Add(time.Minute)); err != nil { + t.Fatal(err) + } + if due, err := s.DueReminders(ctx, now.Add(24*time.Hour)); err != nil || len(due) != 0 { + t.Fatalf("restart auto-replayed ambiguous accepted send: due=%+v err=%v", due, err) + } +} + +func TestPermanentReminderFailureBlocksUntilDeliberatelyUnblocked(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Date(2026, 8, 13, 8, 0, 0, 0, time.UTC) + id, err := s.CreateReminder(ctx, now.Add(-time.Minute), "позвонить врачу", "") + if err != nil { + t.Fatal(err) + } + originals, err := s.DueReminders(ctx, now) + if err != nil || len(originals) != 1 { + t.Fatalf("due originals=%+v err=%v", originals, err) + } + const group = "reminder:revoked-token" + if err := s.CacheReminderPhrase(ctx, originals, group, "Позвони врачу.", "Позвони врачу", "neutral"); err != nil { + t.Fatal(err) + } + originals[0].DeliveryGroup = group + if err := s.BlockReminderDelivery(ctx, originals, now, "ntfy credentials rejected"); err != nil { + t.Fatal(err) + } + if err := s.ScheduleReminderRetry(ctx, originals, now); err != nil { + t.Fatal(err) + } + if due, err := s.DueReminders(ctx, now.Add(24*time.Hour)); err != nil || len(due) != 0 { + t.Fatalf("blocked reminder retried automatically: due=%+v err=%v", due, err) + } + rows, err := s.ListReminders(ctx, 1) + if err != nil || len(rows) != 1 { + t.Fatalf("rows=%+v err=%v", rows, err) + } + if rows[0].DeliveryBlockedTs.IsZero() || rows[0].DeliveryBlockedError != "ntfy credentials rejected" || + rows[0].DeliveryAttempts != 0 || !rows[0].NextAttemptTs.IsZero() { + t.Fatalf("blocked state is not visible/durable: %+v", rows[0]) + } + if err := s.UnblockReminderDelivery(ctx, id); err != nil { + t.Fatal(err) + } + due, err := s.DueReminders(ctx, now) + if err != nil || len(due) != 1 || due[0].ID != id || !due[0].DeliveryBlockedTs.IsZero() || due[0].DeliveryBlockedError != "" { + t.Fatalf("deliberate unblock did not restore eligibility: due=%+v err=%v", due, err) + } +}