Merge pull request 'The audit's open findings have no home: add docs/caveats/ and the doc indexes' (#225) from task/674-caveats into master
This commit was merged in pull request #225.
This commit is contained in:
@@ -17,6 +17,8 @@ under `docs/`. Read that doc before changing the subsystem.
|
||||
| `docs/ecosystem.md` | touching Nexus, Praxis or Hexis |
|
||||
| `docs/rearchitecture.md`, `docs/design.md` | changing the shape of anything |
|
||||
| `docs/workflow.md` | the five stores, the doc tiers, the guards |
|
||||
| `docs/caveats/` | a known limit, its task id and its revisit trigger |
|
||||
| `docs/CLAUDE.md` | which tier a doc belongs in, and what each one holds |
|
||||
| `AGENTS.md` | local preview, screenshots, model downloads |
|
||||
|
||||
## What Maven is
|
||||
|
||||
@@ -0,0 +1,30 @@
|
||||
# docs/
|
||||
|
||||
Everything an agent needs that is not a rule and not code. `CLAUDE.md` at the
|
||||
root carries the rules and points here. Nothing here restates a rule.
|
||||
|
||||
The tier is the path, so staleness is visible from the filename.
|
||||
|
||||
| path | holds | lifetime |
|
||||
| --- | --- | --- |
|
||||
| `docs/*.md` | living. One file per subsystem: the reasoning, corrected in place. Each carries `Last verified: <date> @ <sha>`. | until it is wrong |
|
||||
| `docs/evals/` | dated measurements, one file per measurement. **Never edited after the day.** A newer number is a new file. | forever |
|
||||
| `docs/caveats/` | known limits, one entry per limit, each with a task id and a revisit trigger. Indexed in `docs/caveats/CLAUDE.md`. | until fixed, then deleted |
|
||||
| `docs/plans/` | the plan for one piece of work, frozen once it starts | until the work lands |
|
||||
| `docs/archive/` | dead. Read by nobody by default. | forever |
|
||||
|
||||
## Rules for this directory
|
||||
|
||||
* One fact, one home. A measurement is cited from a living doc, never copied
|
||||
into it. The two drift the moment they are both edited.
|
||||
* A living doc is corrected in place and its `Last verified` line moves with the
|
||||
correction. Do not append a changelog to it.
|
||||
* A number in prose with no `docs/evals/` file behind it is an opinion.
|
||||
* Fixing something deletes its caveat. It does not edit the eval that found it.
|
||||
|
||||
## Where a subsystem's reasoning lives
|
||||
|
||||
`routing.md`, `language.md`, `world.md`, `offload.md`, `deployment.md`,
|
||||
`ecosystem.md`, `workflow.md`, `design.md`, `rearchitecture.md`,
|
||||
`determinism.md`, `protocol.md`, `handler-wiring.md`, `operations.md`, `qa.md`.
|
||||
The root `CLAUDE.md` says which one to read before touching what.
|
||||
@@ -0,0 +1,47 @@
|
||||
# docs/caveats/
|
||||
|
||||
One entry per known limit: something broken, deferred or unsafe that a session
|
||||
will otherwise walk into. An entry names what fails, who it costs, and the
|
||||
condition that makes it worth fixing.
|
||||
|
||||
Two things do not belong here. The evidence is a dated file under `docs/evals/`.
|
||||
The reasoning behind a subsystem is its living doc directly under `docs/`. A
|
||||
caveat is the pointer between them plus the trigger.
|
||||
|
||||
## Rules for this directory
|
||||
|
||||
* One file per area, one `##` section per limit, each carrying its task id.
|
||||
* **A caveat with no revisit trigger is a complaint.** Give it one or delete it.
|
||||
* Closing a limit deletes its entry. It does not edit it to say "fixed", and it
|
||||
never edits the frozen measurement it came from. The durable record of a fix
|
||||
is the commit and the subsystem's living doc.
|
||||
* An entry whose task is closed but whose limit is still live is the failure
|
||||
mode to watch for. The id joins the two directions, so check both.
|
||||
|
||||
## Index
|
||||
|
||||
Every entry below came from the 2026-08-10 deep audit
|
||||
(`docs/evals/2026-08-10-repo-audit.md`). One of the twenty findings, the
|
||||
unauthenticated mavgpud proxy, was fixed as V-673 and has no entry.
|
||||
|
||||
| limit | severity |
|
||||
| --- | --- |
|
||||
| [Go 1.25.5 and x/text 0.14.0 carry 20 reachable advisories](dependencies.md#toolchain) | high |
|
||||
| [Anyone past the proxy can enroll a passkey](security.md#enrollment) | high |
|
||||
| [Passkey credentials are rewritten in place](security.md#credentials) | medium |
|
||||
| [An empty STT transcript reads as a successful one](external-inputs.md#stt) | medium |
|
||||
| [Open-Meteo's empty body becomes 0°C](external-inputs.md#weather) | medium |
|
||||
| [Dialogue persistence errors are swallowed](storage.md#dialogue) | medium |
|
||||
| [The reminder transition is a lost update](storage.md#reminders) | medium |
|
||||
| [A recall miss scans two whole tables](storage.md#recall) | medium |
|
||||
| [Closing a TCP listener can strand Accept](transport.md#accept) | medium |
|
||||
| [PTT reads an unbounded body](transport.md#ptt) | medium |
|
||||
| [mavweb errors cannot be traced](transport.md#errors) | medium |
|
||||
| [Fact enrichment is a 20-call serial waterfall](workers.md#enrichment) | medium |
|
||||
| [A suppressed nudge is phrased anyway](workers.md#nudges) | medium |
|
||||
| [heads_path may equal model_path](invariants.md#heads) | medium |
|
||||
| [baselineGrammars is mirrored by hand](invariants.md#grammars) | medium |
|
||||
| [Committed absolute paths pin the build to this box](config.md#paths) | medium |
|
||||
| [The env example omits deployed variables](config.md#secrets) | medium |
|
||||
| [Domain packages depend on store and IPC types](layering.md#dtos) | low |
|
||||
| [Eleven symbols are unreachable](layering.md#deadcode) | low |
|
||||
@@ -0,0 +1,21 @@
|
||||
# Configuration and environment
|
||||
|
||||
## Committed absolute paths pin the build to this box [#690] {#paths}
|
||||
|
||||
Costs: `go.mod` replaces Hexis with `/home/kami/apps/hexis`, `start-maven.sh`
|
||||
hardcodes the checkout and the data directory, and `deploy/mavgpud.json` holds
|
||||
workstation model and Python paths. Vendoring hides the `go.mod` problem for an
|
||||
ordinary build. `-mod=mod`, `go mod tidy` and a fresh checkout all fail.
|
||||
Revisit when: anyone clones this repo elsewhere, or a `tidy` is needed.
|
||||
Workaround: build only from this checkout, with the vendor directory.
|
||||
|
||||
## The env example omits deployed variables [#691] {#secrets}
|
||||
|
||||
Costs: a fresh deploy can lose remote speech-to-text or ambient authentication
|
||||
and run on fallback behaviour with an apparently valid config. Three variables
|
||||
are referenced and undocumented: `MAVEN_STT_TOKEN`, `MAVEN_AMBIENT_TOKEN` and
|
||||
`CW2_TOKEN`. V-673 added `MAVEN_GPU_TOKEN` to the example.
|
||||
It is not silent. The loader logs which variables were unset and says whatever
|
||||
they configure is off. What is missing is a startup failure.
|
||||
Revisit when: the box is redeployed from scratch, or a new secret is added.
|
||||
Workaround: read that log line at startup.
|
||||
@@ -0,0 +1,17 @@
|
||||
# Dependencies
|
||||
|
||||
## Go 1.25.5 and x/text 0.14.0 carry 20 reachable advisories [#682] {#toolchain}
|
||||
|
||||
Costs: `govulncheck` found 20 reachable advisories, one in `x/text` and 19 in
|
||||
the standard library. They include template XSS, parser denial of service and
|
||||
TLS issues. Reachable traces run through the ONNX embedder's normalization,
|
||||
mavweb's HTML rendering, email header decoding and the mavgpud proxy. The
|
||||
vendored toolchain was built 2025-11-26.
|
||||
Revisit when: now. This is the highest-severity open entry and the fix is
|
||||
mechanical, so it ages badly for no reason.
|
||||
Workaround: none.
|
||||
|
||||
None of `staticcheck`, `govulncheck` or `deadcode` is installed on this box or
|
||||
wired into a make target. `make audit` is a git-grep inventory over loc, todo,
|
||||
stubs, docs, tests and gaps. **Do not read it as a static-analysis gate.** That
|
||||
gate is part of this entry.
|
||||
@@ -0,0 +1,20 @@
|
||||
# External inputs
|
||||
|
||||
What arrives from a service Maven does not run, and what happens when it
|
||||
arrives malformed. The shared shape: a JSON decode into value fields cannot
|
||||
tell "absent" from "zero", so a degraded response becomes a confident answer.
|
||||
|
||||
## An empty STT transcript reads as a successful one [#675] {#stt}
|
||||
|
||||
Costs: one dropped voice turn per malformed 200 from workpc. The mavsttd floor
|
||||
is never asked, because `stt.Pair` falls back on a non-nil error alone.
|
||||
Revisit when: CW2 returns a 200 with no text. Sooner if a proxy is put between
|
||||
homesrv and port 8081.
|
||||
Workaround: none. It is silent by design and the fallback is never spoken.
|
||||
|
||||
## Open-Meteo's empty body becomes 0°C [#676] {#weather}
|
||||
|
||||
Costs: he is told the weather is clear and 0°C when the service answered
|
||||
nothing. Distinct from V-589, which covered the HTTP status and not the body.
|
||||
Revisit when: a weather answer is reported as wrong, or the geocoder changes.
|
||||
Workaround: none.
|
||||
@@ -0,0 +1,27 @@
|
||||
# Unguarded invariants
|
||||
|
||||
`CLAUDE.md` names these as load-bearing. Nothing enforces either one. A rule
|
||||
that lives only in prose gets broken by whoever did not read the prose. Both of
|
||||
these fail silently when broken.
|
||||
|
||||
`tokenizerRev` and `preRouteLadder` were checked and need nothing. The rev is
|
||||
baked into the embedder key, so a bump triggers re-embedding. A missing ladder
|
||||
rung is observable in the decision record.
|
||||
|
||||
## heads_path may equal model_path [#692] {#heads}
|
||||
|
||||
Costs: the routing heads then score with the same graph the resident e5-small
|
||||
uses, and recall degrades. There is no error and no log line, so it reads as
|
||||
ordinary drift rather than a misconfiguration.
|
||||
Revisit when: `deploy/mavend.json` is edited by hand, or a fine-tuned heads
|
||||
graph is swapped in.
|
||||
Workaround: check the two keys by eye. That is the whole guard today.
|
||||
|
||||
## baselineGrammars is mirrored by hand [#693] {#grammars}
|
||||
|
||||
Costs: the eval fixture restates the stage 0 rule set in the daemon's order,
|
||||
and its own comment says so. Three test files score against it. A grammar added
|
||||
to `buildRouter` alone means every routing measurement scores a set nobody
|
||||
runs. `CLAUDE.md` warns about this failure by name.
|
||||
Revisit when: the next stage 0 grammar is added. That is when it bites.
|
||||
Workaround: add to both lists, which is what the rule already says.
|
||||
@@ -0,0 +1,25 @@
|
||||
# Layering and dead surface
|
||||
|
||||
Neither entry breaks anything today. Both make a later change cost more than it
|
||||
should, which is why they are low and not medium.
|
||||
|
||||
## Domain packages depend on store and IPC types [#685] {#dtos}
|
||||
|
||||
Costs: dialogue exposes `store.DialogueSessionRow` in its port, the pure
|
||||
morning planner takes a `store.Fact`, and auth policy imports IPC method and
|
||||
caller types. There is no Go import cycle. A schema change reaches further than
|
||||
it should.
|
||||
Revisit when: the dialogue or fact schema changes, or a second transport
|
||||
appears beside IPC.
|
||||
Workaround: none needed. It compiles and it is correct.
|
||||
|
||||
## Eleven symbols are unreachable [#686] {#deadcode}
|
||||
|
||||
Costs: extra API and test surface, and comments that claim callers which no
|
||||
longer exist. Three of the eleven must not be deleted. `HisGender` is a
|
||||
documented seam tied to V-399. `AudioDuration` duplicates `internal/audio` and
|
||||
should call it. `CountWord` is a one-line alias nobody uses and can go.
|
||||
Revisit when: `deadcode` is wired into the audit gate, which needs the
|
||||
allowlist this entry describes. **An unannotated list invites deleting the
|
||||
three above.**
|
||||
Workaround: none needed.
|
||||
@@ -0,0 +1,26 @@
|
||||
# Security
|
||||
|
||||
Both entries are mavweb's passkey seam. Assertion itself is sound and is not
|
||||
the problem: `userVerification` is required, and a sign count that does not
|
||||
increase is rejected.
|
||||
|
||||
## Anyone past the proxy can enroll a passkey [#683] {#enrollment}
|
||||
|
||||
Costs: registration is gated on nothing, so any client that reaches mavweb can
|
||||
enroll its own key and become him. Step-up is worse than per-client: one
|
||||
process-global `assertedAt` means every client inherits the same five-minute
|
||||
window after any successful assertion. The voice WebSocket accepts every
|
||||
origin, which makes cross-site use easier. V-317 covers which routes are gated
|
||||
and V-605 covers challenge-map growth. Neither covers this.
|
||||
Revisit when: mavweb is reachable from anything but the tunnel, and before any
|
||||
new credential is enrolled.
|
||||
Workaround: the reverse proxy is the only boundary today. That is the finding.
|
||||
|
||||
## Passkey credentials are rewritten in place [#684] {#credentials}
|
||||
|
||||
Costs: `os.WriteFile` over the live file. A crash, a full disk or an
|
||||
interrupted write corrupts every enrolled credential at once, and mavweb will
|
||||
not start afterwards.
|
||||
Revisit when: a second credential is enrolled, since the blast radius grows
|
||||
with the count. Sooner if the box loses power unexpectedly.
|
||||
Workaround: back the file up before enrolling.
|
||||
@@ -0,0 +1,30 @@
|
||||
# Storage
|
||||
|
||||
The DB seam: what it loses quietly, and what it reads more of than it needs.
|
||||
|
||||
## Dialogue persistence errors are swallowed [#677] {#dialogue}
|
||||
|
||||
Costs: restart continuity can vanish with nothing in the log, and a failed
|
||||
delete can bring stale conversation state back. Current-turn dialogue is
|
||||
unaffected, which is why this has never been noticed.
|
||||
Revisit when: a restart is reported as losing context. Sooner if a turn starts
|
||||
reading dialogue rows back to him.
|
||||
Workaround: none. The failure is invisible from outside.
|
||||
|
||||
## The reminder transition is a lost update [#678] {#reminders}
|
||||
|
||||
Costs: a concurrent fire and cancel both succeed and the last writer wins.
|
||||
Medium today because cancellation has no surface. High the moment V-622 adds
|
||||
one, and V-622 does not describe this invariant.
|
||||
Revisit when: V-622 starts, whichever comes first.
|
||||
Workaround: none, but the window is small while nothing can cancel.
|
||||
|
||||
## A recall miss scans two whole tables [#681] {#recall}
|
||||
|
||||
Costs: every missed recall reads all of `memory_vectors` and then decodes and
|
||||
sorts every note vector. Not an N+1, and the memory scan is cheap per losing
|
||||
row on purpose. The duplicated decode is the legacy notes path alone.
|
||||
Revisit when: the note count makes a miss measurably slow. Also when the two
|
||||
exclusion filters are proven to agree. `QueryNotes` uses `notHisWordsSQL` and
|
||||
`Search` uses `memory.NonRecallPrefix`. The fallback cannot go until they match.
|
||||
Workaround: none needed at today's row counts.
|
||||
@@ -0,0 +1,34 @@
|
||||
# Transport
|
||||
|
||||
The HTTP and socket seams. What a client can do to them, and what a shutdown
|
||||
can do to us.
|
||||
|
||||
## Closing a TCP listener can strand Accept [#679] {#accept}
|
||||
|
||||
Costs: during close, both `errc` and `done` are ready in `acceptLoop`'s select.
|
||||
Go picks uniformly. So roughly one close in two leaves a waiting `Accept`
|
||||
blocked forever on a TCP seam. Unix sockets are unaffected.
|
||||
Revisit when: a daemon is seen hanging on shutdown, or before any new TCP
|
||||
listener is added.
|
||||
Workaround: the process usually exits anyway, which hides it.
|
||||
|
||||
## PTT reads an unbounded body [#688] {#ptt}
|
||||
|
||||
Costs: `handlePTT` does an unlimited `io.ReadAll`, and mavweb sets no header or
|
||||
idle timeouts. A client can force unbounded allocation or hold a connection
|
||||
open. mavgpud's half of this was fixed in V-673.
|
||||
Revisit when: mavweb is reachable from anything but the tunnel.
|
||||
Workaround: mavweb is not LAN-exposed today.
|
||||
|
||||
`/ws` rides along with this entry. It never calls `SetReadLimit`, so the
|
||||
dependency default of 32,768 bytes applies, about a second of audio. Nothing
|
||||
reaches it: the browser posts PCM to `/api/ptt`, and only `handlers_test.go`
|
||||
opens `/ws`. It gets a caller and a real limit, or it gets deleted.
|
||||
|
||||
## mavweb errors cannot be traced [#689] {#errors}
|
||||
|
||||
Costs: some handlers return the raw internal error, which discloses internals.
|
||||
Others return a generic one with no identifier, which cannot be joined to its
|
||||
log line. There is no request-id middleware to join them.
|
||||
Revisit when: a reported UI failure cannot be found in the log.
|
||||
Workaround: read the log by timestamp.
|
||||
@@ -0,0 +1,23 @@
|
||||
# Background workers
|
||||
|
||||
Both entries are a tick doing expensive work it did not need to do.
|
||||
|
||||
## Fact enrichment is a 20-call serial waterfall [#680] {#enrichment}
|
||||
|
||||
Costs: each fact is resolved in turn and each ecosystem call can spend ten
|
||||
seconds. A slow but reachable Nexus holds one tick for minutes, so the worker
|
||||
stops observing its configured interval. V-647 covered the duplicate queue
|
||||
scan, not this.
|
||||
Revisit when: Nexus gets slow, or when a batch-resolution endpoint exists.
|
||||
Workaround: an unreachable Nexus is fine. It is the slow-but-answering case
|
||||
that hurts.
|
||||
|
||||
## A suppressed nudge is phrased anyway [#687] {#nudges}
|
||||
|
||||
Costs: `PhraseNudge` runs before the dedupe is known. The `continue` meant to
|
||||
skip it is the last statement in the loop body. Every tick that
|
||||
keeps suppressing the same rule pays the resident model again. The comment
|
||||
above it claims the opposite.
|
||||
Revisit when: digestion ticks show up in the model's load, or when nudge rules
|
||||
grow past a handful.
|
||||
Workaround: none.
|
||||
@@ -0,0 +1,261 @@
|
||||
# Repository deep-audit report
|
||||
|
||||
Date: 2026-08-10
|
||||
Revised: 2026-08-11, a verification pass over every cited line. Six claims were
|
||||
wrong as first written and are corrected in place. Two findings were added.
|
||||
|
||||
Frozen 2026-08-11, V-674. A dated measurement is never edited after the day,
|
||||
and that holds for a finding which later gets fixed. The live state of each one
|
||||
sits in `docs/caveats/` with its task id. The fix sits in the subsystem's living
|
||||
doc under `docs/`. Read this file for the evidence, not for what is still true.
|
||||
|
||||
Read-only audit complete. I found **20 issue-specific candidates not represented by a matching Vikunja task**: **3 High, 15 Medium, 2 Low**. No code or Vikunja tasks were changed.
|
||||
|
||||
I compared all 366 tasks in Maven project 2. Existing items such as V-317, V-589, V-597, V-603, V-605, V-608, V-643 and V-647 were excluded except where a new finding is clearly separate.
|
||||
|
||||
## 1. Unhandled edge cases and silent failures
|
||||
|
||||
### Empty STT responses suppress the local fallback
|
||||
|
||||
**A — Location:** `internal/stt/http.go:85` accepts `{}` as a successful transcript and returns empty text at line 89. `internal/stt/pair.go:143` only falls back when `err != nil`.
|
||||
|
||||
**B — Severity:** Medium. A malformed `200 OK` from the workstation silently drops a voice turn instead of invoking `mavsttd`.
|
||||
|
||||
**C — Proposed fix:** Require nonblank transcript text and a valid confidence range; treat missing required fields as an error so `Pair` falls back. Bound the response decoder and add `{}`, `{"text":""}`, oversized-body and invalid-confidence tests.
|
||||
|
||||
### Open-Meteo can report invented zero-degree weather
|
||||
|
||||
**A — Location:** `internal/weather/openmeteo.go:47` uses value fields, while line 95 accepts `{}` and line 100 interprets it as WMO 0 and 0°C.
|
||||
|
||||
**B — Severity:** Medium. A valid JSON error/degraded response becomes plausible but false weather. This is separate from V-589, which covered HTTP status handling.
|
||||
|
||||
**C — Proposed fix:** Make `current_weather` and its required members pointer/nullable fields, validate presence and ranges, and cap both forecast and geocoder bodies.
|
||||
|
||||
### Dialogue persistence errors disappear completely
|
||||
|
||||
**A — Location:** Corrupt rows are silently skipped at `internal/dialogue/session.go:127`; delete failures are discarded at line 191; marshal and save failures are swallowed at lines 201 and 209.
|
||||
|
||||
**B — Severity:** Medium. Restart continuity can silently vanish, while failed deletes can resurrect stale conversation state.
|
||||
|
||||
**C — Proposed fix:** Return or report persistence errors with session ID and operation; quarantine/delete corrupt rows; use bounded contexts instead of `context.Background()`. Keep current-turn availability, but expose the lost restart guarantee through logs/metrics.
|
||||
|
||||
## 2. Concurrency and race conditions
|
||||
|
||||
### Reminder state transition is a read-then-write lost update
|
||||
|
||||
**A — Location:** `internal/store/reminders.go:172` reads `pending`, then line 182 updates without checking the old state.
|
||||
|
||||
**B — Severity:** Medium now; High once V-622 adds cancellation surfaces. Concurrent fire/cancel operations can both succeed and the last writer wins. This invariant is not described in V-622.
|
||||
|
||||
**C — Proposed fix:** Use one conditional statement: `UPDATE ... WHERE id=? AND status='pending'`; inspect `RowsAffected`, then distinguish not-found from invalid transition. Add simultaneous fired/cancelled tests under `-race`.
|
||||
|
||||
### TCP listener shutdown can leave `Accept` blocked forever
|
||||
|
||||
**A — Location:** `internal/netaddr/netaddr.go:180` waits only on `conns` and `errc`. During close, line 201 may select the already-closed `done` branch without publishing the listener error; `Close` at line 225 does not wake `Accept`.
|
||||
|
||||
**B — Severity:** Medium. During close both `errc` and `done` are ready in `acceptLoop`'s select and Go picks uniformly, so roughly one close in two strands a waiting `Accept` forever on a TCP seam.
|
||||
|
||||
**C — Proposed fix:** Add `case <-l.done: return nil, net.ErrClosed` to `Accept`, and make error/channel closure ownership explicit. Test an in-flight `Accept` concurrently with `Close`.
|
||||
|
||||
## 3. Data fetching: waterfalls and duplicated scans
|
||||
|
||||
### Fact enrichment performs a serial 20-call network waterfall
|
||||
|
||||
**A — Location:** `cmd/mavend/factenrichment.go:160` resolves each fact sequentially; line 223 makes the Nexus call. Each request can consume ten seconds at `cmd/mavend/ecosystem.go:63`.
|
||||
|
||||
**B — Severity:** Medium. A slow-but-reachable Nexus can hold one tick for roughly 20 × 10s, preventing the worker from observing its intended interval. V-647 only covered the duplicate queue scan.
|
||||
|
||||
**C — Proposed fix:** Prefer a Nexus batch-resolution endpoint. Otherwise use bounded concurrency, such as four workers, while preserving per-fact backoff and the 20-attempt ceiling.
|
||||
|
||||
### A recall miss scans two whole tables
|
||||
|
||||
**A — Location:** The query source table runs embed, memory and notes in order at `cmd/mavend/actions_query.go:161` through line 164. `MemoryStore.Search` scans every row of `memory_vectors` at `internal/store/memory.go:82`; a miss then calls the legacy notes query at `cmd/mavend/actions_query.go:718`, which decodes every note vector at `internal/store/notes.go:63` and sorts the whole table at line 69.
|
||||
|
||||
**B — Severity:** Medium at scale. This is not an N+1. It is two full scans per missed recall. The memory scan is already cheap per losing row on purpose, costing one dot product read off the stored bytes with no `[]float32` materialized, so the duplicated decode cost is the notes path alone. Both recall widths are tiny (`memoryRecallWidth` 3, `noteRecallWidth` 5), so the work is in the scan, not the result set. V-581 is a generic sweep of this file, but does not identify this issue.
|
||||
|
||||
**C — Proposed fix:** Establish the invariant that every recallable note exists in `memory_vectors`, then remove the fallback. `internal/store/backfill.go` already rewrites note rows into the unified index, so the backfill exists; what is missing is proof that the two exclusion filters agree, since `QueryNotes` filters on `notHisWordsSQL` while `Search` filters on `memory.NonRecallPrefix`. Until they do, query only notes missing from the unified index and rank with the existing bounded top-K heap.
|
||||
|
||||
## 4. Dependency health and version pinning
|
||||
|
||||
### Reachable published vulnerabilities in the pinned toolchain and `x/text`
|
||||
|
||||
**A — Location:** `go.mod:3` and `Makefile:7` pin Go 1.25.5; `go.mod:25` pins `x/text` 0.14.0. Reachable traces include normalization at `internal/router/onnxembedder.go:364`, HTML rendering at `cmd/mavweb/shell.go:154`, email header decoding at `internal/email/message.go:244`, and reverse proxying at `cmd/mavgpud/main.go:212`.
|
||||
|
||||
**B — Severity:** High. `govulncheck` found **20 reachable advisories**: one in `x/text` and 19 in the Go standard library, including template XSS, parser complexity/DoS and TLS issues. The official database says `x/text` before 0.39.0 can loop on invalid UTF-8; Go 1.25.12 contains the accumulated security corrections. See [GO-2026-5970](https://pkg.go.dev/vuln/GO-2026-5970), [GO-2026-4980](https://pkg.go.dev/vuln/GO-2026-4980), and the [Go release history](https://go.dev/doc/devel/release#go1.25.0).
|
||||
|
||||
**C — Proposed fix:** Upgrade the vendored toolchain to at least 1.25.12, preferably current 1.26.5 after compatibility testing; upgrade `x/text` to at least 0.39.0/current 0.40.0; tidy and re-vendor. Add `govulncheck ./...` to the repository gate.
|
||||
|
||||
No dependency was three major versions behind. The remaining direct updates were minor/patch releases. `.opencode`'s `npm audit` reported zero vulnerabilities and no peer conflicts.
|
||||
|
||||
## 5. Security exposure
|
||||
|
||||
### WebAuthn enrollment is open and step-up state is process-global
|
||||
|
||||
**A — Location:** Registration endpoints have no existing-credential or bootstrap authorization at `cmd/mavweb/webauthn.go:92` and line 103. `RegisterBegin` also answers GET, while `RegisterFinish` requires POST. A single server-wide session is created at `cmd/mavweb/main.go:170`, backed by one `assertedAt` timestamp at `internal/webauthn/session.go:20`. The voice WebSocket accepts every origin at `cmd/mavweb/voiceproxy.go:48`.
|
||||
|
||||
**B — Severity:** High, and scoped to enrollment and session binding. Assertion itself is sound. `internal/webauthn/webauthn.go:221` requests `userVerification: "required"`, and line 304 rejects a sign count that did not increase. What is broken is that any client past the reverse proxy can enroll its own key, and that after any successful assertion every client inherits the same five-minute step-up window. The wildcard WebSocket origin makes cross-site use easier. V-317 covers which routes are gated, and V-605 covers challenge-map growth, not enrollment or session binding.
|
||||
|
||||
**C — Proposed fix:** Permit first enrollment only through a local/one-time bootstrap ceremony; require an already-authenticated credential for subsequent enrollment. Bind step-up to a signed, HttpOnly, SameSite browser session and exact RP origin. Restrict WebSocket origins and add CSRF/origin validation to mutating routes.
|
||||
|
||||
### `mavgpud` exposes an unauthenticated GPU/model proxy on the LAN
|
||||
|
||||
**Closed 2026-08-11, V-673.** The reasoning now lives in `docs/offload.md`,
|
||||
beside the rest of the workstation seam, and `docs/deployment.md` carries the
|
||||
operational line in the daemon table. This block is a pointer, not a second
|
||||
home: read those, not this.
|
||||
|
||||
- Boundary and limits: `cmd/mavgpud/auth.go`, wired in `cmd/mavgpud/main.go`.
|
||||
- Client half: `internal/llm/client.go`, `internal/llm/remote.go`,
|
||||
`internal/config/workstation.go`, `cmd/mavend/voicewire.go`.
|
||||
- Deploy: `token_file` in `deploy/mavgpud.json`, `MAVEN_GPU_TOKEN` in
|
||||
`deploy/telegram.env.example`.
|
||||
- Commits: `95e7427`, `1c13d22`, `5596cdd`, `9bb3425`.
|
||||
|
||||
**Deploy step, not yet done:** write the token to
|
||||
`/home/kami/.config/mavgpud.token` on workpc and put the same value in
|
||||
`MAVEN_GPU_TOKEN` on homesrv, before restarting either side. mavgpud refuses to
|
||||
start without it, and a homesrv missing it falls back to the resident model.
|
||||
|
||||
### Passkey credential persistence is not crash-atomic
|
||||
|
||||
**A — Location:** `cmd/mavweb/credentials.go:36` serializes the full credential map and overwrites the live file directly with `os.WriteFile` at line 41.
|
||||
|
||||
**B — Severity:** Medium. A crash, disk-full event or interrupted write can corrupt every enrolled credential and prevent mavweb from starting.
|
||||
|
||||
**C — Proposed fix:** Write a `0600` temporary file in the same directory, `fsync`, rename atomically, then sync the directory. Preserve the last known-good file and test simulated write failures.
|
||||
|
||||
No new tracked hardcoded keys, raw user-concatenated SQL, `eval`, or shell execution of untrusted strings were found. The historical DB-key exposure is already covered by V-12.
|
||||
|
||||
## 6. Circular dependencies and layering
|
||||
|
||||
### Domain packages depend directly on storage/wire DTOs
|
||||
|
||||
**A — Location:** Dialogue imports `store` and exposes `store.DialogueSessionRow` in its port at `internal/dialogue/session.go:9` and line 75. The pure morning planner accepts `store.Fact` at `internal/morning/plan.go:72`. Auth policy imports IPC method and caller types at `internal/auth/policy.go:8` and `internal/auth/scope.go:33`.
|
||||
|
||||
**B — Severity:** Low. There is no current Go import cycle, but domain changes are coupled to database and IPC schema changes.
|
||||
|
||||
**C — Proposed fix:** Make domain packages own their DTOs and ports—dialogue persistence records, morning evidence, auth operation/caller identity—and adapt them in store/IPC/cmd wiring.
|
||||
|
||||
No circular Go imports were found; compilation and `go vet` both succeeded.
|
||||
|
||||
## 7. Dead code and zombie endpoints
|
||||
|
||||
### Eleven production symbols are unreachable
|
||||
|
||||
**A — Location:** `deadcode` found:
|
||||
|
||||
- `cmd/mavwaked/vad.go:244` — `PCMToF32`
|
||||
- `cmd/mavwaked/vad.go:265` — `AudioDuration`
|
||||
- `internal/crawl/watch.go:86` — `Watcher.Watches`
|
||||
- `internal/phraser/confirm.go:137` — `IsC`
|
||||
- `internal/phraser/plural.go:13` — `CountWord`
|
||||
- `internal/phraser/eval/checks.go:80` — `HisGender`
|
||||
- `internal/update/update.go:363` — `WithClock`
|
||||
- `internal/voice/errors.go:72` — `jsonMarshal`
|
||||
- `internal/voice/errors.go:73` — `jsonUnmarshal`
|
||||
- `internal/webauthn/cbor.go:98` — `cborValue.At`
|
||||
- `internal/worker/server.go:64` — `Server.SetSynthesizer`
|
||||
|
||||
Three of the eleven do not want deleting, and the 2026-08-11 pass checked each:
|
||||
|
||||
- `internal/phraser/eval/checks.go:80` `HisGender` is deliberately exposed and deliberately uncalled. The comment at line 72 ties the trio to V-399, and `cmd/mavend/personaguard.go:94` states why this one is not run on a phrased message. Deleting it removes a documented seam.
|
||||
- `cmd/mavwaked/vad.go:265` `AudioDuration` duplicates what `internal/audio` already computes. Call that instead of deleting the body.
|
||||
- `internal/phraser/plural.go:13` `CountWord` is a one-line alias for `say.CountWord`, and every real caller already uses `say` directly. Safe to delete outright.
|
||||
|
||||
**B — Severity:** Low. They increase API and test surface, and some comments claim callers that no longer exist.
|
||||
|
||||
**C — Proposed fix:** Delete the genuinely obsolete symbols. Where one is an intended extension seam, add the actual caller and a contract test, or record why it stays. Add `deadcode ./...` with an explicit allowlist to the audit gate, since an unannotated list invites deleting the three above.
|
||||
|
||||
The repository history begins on 2026-07-03, so a six-month rotten-feature-flag check is not yet applicable. One zombie HTTP route does exist: `/ws` is wired at `cmd/mavweb/main.go:235` and no shipped client reaches it, since the browser posts to `/api/ptt`. Section 8 carries the detail.
|
||||
|
||||
## 8. Performance hot paths and memory/resource leaks
|
||||
|
||||
### Digest dedupe happens after paying the LLM cost
|
||||
|
||||
**A — Location:** `cmd/mavend/tick_digest.go:147` calls `PhraseNudge` before `EnqueueDigestEntry` reports the dedupe at line 153. The `else if deduped { continue }` is the last statement in the loop body, so it changes nothing.
|
||||
|
||||
**B — Severity:** Medium. Every tick that continues suppressing the same rule can invoke the model again, contrary to the cache claim in the preceding comment.
|
||||
|
||||
**C — Proposed fix:** Check for a live pending entry by stable rule/candidate fingerprint before phrasing, or persist/cache the phrased result with a TTL. Add a test asserting one phraser call across repeated suppressed ticks.
|
||||
|
||||
### PTT reads an unbounded body and neither server sets header or idle limits
|
||||
|
||||
**A — Location:** `handlePTT` performs an unlimited `io.ReadAll` at `cmd/mavweb/voiceproxy.go:125`. Mavweb and mavgpud construct servers without header or idle limits at `cmd/mavweb/main.go:242` and `cmd/mavgpud/main.go:148`. Separately, `handleWS` never calls `conn.SetReadLimit`, so the dependency default of 32,768 bytes applies at `vendor/github.com/coder/websocket/read.go:92`, about one second of 16kHz mono PCM.
|
||||
|
||||
**B — Severity:** Medium for the HTTP side. A client can force unbounded body allocation or hold a connection open indefinitely. Low for the WebSocket read limit, because `/ws` has no caller: the browser client posts PCM to `/api/ptt` at `cmd/mavweb/static/app.js:114`, and `/ws` is wired at `cmd/mavweb/main.go:235` but reached only from `handlers_test.go`. The 64MiB `maxFrame` at `cmd/mavweb/voiceproxy.go:27` is not an unapplied declaration. It caps the mavend voice wire at line 194 and line 212, which is the "either direction" its comment names.
|
||||
|
||||
**C — Proposed fix:** Use `http.MaxBytesReader` for PTT and return 413 on overflow. Configure `ReadHeaderTimeout`, `IdleTimeout` and header limits on both servers. Decide `/ws` separately: either give it an audio-duration read limit and a client, or delete it. See section 7.
|
||||
|
||||
## 9. Error propagation and user feedback
|
||||
|
||||
### Mavweb has no consistent, traceable error contract
|
||||
|
||||
**A — Location:** Some handlers expose raw internal errors, such as `cmd/mavweb/tools.go:57`, `cmd/mavweb/routines.go:58` and `cmd/mavweb/webauthn.go:122`. Others return generic errors without a request/incident identifier, such as `cmd/mavweb/facts.go:90`. No HTTP request-ID middleware was found.
|
||||
|
||||
**B — Severity:** Medium. Raw errors can disclose implementation details, while generic errors cannot be correlated with the correct log entry.
|
||||
|
||||
**C — Proposed fix:** Add a central `writeProblem`/error-page helper with a stable error code and generated request ID; log the full wrapped error server-side and return only a sanitized message plus the ID. Carry the ID into IPC/ecosystem correlation where possible.
|
||||
|
||||
## 10. Configuration drift and environment assumptions
|
||||
|
||||
### Committed absolute paths make builds and deployment host-specific
|
||||
|
||||
**A — Location:** `go.mod:31` replaces Hexis with `/home/kami/apps/hexis`. `start-maven.sh:11` hardcodes the Maven checkout and line 40 hardcodes the data directory. `deploy/mavgpud.json:12` and line 35 contain workstation-specific model/Python paths.
|
||||
|
||||
**B — Severity:** Medium. Vendoring masks the `go.mod` problem for ordinary builds, but `-mod=mod`, tidy and fresh non-Kami checkouts fail. Deployment files cannot be reused safely on another host.
|
||||
|
||||
**C — Proposed fix:** Pin a real Hexis module revision; keep local replacement in an uncommitted `go.work`. Derive script root from the script location and make data paths configurable. Split mavgpud into a committed template plus host-local override.
|
||||
|
||||
### Environment examples do not cover deployed variables
|
||||
|
||||
**A — Location:** Active config references `MAVEN_STT_TOKEN` at `deploy/mavend.json:101`, Compose references `MAVEN_AMBIENT_TOKEN` at `docker-compose.yml:109`, and the GPU service expects `CW2_TOKEN` at `deploy/mavgpud.service:15`. `deploy/telegram.env.example:5` documents only Telegram and ntfy. The loader deliberately converts missing variables to empty settings at `internal/config/config.go:333`.
|
||||
|
||||
**B — Severity:** Medium. A fresh deployment can lose remote STT or ambient authentication and run on fallback behavior despite apparently valid config. It is not silent: `internal/config/config.go:340` logs which variables were unset and states that whatever they configure is off. What is missing is a startup failure and an example file naming them.
|
||||
|
||||
**C — Proposed fix:** Maintain one canonical secret manifest/example covering every referenced variable, or service-specific examples with validation. Fail startup when an enabled integration lacks its required secret; permit empty variables only for explicitly disabled blocks.
|
||||
|
||||
No production/staging debug-mode or mock-gateway drift was found.
|
||||
|
||||
## 11. Declared invariants with no guard
|
||||
|
||||
`CLAUDE.md` names several rules as load-bearing. Two of them are enforced by
|
||||
nothing, which the first pass missed because it audited generic categories only.
|
||||
|
||||
### `heads_path` may equal `model_path` and nothing objects
|
||||
|
||||
**A — Location:** `cmd/mavend/voicewire.go:168` reads `cfg.Voice.Embedder.HeadsPath` and loads it without comparing it to the model path. The rule is stated at `internal/config/voice.go:41`, which says the heads graph is a fine-tuned COPY, and again in `CLAUDE.md`.
|
||||
|
||||
**B — Severity:** Medium. Pointing both keys at the same file degrades recall, because the routing heads then score with the same graph the resident e5-small uses. There is no error and no log line, so the failure looks like ordinary recall drift.
|
||||
|
||||
**C — Proposed fix:** Reject the config at load when `heads_path` equals `model_path` after path cleaning. A daemon that cannot route well should refuse to start rather than answer worse.
|
||||
|
||||
### `baselineGrammars` mirrors `buildRouter` by hand
|
||||
|
||||
**A — Location:** `internal/router/eval/eval_test.go:263` restates the stage 0 rule set in the daemon's order, and its own comment says so. `claims_test.go:30`, `heads_test.go:76` and `eval_test.go:221` all score against it. Nothing compares the two lists.
|
||||
|
||||
**B — Severity:** Medium. A grammar added to `buildRouter` and not to the fixture means every routing measurement scores a set nobody runs, which is the failure mode `CLAUDE.md` warns about by name.
|
||||
|
||||
**C — Proposed fix:** Export the grammar set from one place and have both `buildRouter` and the fixture consume it, or add a test that diffs the two by grammar name and fails on drift.
|
||||
|
||||
`tokenizerRev` and `preRouteLadder` were checked and need nothing.
|
||||
`internal/router/onnxembedder.go:91` bakes the rev into the embedder key, so a
|
||||
bump changes the key and triggers re-embedding. `cmd/mavend/voice.go:278` passes
|
||||
`preRouteLadder` to `decision.Expect`, so a missing rung is observable.
|
||||
|
||||
## Cross-cutting subsystem candidates
|
||||
|
||||
The recurring findings suggest five reusable patterns:
|
||||
|
||||
- A bounded, required-field-validating JSON client for STT, weather, ecosystem and model calls.
|
||||
- Authenticated remote-service middleware providing token checks, body limits, concurrency limits and correlation IDs.
|
||||
- Atomic state-transition helpers using conditional SQL and `RowsAffected`.
|
||||
- A uniform HTTP problem/error envelope.
|
||||
- A repository health gate combining `staticcheck`, `govulncheck`, `deadcode` and dependency audits.
|
||||
|
||||
## Validation
|
||||
|
||||
- `make fmt-check` and `make vet` passed. `make audit` passed too, but it is a git-grep inventory over loc, todo, stubs, docs, tests and gaps (`scripts/audit.sh`), not a static-analysis gate. Do not read it as one.
|
||||
- `staticcheck`, `govulncheck`, `deadcode`, tracked-secret and history scans and npm audit were run out of tree. None of the three Go analyzers is installed on this box or wired into any make target, which is the argument for section 4's proposed gate.
|
||||
- The advisory version numbers in section 4 could not be re-checked offline on 2026-08-11. `deps/go/go/VERSION` reads `go1.25.5`, built 2025-11-26, so the eight-month gap behind current supports the upgrade claim.
|
||||
- The first whole-tree race run failed once in `cmd/mavend` while analyzers were compiling concurrently; a fresh isolated `go test -race -count=1 ./cmd/mavend` passed in 100.6 seconds, so the transient result was not counted as a defect.
|
||||
- The pre-existing `deploy/mavwaked.service` modification and untracked `deploy/asoundrc` remained untouched.
|
||||
Reference in New Issue
Block a user