Stop the full nudge body leaving the box, and survive a panicking sink #9

Closed
claude wants to merge 0 commits from overnight/away-leak into master
Contributor

Vikunja #368 and #369. Two privacy/durability bugs in the away path, plus tests. Four commits — more than my usual one, because the tests cover both fixes and splitting them would have meant me rewriting another agent's test file, which is worse for you to read than one extra commit.

#368 — the full nudge body was leaving the box

Away channels are ntfy and telegram: they go off this machine. The rule is that only the short summary leaves. The dispatcher was handing sinks the full Body too and trusting each sink to pick the right field. Two problems with that:

  1. When Summary was empty, both sinks fell back to Body — and the 0.8B model leaves fields empty routinely, so this was not a rare path. The full phrased detail went to Telegram.
  2. The rule lived in each sink, so any away sink added later leaks by default just by reading the obvious field.

Fix: minimalForAway overwrites both Body and Summary with the minimal message before any sink sees the struct, at all three construction sites. The boundary is now enforced where the boundary actually is. Empty summary gets a generic line:

что-то требует внимания: disk-low

No gendered form in it, so it reads correctly regardless.

Voice is untouched and still gets the full body — voice is local, it never leaves the box.

#369 — a panicking sink left a row pending forever

No recover around Send, and reconciliation only runs at startup, so a panic in a live process left a delivery_attempts row pending permanently. Now safeSend recovers, logs it as a bug in the sink, closes the row as failed, and continues to the next channel — a broken voice sink must not eat the ntfy send for the same sev4 nudge.

Worth a look

  • The sinks' own Summary == "" → Body fallback is left in place. It is now harmless (the body they receive is already minimal) and removing it would have meant rewriting two sink test suites. Belt and braces; the belt now hands over a clean body.
  • Commit 4 is reconciliation, and it is the interesting one. Two agents, working blind to each other, wrote the same panicSink helper and the same two test names for these exact bugs — one as skipped placeholders describing the gap, one as real assertions after fixing it. I kept the real assertions and deleted the placeholders. One of the old placeholders also asserted the wrong thing now: it expected an empty message on empty summary, and the answer turned out to be a generic line instead.
