docs: tier the tree by lifetime, so staleness shows in the path (V-446)

Seventeen markdown files at the repo root, twelve of them dated one-shot reports sitting next to CLAUDE.md. That is why stale docs read as current: nothing in the path said which was which. Root now keeps CLAUDE.md and AGENTS.md. Living docs move under docs/ and carry a Last verified line. Dated measurements move to docs/evals/ ISO-prefixed, and are never edited after the day, so a newer number is a new file. The senior review moves to docs/archive/. Every reference was rewritten across markdown, Go comments, the Makefile and the recall fixture. The touched Go packages still build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-02 03:28:49 +04:00
parent 7079a240f7
commit 93987f2dfc
38 changed files with 99 additions and 85 deletions
@@ -0,0 +1,362 @@
+# 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 — docs/rearchitecture.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 `docs/rearchitecture.md` is sound and mostly *built* — it just is not *connected*.
+
+## what it should become
+
+The thing `docs/rearchitecture.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 `docs/rearchitecture.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."*
+- `docs/rearchitecture.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`, `docs/rearchitecture.md:15`,
+  `SPEC.md:46`, `AGENTS.md:79`, `docs/ecosystem.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 `docs/rearchitecture.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) + `docs/ecosystem.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) +
+`docs/operations.md` + `SPEC.md` + `docs/protocol.md`. `PROGRESS.md:61` annotates its own staleness:
+*"Older LFM references below describe the currently deployed..."*. `docs/rearchitecture.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), `docs/rearchitecture.md` (target
+architecture), `AGENTS.md` (recipes), `docs/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 `docs/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;
+  `docs/rearchitecture.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.
+  `docs/rearchitecture.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).