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).