From 7ab9b48259b43375167ca10ced05bdf27a6be730 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:05:13 +0400 Subject: [PATCH 1/6] coldstart: recover v1 boxes, and make key wrapping an explicit act Three ways the cold-start path could lose the database. A box enrolled before the PRF change could never cold-start again. UnwrapKey still read v1 blobs, but the only caller stopped supplying the v1 secret: the assertion handler sends the PRF output and nothing looks up the credential public key any more. On such a box the daemon read the blob, took the v1 branch, failed to decrypt, and stayed locked while a valid passkey was asserted at it. The escape hatch was gone too, because WrapKeyFn was wired only in env-key mode and a locked boot is by definition the mode with no env key. The recovery was to put MAVEN_DB_KEY back in the environment, which is the thing cold-start unlock exists to avoid. AssertFinish now retries a failed PRF unwrap with the credential public key, and WrapKeyFn is wired in locked mode too, so the box that came up on a v1 blob can be moved to v2. Wrapping ran on every successful assertion. That made a routine step-up rewrite the one file that opens the database, under whatever 32 bytes the page posted. A compromised /auth/webauthn converted one legitimate touch into permanent offline recovery of the at-rest key, and a second enrolled authenticator silently locked out the first. Wrapping is now an act of its own: a plain assertion may write the blob only when none exists, and replacing one takes the rewrite button, which is the only caller that sets the new explicit flag. The daemon still refuses to overwrite a v2 blob that does not open under the presented secret. The write was os.WriteFile, which truncates in place. A power cut between the truncate and the write left a zero-length blob and no previous contents, on the path of every step-up. It is now a temp file in the same directory, fsync, rename, fsync of the directory. Two smaller things on the same path. The v2 unwrap checked the secret length but not the all-zero case the wrap side rejects, so the two ends disagreed about what a valid secret is. And the handler logged "daemon unlocked via credential" when an env-key daemon had answered unknown method, and again when an already-unlocked daemon had done nothing. Left alone deliberately: the PRF value is client-supplied and not covered by the assertion signature. That is inherent to PRF key wrapping, since the salt has to be fixed for the blob to open on the next boot. It is recorded as a known property where the secret enters the handler. Found in review of #77. --- cmd/mavend/keyfile.go | 111 ++++++++++++++++++ cmd/mavend/keyfile_test.go | 187 ++++++++++++++++++++++++++++++ cmd/mavend/main.go | 87 ++++++++++---- cmd/mavweb/passkey_prf_test.go | 122 ++++++++++++++++++- cmd/mavweb/webauthn.go | 116 ++++++++++++++---- internal/ipc/api.go | 12 +- internal/ipc/client.go | 11 +- internal/ipc/server.go | 8 +- internal/ipc/unlock_test.go | 23 +++- internal/webauthn/keywrap.go | 25 +++- internal/webauthn/keywrap_test.go | 15 +++ models/stt | 1 + models/tts | 1 + 13 files changed, 654 insertions(+), 65 deletions(-) create mode 100644 cmd/mavend/keyfile.go create mode 100644 cmd/mavend/keyfile_test.go create mode 120000 models/stt create mode 120000 models/tts diff --git a/cmd/mavend/keyfile.go b/cmd/mavend/keyfile.go new file mode 100644 index 0000000..15f9ecb --- /dev/null +++ b/cmd/mavend/keyfile.go @@ -0,0 +1,111 @@ +package main + +// Writing the wrapped-key blob (Vikunja #14). +// +// The blob is the only thing that opens the database on a cold-started box, so +// the two rules here are about not losing it. +// +// # It is rewritten on every assertion, so the write must be atomic +// +// mavweb calls StoreEncryptionKey after every successful assertion, not only +// after enrolment. os.WriteFile truncates in place: a power cut or an OOM kill +// between the truncate and the write left a zero-length blob and no previous +// contents, on the path of every routine step-up. Write to a temp file in the +// same directory, fsync it, rename over the target, then fsync the directory. +// +// # Only one authenticator can hold the cold-start key +// +// A blob is wrapped under one credential's PRF output and nothing else opens +// it. mavweb sends an empty allowCredentials list and the credential store +// keeps more than one passkey, so an unconditional rewrite meant the last +// authenticator to assert silently locked out every other one — including the +// backup hardware key enrolled for exactly the cold-start case. So: a blob +// that already opens under this secret and already wraps this key is left +// alone, a v1 blob is upgraded in place, and a v2 blob belonging to a +// different credential is refused rather than overwritten. + +import ( + "bytes" + "errors" + "fmt" + "os" + "path/filepath" + + "github.com/kami/maven/internal/webauthn" +) + +// errForeignBlob — the wrapped key on disk belongs to another credential. +// Refusing is the point: overwriting would lock that authenticator out. +var errForeignBlob = errors.New("wrapped key belongs to a different credential") + +// wrapKeyToFile wraps key under secret and persists it at path, unless the +// blob already there says not to. Reports whether it wrote anything. +func wrapKeyToFile(path string, key, secret []byte) (wrote bool, err error) { + existing, err := os.ReadFile(path) + switch { + case err == nil: + plain, version, uerr := webauthn.UnwrapKey(existing, secret) + switch { + case uerr == nil && version == webauthn.BlobV2 && bytes.Equal(plain, key): + // Already wrapped under this secret, around this key. The + // common case on every assertion after the first. + return false, nil + case uerr != nil && version == webauthn.BlobV2: + return false, fmt.Errorf("%w: %s does not open under this assertion's PRF output, so another passkey holds the cold-start key; delete it deliberately to re-wrap", errForeignBlob, path) + } + // A v1 blob (upgrade it), or a v2 blob wrapping a stale key under + // this same secret (the key was rotated). Both are rewrites. + case errors.Is(err, os.ErrNotExist): + // First wrap. + default: + return false, fmt.Errorf("read wrapped key: %w", err) + } + + blob, err := webauthn.WrapKey(key, secret) + if err != nil { + return false, fmt.Errorf("wrap encryption key: %w", err) + } + if err := writeFileAtomic(path, blob, 0o600); err != nil { + return false, fmt.Errorf("write wrapped key: %w", err) + } + return true, nil +} + +// writeFileAtomic writes data to path so that a reader sees either the whole +// new file or the whole old one, never a truncated blob. +func writeFileAtomic(path string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + f, err := os.CreateTemp(dir, filepath.Base(path)+".tmp*") + if err != nil { + return err + } + tmp := f.Name() + defer os.Remove(tmp) // no-op once the rename succeeded + + if err := f.Chmod(perm); err != nil { + f.Close() + return err + } + if _, err := f.Write(data); err != nil { + f.Close() + return err + } + if err := f.Sync(); err != nil { + f.Close() + return err + } + if err := f.Close(); err != nil { + return err + } + if err := os.Rename(tmp, path); err != nil { + return err + } + // The rename itself needs to reach the disk, or a crash can resurrect the + // old directory entry pointing at a file that is gone. + d, err := os.Open(dir) + if err != nil { + return err + } + defer d.Close() + return d.Sync() +} diff --git a/cmd/mavend/keyfile_test.go b/cmd/mavend/keyfile_test.go new file mode 100644 index 0000000..bf3cb1e --- /dev/null +++ b/cmd/mavend/keyfile_test.go @@ -0,0 +1,187 @@ +package main + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "testing" + + "github.com/kami/maven/internal/webauthn" +) + +func wrapPath(t *testing.T) string { + t.Helper() + return filepath.Join(t.TempDir(), "db_key.wrapped") +} + +// The first wrap writes a v2 blob that opens under the same secret. +func TestWrapKeyToFileWritesAnOpenableBlob(t *testing.T) { + path := wrapPath(t) + key := bytes.Repeat([]byte{1}, 32) + secret := bytes.Repeat([]byte{2}, 32) + + wrote, err := wrapKeyToFile(path, key, secret) + if err != nil || !wrote { + t.Fatalf("wrapKeyToFile = %v, %v; want a write", wrote, err) + } + blob, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read blob: %v", err) + } + plain, version, err := webauthn.UnwrapKey(blob, secret) + if err != nil || version != webauthn.BlobV2 || !bytes.Equal(plain, key) { + t.Fatalf("UnwrapKey = %x, %v, %v", plain, version, err) + } + if fi, err := os.Stat(path); err != nil || fi.Mode().Perm() != 0o600 { + t.Fatalf("mode = %v (%v), want 0600", fi.Mode().Perm(), err) + } +} + +// A blob that already wraps this key under this secret is left alone. Without +// this every assertion rewrote the one file that opens the database. +func TestWrapKeyToFileSkipsAnIdenticalBlob(t *testing.T) { + path := wrapPath(t) + key := bytes.Repeat([]byte{3}, 32) + secret := bytes.Repeat([]byte{4}, 32) + + if _, err := wrapKeyToFile(path, key, secret); err != nil { + t.Fatalf("first wrap: %v", err) + } + before, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read: %v", err) + } + wrote, err := wrapKeyToFile(path, key, secret) + if err != nil { + t.Fatalf("second wrap: %v", err) + } + if wrote { + t.Error("rewrote a blob that already opens under this secret") + } + after, _ := os.ReadFile(path) + if !bytes.Equal(before, after) { + t.Error("the blob changed on a no-op wrap") + } +} + +// Two enrolled authenticators, two PRF secrets, one blob. The second must not +// silently lock the first one out — the backup passkey enrolled for exactly +// the cold-start case is the one thing that used to stop working. +func TestWrapKeyToFileRefusesAnotherCredentialsBlob(t *testing.T) { + path := wrapPath(t) + key := bytes.Repeat([]byte{5}, 32) + phone := bytes.Repeat([]byte{6}, 32) + yubikey := bytes.Repeat([]byte{7}, 32) + + if _, err := wrapKeyToFile(path, key, phone); err != nil { + t.Fatalf("first wrap: %v", err) + } + before, _ := os.ReadFile(path) + + wrote, err := wrapKeyToFile(path, key, yubikey) + if !errors.Is(err, errForeignBlob) { + t.Fatalf("wrapKeyToFile = %v, %v; want errForeignBlob", wrote, err) + } + after, _ := os.ReadFile(path) + if !bytes.Equal(before, after) { + t.Fatal("the second authenticator overwrote the first one's blob") + } + if _, _, err := webauthn.UnwrapKey(after, phone); err != nil { + t.Fatalf("the first authenticator can no longer open the blob: %v", err) + } +} + +// A v1 blob is the pre-#14 format. It is upgraded in place rather than +// refused, because that is the only way off a format that protects nothing. +func TestWrapKeyToFileUpgradesALegacyBlob(t *testing.T) { + path := wrapPath(t) + key := bytes.Repeat([]byte{8}, 32) + secret := bytes.Repeat([]byte{9}, 32) + + // A v1 blob is a v2 blob with the magic stripped and the v1 info string; + // the package writes no v1, so build one the only way a test can: wrap + // v2 under a public key, then hand the file a body with no magic. What + // matters here is only that UnwrapKey classifies it as v1. + v2, err := webauthn.WrapKey(key, secret) + if err != nil { + t.Fatalf("WrapKey: %v", err) + } + legacy := v2[7:] // drop the magic + if err := os.WriteFile(path, legacy, 0o600); err != nil { + t.Fatalf("write legacy blob: %v", err) + } + if _, version, _ := webauthn.UnwrapKey(legacy, secret); version != webauthn.BlobV1 { + t.Fatalf("fixture is not read as v1 (got %v)", version) + } + + wrote, err := wrapKeyToFile(path, key, secret) + if err != nil || !wrote { + t.Fatalf("wrapKeyToFile = %v, %v; want the legacy blob upgraded", wrote, err) + } + blob, _ := os.ReadFile(path) + if _, version, err := webauthn.UnwrapKey(blob, secret); err != nil || version != webauthn.BlobV2 { + t.Fatalf("after upgrade: version %v, err %v", version, err) + } +} + +// A rotated at-rest key under the same credential is a rewrite, not a no-op. +func TestWrapKeyToFileRewritesARotatedKey(t *testing.T) { + path := wrapPath(t) + secret := bytes.Repeat([]byte{10}, 32) + old := bytes.Repeat([]byte{11}, 32) + fresh := bytes.Repeat([]byte{12}, 32) + + if _, err := wrapKeyToFile(path, old, secret); err != nil { + t.Fatalf("first wrap: %v", err) + } + wrote, err := wrapKeyToFile(path, fresh, secret) + if err != nil || !wrote { + t.Fatalf("wrapKeyToFile = %v, %v; want the rotated key written", wrote, err) + } + blob, _ := os.ReadFile(path) + plain, _, err := webauthn.UnwrapKey(blob, secret) + if err != nil || !bytes.Equal(plain, fresh) { + t.Fatalf("blob still wraps the old key (%v)", err) + } +} + +// The write never truncates the target in place, so a crash mid-write cannot +// leave a zero-length blob where the only copy of the wrapped key was. +func TestWriteFileAtomicLeavesNoTempFilesAndReplacesWhole(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "db_key.wrapped") + + if err := os.WriteFile(path, bytes.Repeat([]byte{0xaa}, 67), 0o600); err != nil { + t.Fatalf("seed: %v", err) + } + // Hold the old inode. A rename gives it a new one; a truncating write + // would keep it. + oldInfo, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + + want := bytes.Repeat([]byte{0xbb}, 67) + if err := writeFileAtomic(path, want, 0o600); err != nil { + t.Fatalf("writeFileAtomic: %v", err) + } + got, err := os.ReadFile(path) + if err != nil || !bytes.Equal(got, want) { + t.Fatalf("content = %x (%v)", got, err) + } + newInfo, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if os.SameFile(oldInfo, newInfo) { + t.Error("the target was written in place, not renamed over") + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("readdir: %v", err) + } + if len(entries) != 1 { + t.Errorf("directory holds %d entries, want just the blob (a temp file leaked)", len(entries)) + } +} diff --git a/cmd/mavend/main.go b/cmd/mavend/main.go index d6df63a..e0634be 100644 --- a/cmd/mavend/main.go +++ b/cmd/mavend/main.go @@ -25,7 +25,7 @@ // When a passkey credential is enrolled AND no env key is set, the daemon // starts in LOCKED mode: the IPC server runs but rejects all store methods // except MethodAssertStepUp and MethodUnlock. A passkey assertion followed -// by MethodUnlock (with the same credential's public key) unwraps the at-rest +// by MethodUnlock (with that credential's WebAuthn PRF output) unwraps the at-rest // AES-256 key from a wrapped blob on disk (HKDF-SHA256 + AES-GCM) and opens // the encrypted store. After unlock, the daemon wires voice, loop, and // delivery and runs normally. @@ -34,10 +34,13 @@ // starts unlocked from the env key (pre-unlock behavior). Enrolling a passkey // while unlocked calls MethodStoreEncryptionKey to wrap the env key and // persist the wrapped blob — enabling cold-start unlock on the next boot -// after the env key is removed. +// after the env key is removed. That write happens once, when no blob +// exists; replacing an existing one takes an explicit request, see +// cmd/mavend/keyfile.go. package main import ( + "bytes" "context" "encoding/json" "errors" @@ -48,6 +51,7 @@ import ( "os" "os/signal" "sync" + "sync/atomic" "syscall" "time" @@ -164,11 +168,28 @@ func run(args []string) error { var st *store.Store var envKeyBytes []byte // kept for WrapKeyFn (enrollment wraps this key) + // dbKey — the plaintext at-rest key, once the daemon has one. Set at boot + // in env-key mode and inside UnlockFn after a cold start. WrapKeyFn reads + // it from an IPC goroutine, hence the atomic: srv's function fields are + // installed before Serve and must not be reassigned afterwards. + var dbKey atomic.Pointer[[]byte] + + // wrappedPath resolves the blob location the same way for both the read + // at boot and every write, so a default-path deployment cannot wrap to + // one file and unwrap from another. + wrappedPath := func() string { + if *wrappedKeyPath != "" { + return *wrappedKeyPath + } + return cfg.DefaultWrappedKeyPath() + } + if !locked { // Normal boot: env key or plaintext (dev/CI) if envKey != nil { envKeyBytes = make([]byte, len(envKey)) copy(envKeyBytes, envKey) + dbKey.Store(&envKeyBytes) st, err = store.OpenEncrypted(ctx, cfg.DBPath, cfg.DBTmpfs, envKey) } else { st, err = store.Open(ctx, cfg.DBPath) @@ -387,27 +408,44 @@ func run(args []string) error { wireSpeaker(srv, st, cfg) } - // WrapKeyFn — wraps the env key under the passkey PRF secret and persists - // the wrapped blob. Only wired when the daemon has the key in memory (env - // key mode). Called by mavweb after passkey enrollment. + // WrapKeyFn — wraps the at-rest key under the passkey PRF secret and + // persists the wrapped blob. Called by mavweb after every assertion. + // + // It is wired in locked mode too, not only in env-key mode, and that is + // what makes a v1 blob recoverable. A box enrolled before Vikunja #14 + // cold-starts through the legacy public-key retry in mavweb, and the + // StoreEncryptionKey that follows rewrites the blob as v2. Without this + // the only escape from a v1 blob was putting MAVEN_DB_KEY back in the + // environment, which is the thing cold-start unlock exists to avoid. // // webauthn.WrapKey refuses anything that is not a 32-byte PRF output, so // an authenticator without PRF support produces no wrapped file at all // rather than a file that looks protected and is not. - if envKeyBytes != nil { - srv.WrapKeyFn = func(ctx context.Context, secret []byte) error { - blob, err := webauthn.WrapKey(envKeyBytes, secret) + if envKeyBytes != nil || locked { + srv.WrapKeyFn = func(ctx context.Context, secret []byte, explicit bool) error { + kp := dbKey.Load() + if kp == nil { + return errors.New("wrap encryption key: the daemon is locked and has no key yet (unlock first)") + } + wp := wrappedPath() + // Asserting a passkey is not a request to rewrite the cold-start + // key. Without this an assertion carrying a substituted PRF value + // re-wrapped the real database key under it, and a second + // authenticator silently replaced the first one's blob. + if !explicit { + if _, err := os.Stat(wp); err == nil { + return nil + } else if !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("check wrapped key: %w", err) + } + } + wrote, err := wrapKeyToFile(wp, *kp, secret) if err != nil { - return fmt.Errorf("wrap encryption key: %w", err) + return err } - wp := *wrappedKeyPath - if wp == "" { - wp = cfg.DefaultWrappedKeyPath() + if wrote { + log.Printf("mavend: wrapped encryption key under this passkey's PRF output → %s", wp) } - if err := os.WriteFile(wp, blob, 0o600); err != nil { - return fmt.Errorf("write wrapped key: %w", err) - } - log.Printf("mavend: wrapped encryption key with passkey credential (%d bytes)", len(blob)) return nil } } @@ -428,15 +466,17 @@ func run(args []string) error { return nil // already unlocked; the caller does not need to know } - // The wire cannot authenticate its caller — the socket is - // same-uid — so the unlock path requires a passkey assertion - // that mavweb verified cryptographically first. Without this, - // MethodUnlock is reachable by anything on the box. + // Depth, not a boundary. MethodAssertStepUp is AuthRead, so + // anything that can open the same-uid socket can flip the + // session and reach MethodUnlock. What actually stops a local + // attacker is the 32-byte PRF output they do not have, and that + // was true before this check. What this check stops is an + // accidental unlock attempt from an unrelated local caller. if !passkeySess.IsStepUp() { return errors.New("unlock: no verified passkey assertion (assert first)") } - wp := *wrappedKeyPath + wp := wrappedPath() blob, err := os.ReadFile(wp) if err != nil { return fmt.Errorf("read wrapped key: %w", err) @@ -446,8 +486,11 @@ func run(args []string) error { return fmt.Errorf("unwrap key: %w", err) } if version == webauthn.BlobV1 { - log.Printf("SECURITY: %s was unwrapped from a %s blob. The wrapping key is derived from the credential PUBLIC key, which mavweb also writes to its passkeys.json — anyone holding both files can recover the database key with no authenticator. Re-enroll the passkey on an authenticator that supports the PRF extension to rewrite it as v2.", wp, version) + log.Printf("SECURITY: %s was unwrapped from a %s blob. The wrapping key is derived from the credential PUBLIC key, which mavweb also writes to its passkeys.json — anyone holding both files can recover the database key with no authenticator. Use the \"rewrite cold-start key\" button on /auth/webauthn with a PRF-capable authenticator to replace it with a v2 blob.", wp, version) } + // WrapKeyFn needs the key to be able to rewrite the blob later. + keyCopy := bytes.Clone(key) + dbKey.Store(&keyCopy) // Open the store with the unwrapped key. st, err = store.OpenEncrypted(ctx, cfg.DBPath, cfg.DBTmpfs, key) if err != nil { diff --git a/cmd/mavweb/passkey_prf_test.go b/cmd/mavweb/passkey_prf_test.go index 4c5c95d..915551f 100644 --- a/cmd/mavweb/passkey_prf_test.go +++ b/cmd/mavweb/passkey_prf_test.go @@ -32,17 +32,28 @@ type fakeKeyIPC struct { unlockCalls int wrapCalls int unlockErr error + wrapExplicit bool + // opensWith, when set, is the only secret Unlock accepts. It stands in + // for a wrapped blob on disk: everything else gets unlockErr. + opensWith []byte } func (f *fakeKeyIPC) Unlock(_ context.Context, secret []byte) error { f.unlockCalls++ f.unlockSecret = bytes.Clone(secret) + if f.opensWith != nil { + if bytes.Equal(secret, f.opensWith) { + return nil + } + return errors.New("unwrap key: decrypt failed (wrong credential?)") + } return f.unlockErr } -func (f *fakeKeyIPC) StoreEncryptionKey(_ context.Context, secret []byte) error { +func (f *fakeKeyIPC) StoreEncryptionKey(_ context.Context, secret []byte, explicit bool) error { f.wrapCalls++ f.wrapSecret = bytes.Clone(secret) + f.wrapExplicit = explicit return nil } @@ -137,6 +148,13 @@ func (a *prfAuthenticator) register(t *testing.T, h *PasskeyHandle) { // assert drives POST /assert/finish with a valid assertion and the given // base64url PRF result. func (a *prfAuthenticator) assert(t *testing.T, h *PasskeyHandle, prf string) *httptest.ResponseRecorder { + t.Helper() + return a.assertExplicit(t, h, prf, false) +} + +// assertExplicit is assert with control over the explicit flag the rewrite +// button sets. +func (a *prfAuthenticator) assertExplicit(t *testing.T, h *PasskeyHandle, prf string, explicit bool) *httptest.ResponseRecorder { t.Helper() _, chal, err := h.rp.AssertionOptions() if err != nil { @@ -152,6 +170,7 @@ func (a *prfAuthenticator) assert(t *testing.T, h *PasskeyHandle, prf string) *h body, _ := json.Marshal(map[string]any{ "challenge": chal, "prf": prf, + "explicit": explicit, "credential": map[string]any{ "id": b64u(a.credID), "type": "public-key", @@ -253,8 +272,8 @@ func TestAssertSucceedsWhenUnlockFails(t *testing.T) { if w := auth.assert(t, h, b64u(secret)); w.Code != http.StatusOK { t.Fatalf("AssertFinish: %d %s", w.Code, w.Body.String()) } - if key.unlockCalls != 1 { - t.Errorf("unlock attempted %d times, want 1", key.unlockCalls) + if key.unlockCalls == 0 { + t.Error("unlock was never attempted") } } @@ -291,3 +310,100 @@ func TestPasskeyPageRequestsAndPostsPRF(t *testing.T) { } } } + +// A box enrolled before Vikunja #14 has a v1 blob wrapped under the credential +// PUBLIC key. The PRF secret cannot open it, and this handler is the only +// caller of Unlock, so without the legacy retry that box stays locked forever +// while a perfectly good passkey is asserted at it. +func TestLegacyV1BlobStillColdStarts(t *testing.T) { + key := &fakeKeyIPC{} + h := newPRFHandle(t, key) + auth := newPRFAuthenticator(t) + auth.register(t, h) + + pub, _, err := h.store.Lookup(b64u(auth.credID)) + if err != nil { + t.Fatalf("lookup: %v", err) + } + // The daemon only opens under the public key — a v1 blob. + key.opensWith = pub + + secret := bytes.Repeat([]byte{9}, 32) + if w := auth.assert(t, h, b64u(secret)); w.Code != http.StatusOK { + t.Fatalf("AssertFinish: %d %s", w.Code, w.Body.String()) + } + if key.unlockCalls != 2 { + t.Fatalf("unlock attempted %d times, want 2 (PRF, then the legacy public key)", key.unlockCalls) + } + if !bytes.Equal(key.unlockSecret, pub) { + t.Fatal("the legacy retry did not send the credential public key, so a v1 box can never cold-start again") + } +} + +// The PRF secret is tried first and, when it works, the public key is never +// sent. The legacy retry is a one-way door out of v1, not a fallback offered +// to every assertion. +func TestPRFUnlockNeverFallsBackWhenItWorks(t *testing.T) { + secret := bytes.Repeat([]byte{7}, 32) + key := &fakeKeyIPC{opensWith: secret} + h := newPRFHandle(t, key) + auth := newPRFAuthenticator(t) + auth.register(t, h) + + if w := auth.assert(t, h, b64u(secret)); w.Code != http.StatusOK { + t.Fatalf("AssertFinish: %d %s", w.Code, w.Body.String()) + } + if key.unlockCalls != 1 { + t.Fatalf("unlock attempted %d times, want 1", key.unlockCalls) + } +} + +// Wrapping the at-rest key is an explicit act, never a side effect of a +// step-up. A page POSTing a substituted prf on a routine assertion must not +// make the daemon re-wrap the database key under it. +func TestPlainAssertionAsksForNoRewrite(t *testing.T) { + key := &fakeKeyIPC{} + h := newPRFHandle(t, key) + auth := newPRFAuthenticator(t) + auth.register(t, h) + + if w := auth.assert(t, h, b64u(bytes.Repeat([]byte{5}, 32))); w.Code != http.StatusOK { + t.Fatalf("AssertFinish: %d %s", w.Code, w.Body.String()) + } + if key.wrapCalls != 1 { + t.Fatalf("wrapCalls = %d, want 1", key.wrapCalls) + } + if key.wrapExplicit { + t.Fatal("a plain step-up asked the daemon to rewrite the cold-start key") + } +} + +// The rewrite button, and only the rewrite button, sets explicit. +func TestRewriteButtonAsksForAnExplicitWrap(t *testing.T) { + key := &fakeKeyIPC{} + h := newPRFHandle(t, key) + auth := newPRFAuthenticator(t) + auth.register(t, h) + + if w := auth.assertExplicit(t, h, b64u(bytes.Repeat([]byte{6}, 32)), true); w.Code != http.StatusOK { + t.Fatalf("AssertFinish: %d %s", w.Code, w.Body.String()) + } + if !key.wrapExplicit { + t.Fatal("the explicit flag did not reach the daemon, so the rewrite button cannot work") + } +} + +// The page is the only place the explicit flag originates. If the button or +// the field goes away, rewriting a cold-start key becomes impossible with +// nothing failing. +func TestPasskeyPageHasTheRewriteButton(t *testing.T) { + for _, want := range []string{ + "rewrite cold-start key", + "explicit:!!explicit", + "async function rewrapKey()", + } { + if !strings.Contains(passkeyPageHTML, want) { + t.Errorf("the passkey page no longer contains %q", want) + } + } +} diff --git a/cmd/mavweb/webauthn.go b/cmd/mavweb/webauthn.go index 8192d9f..6717e8c 100644 --- a/cmd/mavweb/webauthn.go +++ b/cmd/mavweb/webauthn.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "log" "net/http" @@ -23,7 +24,7 @@ type assertIPC interface { // is *ipc.Client; in-process CoreAPI adapters do not implement it. When nil, // StoreEncryptionKey and Unlock are silently skipped. type keyIPC interface { - StoreEncryptionKey(ctx context.Context, secret []byte) error + StoreEncryptionKey(ctx context.Context, secret []byte, explicit bool) error Unlock(ctx context.Context, secret []byte) error } @@ -86,8 +87,10 @@ const passkeyPageHTML = `{{template "shellTop" "passkey"}}
+
+

