jul31 session: five small fixes plus the voice.go/ipc decomposition #50

Closed
kami wants to merge 0 commits from integration/small-batch into master
Owner

One branch, two kinds of change: five self-contained fixes off the board, and a
mechanical decomposition of the two files that had grown past reading.

Fixes

  • #354 nginx template. deploy/ecosystem/nginx.conf bound listen 80, all
    interfaces, while the live host is LAN/WireGuard-scoped. Now matches
    cmd/mavweb/nginx.conf verbatim: explicit 10.42.0.1 + 192.168.1.104 listens,
    allow/deny all. Deploy was unaffected either way — nginx holds the LAN port and
    proxies to loopback.
  • #317 mavweb default bind. -addr defaults to 127.0.0.1:9200 instead of
    :9200. POST /api/chat is the one mutating handler with no session check, so an
    accidental all-interfaces default was the wrong side to fail on. compose passes
    -addr :9201 explicitly, so deploy behaviour is unchanged.
  • #359 LLM router could never ask for clarification. Two bugs: Confidence: 1.0
    was hardcoded, and the LLM branch never consulted r.threshold at all, so a correct
    low confidence would have been discarded anyway. gateLLMDecision now feeds three
    structural signals into the same stage-3 gate the classifier path already used.
    Prompt untouched, so check_prompt_parity.py still passes. Re-measured on the
    77-case fixture: missed clarify 6/6 -> 1, costing 3 false clarifies and 2.6pt of
    full accuracy. Two of the three false clarifies are acts the model mis-routed and the
    gate caught — asking beats wrongly executing. The third, поужинал, is a real
    defect: the single-token rule is an English intuition and does not transfer to
    Russian. Noted on the task, not fixed here.
  • #43 pattern detection ran only from voice. Now also runs from the digestion tick
    over DistinctEventPairs. Dedupe and dismissal were already free at the store layer
    (UNIQUE(action, object) + ON CONFLICT DO NOTHING, dismissal flips status in place).
  • #281 digest as a real outcome. New digest_entries table (migration 13).
    Suppressed sev2 nudges are queued and resurfaced instead of dropped; sev1 still drops,
    high severity never digests. Only quiet_hours/calendar_busy/presence qualify —
    cooldown and snooze do not. 24h expiry, max 3 spoken items.

Decomposition

cmd/mavend/voice.go 1923 -> 491, in six move-only slices:

file lines
voice.go 491
voicewire.go 434
actions.go 362
ecosystem_acts.go 256
confirm.go 205
ruwords.go 180
strutil.go 85
weatherq.go 50

What remains in voice.go is the handler and nothing else: reactiveHandler,
HandlePushToTalk, handleText, applyAction, detectPattern, resolveQuietToggle,
replySystem, chatHistory, reply.

Every slice was verified move-only by diffing each non-blank removed line against the
new file and requiring zero unmatched, plus zero lines added to voice.go — not by
trusting the diff stat. As a whole-refactor check, all 40 top-level funcs in
master:voice.go were enumerated and confirmed still present somewhere in
cmd/mavend/. One is intentionally absent: jsonStringImpl, a one-line passthrough
collapsed into jsonString.

Two structural changes beyond pure moves:

  • applyAction's 300-line intent switch is now a map[router.Intent]func(...) table.
    All 7 intents present. The destructive-act confirm gate and the enabled-tool
    allowlist turned out not to be cross-cutting — they only ever fire inside IntentAct
    — so they stayed inside actionAct, and clarify.go's invariant holds because
    finishClarified reaches the same handler through the same table.
  • internal/ipc/server.go's 42-arm dispatcher is table-driven, net -65 lines.
    Accounting checked: 30 CoreAPI methods -> 30 table entries; 33 Method constants =
    30 + the 3 that bypass CoreAPI (AssertStepUp, StoreEncryptionKey, Unlock).
    s.Check still runs before the table lookup, so locked mode is unchanged.
    wire.go/client.go/api.go diffs are empty.

The confirm/park gate is security-relevant, so it got a stronger check than move-only:
classifyConfirm and resolveConfirm bodies diff byte-identical against master.
confirmTTL is still 90s and the verdict ordering is unchanged at all three switch
sites.

Also: dead lockedAPI (~90 lines) deleted — srv.Check is default-deny in locked mode,
so it was unreachable. Replaced with a new ipc.UnimplementedCoreAPI, which also let
the test doubles drop 55 stub methods. And mavwaked, an 11 MB build artifact tracked
in master, is deleted and gitignored (.gitignore had 7 of the 8 binaries).

