Maven/REVIEW-30-07-2026.md

# Maven — senior review, 2026-07-30

Scope: code, config, docs, deploy path, test suite. Runtime claims below come from
tests, config and static reading — the daemons were not run and routing latency was
not measured. Working tree at review time: uncommitted `internal/morning` work plus
`cmd/mavend/eval_scenarios_test.go` (both look correct, see "keep").

---

## verdict

**Healthy engineering, misaligned direction.** The code is unusually disciplined for a
personal project — clean boundaries, pure-core/impure-driver separation, argv-only tool
execution, 12k lines of real tests, `go vet` clean. The problem is not code quality. It is
that **the single most important architectural decision has four contradictory sources of
truth and the intended design is switched off in production**, plus a broken green-build
signal and a deploy path that does not enforce the security boundary its own comments
assume.

## what the project is now

```
purpose:            self-hosted RU/EN voice assistant; 8 Go daemons over unix sockets,
                    SQLite, local models
intended users:     one person, one box (homesrv, Ryzen 5 5600U, Vega iGPU, 14GB)
actual users:       same — confirmed by hardcoded TZ, single-passkey auth,
                    /home/kami replace directives in go.mod
critical workflows: voice turn (mic→STT→route→tool/reply→TTS); proactive nudges
                    (tick→dispatcher→telegram/ntfy/voice); web PWA
                    (/dash /chat /tools /ecosystem)
current state:      all 34 test packages pass; vet clean; `make test` still exits 1
                    (finding 2)
known failures:     weak RU query routing — REARCH.md names the cause; the named fix
                    is wired `nil`
maintenance burden: 4,518 lines of root markdown vs 33,319 lines of Go; 15 top-level
                    .md files, 3 of them dated session logs; several contradict
                    each other
constraints:        CPU-only deploy, ~14GB shared RAM, resident model must stay small;
                    never phones home; not autonomous; feminine RU persona
what works well:    internal/tool (argv-only, store allowlist, confirm gate); pure
                    engines (loop, routine, morning, pattern, dialogue — 86–100%
                    coverage); delivery outbox (Begin-before-send/Complete-after);
                    vendored deps (5 direct, tiny surface)
what's obsolete:    llmrouter.go (built, tested, never wired); classifier seed-phrase
                    cascade (interim stopgap, now 4 weeks old);
                    SESSION-05/06-07-2026.md; PLANS.md; ROADMAP.md
```

**Classification: healthy + misaligned.** Not fragile, not overbuilt, not abandoned. The
architecture in `REARCH.md` is sound and mostly *built* — it just is not *connected*.

## what it should become

The thing `REARCH.md` already describes, with the switch flipped and the drift removed:
one resident small model doing both routing and phrasing, classifier demoted from the live
path to the failure floor, embedder demoted to RAG hint. No new architecture is needed.
**The gap is a config/wiring decision plus doc convergence, not a redesign.**

---

## main findings

### 1. Four competing sources of truth on the resident model — and the target router is dead code

`architecture` / `repair`

**problem:** The most load-bearing design decision in the project is stated four different,
incompatible ways, and the code path `REARCH.md` calls "the linchpin" is disabled.

**evidence** (all confirmed):

- `cmd/mavend/voice.go:211` — `rtr := buildRouter(emb, matcher, threshold, nil) // LLM router disabled`,
  with comment *"the classifier handles routing reliably."*
- `REARCH.md:11` says the same classifier is *"the structural cause of 'she messes up
  queries.'"* **The code comment and the design doc make opposite claims about the same
  component.**
- `internal/router/llmrouter.go` (139 lines) + `llmrouter_test.go` — fully built and
  tested, zero non-test callers.
- Model identity, four ways: docs say **Qwen3-1.7B** (`CLAUDE.md:6`, `REARCH.md:15`,
  `SPEC.md:46`, `AGENTS.md:79`, `MAVEN_ECOSYSTEM_ARCHITECTURE.md:72`);
  `deploy/mavend.json:9` says **Qwen3.5-2B-UD-Q4_K_XL**; `models/llm/` on disk holds
  **LFM2.5-1.2B-Thinking**; code comments in 5 files still say **LFM**.
- `deploy/mavend.json:11` sets `"n_gpu_layers": 99` while `CLAUDE.md:4` states the target
  is **CPU-only**. (Interpretation: compose does pass `/dev/dri` + render gid for Vulkan,
  so 99 may be intentional — but then the CPU-only claim is stale. One of the two is
  wrong; the repo alone cannot say which.)

**impact:** You cannot answer "which model is deployed and does it route?" from this repo.
Every future routing decision starts by re-deriving the ground truth — that is the actual
maintenance cost, and it is the reason a 4-week-old "interim stopgap" is still the
committed default. The 2,453 lines of router package carry two engines where one is wanted.

