# 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=` → `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).