make build: 8 binaries. gofmt silent, go vet clean, make test: 38 packages, no
FAIL, no RACE.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ

One branch, two kinds of change: five self-contained fixes off the board, and a mechanical decomposition of the two files that had grown past reading. ## Fixes - **#354 nginx template.** `deploy/ecosystem/nginx.conf` bound `listen 80`, all interfaces, while the live host is LAN/WireGuard-scoped. Now matches `cmd/mavweb/nginx.conf` verbatim: explicit `10.42.0.1` + `192.168.1.104` listens, `allow`/`deny all`. Deploy was unaffected either way — nginx holds the LAN port and proxies to loopback. - **#317 mavweb default bind.** `-addr` defaults to `127.0.0.1:9200` instead of `:9200`. `POST /api/chat` is the one mutating handler with no session check, so an accidental all-interfaces default was the wrong side to fail on. compose passes `-addr :9201` explicitly, so deploy behaviour is unchanged. - **#359 LLM router could never ask for clarification.** Two bugs: `Confidence: 1.0` was hardcoded, and the LLM branch never consulted `r.threshold` at all, so a correct low confidence would have been discarded anyway. `gateLLMDecision` now feeds three structural signals into the same stage-3 gate the classifier path already used. Prompt untouched, so `check_prompt_parity.py` still passes. Re-measured on the 77-case fixture: **missed clarify 6/6 -> 1**, costing 3 false clarifies and 2.6pt of full accuracy. Two of the three false clarifies are acts the model mis-routed and the gate caught — asking beats wrongly executing. The third, `поужинал`, is a real defect: the single-token rule is an English intuition and does not transfer to Russian. Noted on the task, not fixed here. - **#43 pattern detection ran only from voice.** Now also runs from the digestion tick over `DistinctEventPairs`. Dedupe and dismissal were already free at the store layer (`UNIQUE(action, object)` + `ON CONFLICT DO NOTHING`, dismissal flips status in place). - **#281 digest as a real outcome.** New `digest_entries` table (migration 13). Suppressed sev2 nudges are queued and resurfaced instead of dropped; sev1 still drops, high severity never digests. Only `quiet_hours`/`calendar_busy`/`presence` qualify — cooldown and snooze do not. 24h expiry, max 3 spoken items. ## Decomposition `cmd/mavend/voice.go` **1923 -> 491**, in six move-only slices: | file | lines | |---|---| | voice.go | 491 | | voicewire.go | 434 | | actions.go | 362 | | ecosystem_acts.go | 256 | | confirm.go | 205 | | ruwords.go | 180 | | strutil.go | 85 | | weatherq.go | 50 | What remains in voice.go is the handler and nothing else: `reactiveHandler`, `HandlePushToTalk`, `handleText`, `applyAction`, `detectPattern`, `resolveQuietToggle`, `replySystem`, `chatHistory`, `reply`. Every slice was verified move-only by diffing each non-blank removed line against the new file and requiring zero unmatched, plus zero lines *added* to voice.go — not by trusting the diff stat. As a whole-refactor check, all 40 top-level funcs in `master:voice.go` were enumerated and confirmed still present somewhere in `cmd/mavend/`. One is intentionally absent: `jsonStringImpl`, a one-line passthrough collapsed into `jsonString`. Two structural changes beyond pure moves: - `applyAction`'s 300-line intent switch is now a `map[router.Intent]func(...)` table. All 7 intents present. The destructive-act confirm gate and the enabled-tool allowlist turned out not to be cross-cutting — they only ever fire inside `IntentAct` — so they stayed inside `actionAct`, and `clarify.go`'s invariant holds because `finishClarified` reaches the same handler through the same table. - `internal/ipc/server.go`'s 42-arm dispatcher is table-driven, net -65 lines. Accounting checked: 30 CoreAPI methods -> 30 table entries; 33 `Method` constants = 30 + the 3 that bypass CoreAPI (`AssertStepUp`, `StoreEncryptionKey`, `Unlock`). `s.Check` still runs before the table lookup, so locked mode is unchanged. `wire.go`/`client.go`/`api.go` diffs are empty. The confirm/park gate is security-relevant, so it got a stronger check than move-only: `classifyConfirm` and `resolveConfirm` bodies diff **byte-identical against master**. `confirmTTL` is still 90s and the verdict ordering is unchanged at all three switch sites. Also: dead `lockedAPI` (~90 lines) deleted — `srv.Check` is default-deny in locked mode, so it was unreachable. Replaced with a new `ipc.UnimplementedCoreAPI`, which also let the test doubles drop 55 stub methods. And `mavwaked`, an 11 MB build artifact tracked in `master`, is deleted and gitignored (`.gitignore` had 7 of the 8 binaries). `make build`: 8 binaries. `gofmt` silent, `go vet` clean, `make test`: 38 packages, no FAIL, no RACE. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
kami added 19 commits 2026-07-31 21:45:08 +02:00
The template said "drop into your nginx sites" but listened on the
wildcard `listen 80;` with no allow/deny ACL, unlike the actual deployed
hexis.kvmx.ru config which binds only to the WireGuard (10.42.0.1) and
LAN (192.168.1.104) addresses with allow/deny all. Anyone following the
template as written would expose these unauthenticated admin UIs to the
open internet.