**recommended action:** Pick the deployed model, then in one commit: set
`phraser.model_path` to it, wire `NewLLMRouter(llmClient)` into `buildRouter` at
`voice.go:211` behind a config flag (`voice.llm_router: true`) so the classifier stays the
documented failure floor, and update the model name in all 5 docs + 5 code comments to
match. Delete nothing from `internal/router` yet — the classifier is the fallback by design.

**why this level of change:** The components exist and are tested. This is wiring + truth
reconciliation, not a refactor. **Do not rewrite the router.**

**alternatives:** Delete `llmrouter.go` and commit to the classifier — only defensible if
the eval harness shows the classifier is actually adequate, which contradicts `REARCH.md`.

**risk:** Low-moderate. LLM route failures already fall through to the classifier
(`router.go:88-96`), so a bad model cannot break a turn. The real risk is CPU latency.

**verification:** `cmd/mavend/eval_scenarios_test.go` + `testdata/` (already in the working
tree, uncommitted) is exactly the right harness — run RU query scenarios against both
paths and compare before flipping the default.

---

### 2. `make test` always fails, so there is no green signal

`repair` — smallest high-value fix in the repo

**problem:** Every test passes; `make test` exits 1 anyway.

**evidence** (confirmed, root-caused): `make test` ends with
`# github.com/kami/maven/cmd/mavenclient` / `go: no such tool "covdata"` →
`make: *** Error 1`. Cause chain:

- `deps/go/go/VERSION` = **go1.23.4**; `go.mod` requires **go 1.25.5** → `GOTOOLCHAIN=auto`
  re-execs into `/home/kami/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.5.linux-amd64`.
- That cached toolchain ships only 7 of ~17 tools: `asm cgo compile cover link preprofile
  vet`. **`covdata` is missing** (source present, binary absent), as are `pprof`, `nm`,
  `objdump`, `trace`, `test2json`.
- `-coverprofile` invokes `covdata` for the two packages with no test files
  (`cmd/mavenclient`, `cmd/mavend/seedtest`) → failure. `GOTOOLCHAIN=local` confirms the
  vendored 1.23.4 is too old to substitute
  (`go.mod requires go >= 1.25.5 (running go 1.23.4)`).

**impact:** A permanently-red test target trains you to ignore the exit code — the one
signal protecting a 12k-line test suite. `go tool pprof` is also unavailable, which matters
on a latency-sensitive CPU-only box.

**recommended action:** `rm -rf '/home/kami/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.5.linux-amd64'`
and let Go re-download it complete — the cache entry is truncated, not a Makefile bug.
Then bump `deps/go` to a real 1.25.5 so the vendored toolchain is self-sufficient and
`GOTOOLCHAIN=local` works, which is the whole point of vendoring it.

**risk:** None. Worst case the re-download fails and you are where you started.

**verification:** `make test` exits 0.

---

### 3. Deploy path publishes an unauthenticated RCE surface; the code's stated mitigation is not in the repo

`security`

**problem:** The tool-enable gate is deliberately fail-open, documented as safe because it
"sits behind wg+nginx+auth" — but the repo's own deploy path publishes the port with no
such layer and no WebAuthn.

**evidence** (confirmed):

- `cmd/mavweb/main.go:918` — `if session != nil && !session.IsStepUp()`. `stepUpSession`
  is nil unless `-webauthn-origin` **and** `-webauthn-rpid` are both set (`main.go:419`).
  Nil ⇒ **gate skipped entirely.**
- `docker-compose.yml:82` passes neither flag, and `ports: ["9201:9201"]` publishes
  mavweb on the host.
- `handleTools` `enable` takes `name` + `cmd` from form fields and calls `core.EnableTool`
  — so this is not "approve a proposal," it is **define arbitrary argv**. `internal/tool`
  then executes it. `destructive` is a checkbox the caller sets.
- No auth middleware wraps the mux. `/ptt` (`main.go:1037`) proxies straight to the voice
  server, so an unauthenticated caller also gets voice turns — and can answer its own
  confirm turn.
- Separately: `docker-compose.yml` contains a literal committed credential,
  `-kuma-key uk5_mavpoll-key`.

**attack path:** anyone who reaches `homesrv:9201` → `POST /tools` with
`action=enable&name=x&cmd=<any binary + args>` → `POST /ptt` uttering `x` → arbitrary
command execution as the mavend user. No credential needed.

**impact:** The trust boundary the code carefully reasons about (`internal/tool` doc
comment: *"a compromised router can't grant itself a capability"*) is real, but the outer
boundary it depends on is an unwritten deployment assumption. Code safe, deployment not.

