diff --git a/internal/config/voice.go b/internal/config/voice.go index 1e9a9e3..4f3b568 100644 --- a/internal/config/voice.go +++ b/internal/config/voice.go @@ -2,6 +2,9 @@ package config import ( "errors" + "fmt" + "os" + "path/filepath" "time" ) @@ -141,10 +144,55 @@ func (c *Config) validateVoice() error { if e.ModelPath == "" || e.TokenizerPath == "" || e.LibPath == "" { return errors.New("voice.embedder: all three of model_path, tokenizer_path, lib_path must be set, or remove embedder to use the floor stub") } + if err := e.checkHeadsDistinct(); err != nil { + return err + } } return nil } +// checkHeadsDistinct refuses a heads graph that is the embedder's own file +// (V-692). The rule is stated on HeadsPath above and in CLAUDE.md, and until +// now nothing enforced it: the daemon loaded whatever the key pointed at, so +// pointing both keys at one file cost recall with no error and no log line. It +// reads as ordinary drift, which is the worst kind of misconfiguration. +// +// Refusing to start is the right trade here. The heads are an accelerator and a +// broken weights file is deliberately not fatal in voicewire.go, but this is not +// a broken file. It is a working file in the wrong role, and a daemon that +// cannot route well should say so rather than answer worse. +// +// Cleaned and made absolute first, so "./m.onnx" and "$PWD/m.onnx" are one +// path. Then SameFile, which catches the copy that is a symlink or a hard link +// to the original. A path that does not stat is left to the loader, which fails +// on it with a better message than this can give. +func (e *EmbedderConfig) checkHeadsDistinct() error { + if e.HeadsPath == "" || e.ModelPath == "" { + return nil + } + heads, model := absClean(e.HeadsPath), absClean(e.ModelPath) + same := heads == model + if !same { + hi, herr := os.Stat(heads) + mi, merr := os.Stat(model) + same = herr == nil && merr == nil && os.SameFile(hi, mi) + } + if same { + return fmt.Errorf("voice.embedder: heads_path and model_path are the same file (%s) — the heads graph is a fine-tuned copy, and scoring recall with it degrades what the resident embedder already stored", heads) + } + return nil +} + +// absClean — the comparable form of a path. Abs fails only when the working +// directory is unreadable, and a cleaned relative path is still worth comparing, +// so the error falls back rather than propagating. +func absClean(p string) string { + if abs, err := filepath.Abs(p); err == nil { + return abs + } + return filepath.Clean(p) +} + // VoiceConfig — the client↔core TCP surface + the stt/tts worker-module // seams. // diff --git a/internal/config/voice_test.go b/internal/config/voice_test.go new file mode 100644 index 0000000..86c274f --- /dev/null +++ b/internal/config/voice_test.go @@ -0,0 +1,70 @@ +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// The heads graph is a fine-tuned copy of the embedder, and pointing both keys +// at one file degrades recall with no error and no log line (V-692). These are +// the shapes that used to boot clean. +func TestHeadsPathMustNotBeTheModelFile(t *testing.T) { + dir := t.TempDir() + model := filepath.Join(dir, "model.onnx") + heads := filepath.Join(dir, "heads.onnx") + for _, p := range []string{model, heads} { + if err := os.WriteFile(p, []byte("onnx"), 0o600); err != nil { + t.Fatalf("write %s: %v", p, err) + } + } + link := filepath.Join(dir, "link.onnx") + if err := os.Symlink(model, link); err != nil { + t.Fatalf("symlink: %v", err) + } + + // A path that stats and one that does not, because the guard compares the + // cleaned string before it stats anything. + for name, headsPath := range map[string]string{ + "the same path": model, + "a symlink to it": link, + "an uncleaned path": filepath.Join(dir, ".", "sub", "..", "model.onnx"), + "a path on no disk": filepath.Join(dir, "absent.onnx"), + } { + t.Run(name, func(t *testing.T) { + same := headsPath != filepath.Join(dir, "absent.onnx") + err := voiceConfigWith(t, model, headsPath) + if same && err == nil { + t.Fatal("want a startup error, got a daemon that routes worse in silence") + } + if same && !strings.Contains(err.Error(), "heads_path") { + t.Fatalf("the error does not name the key: %v", err) + } + if !same && err != nil { + t.Fatalf("a distinct heads_path was refused: %v", err) + } + }) + } + + if err := voiceConfigWith(t, model, heads); err != nil { + t.Fatalf("two distinct files were refused: %v", err) + } + if err := voiceConfigWith(t, model, ""); err != nil { + t.Fatalf("no heads at all was refused: %v", err) + } +} + +// voiceConfigWith loads a minimal enabled voice block through the real Load, so +// the test exercises the startup path and not just the check in isolation. +func voiceConfigWith(t *testing.T, model, heads string) error { + t.Helper() + body := `{"voice":{"enabled":true,"bind":"127.0.0.1:9100","embedder":{` + + `"model_path":"` + model + `","tokenizer_path":"/t.json","lib_path":"/l.so"` + if heads != "" { + body += `,"heads_path":"` + heads + `"` + } + body += `}}}` + _, err := Load(writeConfig(t, body)) + return err +}