Bind explicitly to those two addresses and add the matching ACL block,
mirroring cmd/mavweb/nginx.conf which already does this correctly.
Added a comment naming both addresses as host-specific so a deploy on a
different box swaps the IPs instead of reverting to `listen 80` when the
bind fails.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
PR #47 added two state-changing routes (POST /chat, POST /routines)
behind the -addr flag, which defaulted to ":9200" (all interfaces).
Default now binds 127.0.0.1:9200; anyone who wants LAN/wider exposure
still passes an explicit bind (as deploy/docker-compose.yml already
does with "-addr :9201" inside the container, unaffected by this
default change).

Vikunja #317.
Confidence was hardcoded to 1.0 for every LLM decision, and the LLM branch
in Router.Route returned straight from fillSlots without ever touching the
stage-3 threshold gate — so the LLM path could not produce a Clarify no
matter what confidence a model reported. That is why all 6 want_clarify
cases in the 77-case RU fixture were missed by every model in the bake-off.

Fix reads structural signal instead of changing the (parity-locked) router
prompt: a single-token utterance ("вода", "бэкап") is flagged thin evidence
in llmrouter.go; a fact left keyless or an act that never resolves to an
allowlisted fn, checked after fillSlots so the deterministic parsers get
first crack, is flagged in router.go's new gateLLMDecision. Anything below
config.DefaultRouterThreshold (0.55) now sets Clarify=true through the same
path the classifier already uses.

Added unit tests with a stubbed Completer proving both directions: thin
cases clarify, clean multi-word/resolved-slot cases stay confident. The
77-case fixture re-run against a live llama-server is still needed to
confirm the 6/6 moves — not done here, no llama-server on this box.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
detectPattern only ever fired as a side effect of a voice fact-write, so a
recurring pattern already sitting in history went unnoticed until he
happened to mention it again by voice — the opposite of proactive.