Vikunja **#368** and **#369**. Two privacy/durability bugs in the away path, plus tests. Four commits — more than my usual one, because the tests cover both fixes and splitting them would have meant me rewriting another agent's test file, which is worse for you to read than one extra commit. ## #368 — the full nudge body was leaving the box Away channels are ntfy and telegram: they go **off this machine**. The rule is that only the short summary leaves. The dispatcher was handing sinks the full `Body` too and trusting each sink to pick the right field. Two problems with that: 1. When `Summary` was empty, both sinks fell back to `Body` — and **the 0.8B model leaves fields empty routinely**, so this was not a rare path. The full phrased detail went to Telegram. 2. The rule lived in each sink, so any away sink added later leaks by default just by reading the obvious field. Fix: `minimalForAway` overwrites **both** `Body` and `Summary` with the minimal message before any sink sees the struct, at all three construction sites. The boundary is now enforced where the boundary actually is. Empty summary gets a generic line: ``` что-то требует внимания: disk-low ``` No gendered form in it, so it reads correctly regardless. Voice is untouched and still gets the full body — voice is local, it never leaves the box. ## #369 — a panicking sink left a row pending forever No `recover` around `Send`, and reconciliation only runs at startup, so a panic in a live process left a `delivery_attempts` row pending permanently. Now `safeSend` recovers, logs it as a bug in the sink, closes the row as failed, and **continues to the next channel** — a broken voice sink must not eat the ntfy send for the same sev4 nudge. ## Worth a look - The sinks' own `Summary == "" → Body` fallback is left in place. It is now harmless (the body they receive is already minimal) and removing it would have meant rewriting two sink test suites. Belt and braces; the belt now hands over a clean body. - **Commit 4 is reconciliation, and it is the interesting one.** Two agents, working blind to each other, wrote the same `panicSink` helper and the same two test names for these exact bugs — one as skipped placeholders describing the gap, one as real assertions after fixing it. I kept the real assertions and deleted the placeholders. One of the old placeholders also asserted the *wrong* thing now: it expected an empty message on empty summary, and the answer turned out to be a generic line instead.
claude added 33 commits 2026-07-31 00:42:45 +02:00
Table-driven tests for water, meal, break, service_down and netdata_critical,
straight against the predicate with a fake State. Reviewers: the no-data rows
(every rule must stay quiet when its key is missing) and the ops forgery rows,
where a fact with the right value but the wrong source must be refused.
No rule fired on missing data, so no fix was needed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
router.Slots already carries the fact payload; the dialogue copy did not, so a clarifying answer had nowhere to put it. InheritSlots carries it like Key.
Reviewer: check the new inherit block does not overwrite a filled value.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Pins the conservative side of Gate(): quiet hours, away, calendar-busy,
cooldown and snooze, plus one nudge per tick at max severity. Reviewers: the
two skipped tests at the bottom are real gaps, not flakes. Reminders ignore
snooze (loop.go:120) and the Gatherer never fills SnoozeUntil (gather.go:153),
so snooze does nothing at runtime. No behaviour was changed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
PendingQuestion plus ClarifyStore: same shape, locking and expiry as SessionStore. Answer fills only the missing slots and never overwrites a filled one. No wiring yet — TODOs mark the daemon hooks.
Reviewer: MaxAttempts is 1 on purpose (Maven asks once, she is not a nag).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Wires cmd/mavend/voice.go to build the LLM router when the operator asks
for it. Default false, so nothing changes on the deploy box.
Look at pickLLMRouter: the flag on with no llama-server logs one line and
keeps the classifier, it never fails a turn.
The default stays off until the router can refuse (#359) and the extractor
runs on LLM decisions — both noted as TODOs in config.go.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
The MODEL default was LFM2, but the deploy runs Qwen3.5-0.8B, so the
pkill pattern matched nothing and the server survived every kill.
Now matches any llama-server serving a .gguf, so changing the model in
deploy/mavend.json cannot break the script again. MODEL still narrows it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Look at internal/store/proposed_routines.go: status flips in place with an
`AND status = 'proposed'` guard, not append-only like facts/voids_id — a
proposal is a question with one answer, same shape as tools.status. The
UNIQUE(action, object) key is what stops a dismissed routine coming back.
New tests cover re-propose-after-dismiss and listing by status.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
.gitignore had deps/ and /models/llm/ with trailing slashes. A trailing slash
only matches a real directory, so a *symlink* with the same name is not ignored
and git add -A commits it as a symlink blob.

That bites anyone working in a git worktree, where deps/ and models/ do not
exist and have to be linked in from the main checkout. It already happened once
tonight.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Measures whether Maven can find the right note again from a paraphrased
question. Review internal/memory/recalleval/recalleval.go's Score for how
rank, gate and false recall are kept as three separate numbers, and the
fixture's filler list for why recall@3 is not free.
Fixture JSON is generated data and does not count toward the diff limit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
The router can already say "I am not sure" (Decision.Clarify) but the daemon
had nowhere to keep the request while it asked. PendingQuestion holds the
original slots, ClarifyStore parks one per dialogue id with a 90s TTL, and
Answer fills only the slots that were missing so an answer can never rewrite
what she already understood. Logic that uses this comes next.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
A table per intent (reminder needs a time, fact needs a key, act needs a fn)
plus one fixed Russian question per slot. Templates, not model output: a 0.8B
would wander and a question that rewords itself is harder to answer. Note,
query, chat and system get no question — for those a clarify decision keeps
the canned reply rather than inventing a question for noise.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Accepting a routine gives the trigger loop a new standing reason to speak to
the human, so it is the same authority tier as enabling a tool and shares the
stepUpOK gate; dismiss only ever makes maven quieter, so it is ungated.
Look at handleRoutines and acceptRoutine in cmd/mavweb/main.go: accept creates
the recurring reminder, then links it via the new ipc AcceptProposedRoutine.
The page now says what maven noticed in her own words (pattern.PhraseRoutine).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Table-driven tests for all four severity bands crossed with present and away,
both as the pure table and end to end through the dispatcher. Three tests are
written to DESIGN.md and skipped because the code does not keep the claim: the
care-away drop is recorded nowhere, and the minimal body is enforced per-sink
rather than by the dispatcher. Also gofmt'd dispatcher_test.go.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
On a clarify decision with one identifiable gap she now asks instead of saying
"не поняла", and parks the request. The next utterance is parsed as the answer
with the router's own extractor and the completed decision runs through
applyAction like any other — so a clarified act still needs the allowlist and
still hits the destructive confirm gate. An answer that does not fill the gap
drops the request; she never asks twice. Also pulls the session-store block
that HandlePushToTalk and handleText both had into rememberTurn, since the
clarify path needed a third copy.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Fallthrough is checked per severity through the outbox trail, so sev3/sev4
reroute and sev1/sev2 still drop. Durability uses a real store on a temp file:
a crash between Begin and Complete becomes unknown, is not resent, is not
dropped, and a late Complete cannot overwrite it. One skipped test marks a real
gap: a panic mid-send leaves a permanent pending row.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Covers: a reminder with no time is asked about and completes on the answer; the
same for a fact; an answer past the TTL falls through as a fresh utterance; a
second unclear answer drops the request with no second question; a clarified act
off the allowlist neither runs nor gets enabled; a clarified destructive act
still parks a confirm; noise keeps the canned reply. No model, no network.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
The gate honours State.SnoozeUntil but nothing ever filled it. New
store.SnoozedUntil returns, per rule, when the newest snooze runs out.
Reviewer: the fixed 2h SnoozeDuration and its reasoning in nudges.go —
nothing upstream can supply a per-nudge length, so no new column.
Expired snoozes are dropped in SQL, so silence can never be permanent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
# Conflicts:
#	internal/dialogue/clarify.go
#	internal/dialogue/clarify_test.go
The Gatherer now fills State.SnoozeUntil from store.SnoozedUntil instead
of nil, so a snooze finally reaches the gate. RemindDecisions gains the
one restraint check that applies to a reminder — quiet hours, presence
and cooldown are still bypassed, so "wake me 7" is unchanged. Reviewer:
the two tests in internal/loop/gate_test.go are the contract.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Agent worktrees land in .claude/worktrees, so the directory shows up as
untracked noise in every git status.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
# Conflicts:
#	internal/loop/gate_test.go
Real recall is 48% after the gate, and one must-be-silent query gets an
answer anyway. Review finding 2 (the score distributions overlap, so no
gate separates a real recall from a false one) and finding 4 (the memStore
branch at voice.go:776 is unreachable for notes). Adds an embedder cache
so the gate sweep does not re-embed the fixture nine times.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Away channels (ntfy, telegram) leave the box, so an empty Summary now sends
a fixed generic line plus the rule name instead of the full Body. The
dispatcher strips detail before any sink sees it, so a sink added later
cannot leak by reading the wrong field. Voice is local and unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
A panic in Send used to unwind past completeOutbox and leave the
delivery_attempts row pending forever, since reconciliation only runs at
startup. safeSend turns the panic into an error, logs it loudly, records the
attempt failed, and lets the other channels for the same nudge still go out.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
The integration branch names one test TestAwayFallsBackToFullBodyWhenSummaryEmpty,
which describes the old bug; it is here as TestAwaySendsGenericLineWhenSummaryEmpty
and asserts the generic line instead of the body. Also covers: a normal summary
goes out unchanged, voice keeps the full body, and one panicking sink does not
eat the other channel for the same nudge. Reformatted one pre-existing struct.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Two agents wrote the same three test helpers and names for the same two
bugs. Kept the real assertions from dispatcher_test.go and removed the
skipped placeholders they replace. panicSink stays in durability_test.go
since both files use it.

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

this one shouldn't go in master.

this one shouldn't go in master.
Owner

Superseded by #47, which landed this whole stack on master as one reviewed integration merge. This PR head is an ancestor of master — its commits are in, nothing here is lost. Closing as merged-by-proxy rather than merged, since the merge came in through #47.

Review threads on this PR were answered or acted on before the merge; the Russian wording fixes went in as #48.

Superseded by #47, which landed this whole stack on master as one reviewed integration merge. This PR head is an ancestor of master — its commits are in, nothing here is lost. Closing as merged-by-proxy rather than merged, since the merge came in through #47. Review threads on this PR were answered or acted on before the merge; the Russian wording fixes went in as #48.
kami closed this pull request 2026-07-31 20:21:42 +02:00

Pull request closed

Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kami/Maven#9