**recommended action:** Two small changes, not a redesign.
(a) Bind mavweb to loopback in compose — `ports: ["127.0.0.1:9201:9201"]` — so reaching it
requires the wg tunnel by construction, not by convention.
(b) Make the fail-open explicit and loud: if `-webauthn-origin` is unset, log a startup
warning naming `/tools` and `/api/revert` as unguarded, and add a `-require-stepup` flag
that fails closed for when you do want it.
Move the kuma key into `deploy/telegram.env` (already gitignored) and rotate it.

**note:** The fail-open is a **documented deliberate choice** (`main.go:411-418` explains
it, and it is correct that gating on an unassertable session would 403 permanently). The
finding is that the compensating control is absent from the repo, not that the reasoning
is wrong.

**risk:** Low. Loopback binding breaks off-box access only if something other than wg
currently reaches 9201 directly — worth confirming on the box first.

---

### 4. `cmd/mavend/voice.go` is 1,715 lines doing six unrelated jobs

`refactor` — later, not now

**problem:** One file holds daemon wiring (`wireVoice`, 240 lines), the turn handler,
intent dispatch (`applyAction`, 280 lines), Praxis/Hexis ecosystem acts (~250 lines), RU
text utilities (`ruPlural`, `stripWake`, `hasDurationWords`, `formatTime`), classifier
seeding, and weather parsing.

**evidence:** confirmed — 33 functions, largest file in the project by 40%; `cmd/mavend`
coverage is 29.8%, the lowest of any substantial package, precisely because this file mixes
wiring (hard to test) with pure logic (trivial to test).

**impact:** Not urgent — nothing is broken. But it is the reason the router flip in finding
1 feels risky: the wiring you must touch sits in the same file as the dispatch logic you
must not break.

**recommended action:** Split along seams that already exist: `wire_voice.go`
(construction), `handler.go` (turn), `acts_ecosystem.go` (Praxis/Hexis — already partly in
`ecosystem.go`), and move the RU helpers to `internal/ttsnorm` or a new `internal/rutext`.
Pure-function extraction lifts `cmd/mavend` coverage with no behavior change. **Do this
after finding 1, not before** — and skip it if it stays purely cosmetic.

---

### 5. Documentation outweighs its own usefulness

`cleanup` / `deletion`

**problem:** 15 root markdown files, 4,518 lines, several stale or superseded, at least
three pairs contradicting each other.

**evidence:** `ROADMAP.md` (759) + `MAVEN_ECOSYSTEM_ARCHITECTURE.md` (884) +
`PROGRESS.md` (456) + `maven.md` (413) + `20-07-2026-BACKLOG.md` (396) +
`SESSION-05-07-2026.md` + `SESSION-06-07-2026.md` (477 combined) + `PLANS.md` (25) +
`START.md` + `SPEC.md` + `PROTOCOL.md`. `PROGRESS.md:61` annotates its own staleness:
*"Older LFM references below describe the currently deployed..."*. `REARCH.md` announces it
"supersedes" a model still described as current elsewhere.

**impact:** The doc set is the reason finding 1 exists. When five documents describe the
architecture, the code becomes the only trustworthy one — which defeats the purpose of
having them.

**recommended action:** Keep `CLAUDE.md` (agent contract), `REARCH.md` (target
architecture), `AGENTS.md` (recipes), `PROTOCOL.md` (wire format),
`20-07-2026-BACKLOG.md` (live queue). Delete the two `SESSION-*.md` and `PLANS.md` — git
history holds them. Fold `SPEC.md` + `maven.md` + `ROADMAP.md` into one `DESIGN.md` and
mark superseded sections instead of leaving them to read as current. Target ~1,500 lines.

---

## keep

- `internal/tool` — argv-only executor, store allowlist, confirm gate. Well-reasoned; the
  doc comment explains *why* rather than *what*.
- `internal/loop`, `routine`, `morning`, `pattern`, `dialogue` — pure engines, impure
  drivers in `tick.go`, 86–100% coverage. That pattern is working; keep applying it.
- The delivery outbox (Begin-before-send / Complete-after / unknown-on-crash).
- Dependency discipline: 5 direct deps for a voice assistant is excellent.
- The uncommitted `internal/morning` work — follows the established pattern correctly, and
  its rationale comment ("why not four timers") is the right kind of comment.

## remove

`SESSION-05-07-2026.md`, `SESSION-06-07-2026.md`, `PLANS.md`. Stale `LFM` comments in 5
files once the model is pinned. The committed kuma key. `n_gpu_layers: 99` **or** the
CPU-only claim in `CLAUDE.md` — whichever is false.

## repair now

1. **Toolchain / `make test` exit code** (finding 2) — ~10 minutes, restores the safety
   net. Do this first; everything else is verified through it.
2. **Bind mavweb to loopback + startup warning on unguarded step-up + rotate the kuma key**
   (finding 3) — ~30 minutes, closes an unauthenticated RCE path.