Split the pipeline: extraction (fact -> normalized event) stays where a fact
is written, in voice.go, since it's tied to that write regardless of who's
talking. Detection (events -> stable pattern -> proposed_routines row) moves
into shared code (patterns.go's detectAndPropose) that both the voice path
and the new tick.go:detectPatterns call. The tick runs it every cycle over
every action+object pair on record (store.DistinctEventPairs, added), so a
pattern gets noticed on the daemon's own schedule.

Idempotence and the dismiss-must-stick requirement turned out to already be
handled by the store, not something the tick needs to reinvent:
proposed_routines has UNIQUE(action, object) and CreateProposedRoutine does
ON CONFLICT DO NOTHING, and DismissProposedRoutine flips status in place
without deleting the row. So a pair already proposed, accepted, OR
dismissed is a silent no-op on every later tick — a dismissed pattern can
never resurface, and re-running the scan never spams the /routines page.
Kept the voice-path call (immediate spoken confirmation is a nice feature
UX-wise and is now redundant-but-harmless with the tick, since both paths
share the same guarded detectAndPropose).

Tick-side detection only ever writes a row; it does not notify, ring, or
speak, keeping Maven "not a nag, not autonomous" — the /routines page is
still the only place a proposal becomes visible, and only accepting it
starts producing nudges (fireAcceptedRoutines).

Also fixed the stale vikunja#46 reference in proposed_routines.go — the
TODO it named is what this commit does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Vikunja #281. The interruption policy promised four outcomes — deliver_now,
queue, digest, drop — but only three existed: a care candidate the restraint
gate suppressed for quiet hours / away / calendar-busy simply vanished in
loop.Tick's `continue`, with only the trace remembering why.

internal/morning turned out not to be the natural drain: it's a fixed
Item/FactKey checklist engine, not a generic message bundler, so gate-
suppressed nudge text has nowhere to plug into its evidence model. Built a
parallel (but small, reusing the outbox's shape) durable digest instead:

- internal/store: digest_entries table + EnqueueDigestEntry (dedupes by
  rule+body, mirroring the delivery outbox's bodyHash), PendingDigestEntries,
  ExpireStaleDigestEntries, DrainDigestEntries (mark, never delete — an
  audit trail of what she actually said).
- internal/loop: DigestEligible(severity, blockedBy) is the pure boundary —
  only genuine restraint blocks (quiet_hours/calendar_busy/presence) even
  qualify (cooldown/snooze are not "suppression"); within care, Sev2 (break)
  digests, Sev1 (water/meal — stale by the time anyone could resurface them)
  drops. High severity never digests; alarms bypass the gate and deliver
  unchanged, on purpose.
- cmd/mavend/tick.go: each tick scans ExplainTick's trace for eligible
  blocked candidates, enqueues them, sweeps stale entries (24h expiry — the
  care rules are daily-cadence, so anything older is describing a day
  that's over), and drains the bundle only once the suppression reason has
  actually cleared, capped at 3 spoken items plus a trailing count so a
  digest can't turn into the exact nagging it was built to avoid.

Tests: store-level round-trip/restart-survival/dedupe/expiry/drain, loop-
level severity-boundary unit tests, and tick-level integration tests for
the drain-only-when-clear and never-digest-high-severity behavior.
dispatch() replaces the hand-written switch with a package-level
map[Method]handlerFunc built once at init. Each entry is one
withParams/withParamsVoid/withoutParams call closing only over the
CoreAPI method it invokes — adding a method is now one table line
instead of a new arm.

Check still runs once at the top before any unmarshal, unchanged. The
three non-CoreAPI methods (assert_stepup, store_encryption_key, unlock)
are special-cased before the table lookup since they drive Server
fields (StepUp/WrapKeyFn/UnlockFn), not store state. The current
CoreAPI is loaded once per dispatch and passed into the handler as an
argument, so SetAPI's runtime swap (the unlock transition) still takes
effect on the next request — the table itself never captures an api
value. No wire-format change; existing round-trip and unknown-method
tests in ipc_test.go pass unmodified.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
applyAction (cmd/mavend/voice.go) dispatched all 7 intents from one giant
switch. Extract each case body verbatim into its own actionXxx method in
new cmd/mavend/actions.go, dispatched from an actionHandlers table keyed by
router.Intent. applyAction itself is now just the dec.Clarify guard plus a
table lookup.

No behaviour change: same reply strings, same side-effect order, comments
moved verbatim. The destructive-act confirm gate and the enabled-tool
allowlist stay entirely inside actionAct, exactly where they lived in the
old switch's IntentAct case — they're act-specific, not cross-cutting, so
they don't move to a separate layer. dec.Clarify short-circuit, dialogue
bookkeeping and detectPattern stay outside the table since they run
regardless of intent.

voice.go: 1638 -> 1344 lines. New actions.go: 362 lines.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
voice.go is still the biggest file in cmd/mavend and most of what is left
has nothing to do with the audio path. The ecosystem integration is one
such lump: it talks to Nexus, Praxis and Hexis over HTTP and only touches
the handler for its store and clock. Lifting it into ecosystem_acts.go
puts it next to ecosystem.go, where the clients it drives already live.

Move-only: handlePraxisAct, recordPraxisTrace (called from nowhere else),
handleHexisAct and execHexis verbatim, plus the two imports that became
unused in voice.go.
kami added 3 commits 2026-07-31 21:54:43 +02:00
kami reviewed 2026-07-31 22:18:48 +02:00
@@ -0,0 +59,4 @@
router.IntentQuery: (*reactiveHandler).actionQuery,
}
func (h *reactiveHandler) actionFact(ctx context.Context, dec router.Decision) string {
Author
Owner

seems like each action should have it's own component in the mavend/actions/ dir.

seems like each action should have it's own component in the mavend/actions/ dir.
kami reviewed 2026-07-31 22:20:33 +02:00
@@ -0,0 +13,4 @@
// confirm. The confirmation is bound to the resolved capability + canonical
// target entity so a later "да" can only execute exactly what was proposed
// (ecosystem invariant: protected actions require bound confirmation).
type pendingHexisExec struct {
Author
Owner

can't we move the types in one .go file and reference them in other .go file with functions?

can't we move the types in one .go file and reference them in other .go file with functions?
kami reviewed 2026-07-31 22:22:18 +02:00
@@ -0,0 +67,4 @@
// so a routine confirm doesn't get eaten by a stale tool pending).
pr := h.pendingRoutine
if pr != nil && !h.now().After(pr.expiry) {
switch classifyConfirm(text) {
Author
Owner

feels like this is the same pattern all over again.
we can also make separate .go files with the concrete implementations and just call them in this layer (higher one) - instead of repeating the same switch-case over and over again.

feels like this is the same pattern all over again. we can also make separate .go files with the concrete implementations and just call them in this layer (higher one) - instead of repeating the same switch-case over and over again.
kami reviewed 2026-07-31 22:23:03 +02:00
@@ -0,0 +24,4 @@
// Map verbs and Russian aliases to Praxis tool calls.
// Each case: if the verb matches, call the tool and return a user-facing reply.
switch fn {
Author
Owner

this too feels like an unnecessary switch-case.

this too feels like an unnecessary switch-case.
kami reviewed 2026-07-31 22:23:48 +02:00
@@ -366,3 +276,2 @@
} else {
// locked mode: dummy CoreAPI that returns errLocked for everything
coreAPI = &lockedAPI{}
// locked mode: no real store yet, so there's no meaningful CoreAPI to
Author
Owner

do we have the task for the store implementation?

do we have the task for the store implementation?
kami reviewed 2026-07-31 22:26:17 +02:00
@@ -345,0 +530,4 @@
log.Printf("tick: drain digest entries: %v", err)
}
}
Author
Owner

this file also feels too long.

this file also feels too long.
kami reviewed 2026-07-31 22:27:26 +02:00
@@ -0,0 +21,4 @@
// extractWeatherLocation parses a location from the utterance, or falls back
// to the configured default. Very basic: just checks for known city names.
func extractWeatherLocation(u, defaultLoc string) string {
Author
Owner

feels fucking useless, because it should take the temperature for only current location specified by the user.

feels fucking useless, because it should take the temperature for only current location specified by the user.
kami reviewed 2026-07-31 22:28:51 +02:00
@@ -501,6 +501,239 @@ func (s *Server) safeDispatch(ctx context.Context, req Request) (result json.Raw
return s.dispatch(ctx, req)
}
Author
Owner

this one is also too long.
is there nothing like putting routes in separate files and referencing them (including) in another file in Go?

this one is also too long. is there nothing like putting routes in separate files and referencing them (including) in another file in Go?
kami reviewed 2026-07-31 22:30:05 +02:00
@@ -0,0 +11,4 @@
// Digest entry statuses. pending = enqueued, waiting for a drain. drained =
// spoken as part of a bundle. expired = the tick loop's expiry sweep found it
// past its expires_ts before a drain happened — dropped, not delivered late.
const (
Author
Owner

there're no enums in Go? again.

there're no enums in Go? again.
Author
Owner

keep in mind there's mavwaked binary in this PR - might need to be removed.

keep in mind there's mavwaked binary in this PR - might need to be removed.
Author
Owner

Already handled — that's what commit 7d676eb does. mavwaked is tracked in master (11 MB blob) and this PR deletes it; the diff stat shows mavwaked | Bin 11505880 -> 0 bytes. It is not present at the PR head.

Root cause: .gitignore listed 7 of the 8 built binaries and was simply missing /mavwaked, so it got committed at some point. The line is added in the same commit, so make build now leaves the tree clean instead of re-staging it.

Verified just now: git cat-file -e HEAD:mavwaked fails (not tracked), git check-ignore resolves it to .gitignore:9, and git status is clean after a full make build.

Already handled — that's what commit 7d676eb does. `mavwaked` is tracked in `master` (11 MB blob) and this PR **deletes** it; the diff stat shows `mavwaked | Bin 11505880 -> 0 bytes`. It is not present at the PR head. Root cause: `.gitignore` listed 7 of the 8 built binaries and was simply missing `/mavwaked`, so it got committed at some point. The line is added in the same commit, so `make build` now leaves the tree clean instead of re-staging it. Verified just now: `git cat-file -e HEAD:mavwaked` fails (not tracked), `git check-ignore` resolves it to `.gitignore:9`, and `git status` is clean after a full `make build`.
Author
Owner

Replies to the nine line comments.

actions.go — one component per action in mavend/actions/. A subdirectory means a new package, and that is not free here. Every handler is a method on reactiveHandler, which has 26 unexported fields (stt, tools, phraser, memStore, pending, …) and handlers touch most of them. Moving them out means either exporting all 26 or inventing an interface to pass through — real plumbing, and it would not be a move-only change. Go's unit of encapsulation is the package, not the file, so actions_fact.go / actions_query.go / … inside main buys the same navigability at zero cost. Happy to do that split if 362 lines is still too long; the package extraction I would rather do deliberately, not bolted onto this PR.

confirm.go — types in one file, functions in another. Yes, Go allows that freely: any file in a package sees every type in it. No objection.

confirm.go — 'the same pattern all over again'. You are right and this is the real one. resolveConfirm has three near-identical blocks — pendingRoutine, pendingHexis, pending — each doing expiry check → classifyConfirm → yes/no/unknown → clear the slot. Three copies of one shape. That collapses to a single generic pending-confirm helper. I deliberately did not touch it in this PR because the confirm gate is the destructive-act path and I wanted it byte-identical to master; classifyConfirm and resolveConfirm diff identical right now, which is the whole reason this slice is safe to merge. Filed separately.

ecosystem_acts.go — unnecessary switch-case. Agreed, and it is the same fix applyAction just got: switch fn over capability names + aliases becomes a map. Filed.

main.go — do we have a task for the store implementation? Yes, Vikunja #14 (cold-start unlock: the -wrapped-key-file seam exists, the passkey→L3 half does not). Note this PR deletes lockedAPI as dead code — srv.Check is already default-deny in locked mode with a 2-method allowlist, so it was unreachable. I have updated #14 so it no longer points at a type that stopped existing.

tick.go also too long. 860 lines, correct. Largest are tick (106), flushDigest (65), maybeDrainDigest (59), fireAcceptedRoutines (53). It splits cleanly along digest / routines / morning. Filed. It grew partly because #43 and #281 in this very PR both appended to it.

weatherq.go — 'feels fucking useless'. Agreed, and it is worse than useless. extractWeatherLocation carries a hardcoded 6-city lookup table and, when nothing matches and no default is configured, returns "Moscow" — a made-up answer presented as fact. The configured voice.weather.default_location should be the only source, with no reply at all when it is unset. Filed. This PR only moved the function; I did not want a behaviour change hiding inside a move-only slice.

internal/ipc/server.go too long — can Go include routes from other files? Yes, exactly like the split this PR does to voice.go: same package, more files, no imports or indirection needed. The 882 lines here are two unrelated things — storeAPI, the sqlite-backed CoreAPI implementation (~400 lines of thin passthroughs), and the dispatcher table. Those should be storeapi.go and server.go. Filed.

digest.go — no enums in Go? again. Correct, Go has no enum type. The idiom is a defined type plus constants, which confirm.go in this PR already does (type confirmVerdict int, confirmYes/No/Unknown). digest.go uses bare untyped strings, so nothing stops "drained" being passed where a status is expected. Should be type DigestStatus string. Filed.

Replies to the nine line comments. **`actions.go` — one component per action in `mavend/actions/`.** A subdirectory means a new package, and that is not free here. Every handler is a method on `reactiveHandler`, which has 26 unexported fields (`stt`, `tools`, `phraser`, `memStore`, `pending`, …) and handlers touch most of them. Moving them out means either exporting all 26 or inventing an interface to pass through — real plumbing, and it would not be a move-only change. Go's unit of encapsulation is the package, not the file, so `actions_fact.go` / `actions_query.go` / … inside `main` buys the same navigability at zero cost. Happy to do that split if 362 lines is still too long; the package extraction I would rather do deliberately, not bolted onto this PR. **`confirm.go` — types in one file, functions in another.** Yes, Go allows that freely: any file in a package sees every type in it. No objection. **`confirm.go` — 'the same pattern all over again'.** You are right and this is the real one. `resolveConfirm` has *three* near-identical blocks — `pendingRoutine`, `pendingHexis`, `pending` — each doing expiry check → `classifyConfirm` → yes/no/unknown → clear the slot. Three copies of one shape. That collapses to a single generic pending-confirm helper. I deliberately did not touch it in this PR because the confirm gate is the destructive-act path and I wanted it byte-identical to master; `classifyConfirm` and `resolveConfirm` diff identical right now, which is the whole reason this slice is safe to merge. Filed separately. **`ecosystem_acts.go` — unnecessary switch-case.** Agreed, and it is the same fix `applyAction` just got: `switch fn` over capability names + aliases becomes a map. Filed. **`main.go` — do we have a task for the store implementation?** Yes, Vikunja #14 (cold-start unlock: the `-wrapped-key-file` seam exists, the passkey→L3 half does not). Note this PR *deletes* `lockedAPI` as dead code — `srv.Check` is already default-deny in locked mode with a 2-method allowlist, so it was unreachable. I have updated #14 so it no longer points at a type that stopped existing. **`tick.go` also too long.** 860 lines, correct. Largest are `tick` (106), `flushDigest` (65), `maybeDrainDigest` (59), `fireAcceptedRoutines` (53). It splits cleanly along digest / routines / morning. Filed. It grew partly because #43 and #281 in this very PR both appended to it. **`weatherq.go` — 'feels fucking useless'.** Agreed, and it is worse than useless. `extractWeatherLocation` carries a hardcoded 6-city lookup table and, when nothing matches and no default is configured, **returns "Moscow"** — a made-up answer presented as fact. The configured `voice.weather.default_location` should be the only source, with no reply at all when it is unset. Filed. This PR only moved the function; I did not want a behaviour change hiding inside a move-only slice. **`internal/ipc/server.go` too long — can Go include routes from other files?** Yes, exactly like the split this PR does to voice.go: same package, more files, no imports or indirection needed. The 882 lines here are two unrelated things — `storeAPI`, the sqlite-backed `CoreAPI` implementation (~400 lines of thin passthroughs), and the dispatcher table. Those should be `storeapi.go` and `server.go`. Filed. **`digest.go` — no enums in Go? again.** Correct, Go has no enum type. The idiom is a defined type plus constants, which `confirm.go` in this PR already does (`type confirmVerdict int`, `confirmYes/No/Unknown`). `digest.go` uses bare untyped strings, so nothing stops `"drained"` being passed where a status is expected. Should be `type DigestStatus string`. Filed.
kami added 3 commits 2026-07-31 22:45:25 +02:00
kami added 1 commit 2026-07-31 22:50:30 +02:00
HandlePushToTalk and handleText hand-wrote the same eight-step turn
sequence twice, comments in the latter saying "same as HandlePushToTalk"
four times. Extract it into runTurn(ctx, text) string: the voice path
wraps it in stt/tts, the text path returns it directly.

The two had drifted. The text path was missing the quiet-hours toggle
check entirely, so "тихий режим" over IPC/telegram fell through to the
classifier; unifying gives it the check. It also logged the route result
and applyAction return where the voice path did not — both logs are kept
for both paths.
kami added 3 commits 2026-07-31 22:55:19 +02:00
The six answer sources were hand-unrolled inside one 127-line function.
The intent table is a closed set of 7, but this list is open-ended —
Kiwix (#286), RSS (#258), the crawler (#259) and email (#246) each add
one. Each is now a registry entry: a name plus a method on the handler,
walked in order until one claims the question.

Order is unchanged and still load-bearing (memory before the notes-only
pass, #373), the confidence gate keeps its position and semantics, and
every reply string, log line and best-effort failure is verbatim.
Pure move: actionFact, actionReminder, actionAct and actionNote each get
their own actions_<intent>.go. The two small ones (chat, system) and the
actionHandlers table stay in actions.go, which is now just the dispatch
layer and the notes about what does not belong in it. No behaviour
change — only the file a handler is read in.
kami added 1 commit 2026-07-31 23:03:23 +02:00
claude reviewed 2026-08-01 11:20:14 +02:00
claude left a comment
Contributor

Read the whole batch. The decomposition is faithful. voice.go shrinks by 1300 lines, and the moved blocks (voicewire.go, actions_*.go, confirm.go, patterns.go) match what they replace. The ipc method table and UnimplementedCoreAPI beat the 42-arm switch and the hand-written lockedAPI. A new CoreAPI method now fails loudly instead of going silently unguarded. store/meta.go treating a marker-less DB that already holds vectors as a mismatch is the right call, and the reason is written down.

Three things worth changing, none blocking.

1. A reminder missing both slots dies instead of asking again. cmd/mavend/clarify.go:31 declares IntentReminder: {SlotText, SlotTime}. But askClarify parks Missing: []dialogue.Slot{slot}, only the first gap. Say "напомни" with no subject and no time. She asks "О чём напомнить?". He answers "позвонить маме". StillMissing over the one-element Missing is now empty, so resolveClarifyAnswer rebuilds the decision and hands it to applyAction. That has no time and replies "не получилось разобрать время напоминания." The comment says she asks about one thing on purpose, which is right for one turn. The second gap should still re-enter the clarify loop rather than fall out as an error. Re-park with the remaining wantedSlots after a successful fill.

2. The expiry notice is dropped on a confirm turn. In runTurn, step 1 returns resolveConfirm's reply directly, and expiredNotice is only computed at step 2. So she asks a question, he walks off, the question expires, he comes back and says "да" to a still-parked confirm. The confirm answers and he never hears that the older request was let go. Every other exit in runTurn goes through withNotice. Move the clarifyExpiredNotice() call above the confirm check and wrap that return.

3. clarifyExpiredVariants is prose the eval never sees. Five hand-written lines, with feminine self-reference ("ждала", "отпустила", "не стала ждать") and a plain imperative. None of it runs through CheckAddress or CheckFeminine. It reads correct to me. It is also exactly the kind of string someone later edits reaching for a synonym. A table test asserting the checks pass over clarifyExpiredVariants and clarifyGaveUp costs ten lines.

Non-blocking notes:

  • internal/store/backfill.go embeds every note and vector inside one write transaction. Correct for crash safety, and it only runs under -reembed before serving, so the lock hold cannot block a live daemon. Worth a line in the flag help saying the daemon does not answer until it finishes.
  • internal/persona/persona.go is the right shape. One block, five prompts, rules in code rather than config. can() listing only configured capabilities is the detail that makes it honest.
Read the whole batch. The decomposition is faithful. `voice.go` shrinks by 1300 lines, and the moved blocks (`voicewire.go`, `actions_*.go`, `confirm.go`, `patterns.go`) match what they replace. The `ipc` method table and `UnimplementedCoreAPI` beat the 42-arm switch and the hand-written `lockedAPI`. A new CoreAPI method now fails loudly instead of going silently unguarded. `store/meta.go` treating a marker-less DB that already holds vectors as a mismatch is the right call, and the reason is written down. Three things worth changing, none blocking. **1. A reminder missing both slots dies instead of asking again.** `cmd/mavend/clarify.go:31` declares `IntentReminder: {SlotText, SlotTime}`. But `askClarify` parks `Missing: []dialogue.Slot{slot}`, only the first gap. Say "напомни" with no subject and no time. She asks "О чём напомнить?". He answers "позвонить маме". `StillMissing` over the one-element `Missing` is now empty, so `resolveClarifyAnswer` rebuilds the decision and hands it to `applyAction`. That has no time and replies "не получилось разобрать время напоминания." The comment says she asks about one thing on purpose, which is right for one turn. The second gap should still re-enter the clarify loop rather than fall out as an error. Re-park with the remaining `wantedSlots` after a successful fill. **2. The expiry notice is dropped on a confirm turn.** In `runTurn`, step 1 returns `resolveConfirm`'s reply directly, and `expiredNotice` is only computed at step 2. So she asks a question, he walks off, the question expires, he comes back and says "да" to a still-parked confirm. The confirm answers and he never hears that the older request was let go. Every other exit in `runTurn` goes through `withNotice`. Move the `clarifyExpiredNotice()` call above the confirm check and wrap that return. **3. `clarifyExpiredVariants` is prose the eval never sees.** Five hand-written lines, with feminine self-reference ("ждала", "отпустила", "не стала ждать") and a plain imperative. None of it runs through `CheckAddress` or `CheckFeminine`. It reads correct to me. It is also exactly the kind of string someone later edits reaching for a synonym. A table test asserting the checks pass over `clarifyExpiredVariants` and `clarifyGaveUp` costs ten lines. Non-blocking notes: - `internal/store/backfill.go` embeds every note and vector inside one write transaction. Correct for crash safety, and it only runs under `-reembed` before serving, so the lock hold cannot block a live daemon. Worth a line in the flag help saying the daemon does not answer until it finishes. - `internal/persona/persona.go` is the right shape. One block, five prompts, rules in code rather than config. `can()` listing only configured capabilities is the detail that makes it honest.
kami closed this pull request 2026-08-01 14:51:31 +02:00
Author
Owner

Landed on master. The stack was one linear chain, so #84 carried every commit from #50 up, and master now contains this branch in full. Merging this PR on its own is an empty diff, so it is closed rather than merged. The review findings for it were fixed in the 2026-08-01 pass and are on master as commits on the stack tip, not on this branch.

Landed on master. The stack was one linear chain, so #84 carried every commit from #50 up, and master now contains this branch in full. Merging this PR on its own is an empty diff, so it is closed rather than merged. The review findings for it were fixed in the 2026-08-01 pass and are on master as commits on the stack tip, not on this branch.

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#50