diff --git a/cmd/mavend/main.go b/cmd/mavend/main.go index d6df63a..8211d0e 100644 --- a/cmd/mavend/main.go +++ b/cmd/mavend/main.go @@ -368,6 +368,11 @@ func run(args []string) error { srv.StepUp = func(ctx context.Context) error { return passkeySess.Assert(ctx, auth.Scope{}) } + // wg is declared here rather than next to srv.Serve because the media + // retention loop starts on this path too, and shutdown has to wait for a + // prune in flight: it deletes files. + var wg sync.WaitGroup + // Mail ingestion (Vikunja #246): the hook stays nil unless an email block is // configured and there is a llama-server to extract with, in which case // ipc.MethodIngestMail reports ErrUnknownMethod. @@ -376,7 +381,7 @@ func run(args []string) error { wireModelSwap(srv, phr, cfg) // Vision + the media blob store (Vikunja #252). Both stay dark without a // media block; MethodDescribeImage answers ErrUnknownMethod then. - keeper := wireVision(ctx, srv, st, embedderOf(voiceW), cfg) + keeper := wireVision(ctx, &wg, srv, st, embedderOf(voiceW), cfg) // The meeting recorder (Vikunja #253) shares that blob store and its // retention loop. Off unless a capture block enables it, in which case // all four capture methods answer ErrUnknownMethod. @@ -555,7 +560,7 @@ func run(args []string) error { srv.Check = (&auth.Gate{Enrollment: auth.NewFloorEnrollment(), Session: passkeySess}).Check wireMailIntake(srv, st, phr, cfg, evBus) wireModelSwap(srv, phr, cfg) - keeper := wireVision(ctx, srv, st, embedderOf(voiceW), cfg) + keeper := wireVision(ctx, &wg, srv, st, embedderOf(voiceW), cfg) wireCapture(srv, keeper, st, voiceW, phr, cfg) // Voice identification (Vikunja #255). Enrolment plumbing only until a // speaker-embedding model exists on disk; off entirely without a speaker @@ -622,7 +627,6 @@ func run(args []string) error { } } - var wg sync.WaitGroup wg.Add(1) go func() { defer wg.Done() diff --git a/cmd/mavend/vision.go b/cmd/mavend/vision.go index 93b0c2d..172b7a8 100644 --- a/cmd/mavend/vision.go +++ b/cmd/mavend/vision.go @@ -7,11 +7,14 @@ // media.dir, prepares a downscaled JPEG, and asks a local vision server what it // is. The description comes back as words; nothing about the image is echoed. // -// Off unless configured twice over: no `media` block ⇒ nowhere to keep the -// bytes, so the method does not exist; no `vision` block with enabled + a local -// endpoint ⇒ the store is wired but the describing half refuses, and the method -// still does not exist. A surface cannot make Maven look at pictures by merely -// sending one. +// Off unless configured: no `media` block ⇒ nowhere to keep the bytes, so the +// method does not exist and a surface cannot make Maven accept a photo by +// merely sending one. A `media` block with no `vision` block is a real state, +// the one this box is in today: the store is wired, the method exists, the +// bytes are kept and the reply says she cannot read the picture yet. That reply +// is re-runnable by id on the day a vision model lands, which is the reason to +// keep the bytes at all. Saving the description as a note needs more than the +// read rung — see the scope check on auth.ImageNoteSource. // // Two things this file deliberately does not do: // @@ -29,6 +32,7 @@ import ( "fmt" "log" "path/filepath" + "sync" "time" "github.com/kami/maven/internal/config" @@ -64,12 +68,14 @@ func openMediaStore(cfg *config.Config) *mediaKeeper { if !filepath.IsAbs(dir) && cfg.StateDir != "" { dir = filepath.Join(cfg.StateDir, dir) } - st, err := media.Open(dir, cfg.Media.MaxBytes, time.Duration(cfg.Media.Retention)) + st, err := media.OpenWithBudget(dir, cfg.Media.MaxBytes, cfg.Media.MaxTotalBytes, + time.Duration(cfg.Media.Retention)) if err != nil { log.Printf("media: %v — image and audio intake disabled", err) return nil } - log.Printf("media: blob store at %s, retention %s", st.Dir(), st.Retention()) + log.Printf("media: blob store at %s, retention %s, %d of %d bytes used", + st.Dir(), st.Retention(), st.Total(), st.Budget()) return &mediaKeeper{store: st} } @@ -159,6 +165,12 @@ func (v *visionIntake) describe(ctx context.Context, req ipc.DescribeImageReq) ( if len(req.Data) == 0 && req.ID == "" { return ipc.DescribeImageResp{}, fmt.Errorf("describe image: neither data nor id") } + if len(req.Data) > 0 && req.ID != "" { + // The contract says exactly one. Taking the ID branch and dropping the + // bytes silently is the worst of the three possible answers: the caller + // believes it sent a new image and nothing says otherwise. + return ipc.DescribeImageResp{}, fmt.Errorf("describe image: both data and id given, send one") + } var ( res vision.Result @@ -205,6 +217,12 @@ func (v *visionIntake) describe(ctx context.Context, req ipc.DescribeImageReq) ( return resp, nil } +// noteMarker prefixes a stored description. Without it the note reads exactly +// like something he told her, and it is not: it is a small VLM's guess about a +// picture, embedded and recalled as if it were his own words. Four characters +// of provenance in the text are cheaper than believing it later. +const noteMarker = "Со снимка: " + // writeNote stores the description as an ordinary note so it is recallable. The // note carries the blob id in its source, which is the only link back to the // bytes — the note text is words about the picture, never the picture. @@ -221,7 +239,7 @@ func (v *visionIntake) writeNote(ctx context.Context, res vision.Result) (int64, } } source := "media:image:" + res.Blob.ID[:12] - return v.st.WriteNote(ctx, v.now(), res.Description, vec, source) + return v.st.WriteNote(ctx, v.now(), noteMarker+res.Description, vec, source) } // sourceOrDefault labels a blob whose sender did not say where it came from. @@ -241,12 +259,19 @@ func sourceOrDefault(s string) string { // with one retention loop holds both the images and the audio, which is the // whole point of internal/media being a shared package. nil ⇒ no media block, // and neither capability exists. -func wireVision(ctx context.Context, srv *ipc.Server, st *store.Store, emb router.Embedder, cfg *config.Config) *mediaKeeper { +func wireVision(ctx context.Context, wg *sync.WaitGroup, srv *ipc.Server, st *store.Store, emb router.Embedder, cfg *config.Config) *mediaKeeper { keeper := openMediaStore(cfg) if keeper == nil { return nil } - go keeper.runPrune(ctx) + // In the daemon's WaitGroup like every other loop in run: a prune deletes + // files, and shutting down in the middle of one was the single loop nobody + // waited for. + wg.Add(1) + go func() { + defer wg.Done() + keeper.runPrune(ctx) + }() vi := newVisionIntake(keeper, st, emb, cfg) if vi == nil { diff --git a/cmd/mavend/vision_test.go b/cmd/mavend/vision_test.go new file mode 100644 index 0000000..ab69be0 --- /dev/null +++ b/cmd/mavend/vision_test.go @@ -0,0 +1,72 @@ +package main + +import ( + "bytes" + "context" + "image" + "image/png" + "strings" + "testing" + "time" + + "github.com/kami/maven/internal/config" + "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/media" + "github.com/kami/maven/internal/vision" +) + +func testIntake(t *testing.T) *visionIntake { + t.Helper() + st := newTestStore(t) + blobs, err := media.Open(t.TempDir(), 0, 0) + if err != nil { + t.Fatal(err) + } + return &visionIntake{ + in: vision.NewIntake(blobs, vision.Disabled{}, 0), + st: st, + now: time.Now, + } +} + +// The contract says exactly one of Data or ID. Taking the ID branch and +// dropping the bytes silently is the worst of the three possible answers: the +// caller believes it sent a new image and nothing says otherwise. +func TestDescribeRefusesBothDataAndID(t *testing.T) { + v := testIntake(t) + _, err := v.describe(context.Background(), ipc.DescribeImageReq{ + Data: []byte("bytes"), ID: strings.Repeat("a", 64), + }) + if err == nil { + t.Fatal("both data and id must be refused") + } + if !strings.Contains(err.Error(), "send one") { + t.Fatalf("err = %v, want it to name the contract", err) + } +} + +// Vision being off does not remove the method: the bytes are stored and the +// answer says she cannot read the picture yet, which is re-runnable by id. That +// is the state this box is in today, and three doc comments used to claim the +// opposite. +func TestVisionOffStillStores(t *testing.T) { + v := testIntake(t) + var buf bytes.Buffer + if err := png.Encode(&buf, image.NewRGBA(image.Rect(0, 0, 4, 4))); err != nil { + t.Fatal(err) + } + resp, err := v.describe(context.Background(), ipc.DescribeImageReq{Data: buf.Bytes(), Source: "web:upload"}) + if err != nil { + t.Fatalf("storing must succeed even with no vision model: %v", err) + } + if len(resp.ID) != 64 { + t.Fatalf("no blob id came back: %+v", resp) + } + if resp.Description != "" { + t.Errorf("description = %q, want none", resp.Description) + } + // And with no media block at all the method does not exist. + if vi := newVisionIntake(nil, nil, nil, &config.Config{}); vi != nil { + t.Fatal("no media block must leave the method nonexistent") + } +} diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 626e933..8f38f25 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -472,3 +472,30 @@ func TestRequirement_Speaker(t *testing.T) { t.Errorf("voice listing speakers = %v; want allowed", err) } } + +// Describing an image is a read. Saving the description is a write of recall +// corpus under a source no enrollment owns, so it is held to the same +// source-scope rule WriteFact is. Before this, any AuthRead caller could put a +// small VLM's guess into what Maven knows. +func TestCan_DescribeImage_SaveNoteNeedsScope(t *testing.T) { + poller := Scope{Surface: SurfaceTelegram, Module: "poll", SourceScope: []string{"poll:healthcheck"}} + web := Scope{Surface: SurfaceAuthedPage, Module: "web", SourceScope: []string{"*"}} + + plain, err := json.Marshal(ipc.DescribeImageReq{Data: []byte("x")}) + if err != nil { + t.Fatal(err) + } + noting, err := json.Marshal(ipc.DescribeImageReq{Data: []byte("x"), SaveNote: true}) + if err != nil { + t.Fatal(err) + } + if err := Can(ipc.MethodDescribeImage, poller, plain); err != nil { + t.Errorf("describing without saving must stay a read: %v", err) + } + if err := Can(ipc.MethodDescribeImage, poller, noting); !errors.Is(err, ErrForbidden) { + t.Errorf("save_note out of scope = %v, want ErrForbidden", err) + } + if err := Can(ipc.MethodDescribeImage, web, noting); err != nil { + t.Errorf("a module scoped to everything must still be allowed: %v", err) + } +} diff --git a/internal/auth/policy.go b/internal/auth/policy.go index f940f80..6c9fc5b 100644 --- a/internal/auth/policy.go +++ b/internal/auth/policy.go @@ -178,6 +178,18 @@ func Can(m ipc.Method, scope Scope, params json.RawMessage) error { switch Requirement(m) { case AuthRead: + // Describing an image is a read. Saving the description as a note is + // not: writeNote embeds it, so it comes back in a later turn as + // something Maven knows, under the source media:image:, which no + // enrollment owns. The rung's own argument was that the method "cannot + // write a fact, set a reminder, or touch the tool allowlist" — it can + // write recall corpus, and that is what AuthWrite exists to scope. So + // the note half is held to the same source-scope rule WriteFact is. + if m == ipc.MethodDescribeImage && wantsNote(params) { + if !SourceAllowed(scope.SourceScope, ImageNoteSource) { + return fmt.Errorf("%w: source %q out of scope", ErrForbidden, ImageNoteSource) + } + } // Any enrolled module may read. Reads through the surface level the // Enrollment set (voice-L0 wouldn't be enrolled to write at all). return nil @@ -214,6 +226,26 @@ func Can(m ipc.Method, scope Scope, params json.RawMessage) error { return nil } +// ImageNoteSource is the source scope a caller needs to turn a described image +// into a note. The note itself is stored under "media:image:"; the +// scope is checked against this stem, because the id is not known until the +// bytes arrive and no enrollment could name it in advance. +const ImageNoteSource = "media:image" + +// wantsNote reports whether a DescribeImage call asked for the description to +// be remembered. Malformed params read as no: dispatch rejects them a moment +// later with a better error. +func wantsNote(raw json.RawMessage) bool { + if len(raw) == 0 { + return false + } + var p ipc.DescribeImageReq + if json.Unmarshal(raw, &p) != nil { + return false + } + return p.SaveNote +} + // SourceAllowed — true iff src is in scope (the wildcard "*" matches all). // Empty scope ⇒ fail closed. The function is pure; we keep it exported so a // future enrollment table can call into the same matching logic. diff --git a/internal/config/config.go b/internal/config/config.go index 84af890..4fdae93 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,6 +27,7 @@ import ( "github.com/kami/maven/internal/morning" "github.com/kami/maven/internal/netscan" "github.com/kami/maven/internal/smarthome" + "github.com/kami/maven/internal/vision" "github.com/kami/maven/internal/update" "github.com/robfig/cron/v3" ) @@ -693,6 +694,12 @@ type MediaConfig struct { // MaxBytes — per-blob cap. 0 ⇒ media.DefaultMaxBytes (64 MiB). MaxBytes int64 `json:"max_bytes,omitempty"` + + // MaxTotalBytes — whole-store cap. 0 ⇒ media.DefaultMaxTotalBytes (4 GiB). + // The per-blob cap bounds one call; this one bounds the sum of them, which + // is what actually decides whether the disk mavend's database lives on can + // be filled from outside. + MaxTotalBytes int64 `json:"max_total_bytes,omitempty"` } // StoreDir reports the configured blob directory, or "" when media is not @@ -1428,6 +1435,36 @@ func (c *Config) validate() error { return err } } + // A media dir that cannot be created, or a vision endpoint that is a typo, + // used to be logged at wiring time and the capability just stayed off. A + // capability silently not existing is the hardest kind of misconfiguration + // to notice, so both fail here instead. + if c.Media != nil { + if c.Media.StoreDir() == "" { + return errors.New("media.dir is required when a media block is present") + } + if c.Media.MaxBytes < 0 || c.Media.MaxTotalBytes < 0 { + return errors.New("media: max_bytes and max_total_bytes cannot be negative") + } + if c.Media.MaxTotalBytes > 0 && c.Media.MaxBytes > c.Media.MaxTotalBytes { + return fmt.Errorf("media: max_bytes %d is above max_total_bytes %d", + c.Media.MaxBytes, c.Media.MaxTotalBytes) + } + } + if c.Vision != nil && c.Vision.Enabled { + if strings.TrimSpace(c.Vision.Endpoint) == "" { + return errors.New("vision.enabled set but vision.endpoint is empty") + } + if err := vision.ValidateEndpoint(c.Vision.Endpoint); err != nil { + return err + } + if c.Media.StoreDir() == "" { + return errors.New("vision.enabled set but there is no media block to keep the bytes in") + } + } + if c.Capture.Records() && c.Media.StoreDir() == "" { + return errors.New("capture.enabled set but there is no media block to keep the audio in") + } if len(c.MorningRoutines) > 0 { if err := morning.Validate(morningRoutinesFromConfig(c.MorningRoutines)); err != nil { return err diff --git a/internal/config/senses_test.go b/internal/config/senses_test.go index d74b454..8f50a17 100644 --- a/internal/config/senses_test.go +++ b/internal/config/senses_test.go @@ -221,3 +221,40 @@ func TestSpeakerBlockParsesFromJSON(t *testing.T) { t.Errorf("thresholds = %+v", cfg.Speaker) } } + +// A typo in the vision endpoint, or a media block with no dir, used to be +// logged once at wiring time and the capability just stayed off. A capability +// that silently does not exist is the hardest misconfiguration to notice, so +// both fail at startup now. +func TestSensesBlocksAreValidatedAtStartup(t *testing.T) { + bad := map[string]string{ + "media with no dir": `{"media":{"retention":"48h"}}`, + "negative budget": `{"media":{"dir":"/srv/media","max_total_bytes":-1}}`, + "blob over the budget": `{"media":{"dir":"/srv/media","max_bytes":100,"max_total_bytes":10}}`, + "vision with no media dir": `{"vision":{"enabled":true,"endpoint":"http://127.0.0.1:8081"}}`, + "vision endpoint typo": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true,"endpoint":"127.0.0.1:8081"}}`, + "vision on the wan": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true,"endpoint":"http://8.8.8.8:8081"}}`, + "vision, empty endpoint": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true}}`, + "capture with no store": `{"capture":{"enabled":true}}`, + } + for name, body := range bad { + t.Run(name, func(t *testing.T) { + if _, err := Load(writeConfig(t, body)); err == nil { + t.Fatal("want a startup error") + } + }) + } + good := map[string]string{ + "media alone": `{"media":{"dir":"/srv/media"}}`, + "media + vision": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true,"endpoint":"http://127.0.0.1:8081"}}`, + "media + capture": `{"media":{"dir":"/srv/media"},"capture":{"enabled":true}}`, + "vision off": `{"vision":{"endpoint":"http://8.8.8.8:8081"}}`, + } + for name, body := range good { + t.Run(name, func(t *testing.T) { + if _, err := Load(writeConfig(t, body)); err != nil { + t.Fatalf("valid config refused: %v", err) + } + }) + } +} diff --git a/internal/ipc/api.go b/internal/ipc/api.go index 30cc4ab..c2ff155 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -190,14 +190,16 @@ type IngestMailResp struct { // // Source is provenance recorded on the stored blob: "telegram", "web:upload". // -// Exactly one of Data or ID is set. ID re-describes an image core already has — -// a different question, or the first attempt that succeeds after a vision model -// finally lands on disk. +// Exactly one of Data or ID is set, and core refuses a request carrying both: +// it used to take the ID branch and drop the bytes without a word. // -// The method exists only when core has both a media store and an enabled vision -// block; otherwise it answers ErrUnknownMethod, which is what "off unless -// configured" looks like at the wire. A surface cannot make Maven look at -// pictures by merely sending one. +// The method exists when core has a media store. Vision being off does NOT +// remove it: the bytes are stored and the answer says she cannot read the +// picture yet, which is re-runnable by ID once a vision model is on disk, and +// it is the state this box is in today. So a surface that gets a reply with an +// id and an empty description has not failed, it has stored something. With no +// media block the method answers ErrUnknownMethod, which is what "off unless +// configured" looks like at the wire. type DescribeImageReq struct { Data []byte `json:"data,omitempty"` ID string `json:"id,omitempty"` @@ -206,6 +208,10 @@ type DescribeImageReq struct { // SaveNote — also write the description as a note (source // "media:image:") so it is recallable later. Default false: a // glance at a screenshot is not automatically a memory. + // + // Setting it raises what the call needs: an embedded note is recall corpus, + // so the caller's source scope must cover auth.ImageNoteSource. Describing + // without saving stays an ordinary read. SaveNote bool `json:"save_note,omitempty"` } diff --git a/internal/ipc/server.go b/internal/ipc/server.go index cee5355..a6f48ac 100644 --- a/internal/ipc/server.go +++ b/internal/ipc/server.go @@ -455,10 +455,12 @@ type Server struct { SwapModelFn SwapModelFunc ModelStatusFn ModelStatusFunc - // DescribeImageFn — looks at one image (Vikunja #252). Set by the daemon only - // when a media store is configured AND vision is enabled with a local - // endpoint; nil ⇒ MethodDescribeImage answers ErrUnknownMethod, so a surface - // cannot make Maven accept a photo by merely sending one. + // DescribeImageFn — looks at one image (Vikunja #252). Set by the daemon + // whenever a media store is configured. Vision being off does not clear it: + // the image is stored and the reply says she cannot read it yet, which is + // re-runnable by id later. nil ⇒ no media block ⇒ MethodDescribeImage + // answers ErrUnknownMethod, so a surface cannot make Maven accept a photo + // by merely sending one. // // It bypasses CoreAPI for the same reason IngestMailFn does: it needs a blob // store and a vision server, neither of which is a store operation, and no