3. **Pin the model and reconcile all five docs + config** (finding 1, first half) — the
   decision is yours; the edit is mechanical once made.
4. **Flip the LLM router behind a config flag and evaluate with `eval_scenarios_test.go`**
   (finding 1, second half) — the change that addresses the actual user-facing complaint.

## refactor later

Split `voice.go` (finding 4). Consolidate docs (finding 5). Raise `internal/phraser`
coverage from 16.5% — lowest in the project, and it owns the llama-server subprocess
lifecycle, exactly where a leak or orphan would hide (the `Pdeathsig` handling looks right,
but it is nearly untested).

## rewrite only if

Nothing here justifies a rewrite. `internal/router` is the only candidate and it fails the
test: the target engine is already written and tested, the fallback is deliberate, and
there is no compatibility burden. **Incremental correction is clearly cheaper.** Revisit
only if the LLM router needs a fundamentally different `Decision` shape than the cascade
produces — evaluate that with the scenario harness before touching structure.

```
incremental repair cost:        one wiring change + config flag + doc sweep
rewrite cost:                   re-derive 7-intent contract, slot extraction, RU
                                grammars, stage-0 fast path
migration cost:                 n/a — single user, no external consumers
behavior at risk:            	stage-0 fast path, RU slot extraction, confirm turns
tests available:                router_test, slots_ru_test, eval_scenarios_test
hidden knowledge in code:       high — RU qualifier pre-processing, seed collisions,
                                wake-token stripping across scripts
expected maintenance gain:      none over the incremental path
```

## verification plan

1. `make test` exits 0 (currently broken, so this is step zero).
2. `make build` — all 8 binaries.
3. `go vet ./internal/... ./cmd/...` stays clean.
4. Run `eval_scenarios_test.go` against classifier-only and LLM-router paths; compare RU
   query accuracy.
5. `start-maven.sh` locally; exercise a voice turn and a destructive-tool confirm turn.
6. Confirm `curl homesrv:9201/tools` is unreachable from off-box after the bind change.

## uncertainties

- **Which model is actually running on homesrv.** Repo evidence points three ways;
  unresolvable without the box. Everything in finding 1 depends on the answer.
- **Whether `n_gpu_layers: 99` is correct.** Compose passes `/dev/dri` and the render gid,
  suggesting Vulkan offload is intended and working — but `CLAUDE.md` says CPU-only. Likely
  the doc is stale, not the config; unverified.
- **Whether the classifier is genuinely adequate.** `voice.go:211` asserts it is;
  `REARCH.md` asserts it is not. Both are claims, neither is measured. The uncommitted eval
  harness is the instrument to settle it — resolve before flipping the router, not after.
- **Whether wg+nginx+auth actually fronts 9201 in production.** Not in this repo. If it
  does, finding 3 drops from "unauthenticated RCE" to "the control is not reproducible from
  the repo" — still worth fixing, much less urgent.
- The daemons were not run and routing latency was not measured.

---

## second opinion (Claude, 2026-07-30)

Key claims independently re-verified against the working tree; the verdict stands.

**Confirmed directly:**

- Finding 1: `voice.go:211` passes `nil` for the LLM router; `deploy/mavend.json:9`
  points at `Qwen3.5-2B-UD-Q4_K_XL.gguf`; `models/llm/` on disk holds only
  `LFM2.5-1.2B-Thinking-Q4_K_M.gguf`; `CLAUDE.md` says Qwen3-1.7B. At least three live
  contradictions, exactly as stated.
- Finding 2: reproduced — all packages pass, `make: *** Error 1` anyway; vendored
  toolchain is go1.23.4 vs `go 1.25.5` in `go.mod`. Correctly sequenced first.
- Finding 3: `docker-compose.yml` publishes `9201:9201` with no webauthn flags, and the
  kuma key `uk5_mavpoll-key` is committed verbatim. Attack path is coherent.

**Nuances to the review's framing:**

- Finding 3's severity depends entirely on network topology. If 9201 is only reachable
  over WireGuard on homesrv, this is hygiene, not an emergency — but the loopback bind
  and startup warning are cheap insurance either way, so do them regardless.
- Finding 1's "flip the router" step should be gated harder on measurement.
  `REARCH.md`'s claim that the classifier causes weak RU queries is itself unmeasured —
  the review admits this under uncertainties, but the "repair now" ordering buries it.
  Run `eval_scenarios_test.go` against both paths **before** deciding to flip, not
  after. A 2B model on CPU may add enough latency that the classifier wins in practice
  even if it is dumber.
- Finding 5 (doc consolidation) is what prevents finding 1 from recurring. Do the doc
  sweep as part of the model-pinning commit, not "later."

**Agreed without reservation:** the no-rewrite conclusion, the keep list, and the
overall classification (healthy engineering, misaligned direction).