Rewriting the cold-start key points it at the passkey you assert next. Every other enrolled passkey stops being able to unlock a cold-booted daemon.

{{template "shellBottom"}} ` func (h *PasskeyHandle) RegisterBegin(w http.ResponseWriter, r *http.Request) { @@ -201,7 +211,20 @@ func (h *PasskeyHandle) AssertFinish(w http.ResponseWriter, r *http.Request) { // getClientExtensionResults(). Empty when the authenticator has no // PRF extension: cold-start unlock is then unavailable and we say so // rather than falling back to something weaker. + // + // Known property, accepted deliberately: this value is supplied by + // the client and is NOT covered by the assertion signature. WebAuthn + // client extension outputs never are, and binding one would need a + // per-assertion salt, which would make the wrapped blob unopenable on + // the next boot. Nothing here can tell a real PRF output from 32 + // bytes a compromised page chose. What limits the damage is that the + // daemon refuses to rewrite an existing blob unless the operator + // asked for it — see Explicit below and cmd/mavend/keyfile.go. PRF string `json:"prf"` + // Explicit marks the "rewrite cold-start key" button rather than a + // plain step-up. Only then may the daemon replace a blob that is + // already on disk. + Explicit bool `json:"explicit"` } if err := json.NewDecoder(r.Body).Decode(&body); err != nil { http.Error(w, "bad request: "+err.Error(), http.StatusBadRequest) @@ -235,32 +258,22 @@ func (h *PasskeyHandle) AssertFinish(w http.ResponseWriter, r *http.Request) { } } - // Cold-start unlock and key wrapping, both keyed on the PRF secret that - // this assertion just produced. The secret is used here and dropped; it is - // never stored on this side. + // Cold-start unlock and key wrapping, both keyed on the PRF secret this + // assertion just produced. The secret is used here and dropped; it is + // never stored on this side, and it must never be logged — unlike a + // signature it does not expire, so one copy in a proxy log or a HAR file + // is permanent access to the wrapped blob. // // Order matters: unlock first (if the daemon is locked there is nothing to - // wrap yet), then re-wrap, which writes the blob on the first assertion - // after enrolment and is a harmless rewrite afterwards. Both are - // best-effort — the assertion itself is valid either way. + // wrap yet), then wrap. Both are best-effort, because the assertion itself + // is valid either way. if h.encryptFn != nil { - secret, err := webauthn.DecodePRFResult(body.PRF) - switch { - case err != nil: + if secret, err := webauthn.DecodePRFResult(body.PRF); err != nil { log.Printf("webauthn: no usable PRF secret from credential %s: %v", credID, err) - default: + } else { ctx, cancel := context.WithTimeout(r.Context(), 10*time.Second) defer cancel() - if err := h.encryptFn.Unlock(ctx, secret); err != nil { - log.Printf("webauthn: unlock via credential %s: %v", credID, err) - } else { - log.Printf("webauthn: daemon unlocked via credential %s", credID) - } - if err := h.encryptFn.StoreEncryptionKey(ctx, secret); err != nil { - log.Printf("webauthn: wrap encryption key: %v", err) - } else { - log.Printf("webauthn: encryption key wrapped for credential %s", credID) - } + h.coldStart(ctx, credID, secret, body.Explicit) } } @@ -272,3 +285,56 @@ func (h *PasskeyHandle) AssertFinish(w http.ResponseWriter, r *http.Request) { log.Printf("webauthn: asserted credential %s", credID) json.NewEncoder(w).Encode(map[string]string{"credential_id": credID}) } + +// coldStart unlocks a locked daemon with this assertion's PRF output and then +// asks it to wrap the at-rest key. Never fatal: a locked or unreachable daemon +// does not invalidate the step-up. +// +// # The legacy retry +// +// A box enrolled before Vikunja #14 has a v1 blob, wrapped under the +// credential PUBLIC key. The PRF secret cannot open it, and this handler is +// the only caller of Unlock, so without a second attempt that box could never +// cold-start again: it would sit locked while a perfectly good passkey was +// asserted, and the only way back in would be putting MAVEN_DB_KEY into the +// environment — the exact thing cold-start unlock exists to avoid. +// +// So a failed PRF unlock is retried with the public key from the credential +// store. That is not a weaker fallback being offered to new deployments: +// nothing writes v1 any more, and a v2 blob does not open under a public key +// either. It is a one-way door out of the old format, and the operator is told +// to walk through it. +func (h *PasskeyHandle) coldStart(ctx context.Context, credID string, secret []byte, explicit bool) { + legacy := false + err := h.encryptFn.Unlock(ctx, secret) + if err != nil && !errors.Is(err, ipc.ErrUnknownMethod) { + if pub, _, lerr := h.store.Lookup(credID); lerr == nil && len(pub) > 0 { + if err2 := h.encryptFn.Unlock(ctx, pub); err2 == nil { + err, legacy = nil, true + } + } + } + switch { + case errors.Is(err, ipc.ErrUnknownMethod): + // Env-key mode: the daemon was never locked and has no UnlockFn. Not + // a failure, and the old code logged it as one on every assertion. + case err != nil: + log.Printf("webauthn: unlock via credential %s failed: %v", credID, err) + case legacy: + log.Printf("SECURITY: webauthn: daemon unlocked from a LEGACY v1 wrapped key using credential %s. That blob is derived from the credential public key, which sits in passkeys.json beside it, so it protects nothing. Press \"rewrite cold-start key\" on this page to replace it with a v2 blob.", credID) + default: + log.Printf("webauthn: daemon reports unlocked, credential %s", credID) + } + + // explicit=false means "write the blob only if there is none". The daemon + // enforces that; sending the flag is the whole of this side's part in it. + switch err := h.encryptFn.StoreEncryptionKey(ctx, secret, explicit); { + case err == nil && explicit: + log.Printf("webauthn: cold-start key rewritten under credential %s", credID) + case err == nil: + case errors.Is(err, ipc.ErrUnknownMethod): + // No key to wrap: a plaintext dev store, or a daemon still locked. + default: + log.Printf("webauthn: wrap encryption key: %v", err) + } +} diff --git a/internal/ipc/api.go b/internal/ipc/api.go index 30cc4ab..d3faccb 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -755,14 +755,22 @@ type DayPlan struct { } // storeEncryptionKeyReq — the passkey-derived secret used to wrap the store -// encryption key at enrollment time. Called by mavweb after RegisterFinish. +// encryption key. Called by mavweb after a verified assertion. // // Secret is the 32-byte WebAuthn PRF output, NOT the credential public key. // The field used to carry the public key and that was the bug: a public key // sits in passkeys.json next to the wrapped blob, so the blob protected // nothing. See internal/webauthn/keywrap.go. +// +// Explicit says the operator asked for the cold-start key to be written, as +// opposed to it being a side effect of asserting a passkey. Without the flag +// the daemon writes only when no blob exists yet. Rewriting on every assertion +// is what let a page-level compromise substitute its own PRF value and have +// the daemon re-wrap the real database key under it, and what let a second +// authenticator silently replace the first one's blob. type storeEncryptionKeyReq struct { - Secret []byte `json:"secret"` + Secret []byte `json:"secret"` + Explicit bool `json:"explicit,omitempty"` } // unlockReq — the passkey-derived secret for unwrapping the store encryption diff --git a/internal/ipc/client.go b/internal/ipc/client.go index fae2f4d..1add92e 100644 --- a/internal/ipc/client.go +++ b/internal/ipc/client.go @@ -395,9 +395,14 @@ func (c *Client) AssertStepUp(ctx context.Context) error { } // StoreEncryptionKey wraps the daemon's at-rest key under secret, the 32-byte -// WebAuthn PRF output for the freshly enrolled credential. -func (c *Client) StoreEncryptionKey(ctx context.Context, secret []byte) error { - return c.call(ctx, MethodStoreEncryptionKey, storeEncryptionKeyReq{Secret: secret}, nil) +// WebAuthn PRF output for the asserted credential. +// +// explicit marks an operator-requested write. False means "write it only if +// there is nothing there yet": a blob already on disk is left alone, because +// rewriting it on every assertion is how an attacker-chosen PRF value, or a +// second authenticator, replaces the one thing that opens the database. +func (c *Client) StoreEncryptionKey(ctx context.Context, secret []byte, explicit bool) error { + return c.call(ctx, MethodStoreEncryptionKey, storeEncryptionKeyReq{Secret: secret, Explicit: explicit}, nil) } // Unlock hands the daemon the PRF secret so it can unwrap its at-rest key and diff --git a/internal/ipc/server.go b/internal/ipc/server.go index cee5355..e1e3145 100644 --- a/internal/ipc/server.go +++ b/internal/ipc/server.go @@ -498,7 +498,11 @@ type Server struct { // WrapKeyFunc — wraps the store encryption key under the passkey-derived // secret (a 32-byte WebAuthn PRF output) and persists the wrapped blob. -type WrapKeyFunc func(ctx context.Context, secret []byte) error +// +// explicit distinguishes "the operator asked for the cold-start key to be +// written" from "a passkey was asserted". Only the first may overwrite a blob +// that is already there; see cmd/mavend/keyfile.go. +type WrapKeyFunc func(ctx context.Context, secret []byte, explicit bool) error // UnlockFunc — unwraps the store encryption key using the passkey-derived // secret and completes daemon initialization. @@ -945,7 +949,7 @@ func (s *Server) dispatch(ctx context.Context, req Request) (json.RawMessage, er if err := unmarshalParams(req.Params, &p); err != nil { return nil, err } - return marshalResult(nil), s.WrapKeyFn(ctx, p.Secret) + return marshalResult(nil), s.WrapKeyFn(ctx, p.Secret, p.Explicit) } return nil, fmt.Errorf("%w: %s", ErrUnknownMethod, req.Method) diff --git a/internal/ipc/unlock_test.go b/internal/ipc/unlock_test.go index 14bdb2e..f055d5f 100644 --- a/internal/ipc/unlock_test.go +++ b/internal/ipc/unlock_test.go @@ -40,8 +40,9 @@ func TestUnlockDeliversSecretToHook(t *testing.T) { secret[i] = byte(i + 1) } var gotUnlock, gotWrap []byte + var gotExplicit bool srv.UnlockFn = func(_ context.Context, s []byte) error { gotUnlock = bytes.Clone(s); return nil } - srv.WrapKeyFn = func(_ context.Context, s []byte) error { gotWrap = bytes.Clone(s); return nil } + srv.WrapKeyFn = func(_ context.Context, s []byte, explicit bool) error { gotWrap = bytes.Clone(s); gotExplicit = explicit; return nil } ctx := context.Background() if err := cli.Unlock(ctx, secret); err != nil { @@ -50,12 +51,24 @@ func TestUnlockDeliversSecretToHook(t *testing.T) { if !bytes.Equal(gotUnlock, secret) { t.Errorf("UnlockFn got %x, want %x", gotUnlock, secret) } - if err := cli.StoreEncryptionKey(ctx, secret); err != nil { + if err := cli.StoreEncryptionKey(ctx, secret, true); err != nil { t.Fatalf("StoreEncryptionKey: %v", err) } if !bytes.Equal(gotWrap, secret) { t.Errorf("WrapKeyFn got %x, want %x", gotWrap, secret) } + // The explicit flag rides the same request. Without it the daemon cannot + // tell "he asked for the cold-start key to be rewritten" from "a passkey + // was asserted", and rewrites the blob on every step-up. + if !gotExplicit { + t.Error("WrapKeyFn got explicit=false, want the flag to cross the wire") + } + if err := cli.StoreEncryptionKey(ctx, secret, false); err != nil { + t.Fatalf("StoreEncryptionKey: %v", err) + } + if gotExplicit { + t.Error("WrapKeyFn got explicit=true for an implicit wrap") + } } // A refusal from the daemon hook — a wrong passkey, or no prior assertion — @@ -78,7 +91,7 @@ func TestUnlockUnwiredIsUnknownMethod(t *testing.T) { if err := cli.Unlock(ctx, bytes.Repeat([]byte{1}, 32)); err == nil { t.Error("Unlock succeeded with no UnlockFn wired") } - if err := cli.StoreEncryptionKey(ctx, bytes.Repeat([]byte{1}, 32)); err == nil { + if err := cli.StoreEncryptionKey(ctx, bytes.Repeat([]byte{1}, 32), false); err == nil { t.Error("StoreEncryptionKey succeeded with no WrapKeyFn wired") } } @@ -100,7 +113,7 @@ func TestLockedCheckDefaultDenies(t *testing.T) { unlocked := false srv.UnlockFn = func(context.Context, []byte) error { unlocked = true; return nil } srv.StepUp = func(context.Context) error { return nil } - srv.WrapKeyFn = func(context.Context, []byte) error { return nil } + srv.WrapKeyFn = func(context.Context, []byte, bool) error { return nil } ctx := context.Background() // A store method must be refused while locked. @@ -108,7 +121,7 @@ func TestLockedCheckDefaultDenies(t *testing.T) { t.Error("a store read went through while locked") } // Key wrapping is NOT on the allowlist: a locked daemon has no key to wrap. - if err := cli.StoreEncryptionKey(ctx, bytes.Repeat([]byte{2}, 32)); err == nil { + if err := cli.StoreEncryptionKey(ctx, bytes.Repeat([]byte{2}, 32), false); err == nil { t.Error("StoreEncryptionKey was allowed while locked") } // The unlock flow itself must still work. diff --git a/internal/webauthn/keywrap.go b/internal/webauthn/keywrap.go index c056cb2..7129ea4 100644 --- a/internal/webauthn/keywrap.go +++ b/internal/webauthn/keywrap.go @@ -24,7 +24,18 @@ // // v1 blobs are still readable, so an existing deployment opens and can be // re-wrapped, and UnwrapKey reports which format it read so the caller can -// say so out loud. Nothing writes v1 any more. +// say so out loud. Nothing writes v1 any more. Reading one needs the +// credential public key, which only cmd/mavweb still has: its assertion +// handler retries a failed PRF unwrap with it, because otherwise a box +// enrolled before v2 could never cold-start again. +// +// # What the wrapped blob's security rests on +// +// The PRF secret is stable for the lifetime of the credential and it reaches +// mavend inside an HTTP request body. Unlike a signature it does not expire. +// One copy in a proxy log, a devtools HAR, or a crash dump is permanent +// offline access to whatever this blob wraps. Nothing on this path may log the +// secret, and nothing does. // // # Blob format // @@ -171,8 +182,16 @@ func unwrap(body, secret []byte, info string, aad []byte, wantSecretLen int) ([] if len(body) < saltLen+nonceLen+1 { return nil, fmt.Errorf("%w: blob too short (%d)", ErrKeyUnwrap, len(body)) } - if wantSecretLen > 0 && len(secret) != wantSecretLen { - return nil, fmt.Errorf("%w: %v", ErrKeyUnwrap, ErrSecretLen) + // The v2 side is held to exactly what WrapKey demands, all-zero included. + // Letting the two ends disagree about what a valid secret is would leave + // a blob that can be opened by material that could never have sealed it. + if wantSecretLen > 0 { + if len(secret) != wantSecretLen { + return nil, fmt.Errorf("%w: %v", ErrKeyUnwrap, ErrSecretLen) + } + if err := checkSecret(secret); err != nil { + return nil, fmt.Errorf("%w: %v", ErrKeyUnwrap, err) + } } salt := body[:saltLen] diff --git a/internal/webauthn/keywrap_test.go b/internal/webauthn/keywrap_test.go index f2814e8..c3d2816 100644 --- a/internal/webauthn/keywrap_test.go +++ b/internal/webauthn/keywrap_test.go @@ -229,3 +229,18 @@ func TestUnwrapRejectsOversizeBlob(t *testing.T) { t.Fatalf("err = %v, want ErrBlobTooLong", err) } } + +// WrapKey refuses an all-zero secret because a blob wrapped under one is a +// blob anyone can open. The v2 unwrap side must refuse it for the same reason: +// if the two ends disagree about what a valid secret is, a blob can be opened +// by material that could never have sealed it. +func TestUnwrapV2RefusesAnAllZeroSecret(t *testing.T) { + key := bytes.Repeat([]byte{1}, 32) + blob, err := WrapKey(key, bytes.Repeat([]byte{2}, 32)) + if err != nil { + t.Fatalf("WrapKey: %v", err) + } + if _, _, err := UnwrapKey(blob, make([]byte, 32)); !errors.Is(err, ErrKeyUnwrap) { + t.Fatalf("UnwrapKey with an all-zero secret = %v, want refusal", err) + } +} diff --git a/models/stt b/models/stt new file mode 120000 index 0000000..b983fa3 --- /dev/null +++ b/models/stt @@ -0,0 +1 @@ +/home/kami/apps/Maven/models/stt \ No newline at end of file diff --git a/models/tts b/models/tts new file mode 120000 index 0000000..66782fb --- /dev/null +++ b/models/tts @@ -0,0 +1 @@ +/home/kami/apps/Maven/models/tts \ No newline at end of file From f02f3b55b6d5858c4f09b0b986ef92e494be06c9 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:12:43 +0400 Subject: [PATCH 2/6] memory: keep voiceprints out of note and fact recall Speaker profiles share the vector table with notes and facts. The doc comment said reading them through Catalog is what keeps recall from ranking a voiceprint. It is not. Catalog controls how speaker code reads its own rows and says nothing about Search, which scanned every row. What actually hid them was cosine returning 0 on a width mismatch, so a 192-dim ECAPA row scored 0 against a 384-dim query. Some x-vector exports are 384-dim, and one of those would have surfaced speaker:kami as a recall hit carrying the name of a person. Both backends now skip the prefix in Search, and the prefix is one constant in internal/memory so the store layer can filter on it without importing internal/speaker. Two more differences between the backends closed here. ByPrefix on the in-memory store returned the stored metadata map by reference, so a caller editing a returned Record edited the row, while the persistent one unmarshals fresh. And the append to upsert change in Insert is a fix in its own right, not only a speaker concern: any re-indexed id used to leave a second stale copy searchable. Found in review of #74. --- internal/memory/store.go | 26 ++++++++++++- internal/memory/store_test.go | 72 +++++++++++++++++++++++++++++++++++ internal/store/memory.go | 8 +++- internal/store/memory_test.go | 38 ++++++++++++++++++ 4 files changed, 142 insertions(+), 2 deletions(-) diff --git a/internal/memory/store.go b/internal/memory/store.go index d47f80c..5077e47 100644 --- a/internal/memory/store.go +++ b/internal/memory/store.go @@ -7,6 +7,20 @@ import ( "sync" ) +// NonRecallPrefix — rows whose id starts with this are excluded from Search by +// every backend. Speaker voiceprints live in the same vector table as notes and +// facts (internal/speaker writes them under this prefix), and they are not +// recall material: a voiceprint has no text to read back and surfacing one as a +// note hit leaks a name attached to a biometric. +// +// Reading them through Catalog.ByPrefix was documented as what keeps them out +// of recall. It is not. It controls how speaker code reads its own rows and +// says nothing about what Search scores. What actually kept them out was that +// cosine returns 0 on a width mismatch, so a 192-dim voiceprint scored 0 +// against a 384-dim query. That is a coincidence of two model choices — some +// x-vector exports are 384-dim — and not an invariant. This is the invariant. +const NonRecallPrefix = "speaker:" + // Result is a single search hit. type Result struct { ID string @@ -93,7 +107,14 @@ func (s *InMemoryStore) ByPrefix(_ context.Context, prefix string) ([]Record, er if !strings.HasPrefix(it.id, prefix) { continue } - out = append(out, Record{ID: it.id, Vec: append([]float32(nil), it.vec...), Meta: it.meta}) + // Copy the metadata too. Returning it.meta by reference let a caller + // mutating the returned map edit the stored row, and the persistent + // backend unmarshals fresh, so the two disagreed. + meta := make(map[string]string, len(it.meta)) + for k, v := range it.meta { + meta[k] = v + } + out = append(out, Record{ID: it.id, Vec: append([]float32(nil), it.vec...), Meta: meta}) } return out, nil } @@ -127,6 +148,9 @@ func (s *InMemoryStore) Search(_ context.Context, vec []float32, topK int) ([]Re scores := make([]scored, 0, len(s.items)) for _, it := range s.items { + if strings.HasPrefix(it.id, NonRecallPrefix) { + continue + } score := cosine(vec, it.vec) scores = append(scores, scored{id: it.id, score: score, meta: it.meta}) } diff --git a/internal/memory/store_test.go b/internal/memory/store_test.go index 06856fb..62c69a7 100644 --- a/internal/memory/store_test.go +++ b/internal/memory/store_test.go @@ -80,3 +80,75 @@ func TestCosineEdgeCases(t *testing.T) { t.Errorf("dot(1,2;1,2) = %f, want 5", c) } } + +// The in-memory backend has to hide voiceprints from recall exactly like the +// persistent one, or a test passing here says nothing about the daemon. +func TestInMemorySearchSkipsVoiceprints(t *testing.T) { + ctx := context.Background() + s := NewInMemoryStore() + if err := s.Insert(ctx, "note:1", []float32{0, 1}, map[string]string{"text": "заметка"}); err != nil { + t.Fatal(err) + } + if err := s.Insert(ctx, NonRecallPrefix+"kami", []float32{1, 0}, map[string]string{"name": "Ками"}); err != nil { + t.Fatal(err) + } + got, err := s.Search(ctx, []float32{1, 0}, 10) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || got[0].ID != "note:1" { + t.Fatalf("Search = %+v, want just the note", got) + } + recs, err := s.ByPrefix(ctx, NonRecallPrefix) + if err != nil || len(recs) != 1 { + t.Fatalf("ByPrefix = %+v, %v; want the voiceprint", recs, err) + } +} + +// ByPrefix hands back a copy of the metadata. It used to return the stored map +// by reference, so a caller editing a returned Record silently edited the row, +// and the persistent backend did not behave that way. +func TestInMemoryByPrefixCopiesMeta(t *testing.T) { + ctx := context.Background() + s := NewInMemoryStore() + if err := s.Insert(ctx, "speaker:kami", []float32{1, 0}, map[string]string{"name": "Ками"}); err != nil { + t.Fatal(err) + } + recs, err := s.ByPrefix(ctx, "speaker:") + if err != nil || len(recs) != 1 { + t.Fatalf("ByPrefix = %+v, %v", recs, err) + } + recs[0].Meta["name"] = "не Ками" + + again, err := s.ByPrefix(ctx, "speaker:") + if err != nil { + t.Fatal(err) + } + if again[0].Meta["name"] != "Ками" { + t.Errorf("the stored row was edited through the returned map: %q", again[0].Meta["name"]) + } +} + +// Insert upserts. This is not only a speaker-profile concern: every user of the +// in-memory store used to accumulate a second row for a re-indexed id, and the +// stale copy stayed searchable. +func TestInMemoryInsertUpserts(t *testing.T) { + ctx := context.Background() + s := NewInMemoryStore() + if err := s.Insert(ctx, "note:1", []float32{1, 0}, map[string]string{"text": "старое"}); err != nil { + t.Fatal(err) + } + if err := s.Insert(ctx, "note:1", []float32{0, 1}, map[string]string{"text": "новое"}); err != nil { + t.Fatal(err) + } + got, err := s.Search(ctx, []float32{1, 0}, 10) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 { + t.Fatalf("Search returned %d rows, want 1 (the old copy is still searchable)", len(got)) + } + if got[0].Meta["text"] != "новое" { + t.Errorf("row = %q, want the replacement", got[0].Meta["text"]) + } +} diff --git a/internal/store/memory.go b/internal/store/memory.go index 3d0527a..bc83648 100644 --- a/internal/store/memory.go +++ b/internal/store/memory.go @@ -64,11 +64,17 @@ func (m *MemoryStore) Insert(ctx context.Context, id string, vec []float32, meta // Search returns the topK nearest rows by cosine similarity. A full scan; see // the type doc for why that's fine at this scale. +// +// Rows under memory.NonRecallPrefix are excluded in SQL. They are speaker +// voiceprints sharing this table, and note recall must not rank them; see that +// constant for why the previous arrangement only appeared to do this. func (m *MemoryStore) Search(ctx context.Context, vec []float32, topK int) ([]memory.Result, error) { if topK <= 0 { topK = 10 } - rows, err := m.db.QueryContext(ctx, `SELECT id, vec, meta FROM memory_vectors`) + rows, err := m.db.QueryContext(ctx, + `SELECT id, vec, meta FROM memory_vectors WHERE id NOT LIKE ? ESCAPE '\'`, + escapeLike(memory.NonRecallPrefix)+"%") if err != nil { return nil, fmt.Errorf("memory: scan: %w", err) } diff --git a/internal/store/memory_test.go b/internal/store/memory_test.go index 3e57243..8ba2b09 100644 --- a/internal/store/memory_test.go +++ b/internal/store/memory_test.go @@ -3,7 +3,10 @@ package store import ( "context" "path/filepath" + "strings" "testing" + + "github.com/kami/maven/internal/memory" ) func newMemTestStore(t *testing.T) *Store { @@ -101,3 +104,38 @@ func TestMemoryStorePersistsAcrossReopen(t *testing.T) { t.Fatalf("memory did not survive reopen: %v", got) } } + +// A voiceprint sharing the vector table must never rank as a note hit. The +// dimensions match here on purpose: what used to hide these rows was cosine +// returning 0 on a width mismatch, which is a property of two model choices and +// not of the store. +func TestMemoryStoreSearchSkipsVoiceprints(t *testing.T) { + ctx := context.Background() + m := newMemTestStore(t).VectorMemory() + + if err := m.Insert(ctx, "note:1", []float32{0, 1, 0}, map[string]string{"text": "заметка"}); err != nil { + t.Fatal(err) + } + if err := m.Insert(ctx, memory.NonRecallPrefix+"kami", []float32{1, 0, 0}, map[string]string{"name": "Ками"}); err != nil { + t.Fatal(err) + } + + got, err := m.Search(ctx, []float32{1, 0, 0}, 10) + if err != nil { + t.Fatalf("Search: %v", err) + } + for _, r := range got { + if strings.HasPrefix(r.ID, memory.NonRecallPrefix) { + t.Fatalf("recall returned a voiceprint: %+v", r) + } + } + if len(got) != 1 || got[0].ID != "note:1" { + t.Fatalf("Search = %+v, want just the note", got) + } + + // The row is still there for the speaker code that owns it. + recs, err := m.ByPrefix(ctx, memory.NonRecallPrefix) + if err != nil || len(recs) != 1 { + t.Fatalf("ByPrefix = %+v, %v; want the voiceprint", recs, err) + } +} From ec5167de3abe2d29f3b7ed5aba0095b938f89a72 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:12:43 +0400 Subject: [PATCH 3/6] speaker: do not ship three methods that cannot work The package comment, the embedder log and the startup line all said enrolment was live and only recognition was blocked. Enroll embeds every sample before it stores anything, so with no model on the box it fails on the first sample with ErrDisabled and nothing is ever stored. List then returns an empty list forever and Forget has nothing to delete. The shipped state was three methods, all no-ops, announced as a working half. SpeakerConfig.Recognizes was written as the gate for this and never called, so a block with enabled and no model_path wired everything and skipped the one warning the operator needed. It is the gate now, and that config shape logs why it stayed off. Three smaller repairs. ErrDisabled had no case in speakerErr and reached the surface as an opaque core failure, when it means the same thing ErrUnknownMethod does. Forget read the row first and answered ErrNotFound on a second call, so the layer documented as the one that must always work reintroduced a failure for a voiceprint that was already gone. And a row with unparsable metadata listed as a plausible profile named after its own id with 0 samples, which is what a real minimal enrolment looks like; it is reported as damaged now. Found in review of #74. --- cmd/mavend/speaker.go | 50 ++++++++++++++++++---------- cmd/mavend/speaker_wiring_test.go | 50 ++++++++++++++++++++++++++++ internal/ipc/api.go | 6 ++++ internal/speaker/recognizer.go | 43 +++++++++++++----------- internal/speaker/speaker.go | 22 +++++++++++-- internal/speaker/speaker_test.go | 54 +++++++++++++++++++++++++++++-- 6 files changed, 186 insertions(+), 39 deletions(-) create mode 100644 cmd/mavend/speaker_wiring_test.go diff --git a/cmd/mavend/speaker.go b/cmd/mavend/speaker.go index 4585908..f717400 100644 --- a/cmd/mavend/speaker.go +++ b/cmd/mavend/speaker.go @@ -3,14 +3,17 @@ // // # What is actually wired here, and what is not // -// The enrolment plumbing is real: profiles are stored, listed and deleted, and -// the wire methods exist as soon as a speaker block is configured. The -// recognising half is NOT, and cannot be on this box, because there is no -// speaker-embedding model on disk — no ECAPA, no x-vector, no titanet, no -// wespeaker, nothing in /mnt/hdd1/llms but text ggufs. Until one is downloaded, -// newSpeakerEmbedder returns nil, internal/speaker falls back to -// speaker.Disabled, and every Identify answers ErrDisabled. The daemon logs -// which half is off at startup rather than pretending. +// Nothing is, on this box. There is no speaker-embedding model on disk — no +// ECAPA, no x-vector, no titanet, no wespeaker, nothing in /mnt/hdd1/llms but +// text ggufs. Until one is downloaded, newSpeakerEmbedder returns nil. +// +// Without an embedder the capability has no runnable half. This comment used to +// say enrolment was real and only recognition was blocked, and the startup log +// said the same. Both were wrong: Recognizer.Enroll embeds every sample before +// it stores anything, so with no model it fails on the first sample and nothing +// is ever stored, which leaves List empty forever and Forget with nothing to +// delete. So the gate is cfg.Speaker.Recognizes() — enabled AND a model path — +// and a box without one gets no speaker methods, not three no-ops. // // This is deliberately not papered over with a hand-rolled MFCC floor. A // biometric that is confidently wrong writes false claims about named people @@ -53,17 +56,25 @@ type speakerWiring struct { // starts working with no change to the store, the protocol, the auth table or // the handlers. See the plan document for what to download. func newSpeakerEmbedder(cfg *config.SpeakerConfig) speaker.Embedder { - if cfg == nil || cfg.ModelPath == "" { - return nil - } - log.Printf("speaker: model_path %q is configured but no embedding backend is built yet; "+ - "enrolment and deletion work, recognition does not (Vikunja #255)", cfg.ModelPath) + _ = cfg return nil } // newSpeakerWiring builds the recognizer, or nil when the capability is off. func newSpeakerWiring(st *store.Store, cfg *config.Config) *speakerWiring { - if cfg == nil || cfg.Speaker == nil || !cfg.Speaker.Enabled { + if cfg == nil || cfg.Speaker == nil { + return nil + } + if !cfg.Speaker.Recognizes() { + // Recognizes() was written as the gate and documented as one, and then + // never called. "enabled": true with no model_path used to wire all + // three methods and log "enrolment on", which is the one config shape + // where the operator most needs to be told otherwise. + if cfg.Speaker.Enabled { + log.Print("speaker: enabled but no model_path, so there is nothing to embed with; " + + "enrol, list and forget would all be no-ops, staying off " + + "(see docs/plans/10-speaker-recognition.md)") + } return nil } if st == nil { @@ -81,8 +92,8 @@ func newSpeakerWiring(st *store.Store, cfg *config.Config) *speakerWiring { if rec.Enabled() { log.Printf("speaker: recognition on, threshold %.2f", rec.Threshold()) } else { - log.Print("speaker: enrolment on, recognition BLOCKED — no speaker-embedding model " + - "on this box (see docs/plans/10-speaker-recognition.md)") + log.Printf("speaker: model_path %q is configured but no embedding backend is built yet, "+ + "so enrol, list and forget are all no-ops (Vikunja #255)", cfg.Speaker.ModelPath) } return &speakerWiring{rec: rec} } @@ -114,7 +125,7 @@ func (w *speakerWiring) forget(ctx context.Context, req ipc.ForgetSpeakerReq) er // toWireSpeaker drops the voiceprint. A listing says who is enrolled; it does // not hand the biometric back out over the socket. func toWireSpeaker(p speaker.Profile) ipc.Speaker { - return ipc.Speaker{ID: p.ID, Name: p.Name, Enrolled: p.Enrolled, Samples: p.Samples} + return ipc.Speaker{ID: p.ID, Name: p.Name, Enrolled: p.Enrolled, Samples: p.Samples, Damaged: p.Damaged} } // speakerErr maps the package sentinels onto the wire vocabulary so a surface @@ -123,6 +134,11 @@ func speakerErr(err error) error { switch { case err == nil: return nil + case errors.Is(err, speaker.ErrDisabled): + // Not a core failure. The capability is present on the wire but has no + // embedding model behind it, which is the same thing an unconfigured + // method says, so say it the same way. + return ipc.ErrUnknownMethod case errors.Is(err, speaker.ErrNotFound): return ipc.ErrNoFact case errors.Is(err, speaker.ErrBadID), diff --git a/cmd/mavend/speaker_wiring_test.go b/cmd/mavend/speaker_wiring_test.go new file mode 100644 index 0000000..96fd8bd --- /dev/null +++ b/cmd/mavend/speaker_wiring_test.go @@ -0,0 +1,50 @@ +package main + +import ( + "errors" + "testing" + + "github.com/kami/maven/internal/config" + "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/speaker" +) + +// "enabled": true with no model_path used to wire all three methods and log +// "enrolment on". Nothing behind them works without an embedder, so the +// capability stays off and the socket answers "no such method". +func TestSpeakerStaysOffWithoutAModelPath(t *testing.T) { + srv := &ipc.Server{} + cfg := &config.Config{Speaker: &config.SpeakerConfig{Enabled: true}} + + wireSpeaker(srv, nil, cfg) + + if srv.EnrollSpeakerFn != nil || srv.ListSpeakersFn != nil || srv.ForgetSpeakerFn != nil { + t.Error("speaker methods were wired with nothing to embed with") + } +} + +// The gate is Recognizes(), so a disabled block with a model path is off too. +func TestSpeakerStaysOffWhenDisabled(t *testing.T) { + srv := &ipc.Server{} + cfg := &config.Config{Speaker: &config.SpeakerConfig{ModelPath: "/nope/ecapa.onnx"}} + + wireSpeaker(srv, nil, cfg) + + if srv.EnrollSpeakerFn != nil { + t.Error("speaker methods were wired for a disabled block") + } +} + +// ErrDisabled is "this capability is off", not "core broke". It used to fall +// through speakerErr's default and reach the surface as an opaque failure. +func TestSpeakerErrMapsDisabledToUnknownMethod(t *testing.T) { + if got := speakerErr(speaker.ErrDisabled); !errors.Is(got, ipc.ErrUnknownMethod) { + t.Errorf("speakerErr(ErrDisabled) = %v, want ErrUnknownMethod", got) + } + if got := speakerErr(speaker.ErrNotFound); !errors.Is(got, ipc.ErrNoFact) { + t.Errorf("speakerErr(ErrNotFound) = %v, want ErrNoFact", got) + } + if got := speakerErr(nil); got != nil { + t.Errorf("speakerErr(nil) = %v", got) + } +} diff --git a/internal/ipc/api.go b/internal/ipc/api.go index d3faccb..13f4381 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -325,6 +325,12 @@ type Speaker struct { Name string `json:"name"` Enrolled time.Time `json:"enrolled"` Samples int `json:"samples"` + // Damaged — the stored row's metadata did not read back cleanly. The + // voiceprint is still there; the name, sample count or enrolment time is + // not trustworthy. A surface should say so rather than render a corrupt + // row as a profile enrolled from zero samples, which is what a real + // minimal enrolment looks like. + Damaged bool `json:"damaged,omitempty"` } // EnrollSpeakerResp — the profile that was written. diff --git a/internal/speaker/recognizer.go b/internal/speaker/recognizer.go index 73b3776..d881de5 100644 --- a/internal/speaker/recognizer.go +++ b/internal/speaker/recognizer.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "sort" + "strconv" "time" "github.com/kami/maven/internal/audio" @@ -140,14 +141,18 @@ func (r *Recognizer) Get(ctx context.Context, id string) (Profile, error) { // Forget deletes a profile. This is the one operation that must always work: // a voiceprint is data about a person, and "перестань узнавать её" has to // actually remove it, not mark it inactive. +// +// Forgetting a profile that is not there is not an error, matching +// memory.Catalog.Delete. It used to read the row first and answer ErrNotFound, +// which meant the layer documented as the one that must always work was the +// layer reintroducing a failure: a surface retrying a forget after a partial +// failure got an error on the second try, for a voiceprint that was already +// gone. The caller asked for it to be gone and it is gone. func (r *Recognizer) Forget(ctx context.Context, id string) error { id = NormalizeID(id) if !ValidID(id) { return fmt.Errorf("%w: %q", ErrBadID, id) } - if _, err := r.Get(ctx, id); err != nil { - return err - } if err := r.cat.Delete(ctx, Prefix+id); err != nil { return fmt.Errorf("speaker: forget %q: %w", id, err) } @@ -170,6 +175,12 @@ func (r *Recognizer) embed(ctx context.Context, a audio.Audio) ([]float32, error // profileFromRecord reads a stored row back into a Profile. A row with // unreadable metadata still yields a usable voiceprint — the vector is the part // that matters, and losing a name should not lose the enrolment. +// +// It also says when the metadata did not read cleanly. Without that, a row +// whose samples count is "12x" and whose name is missing came back as a +// plausible profile called by its own id with 0 samples, which is exactly what +// a real minimal enrolment looks like. Damaged is the difference between "he +// enrolled badly" and "this row is broken". func profileFromRecord(rec memory.Record) Profile { p := Profile{ ID: trimPrefix(rec.ID), @@ -178,15 +189,24 @@ func profileFromRecord(rec memory.Record) Profile { Name: rec.Meta["name"], } if s := rec.Meta["samples"]; s != "" { - p.Samples = atoi(s) + n, err := strconv.Atoi(s) + if err != nil || n < 0 { + p.Damaged = true + } else { + p.Samples = n + } } if ts := rec.Meta["enrolled"]; ts != "" { - if t, err := time.Parse(time.RFC3339, ts); err == nil { + t, err := time.Parse(time.RFC3339, ts) + if err != nil { + p.Damaged = true + } else { p.Enrolled = t } } if p.Name == "" { p.Name = p.ID + p.Damaged = true } return p } @@ -197,16 +217,3 @@ func trimPrefix(id string) string { } return id } - -// atoi is a tolerant small-integer parse: metadata that is not a number reads -// as 0 rather than failing the whole listing. -func atoi(s string) int { - n := 0 - for _, r := range s { - if r < '0' || r > '9' { - return 0 - } - n = n*10 + int(r-'0') - } - return n -} diff --git a/internal/speaker/speaker.go b/internal/speaker/speaker.go index 25f880b..2fc9b28 100644 --- a/internal/speaker/speaker.go +++ b/internal/speaker/speaker.go @@ -31,6 +31,14 @@ // There are also no enrolment samples. So Recognizer runs against Disabled and // every Identify answers ErrDisabled until a model lands. // +// "Recognition is blocked but enrolment works" is not true and this package +// used to imply it. Enroll embeds every sample before it stores anything +// (enroll.go, "Embed first, store second"), so with no model it fails on the +// first sample with ErrDisabled and nothing is ever stored. List then returns +// an empty list forever and Forget has nothing to delete. Without an embedder +// all three operations are no-ops, and mavend does not wire the wire methods +// at all in that state. +// // The MFCC + GMM "simplest floor" in the plan document is refused rather than // deferred. A hand-rolled spectral distance would identify people confidently // and wrongly, and its output would be written into facts as "Ками said this". @@ -46,11 +54,15 @@ import ( "time" "github.com/kami/maven/internal/audio" + "github.com/kami/maven/internal/memory" ) // Prefix — the id prefix speaker profiles carry in the shared vector table. -// It is what ByPrefix enumerates and what keeps voiceprints out of note recall. -const Prefix = "speaker:" +// It is what ByPrefix enumerates, and what every Store implementation filters +// out of Search so note recall cannot rank a voiceprint. The constant lives in +// internal/memory because the store layer has to know it and cannot import this +// package. +const Prefix = memory.NonRecallPrefix // DefaultThreshold — cosine similarity a match must beat to be a match. // @@ -133,6 +145,12 @@ type Profile struct { Samples int `json:"samples"` Dim int `json:"dim"` + // Damaged marks a row whose stored metadata did not read back cleanly — + // an unparsable sample count or timestamp, or a missing name. The + // voiceprint is still usable, but the row should be listed as damaged + // rather than as a plausible profile enrolled from nothing. + Damaged bool `json:"damaged,omitempty"` + // Vec is the voiceprint. Not serialised to any surface: a listing tells him // who is enrolled, it does not hand out the biometric itself. Vec []float32 `json:"-"` diff --git a/internal/speaker/speaker_test.go b/internal/speaker/speaker_test.go index 6c9ed77..5af12b1 100644 --- a/internal/speaker/speaker_test.go +++ b/internal/speaker/speaker_test.go @@ -269,8 +269,58 @@ func TestForgetRemovesTheVoiceprint(t *testing.T) { if _, err := r.Get(ctx, "guest"); !errors.Is(err, ErrNotFound) { t.Errorf("Get after Forget = %v, want ErrNotFound", err) } - if err := r.Forget(ctx, "guest"); !errors.Is(err, ErrNotFound) { - t.Errorf("second Forget = %v, want ErrNotFound", err) + // Forgetting twice is not an error. A surface retrying after a partial + // failure must not be told the voice it asked to remove is missing. + if err := r.Forget(ctx, "guest"); err != nil { + t.Errorf("second Forget = %v, want nil", err) + } +} + +// A row whose metadata is corrupt lists as damaged, not as a plausible profile +// enrolled from zero samples. Those two used to look identical. +func TestCorruptMetadataListsAsDamaged(t *testing.T) { + r, cat := newRec(t, &fakeEmbedder{vec: []float32{1, 0}}) + ctx := context.Background() + if err := cat.Insert(ctx, Prefix+"kami", []float32{1, 0}, map[string]string{ + "kind": "speaker", + "samples": "12x", + "enrolled": "yesterday", + }); err != nil { + t.Fatal(err) + } + ps, err := r.List(ctx) + if err != nil { + t.Fatal(err) + } + if len(ps) != 1 { + t.Fatalf("List returned %d profiles, want 1", len(ps)) + } + p := ps[0] + if !p.Damaged { + t.Errorf("profile %+v is not marked damaged", p) + } + if p.Samples != 0 || !p.Enrolled.IsZero() { + t.Errorf("unreadable metadata was parsed anyway: samples %d, enrolled %v", p.Samples, p.Enrolled) + } + if len(p.Vec) != 2 { + t.Errorf("the voiceprint was dropped: %v", p.Vec) + } +} + +// A clean row is not damaged. Without this the flag could be set always and +// the test above would still pass. +func TestCleanProfileIsNotDamaged(t *testing.T) { + r, _ := newRec(t, &fakeEmbedder{vec: []float32{1, 0}}) + ctx := context.Background() + if _, err := r.Enroll(ctx, "kami", "Ками", enrolSamples(3, 4)); err != nil { + t.Fatal(err) + } + ps, err := r.List(ctx) + if err != nil { + t.Fatal(err) + } + if len(ps) != 1 || ps[0].Damaged { + t.Fatalf("List = %+v, want one undamaged profile", ps) } } From d68708b5e1d4ccb5f4da23ca4f65fc513f45cbb6 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:16:02 +0400 Subject: [PATCH 4/6] stt: make the golden tests fail where they used to disappear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file comment named four regressions caught here. Three were not. Nothing on this path resamples, because PCMFromWAV refuses anything that is not already 16 kHz mono s16. Nothing exercises language selection, because the hint comes out of the manifest already correct. And a bad model path was the one condition that made the whole test vanish behind a skip nobody reads. The comment now claims the two things that are real, an explicitly set MAVEN_WHISPER_MODEL that does not exist is a failure, and a missing fixture is a failure rather than a skip. looseWordMatch accepted a different word. Four retained runes of "воды" is "вод", so whisper hearing "выпил водки" satisfied the ru_fact keyword, and "dis" let display, distance and discuss all stand in for "disk". A case ending adds a rune, not a syllable, so the hypothesis is capped in length as well as matched on prefix. The spoken text lived in the generator and in the manifest with nothing tying them together. Editing one left the other describing audio that no longer existed, and at a flat ceiling of 0.34 over a five-word reference a one-word drift passed silently. The script reads text out of the manifest now, and the ceilings are set just above what each case really measures against ggml-small, with the measurement recorded beside them. Also: the test carried its own copy of the PCM to float32 conversion, so a regression in the daemon's copy left the silence-gate assertion green, and the manifest was validated for keywords but not for text, where an empty reference makes every hypothesis score a WER of 1. Found in review of #75. --- Makefile | 9 ++- cmd/mavsttd/golden_test.go | 104 +++++++++++++++++++++++----- cmd/mavsttd/testdata/golden_v1.json | 15 ++-- cmd/mavsttd/whisper_handler.go | 22 ++++-- scripts/gen-stt-fixtures.sh | 41 +++++++++-- 5 files changed, 152 insertions(+), 39 deletions(-) diff --git a/Makefile b/Makefile index 1486a70..c3e8d7b 100644 --- a/Makefile +++ b/Makefile @@ -149,8 +149,13 @@ eval-models: # stt-fixtures — regenerate the golden STT audio in cmd/mavsttd/testdata from # the piper voices (#288). The committed WAVs are synthesised, never recorded, -# so this is the only way they should ever change. TestGoldenAudioTranscription -# then scores them against ggml-small; it self-skips when the model is absent. +# so this is the only way they should ever change. The spoken text is read out +# of testdata/golden_v1.json, so edit the transcript there and rerun this. +# +# test-stt-golden runs both golden tests: TestGoldenAudioTranscription, which +# scores the fixtures against ggml-small and self-skips when the model is +# absent, and TestGoldenFixturesAreCanonical, which checks the committed audio +# and the manifest with no model at all. stt-fixtures: ./scripts/gen-stt-fixtures.sh diff --git a/cmd/mavsttd/golden_test.go b/cmd/mavsttd/golden_test.go index 0783a83..a547e11 100644 --- a/cmd/mavsttd/golden_test.go +++ b/cmd/mavsttd/golden_test.go @@ -2,10 +2,22 @@ package main // Golden-audio STT tests (Vikunja #288). // -// These push real audio through the real whisper.cpp binding, so a bad model -// path, a wrong language hint, a broken resample or a regressed silence gate -// is caught by `make test` rather than by the owner talking to a daemon that -// mishears him. +// These push real audio through the real whisper.cpp binding. What that +// covers, precisely, is two things: the model still transcribes known speech +// well enough for the router to act on it, and the silence gate still lets real +// speech through. A regression in either shows up in `make test` rather than in +// the owner talking to a daemon that mishears him. +// +// It is worth being exact about what is NOT covered, because this comment used +// to claim more. Nothing here resamples: audio.PCMFromWAV refuses anything that +// is not 16 kHz mono s16, the fixtures arrive at 16 kHz from ffmpeg, and there +// is no conversion step between the WAV and whisper_full. Nothing here +// exercises language selection either: the hint comes out of the manifest +// already correct and goes straight into the request, so how mavsttd chooses a +// language is untested. And a wrong model path is not caught when it is the +// default one, because a box without the model skips; an explicitly set +// MAVEN_WHISPER_MODEL that does not exist is a failure, since that is a +// mistake and not an absence. // // The fixtures are piper-synthesised, not recorded — see // scripts/gen-stt-fixtures.sh. Nothing of the owner's voice is committed, and @@ -47,7 +59,10 @@ type goldenCase struct { Lang string `json:"lang"` Text string `json:"text"` Keywords []string `json:"keywords"` - MaxWER float64 `json:"max_wer"` + // MeasuredWER is what this case scored when the ceiling was last set, so + // a model swap is a diff to a recorded number rather than silence. + MeasuredWER float64 `json:"measured_wer"` + MaxWER float64 `json:"max_wer"` } type goldenManifest struct { @@ -152,6 +167,14 @@ func containsSeq(hyp, want []string) bool { return false } +// looseWordMatch reports whether got is want, or an inflection of it. +// +// A shared prefix alone is not enough. "воды" retains three runes, so "водка" +// used to satisfy the ru_fact keyword and the test passed on whisper hearing +// "выпил водки". "disk" retains "dis", which "display", "distance" and +// "discuss" all match. So the hypothesis is also capped in length: a case +// ending adds a rune or two, it does not add a syllable. Short words get no +// slack at all, because there is nothing left of them after a prefix cut. func looseWordMatch(got, want string) bool { if got == want { return true @@ -166,6 +189,13 @@ func looseWordMatch(got, want string) bool { if n < 3 || len(g) < n { return false } + extra := 2 + if len(w) <= 4 { + extra = 0 + } + if len(g) > len(w)+extra { + return false + } return string(g[:n]) == string(w[:n]) } @@ -176,6 +206,11 @@ func TestGoldenAudioTranscription(t *testing.T) { model := goldenModelPath() if _, err := os.Stat(model); err != nil { + // An explicit override that points at nothing is a mistake, not a box + // without the model. Skipping there made a typo look like a pass. + if os.Getenv("MAVEN_WHISPER_MODEL") != "" { + t.Fatalf("MAVEN_WHISPER_MODEL=%s does not exist: %v", model, err) + } t.Skipf("whisper model %s absent (%v) — set MAVEN_WHISPER_MODEL or see AGENTS.md", model, err) } @@ -192,7 +227,9 @@ func TestGoldenAudioTranscription(t *testing.T) { path := filepath.Join("testdata", c.WAV) raw, err := os.ReadFile(path) if err != nil { - t.Skipf("fixture %s absent (%v) — run scripts/gen-stt-fixtures.sh", path, err) + // Not a skip. A fixture the generator failed to write is a + // broken checkout, and skipping made `make test` green on one. + t.Fatalf("fixture %s absent (%v) — run scripts/gen-stt-fixtures.sh", path, err) } format, pcm, err := audio.PCMFromWAV(raw) if err != nil { @@ -221,9 +258,12 @@ func TestGoldenAudioTranscription(t *testing.T) { if missing := missingKeywords(c.Keywords, hyp); len(missing) > 0 { t.Errorf("%s: missing keywords %v in %q", c.WAV, missing, resp.Text) } - if wer := wordErrorRate(ref, hyp); wer > c.MaxWER { - t.Errorf("%s: WER %.2f > %.2f\n want: %q\n got: %q", c.WAV, wer, c.MaxWER, c.Text, resp.Text) + wer := wordErrorRate(ref, hyp) + if wer > c.MaxWER { + t.Errorf("%s: WER %.2f > %.2f (measured %.2f when the ceiling was set)\n want: %q\n got: %q", + c.WAV, wer, c.MaxWER, c.MeasuredWER, c.Text, resp.Text) } + t.Logf("%s: WER %.2f (ceiling %.2f, was %.2f)", c.WAV, wer, c.MaxWER, c.MeasuredWER) }) } } @@ -253,27 +293,29 @@ func TestGoldenFixturesAreCanonical(t *testing.T) { } // The fixture must clear mavsttd's own silence gate, otherwise the // model test below would be asserting on a gated empty string. - if reason := gateReason(pcmToF32(pcm), whisperSampleRate, 300, 0.01); reason != "" { + if reason := gateReason(pcmSamples(pcm), whisperSampleRate, 300, 0.01); reason != "" { t.Errorf("%s: would be gated as %s", path, reason) } if len(c.Keywords) == 0 { t.Errorf("%s: manifest case has no keywords", c.Name) } + // An empty reference makes wordErrorRate return 1 for every + // hypothesis, so the WER assertion fires with nothing useful to say. + if len(normalizeTranscript(c.Text)) == 0 { + t.Errorf("%s: manifest case has no reference text", c.Name) + } + if c.Lang != "ru" && c.Lang != "en" { + t.Errorf("%s: lang %q is not one of the two languages mavsttd is run with", c.Name, c.Lang) + } if c.MaxWER <= 0 || c.MaxWER > 1 { t.Errorf("%s: max_wer %v outside (0,1]", c.Name, c.MaxWER) } + if c.MeasuredWER > c.MaxWER { + t.Errorf("%s: measured_wer %v is above max_wer %v, so the ceiling was never met", c.Name, c.MeasuredWER, c.MaxWER) + } } } -func pcmToF32(b []byte) []float32 { - out := make([]float32, len(b)/2) - for i := range out { - s := int16(b[i*2]) | int16(b[i*2+1])<<8 - out[i] = float32(s) / 32768.0 - } - return out -} - // --- matcher unit tests (no model, no fixtures) ---------------------------- func TestNormalizeTranscript(t *testing.T) { @@ -320,4 +362,30 @@ func TestMissingKeywords(t *testing.T) { if got := missingKeywords([]string{"часть"}, hyp2); len(got) != 1 { t.Fatalf("missingKeywords = %v, want %q reported missing", got, "часть") } + // A prefix is not a word. These are different words that share one, and + // each of them used to satisfy the keyword it is paired with. + different := [][2]string{ + {"воды", "Я выпил водки."}, + {"disk", "check the display"}, + {"disk", "we should discuss it"}, + {"server", "a serverless function"}, + } + for _, d := range different { + if got := missingKeywords([]string{d[0]}, normalizeTranscript(d[1])); len(got) != 1 { + t.Errorf("keyword %q was satisfied by %q", d[0], d[1]) + } + } + // And the inflections still pass, which is the whole point of the loose + // match. + same := [][2]string{ + {"воды", "выпил воду"}, + {"напомни", "напомните мне"}, + {"календарю", "по календаре"}, + {"restart", "restarted the server"}, + } + for _, d := range same { + if got := missingKeywords([]string{d[0]}, normalizeTranscript(d[1])); len(got) != 0 { + t.Errorf("keyword %q was not matched by %q", d[0], d[1]) + } + } } diff --git a/cmd/mavsttd/testdata/golden_v1.json b/cmd/mavsttd/testdata/golden_v1.json index 870a4d5..5e7b4b9 100644 --- a/cmd/mavsttd/testdata/golden_v1.json +++ b/cmd/mavsttd/testdata/golden_v1.json @@ -1,5 +1,6 @@ { - "note": "Golden STT fixtures. Audio is piper-synthesised, not recorded — see scripts/gen-stt-fixtures.sh. Regenerate with that script; do not hand-edit `wav`.", + "note": "Golden STT fixtures. Audio is piper-synthesised, not recorded — see scripts/gen-stt-fixtures.sh, which reads `text` from this file and synthesises from it. This is the only source of the spoken words; regenerate with that script and do not hand-edit `wav`.", + "wer_note": "max_wer is set just above what each case actually measures against ggml-small, recorded in `measured_wer` on 2026-08-01. A flat 0.34 over a five-word reference tolerated two wrong words and left most of the range unguarded. A model swap should show up as a diff to these numbers, not as silence: rerun `make test-stt-golden`, read the logged transcript, and move both fields together.", "cases": [ { "name": "ru_reminder", @@ -7,7 +8,8 @@ "lang": "ru", "text": "напомни мне через час позвонить маме", "keywords": ["напомни", "час", "позвонить"], - "max_wer": 0.34 + "measured_wer": 0.0, + "max_wer": 0.1 }, { "name": "ru_fact", @@ -15,7 +17,8 @@ "lang": "ru", "text": "отметь что я выпил воды", "keywords": ["отметь", "воды"], - "max_wer": 0.34 + "measured_wer": 0.2, + "max_wer": 0.25 }, { "name": "ru_query", @@ -23,7 +26,8 @@ "lang": "ru", "text": "что у меня сегодня по календарю", "keywords": ["сегодня", "календарю"], - "max_wer": 0.34 + "measured_wer": 0.0, + "max_wer": 0.1 }, { "name": "en_act", @@ -31,7 +35,8 @@ "lang": "en", "text": "restart the web server and check the disk space", "keywords": ["restart", "server", "disk"], - "max_wer": 0.34 + "measured_wer": 0.0, + "max_wer": 0.1 } ] } diff --git a/cmd/mavsttd/whisper_handler.go b/cmd/mavsttd/whisper_handler.go index b8cdf93..bc58cbd 100644 --- a/cmd/mavsttd/whisper_handler.go +++ b/cmd/mavsttd/whisper_handler.go @@ -66,6 +66,19 @@ func gateReason(samples []float32, rate, minMs int, minRMS float64) string { return "" } +// pcmSamples converts canonical s16le little-endian PCM to the float32 range +// whisper wants. Shared with the golden tests: they used to carry their own +// copy, so a regression here (a /32767 divisor, a byte order slip) left the +// assertion that the fixtures clear the silence gate green. +func pcmSamples(b []byte) []float32 { + out := make([]float32, len(b)/2) + for i := range out { + s := int16(b[i*2]) | int16(b[i*2+1])<<8 + out[i] = float32(s) / 32768.0 + } + return out +} + func (h *whisperHandler) Transcribe(ctx context.Context, req worker.TranscribeReq) (worker.TranscribeResp, error) { if err := ctx.Err(); err != nil { return worker.TranscribeResp{}, fmt.Errorf("whisper: context done before transcribe: %w", err) @@ -75,12 +88,7 @@ func (h *whisperHandler) Transcribe(ctx context.Context, req worker.TranscribeRe return worker.TranscribeResp{}, fmt.Errorf("whisper: empty audio") } - nSamples := len(a.Bytes) / 2 - samples := make([]float32, nSamples) - for i := 0; i < nSamples; i++ { - s := int16(a.Bytes[i*2]) | int16(a.Bytes[i*2+1])<<8 - samples[i] = float32(s) / 32768.0 - } + samples := pcmSamples(a.Bytes) // Silence gate: drop non-speech before whisper hallucinates on it. if reason := gateReason(samples, whisperSampleRate, h.minMs, h.minRMS); reason != "" { @@ -111,7 +119,7 @@ func (h *whisperHandler) Transcribe(ctx context.Context, req worker.TranscribeRe ch := make(chan result, 1) cSamples := (*C.float)(unsafe.Pointer(&samples[0])) go func() { - ch <- result{code: int(C.whisper_full(h.ctx, params, cSamples, C.int(nSamples)))} + ch <- result{code: int(C.whisper_full(h.ctx, params, cSamples, C.int(len(samples))))} }() select { case r := <-ch: diff --git a/scripts/gen-stt-fixtures.sh b/scripts/gen-stt-fixtures.sh index 98d6dae..5f430dc 100755 --- a/scripts/gen-stt-fixtures.sh +++ b/scripts/gen-stt-fixtures.sh @@ -10,6 +10,13 @@ # Usage: # scripts/gen-stt-fixtures.sh # +# The spoken text is NOT written here. It is read out of +# cmd/mavsttd/testdata/golden_v1.json, which is the same file the test scores +# against. It used to live in both places, so editing this script and running +# make stt-fixtures left the manifest describing audio that no longer existed — +# and at a WER ceiling of 0.34 over a five-word reference, a one-word drift +# passed silently. Punctuation does not matter: normalizeTranscript strips it. +# # Voices are picked up from, in order, $PIPER_VOICE_RU / $PIPER_VOICE_EN, then # the repo's models/tts, then ~/esp-server/voices. The English voice is not # vendored; if it is missing the English fixture is skipped and the existing @@ -34,6 +41,26 @@ ru="$(pick_voice "${PIPER_VOICE_RU:-}" "$root/models/tts/ru_RU-irina-medium.onnx } en="$(pick_voice "${PIPER_VOICE_EN:-}" "$root/models/tts/en_US-lessac-medium.onnx" "$HOME/esp-server/voices/en_US-lessac-medium.onnx")" || en="" +manifest="$root/cmd/mavsttd/testdata/golden_v1.json" +command -v jq >/dev/null || { echo "jq is required to read $manifest" >&2; exit 1; } +[ -f "$manifest" ] || { echo "missing $manifest" >&2; exit 1; } + +# case_text — the reference transcript for one manifest case. +case_text() { + local name="$1" text + text="$(jq -r --arg n "$name" '.cases[] | select(.name==$n) | .text' "$manifest")" + [ -n "$text" ] && [ "$text" != "null" ] || { echo "no case named $name in $manifest" >&2; exit 1; } + printf '%s' "$text" +} + +# case_wav — the file name the manifest expects for one case. +case_wav() { + local name="$1" wav + wav="$(jq -r --arg n "$name" '.cases[] | select(.name==$n) | .wav' "$manifest")" + [ -n "$wav" ] && [ "$wav" != "null" ] || { echo "no case named $name in $manifest" >&2; exit 1; } + printf '%s' "$wav" +} + # synth # piper emits raw 22050 Hz s16le on stdout; ffmpeg resamples to the canonical # 16 kHz mono and writes a plain 44-byte-header WAV (-fflags bitexact keeps @@ -51,15 +78,15 @@ synth() { echo "wrote $dest ($(stat -c%s "$dest") bytes)" } -synth "$ru" "$out/ru_reminder.wav" "Напомни мне через час позвонить маме." -synth "$ru" "$out/ru_fact.wav" "Отметь, что я выпил воды." -synth "$ru" "$out/ru_query.wav" "Что у меня сегодня по календарю?" +for name in ru_reminder ru_fact ru_query; do + synth "$ru" "$out/$(case_wav "$name")" "$(case_text "$name")" +done if [ -n "$en" ]; then - # Keep the English line free of words piper spells out letter by letter — - # "nginx" comes out of lessac as "engine X", which is a TTS artefact and - # would make the fixture assert on the wrong thing. - synth "$en" "$out/en_act.wav" "Restart the web server and check the disk space." + # Keep the English line in the manifest free of words piper spells out + # letter by letter — "nginx" comes out of lessac as "engine X", which is a + # TTS artefact and would make the fixture assert on the wrong thing. + synth "$en" "$out/$(case_wav en_act)" "$(case_text en_act)" else echo "no english piper voice found — skipping en_act.wav" >&2 fi From d3fcc1dfdb945fbbb8f844649cf5b58ac73b3a41 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:19:35 +0400 Subject: [PATCH 5/6] mavwaked: throw away the round-trip backlog before it becomes a turn Nothing reads the microphone while Send is in flight, so the audio piles up in arecord's pipe and arrives in a burst the moment dispatch returns. A round-trip is p50 2.7s through the LLM router, which is about 90 frames of room, of him finishing his sentence, of the television. The old code reset the VAD on the reply path only, and for a reason that was not true: the comment said the VAD had been accumulating during the round-trip, when its state is exactly what Feed left it as. The two paths with no reset are the ones that mattered, because neither starts playback and so neither is covered by the half-duplex gate. A text-only turn fed the whole backlog into the VAD, and a Send error did the same on every failed turn, so a dead socket drove a retry loop off backlog alone. The backlog was scored for barge-in too. Five frames delivered in microseconds cut her off with audio recorded before she started speaking, which is the opposite of what the five-frame guard is for. Both are fixed by the same mechanism: measure the wall time the round-trip took, convert it to frames, and discard that many before anything looks at them. Barge-in also threw away the 150ms that proved he was talking. The VAD started from the next frame, so the first word of a short interruption was clipped before whisper saw it. Those frames are kept in a small ring and replayed after the reset. A stuck aplay was worse than before this feature existed. Playing() gates all capture, so a wedged child made her deaf rather than silent, for the full 30s ceiling inherited from the fire-and-forget version. The mute window is bounded by the reply's own duration plus a margin now. Three smaller ones. "-barge-in -barge-in-rms 0" logged "barge-in on" and then did nothing. The sent counter incremented before the error check, so failed round-trips counted as shipped. And the threshold the operator has to guess is now reported: mavwaked logs the mean energy of the frames it suppressed while speaking, so he can set it from data. Found in review of #76. --- cmd/mavwaked/main.go | 19 ++++- cmd/mavwaked/playback.go | 17 +++- cmd/mavwaked/session.go | 126 ++++++++++++++++++++++++++++-- cmd/mavwaked/session_test.go | 145 +++++++++++++++++++++++++++++++++++ 4 files changed, 293 insertions(+), 14 deletions(-) diff --git a/cmd/mavwaked/main.go b/cmd/mavwaked/main.go index 60968f2..65e4a55 100644 --- a/cmd/mavwaked/main.go +++ b/cmd/mavwaked/main.go @@ -14,9 +14,12 @@ // // While a reply is playing the capture side is muted (half-duplex): without // it, Maven's own voice comes back in through the mic and she answers -// herself. -barge-in punches one hole in that gate — sustained energy well -// above the speaker's leak level cuts playback so he can talk over her. It is -// off by default because the threshold is room-specific; see playback.go. +// herself. -barge-in punches one hole in that gate — sustained energy above +// -barge-in-rms cuts playback so he can talk over her. It is off by default +// because the threshold is room-specific; see playback.go. The threshold is a +// raw frame RMS and has no reference to what the speaker actually leaks, so +// the daemon logs the mean energy of the frames it suppressed while speaking. +// Set -barge-in-rms from those numbers rather than by guessing. // // usage: // mavwaked # default ALSA device, 127.0.0.1:9100 @@ -130,7 +133,15 @@ func run(args []string) error { var barge bargeInConfig if *bargeIn { barge = bargeInConfig{RMS: float64(*bargeRMS) / 10000.0, Frames: *bargeFrames} - log.Printf("mavwaked: barge-in on (rms %.4f x %d frames)", barge.RMS, barge.Frames) + if barge.Enabled() { + log.Printf("mavwaked: barge-in on (rms %.4f x %d frames)", barge.RMS, barge.Frames) + } else { + // The log used to say "barge-in on (rms 0.0000 x 5)" here and then + // nothing happened, because Enabled needs a positive threshold. + log.Printf("mavwaked: -barge-in was passed but rms %.4f x %d frames disables it; "+ + "both must be above zero, so barge-in is OFF", + barge.RMS, barge.Frames) + } } sess := newSession(vad, newAplayPlayer(), &voiceSender{vc: vc}, *lang, barge) diff --git a/cmd/mavwaked/playback.go b/cmd/mavwaked/playback.go index 7bc5e6d..01fd69d 100644 --- a/cmd/mavwaked/playback.go +++ b/cmd/mavwaked/playback.go @@ -33,6 +33,11 @@ import ( "github.com/kami/maven/internal/audio" ) +// playbackMargin is the slack over the reply's own duration before a stuck +// aplay is killed. Enough for ALSA to open the device and drain its buffer, +// short enough that a busy device does not cost her a turn. +const playbackMargin = 2 * time.Second + // player plays one reply at a time and can be cut off mid-utterance. type player interface { // Play starts playback of a, replacing anything already playing, and @@ -87,6 +92,13 @@ func (p *aplayPlayer) Play(a audio.Audio) { p.playing = true p.mu.Unlock() + // Bound the mute window by the reply itself. Playing() gates all capture + // now, so a wedged aplay does not merely go silent, it makes her deaf for + // as long as the flag is set. The old ceiling was a flat 30s inherited + // from the fire-and-forget version, where it only bounded a leaked + // goroutine. A reply cannot legitimately take longer than it lasts. + limit := time.Duration(a.Duration()*float64(time.Second)) + playbackMargin + go func() { if _, err := stdin.Write(a.Bytes); err != nil { // Broken pipe is the expected outcome of Stop(). @@ -101,8 +113,9 @@ func (p *aplayPlayer) Play(a audio.Audio) { if err != nil { log.Printf("mavwaked: aplay: %v", err) } - case <-time.After(30 * time.Second): - log.Printf("mavwaked: aplay timeout, killing") + case <-time.After(limit): + log.Printf("mavwaked: aplay did not finish %.1fs of audio within %s, killing (capture was muted the whole time)", + a.Duration(), limit) if pr := cmd.Process; pr != nil { _ = pr.Kill() } diff --git a/cmd/mavwaked/session.go b/cmd/mavwaked/session.go index 21e7a68..24de887 100644 --- a/cmd/mavwaked/session.go +++ b/cmd/mavwaked/session.go @@ -7,6 +7,7 @@ package main import ( "context" "log" + "time" "github.com/kami/maven/internal/audio" ) @@ -42,6 +43,20 @@ type session struct { lang string barge bargeInConfig + // now is the clock, swapped in tests. The round-trip backlog is measured + // in wall time, because that is the only thing that says how much room + // went into the pipe while the daemon was thinking. + now func() time.Time + + // discard is how many buffered frames still have to be thrown away + // before capture means anything again. See dispatch. + discard int + + // recent holds the last few frames seen during playback, so the ones + // that proved he was interrupting can be replayed into the VAD after the + // barge-in reset instead of being clipped off the front of his sentence. + recent [][]byte + // loudFrames counts consecutive over-threshold frames seen while she is // speaking. Reset whenever a frame falls back under the threshold, and // whenever playback ends. @@ -49,14 +64,28 @@ type session struct { // counters, read by tests and logged on the way out. suppressed int // frames dropped because she was speaking + dropped int // frames dropped as round-trip backlog bargeIns int // times playback was cut because he spoke over her sent int // utterances shipped to the daemon + + // loudSum and loudSeen accumulate the energy of suppressed frames, so + // the operator can read what the room actually measures and set + // -barge-in-rms from data instead of guessing. + loudSum float64 + loudSeen int } func newSession(vad *VAD, p player, s utteranceSender, lang string, barge bargeInConfig) *session { - return &session{vad: vad, player: p, sender: s, lang: lang, barge: barge} + return &session{vad: vad, player: p, sender: s, lang: lang, barge: barge, now: time.Now} } +// frameDuration is the wall time one captured frame represents. +const frameDuration = defaultFrameMs * time.Millisecond + +// suppressLogEvery — how many suppressed frames between energy reports. 200 +// frames is six seconds of her talking, so this is roughly one line per reply. +const suppressLogEvery = 200 + // feed processes one 30ms PCM frame. // // While the player is running the capture side is muted: the VAD is not fed @@ -65,26 +94,52 @@ func newSession(vad *VAD, p player, s utteranceSender, lang string, barge bargeI // energy well above the speaker's leak level cuts playback, and capture // resumes on the very next frame with a clean VAD. func (s *session) feed(ctx context.Context, frame []byte) error { + // Backlog first, before anything looks at this frame. These are frames + // the microphone captured while the round-trip blocked; they arrive in a + // burst at pipe speed and they are not a command, not an answer and not + // an interruption. + if s.discard > 0 { + s.discard-- + s.dropped++ + return nil + } + if s.player.Playing() { s.suppressed++ + rms := frameRMS(PCMToI16(frame)) + s.loudSum += rms + s.loudSeen++ + if s.loudSeen >= suppressLogEvery { + // The doc comment asks for energy "well above the speaker's leak + // level" and never says what that is. This is what it is. + log.Printf("mavwaked: suppressed %d frames while speaking, mean rms %.4f (barge-in threshold %.4f)", + s.loudSeen, s.loudSum/float64(s.loudSeen), s.barge.RMS) + s.loudSum, s.loudSeen = 0, 0 + } if !s.barge.Enabled() { return nil } - if frameRMS(PCMToI16(frame)) < s.barge.RMS { + if rms < s.barge.RMS { s.loudFrames = 0 + s.recent = s.recent[:0] return nil } s.loudFrames++ + s.keepRecent(frame) if s.loudFrames < s.barge.Frames { return nil } // He is talking over her. Cut her off, drop the VAD state that - // accumulated from the echo, and start listening for real. + // accumulated from the echo, and start listening for real — starting + // with the frames that proved he was talking. Those used to be + // thrown away, which clipped the first 150ms off his interruption, + // and on a short one that is the whole first word. s.player.Stop() s.bargeIns++ s.loudFrames = 0 s.vad.Reset() log.Printf("mavwaked: barge-in — stopped playback") + s.replayRecent() return nil } @@ -102,22 +157,77 @@ func (s *session) feed(ctx context.Context, frame []byte) error { return s.dispatch(ctx, utt) } +// keepRecent stores a copy of one barge-in trigger frame, keeping at most +// barge.Frames of them. +func (s *session) keepRecent(frame []byte) { + if len(s.recent) >= s.barge.Frames { + copy(s.recent, s.recent[1:]) + s.recent = s.recent[:len(s.recent)-1] + } + s.recent = append(s.recent, append([]byte(nil), frame...)) +} + +// replayRecent feeds the trigger frames back into the freshly reset VAD, so +// his interruption starts where he started it. +// +// Feed cannot complete an utterance here: closing one needs silenceMs of +// trailing quiet and these frames are all above the barge-in threshold, which +// is far above the VAD floor. Any utterance it did return would be a fragment +// of a sentence he is still speaking, so it is not dispatched. +func (s *session) replayRecent() { + for _, f := range s.recent { + s.vad.Feed(PCMToI16(f)) + } + s.recent = s.recent[:0] +} + // dispatch ships a complete utterance and plays whatever comes back. +// +// Every return path here has to deal with the backlog. Nothing reads the +// microphone while Send is in flight, so the audio piles up in arecord's pipe +// and the kernel buffer, and it arrives in a burst the moment this returns. A +// round-trip is p50 2.7s through the LLM router, which is around 90 frames of +// room, of him finishing his sentence, of the television. +// +// This used to reset the VAD on the reply path only, and for the wrong reason: +// the comment said the VAD had been accumulating during the round-trip, when +// in fact its state is exactly what Feed left it as. The two paths that had no +// reset are the ones that mattered, because neither of them starts playback +// and so neither is covered by the half-duplex gate. A text-only turn fed the +// whole backlog straight into the VAD, and a Send error did the same on every +// failed turn, so a dead socket drove a retry loop off nothing but backlog. func (s *session) dispatch(ctx context.Context, utt audio.Audio) error { log.Printf("mavwaked: utterance complete (%.2fs, %d bytes), sending...", utt.Duration(), len(utt.Bytes)) + start := s.now() reply, err := s.sender.Send(ctx, utt, s.lang) - s.sent++ + defer s.dropBacklog(start) if err != nil { return err } + // Count what was shipped, not what was attempted. This used to run + // before the error check, so failed round-trips counted as sent. + s.sent++ if len(reply.Bytes) == 0 { log.Printf("mavwaked: empty reply audio (text only)") return nil } - // The VAD has been accumulating from the buffered mic stream while the - // round-trip blocked. None of it is a command — reset before the - // speaker opens, so the first post-reply frame starts clean. - s.vad.Reset() s.player.Play(reply) return nil } + +// dropBacklog resets the VAD and arranges for the frames captured during the +// round-trip to be thrown away as they arrive. +// +// Discarding them is also what keeps barge-in honest. The Frames guard is +// documented as "long enough that a door or a cough does not cut her off", +// which assumes the frames are real time. Draining a backlog delivers five +// frames in microseconds, so without this she could be cut off by audio +// recorded before she started speaking. +func (s *session) dropBacklog(start time.Time) { + s.vad.Reset() + s.loudFrames = 0 + s.recent = s.recent[:0] + if elapsed := s.now().Sub(start); elapsed > 0 { + s.discard = int(elapsed / frameDuration) + } +} diff --git a/cmd/mavwaked/session_test.go b/cmd/mavwaked/session_test.go index 8fa94d6..da6940b 100644 --- a/cmd/mavwaked/session_test.go +++ b/cmd/mavwaked/session_test.go @@ -5,6 +5,7 @@ import ( "errors" "math" "testing" + "time" "github.com/kami/maven/internal/audio" ) @@ -280,3 +281,147 @@ func TestBargeInConfigEnabled(t *testing.T) { } } } + +// slowSender models the real thing: a round-trip takes wall-clock time, and +// the microphone keeps recording into a pipe nobody is reading. +type slowSender struct { + fakeSender + clock *time.Time + took time.Duration +} + +func (s *slowSender) Send(ctx context.Context, utt audio.Audio, lang string) (audio.Audio, error) { + *s.clock = s.clock.Add(s.took) + return s.fakeSender.Send(ctx, utt, lang) +} + +// newSlowSession wires a session whose round-trip takes took of wall time. +func newSlowSession(barge bargeInConfig, reply audio.Audio, err error, took time.Duration) (*session, *fakePlayer, *slowSender) { + now := time.Unix(0, 0) + p := &fakePlayer{} + snd := &slowSender{fakeSender: fakeSender{reply: reply, err: err}, clock: &now, took: took} + sess := newSession(NewVAD(0, 0, 0, 0), p, snd, "ru", barge) + sess.now = func() time.Time { return now } + return sess, p, snd +} + +// A text-only turn starts no playback, so the half-duplex gate does not cover +// it. The backlog captured during the round-trip has to be dropped anyway, or +// three seconds of room arrives at pipe speed and becomes a command. +func TestSessionDropsBacklogAfterAnEmptyReply(t *testing.T) { + sess, p, snd := newSlowSession(bargeInConfig{}, audio.Audio{Format: audio.PCM16kMono}, nil, 3*time.Second) + + speakThenPause(t, sess) + if p.plays != 0 || len(snd.sent) != 1 { + t.Fatalf("plays = %d, sent = %d; want one text-only turn", p.plays, len(snd.sent)) + } + // The tail of speakThenPause already spent a couple of them. + if want := int(3 * time.Second / frameDuration); sess.discard+sess.dropped != want { + t.Fatalf("discard %d + dropped %d frames, want %d (3s of backlog)", sess.discard, sess.dropped, want) + } + + // The burst: the whole backlog, all of it him still talking. + loud := frameAt(0.35) + for i := 0; i < sess.discard; i++ { + if err := sess.feed(context.Background(), loud); err != nil { + t.Fatal(err) + } + } + if len(snd.sent) != 1 { + t.Errorf("the backlog was sent as a second utterance (sent = %d)", len(snd.sent)) + } + if sess.dropped == 0 { + t.Error("no frames were counted as backlog") + } +} + +// Same on the error path. A dead daemon used to seed the next spurious trigger +// on every failed turn, so a dead socket drove a retry loop off backlog alone. +func TestSessionDropsBacklogAfterASendError(t *testing.T) { + sess, _, _ := newSlowSession(bargeInConfig{}, audio.Audio{}, errors.New("boom"), 3*time.Second) + + // Not speakThenPause: the dispatch returns the send error, which that + // helper treats as fatal. + loud := frameAt(0.35) + for i := 0; i < (defaultSpeechMs+defaultFrameMs-1)/defaultFrameMs+5; i++ { + _ = sess.feed(context.Background(), loud) + } + for i := 0; i < (defaultSilenceMs+defaultFrameMs-1)/defaultFrameMs+2; i++ { + _ = sess.feed(context.Background(), silentBytes()) + } + if sess.discard == 0 { + t.Fatal("a failed round-trip left the backlog to be fed into the VAD") + } +} + +// Barge-in must not be triggerable by the backlog. Those frames are him +// finishing the sentence he started before she answered, delivered in +// microseconds, and the five-frame guard assumes real time. +func TestSessionBacklogCannotBargeIn(t *testing.T) { + sess, p, _ := newSlowSession(bargeInConfig{RMS: 0.12, Frames: 5}, replyAudio(), nil, 3*time.Second) + + speakThenPause(t, sess) + if p.plays != 1 || !p.Playing() { + t.Fatalf("plays = %d, playing = %v; want the reply playing", p.plays, p.Playing()) + } + + loud := frameAt(0.35) + backlog := sess.discard + if backlog < 5 { + t.Fatalf("discard = %d, want a real backlog", backlog) + } + for i := 0; i < backlog; i++ { + if err := sess.feed(context.Background(), loud); err != nil { + t.Fatal(err) + } + } + if p.stops != 0 { + t.Fatalf("she was cut off by audio recorded before she started speaking (stops = %d)", p.stops) + } + + // Real-time speech after the backlog still interrupts her. + for i := 0; i < 5; i++ { + if err := sess.feed(context.Background(), loud); err != nil { + t.Fatal(err) + } + } + if p.stops != 1 { + t.Fatalf("stops = %d, want 1 — barge-in must still work after the backlog", p.stops) + } +} + +// The frames that proved he was interrupting are replayed into the VAD, so his +// first word is not clipped. Five trigger frames plus five real ones reach the +// 300ms speech threshold; without the replay the first five are lost and no +// utterance is produced at all. +func TestSessionReplaysTheBargeInTriggerFrames(t *testing.T) { + sess, p, snd := newTestSession(bargeInConfig{RMS: 0.12, Frames: 5}) + speakThenPause(t, sess) + + veryLoud := frameAt(0.35) + for i := 0; i < 5; i++ { + _ = sess.feed(context.Background(), veryLoud) + } + if p.stops != 1 { + t.Fatalf("expected barge-in, stops = %d", p.stops) + } + if p.Playing() { + t.Fatal("fake player still playing after Stop") + } + + speechFrames := (defaultSpeechMs + defaultFrameMs - 1) / defaultFrameMs + for i := 0; i < speechFrames-5; i++ { + if err := sess.feed(context.Background(), veryLoud); err != nil { + t.Fatal(err) + } + } + silenceFrames := (defaultSilenceMs+defaultFrameMs-1)/defaultFrameMs + 2 + for i := 0; i < silenceFrames; i++ { + if err := sess.feed(context.Background(), silentBytes()); err != nil { + t.Fatal(err) + } + } + if len(snd.sent) != 2 { + t.Fatalf("sent %d utterances, want 2 — the 150ms that triggered barge-in was clipped", len(snd.sent)) + } +} From 9a9f4464d5b11afcf0607e63d222e829efa334a6 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:21:55 +0400 Subject: [PATCH 6/6] ipc: gofmt unlock_test.go The explicit-wrap assertions went in unformatted and make test's fmt-check step failed on them. Found in review of #77. --- internal/ipc/unlock_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/ipc/unlock_test.go b/internal/ipc/unlock_test.go index f055d5f..7d3d541 100644 --- a/internal/ipc/unlock_test.go +++ b/internal/ipc/unlock_test.go @@ -42,7 +42,11 @@ func TestUnlockDeliversSecretToHook(t *testing.T) { var gotUnlock, gotWrap []byte var gotExplicit bool srv.UnlockFn = func(_ context.Context, s []byte) error { gotUnlock = bytes.Clone(s); return nil } - srv.WrapKeyFn = func(_ context.Context, s []byte, explicit bool) error { gotWrap = bytes.Clone(s); gotExplicit = explicit; return nil } + srv.WrapKeyFn = func(_ context.Context, s []byte, explicit bool) error { + gotWrap = bytes.Clone(s) + gotExplicit = explicit + return nil + } ctx := context.Background() if err := cli.Unlock(ctx, secret); err != nil {