diff --git a/cmd/mavend/actions_act.go b/cmd/mavend/actions_act.go index be83e9e..2b96855 100644 --- a/cmd/mavend/actions_act.go +++ b/cmd/mavend/actions_act.go @@ -53,6 +53,13 @@ func (h *reactiveHandler) actionAct(ctx context.Context, dec router.Decision) st return "выполнить «" + phrase + "»? скажи «да» или «нет»." case errors.Is(err, tool.ErrNotEnabled): return h.proposeGap(ctx, dec) + case errors.Is(err, tool.ErrNotConnected), errors.Is(err, mcp.ErrNotConnected), errors.Is(err, mcp.ErrNoServer): + // The row is enabled and the backend is gone. Drafting a proposal + // for it (the ErrNotEnabled path) would be answering the wrong + // question. + return "этот инструмент включён, но сервер, который его выполняет, сейчас не подключён." + case errors.Is(err, mcp.ErrToolGone): + return "сервер больше не предлагает этот инструмент — я сняла его с разрешённых, посмотри на /tools." case errors.Is(err, mcp.ErrNeedsArgs): // An MCP tool that wants named arguments a spoken verb cannot // supply. Guessing them would be a wrong act, so she says so diff --git a/cmd/mavend/capture.go b/cmd/mavend/capture.go index de53a70..2393976 100644 --- a/cmd/mavend/capture.go +++ b/cmd/mavend/capture.go @@ -34,6 +34,7 @@ import ( "errors" "fmt" "log" + "sync" "time" "github.com/kami/maven/internal/capture" @@ -45,10 +46,12 @@ import ( "github.com/kami/maven/internal/store" ) -// captureSummaryTimeout — the budget for one Stop, which is a map-reduce over +// captureSummaryTimeout — the budget for one summary, which is a map-reduce over // the whole meeting: one model call per transcript window plus a reduce, each of // which is seconds on this box. Forty windows is the configured ceiling, so the -// budget has to be minutes, not the 60s the reply path uses. +// budget has to be minutes, not the 60s the reply path uses. It is spent on a +// background goroutine, never inside the capture_stop request: a client that +// asks Maven to stop recording gets the transcript back in seconds. const captureSummaryTimeout = 20 * time.Minute // llmCompleter adapts *llm.Client to capture.Completer. The pure package names @@ -70,6 +73,12 @@ type captureWiring struct { emb router.Embedder cfg *config.CaptureConfig now func() time.Time + + // ctx and wg belong to the daemon, not to the request. Summarising happens + // after the reply has gone out, so it needs a lifetime that outlives the + // call and a shutdown that waits for it. + ctx context.Context + wg *sync.WaitGroup } // newCaptureWiring returns nil when the recorder should not exist: no media @@ -79,7 +88,7 @@ type captureWiring struct { // recording is still made, stored and transcribed, and the summary is simply // absent — the honest degradation, and much better than refusing to record a // meeting that is happening now. -func newCaptureWiring(keeper *mediaKeeper, st *store.Store, voiceW *voiceWiring, phr phraser.Phraser, emb router.Embedder, cfg *config.Config) *captureWiring { +func newCaptureWiring(ctx context.Context, wg *sync.WaitGroup, keeper *mediaKeeper, st *store.Store, voiceW *voiceWiring, phr phraser.Phraser, emb router.Embedder, cfg *config.Config) *captureWiring { if keeper == nil || !cfg.Capture.Records() { return nil } @@ -113,7 +122,7 @@ func newCaptureWiring(keeper *mediaKeeper, st *store.Store, voiceW *voiceWiring, return nil } log.Printf("capture: enabled, sessions capped at %s", rec.MaxDuration()) - return &captureWiring{rec: rec, st: st, emb: emb, cfg: cc, now: time.Now} + return &captureWiring{rec: rec, st: st, emb: emb, cfg: cc, now: time.Now, ctx: ctx, wg: wg} } // start handles ipc.MethodCaptureStart. @@ -127,6 +136,7 @@ func (c *captureWiring) start(_ context.Context, req ipc.CaptureStartReq) (ipc.C return ipc.CaptureStartResp{ Label: s.Label, Started: s.Started, + Token: s.Token, MaxSeconds: int(c.rec.MaxDuration().Seconds()), }, nil } @@ -135,7 +145,7 @@ func (c *captureWiring) start(_ context.Context, req ipc.CaptureStartReq) (ipc.C // response with Expired set rather than an error: the cap firing is the designed // behaviour, and the client needs the flag to stop sending and call stop. func (c *captureWiring) append(_ context.Context, req ipc.CaptureAppendReq) (ipc.CaptureAppendResp, error) { - err := c.rec.Append(req.Audio) + err := c.rec.Append(req.Token, req.Audio) st := c.rec.Status() if errors.Is(err, capture.ErrExpired) { log.Printf("capture: %q hit the %s cap — stopping", st.Label, c.rec.MaxDuration()) @@ -150,21 +160,26 @@ func (c *captureWiring) append(_ context.Context, req ipc.CaptureAppendReq) (ipc // stop handles ipc.MethodCaptureStop. // // The error handling here mirrors vision's, and for the same reason: the audio is -// stored first, so a transcription or summary failure returns what exists rather -// than nothing. A response can carry a blob id with no transcript (STT failed, -// re-runnable), or a transcript with no summary (the model failed, the words are -// kept) — both are degraded successes and neither is an error to the caller. +// stored first, so a transcription failure returns what exists rather than +// nothing. A response can carry a blob id with no transcript (STT failed, +// re-runnable) — a degraded success, not an error to the caller. +// +// Summarising is NOT done here. A two-hour meeting is forty model calls, which +// on this box is minutes, and holding the IPC request open for them means the +// client that said "стоп" sits there with no answer while its own deadline runs +// out. Stop returns the transcript, and the summary note is written by a +// goroutine in the daemon's WaitGroup afterwards. func (c *captureWiring) stop(ctx context.Context, req ipc.CaptureStopReq) (ipc.CaptureStopResp, error) { if req.Discard { // "забудь, не записывай" — nothing is stored, transcribed or noted. - if !c.rec.Abort() { + if !c.rec.Abort(req.Token) { return ipc.CaptureStopResp{}, capture.ErrNoSession } log.Printf("capture: session discarded on request") return ipc.CaptureStopResp{Discarded: true}, nil } - res, err := c.rec.Stop(ctx) + res, err := c.rec.Stop(ctx, req.Token) resp := ipc.CaptureStopResp{ BlobID: res.BlobID, Label: res.Label, @@ -183,19 +198,47 @@ func (c *captureWiring) stop(ctx context.Context, req ipc.CaptureStopReq) (ipc.C log.Printf("capture: %q partially finished: %v", res.Label, err) } - if id, werr := c.writeNotes(ctx, res); werr != nil { - log.Printf("capture: note write for %q failed: %v", res.Label, werr) - } else { - resp.NoteID = id - } - log.Printf("capture: finished %q — %s of audio, %d summary chunk(s)", - res.Label, res.Duration.Round(time.Second), res.Chunks) + c.summarizeLater(res) + log.Printf("capture: finished %q — %s of audio, %d bytes of transcript", + res.Label, res.Duration.Round(time.Second), len(res.Transcript)) return resp, nil } +// summarizeLater runs the map-reduce and writes the notes after stop replied. +// The context is the daemon's, not the request's: the request is already +// answered, and cancelling the summary because the client hung up would throw +// away the only readable record of the meeting. +func (c *captureWiring) summarizeLater(res capture.Result) { + if res.Transcript == "" { + return + } + c.wg.Add(1) + go func() { + defer c.wg.Done() + ctx, cancel := context.WithTimeout(c.ctx, captureSummaryTimeout) + defer cancel() + if err := c.rec.Summarize(ctx, &res); err != nil { + // Not fatal: writeNotes falls back to the transcript, so a dead + // llama-server costs the summary and not the meeting. + log.Printf("capture: summary for %q failed: %v", res.Label, err) + } + if _, err := c.writeNotes(ctx, res); err != nil { + log.Printf("capture: note write for %q failed: %v", res.Label, err) + return + } + log.Printf("capture: summarised %q in %d chunk(s)", res.Label, res.Chunks) + }() +} + // writeNotes stores the summary as a note, and the transcript too when -// capture.save_transcript is set. Returns the summary note's id, or 0 when there -// was no summary to write. +// capture.save_transcript is set. Returns the id of the note that carries the +// meeting. +// +// With no summary the transcript is written instead, whatever save_transcript +// says. That flag is about keeping the verbatim record IN ADDITION to a summary, +// not about whether the meeting is remembered at all. Without this fallback a +// llama-server that was down at stop time meant an hour of recorded meeting left +// no note behind and nothing recalled it later. // // The note source carries the blob id, which is the only link back to the audio. // When retention prunes the blob the note remains — words about a meeting are a @@ -212,6 +255,13 @@ func (c *captureWiring) writeNotes(ctx context.Context, res capture.Result) (int if err != nil { return 0, fmt.Errorf("summary note: %w", err) } + } else if res.Transcript != "" { + var err error + id, err = c.writeNote(ctx, res.Transcript, source+":transcript") + if err != nil { + return 0, fmt.Errorf("transcript note: %w", err) + } + return id, nil } if c.cfg.SaveTranscript && res.Transcript != "" { if _, err := c.writeNote(ctx, res.Transcript, source+":transcript"); err != nil { @@ -251,8 +301,8 @@ func (c *captureWiring) status(_ context.Context) (ipc.CaptureStatusResp, error) // wireCapture installs the four IPC hooks, or leaves them nil so every capture // method reports ErrUnknownMethod. Takes the media keeper wireVision already // opened: one blob store, one retention loop, images and audio side by side. -func wireCapture(srv *ipc.Server, keeper *mediaKeeper, st *store.Store, voiceW *voiceWiring, phr phraser.Phraser, cfg *config.Config) { - cw := newCaptureWiring(keeper, st, voiceW, phr, embedderOf(voiceW), cfg) +func wireCapture(ctx context.Context, wg *sync.WaitGroup, srv *ipc.Server, keeper *mediaKeeper, st *store.Store, voiceW *voiceWiring, phr phraser.Phraser, cfg *config.Config) { + cw := newCaptureWiring(ctx, wg, keeper, st, voiceW, phr, embedderOf(voiceW), cfg) if cw == nil { return } diff --git a/cmd/mavend/capture_test.go b/cmd/mavend/capture_test.go new file mode 100644 index 0000000..e09bdad --- /dev/null +++ b/cmd/mavend/capture_test.go @@ -0,0 +1,118 @@ +package main + +import ( + "context" + "strings" + "sync" + "testing" + "time" + + "github.com/kami/maven/internal/audio" + "github.com/kami/maven/internal/capture" + "github.com/kami/maven/internal/config" + "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/media" +) + +// silentTranscriber stands in for mavsttd: one fixed phrase per window, so the +// wiring can be tested without whisper. +type silentTranscriber struct{} + +func (silentTranscriber) Transcribe(_ context.Context, _ audio.Audio) (string, float64, error) { + return "решили купить насос", 1.0, nil +} + +func testCaptureWiring(t *testing.T) (*captureWiring, *sync.WaitGroup) { + t.Helper() + blobs, err := media.Open(t.TempDir(), 0, 0) + if err != nil { + t.Fatal(err) + } + rec, err := capture.New(blobs, silentTranscriber{}, nil, capture.Config{}) + if err != nil { + t.Fatal(err) + } + var wg sync.WaitGroup + return &captureWiring{ + rec: rec, + st: newTestStore(t), + cfg: &config.CaptureConfig{}, + now: time.Now, + ctx: context.Background(), + wg: &wg, + }, &wg +} + +// A frame carrying the wrong token must not land in the running session. Append +// and stop used to address "whatever is running now", so a client whose session +// had already ended went on recording into somebody else's meeting, and any +// client could end a recording it never started. +func TestCaptureRefusesAnotherClientsToken(t *testing.T) { + c, _ := testCaptureWiring(t) + start, err := c.start(context.Background(), ipc.CaptureStartReq{Label: "встреча"}) + if err != nil { + t.Fatal(err) + } + if start.Token == "" { + t.Fatal("start handed back no session token") + } + if _, err := c.append(context.Background(), ipc.CaptureAppendReq{ + Token: "not-mine", + Audio: audio.Audio{Format: audio.PCM16kMono, Bytes: make([]byte, 3200)}, + }); err == nil { + t.Error("a frame with the wrong token was accepted") + } + if _, err := c.stop(context.Background(), ipc.CaptureStopReq{Token: "not-mine"}); err == nil { + t.Error("a stop with the wrong token ended the session") + } + if st, _ := c.status(context.Background()); !st.Running { + t.Error("the session was ended by a client that does not own it") + } +} + +// Stop answers with the transcript and does not wait for the summary. The +// summary is up to forty model calls, and holding the IPC request for them meant +// the client that said "стоп" sat with no answer for minutes. +// +// With no summariser wired the note still has to be written, from the transcript. +// save_transcript is about keeping the verbatim record IN ADDITION to a summary, +// not about whether the meeting is remembered at all — without this fallback a +// dead llama-server meant an hour of meeting left no note behind. +func TestStopReturnsTranscriptAndNotesItWithoutASummary(t *testing.T) { + c, wg := testCaptureWiring(t) + start, err := c.start(context.Background(), ipc.CaptureStartReq{Label: "планёрка"}) + if err != nil { + t.Fatal(err) + } + if _, err := c.append(context.Background(), ipc.CaptureAppendReq{ + Token: start.Token, + Audio: audio.Audio{Format: audio.PCM16kMono, Bytes: make([]byte, 32000)}, + }); err != nil { + t.Fatal(err) + } + resp, err := c.stop(context.Background(), ipc.CaptureStopReq{Token: start.Token}) + if err != nil { + t.Fatalf("stop: %v", err) + } + if resp.Transcript == "" { + t.Fatal("stop returned no transcript") + } + if resp.Summary != "" { + t.Errorf("summary = %q, want none inside the request", resp.Summary) + } + wg.Wait() + + notes, err := c.st.RecentNotes(context.Background(), 10) + if err != nil { + t.Fatal(err) + } + var found bool + for _, n := range notes { + if strings.Contains(n.Text, "насос") { + found = true + } + } + if !found { + t.Fatalf("the meeting left no note behind: %+v", notes) + } +} diff --git a/cmd/mavend/main.go b/cmd/mavend/main.go index 8befe8f..b74b080 100644 --- a/cmd/mavend/main.go +++ b/cmd/mavend/main.go @@ -394,6 +394,11 @@ func run(args []string) error { srv.StepUp = func(ctx context.Context) error { return passkeySess.Assert(ctx, auth.Scope{}) } srv.LockedFn = dl.isLocked + // 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. @@ -402,11 +407,11 @@ 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. - wireCapture(srv, keeper, st, voiceW, phr, cfg) + wireCapture(ctx, &wg, 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 // block, so no wire path takes a voiceprint on a default box. @@ -603,8 +608,8 @@ 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) - wireCapture(srv, keeper, st, voiceW, phr, cfg) + keeper := wireVision(ctx, &wg, srv, st, embedderOf(voiceW), cfg) + wireCapture(ctx, &wg, 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 // block, so no wire path takes a voiceprint on a default box. @@ -670,7 +675,6 @@ func run(args []string) error { } } - var wg sync.WaitGroup wg.Add(1) go func() { defer wg.Done() diff --git a/cmd/mavend/mcp.go b/cmd/mavend/mcp.go index 7b31117..c350b80 100644 --- a/cmd/mavend/mcp.go +++ b/cmd/mavend/mcp.go @@ -13,8 +13,11 @@ import ( "github.com/kami/maven/internal/webfetch" ) -// mcpRefreshInterval — how often the manager re-dials a server that is down. -// The manager applies its own backoff on top, so this being short is cheap. +// mcpRefreshInterval — how often the manager is asked to re-dial servers that +// are down. It is a tick, not a retry rate: mcp.Manager holds a per-server +// backoff that starts at DefaultReconnectEvery and doubles to +// MaxReconnectEvery, so a permanently misconfigured stdio server is not +// re-exec'd once a minute forever. const mcpRefreshInterval = time.Minute // mcpWiring — the MCP client, when the `mcp` block configures at least one @@ -29,9 +32,16 @@ type mcpWiring struct { st *store.Store } -// wireMCP builds the manager, connects, and proposes what it found. It never -// fails the daemon: a server that is unreachable at boot is logged and retried, -// because Maven starting is not contingent on someone else's process. +// wireMCP builds the manager. It does NOT dial: run does that, on its own +// goroutine, which is what makes "Maven starting is not contingent on someone +// else's process" true rather than merely intended. +// +// Dialing here used to be synchronous with a 30s budget, from wireVoice, from +// run. Connect dials serially and each HTTP dial is three requests against +// that server's timeout, so one black-holed endpoint cost 15s of boot and two +// cost the whole budget. On the passkey path wireVoice runs inside the unlock +// handler, so it delayed the answer to an unlock as well. Not failing and not +// blocking are different properties and only the first one held. func wireMCP(cfg *config.Config, st *store.Store) *mcpWiring { servers := cfg.MCPServers() if len(servers) == 0 { @@ -43,6 +53,7 @@ func wireMCP(cfg *config.Config, st *store.Store) *mcpWiring { limits.DenyHosts = cfg.MCP.DenyHosts limits.MaxBytes = cfg.MCP.MaxBytes limits.Timeout = time.Duration(cfg.MCP.Timeout) + limits.HostInterval = time.Duration(cfg.MCP.HostInterval) } mgr, err := mcp.NewManager(mcp.WebfetchDoor(limits), servers) if err != nil { @@ -52,30 +63,58 @@ func wireMCP(cfg *config.Config, st *store.Store) *mcpWiring { log.Printf("mcp: not wired: %v", err) return nil } - w := &mcpWiring{mgr: mgr, st: st} - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - mgr.Connect(ctx) - w.propose(ctx) - return w + return &mcpWiring{mgr: mgr, st: st} } -// propose writes a 'proposed' allowlist row for every discovered tool. It does -// NOT enable anything: a configured server is a place Maven may look, not a -// capability she has. Kami enables what he wants on /tools, behind step-up, -// which is the same gate a shell tool goes through. +// connect dials every server and reconciles what came back. Called from run, +// under the daemon's context, so a shutdown during a slow dial is observed. +func (w *mcpWiring) connect(ctx context.Context) { + if w == nil { + return + } + w.mgr.Connect(ctx) + w.propose(ctx) +} + +// propose writes a 'proposed' allowlist row for every discovered tool, and +// reconciles the rows that already exist against what the server offers today. +// It does NOT enable anything: a configured server is a place Maven may look, +// not a capability she has. Kami enables what he wants on /tools, behind +// step-up, which is the same gate a shell tool goes through. // -// Re-running on every boot is idempotent — ProposeMCPTool never touches an -// existing row, so a tool he disabled stays disabled and one he enabled keeps -// the cmd he enabled it with. +// Three things happen per discovered tool. +// +// A name not in the store becomes a proposal, carrying the tool's fingerprint. +// +// A name already in the store is reconciled against that fingerprint. A tool +// whose description, schema or readOnlyHint changed since it was approved drops +// back to 'proposed' and, if it stopped claiming read-only, to destructive=1. +// Insert-or-skip was not enough on its own: the cmd is a late-bound reference +// to a name the far end owns, so the server can redefine list_tasks into +// something that writes without the row changing at all. +// +// A row whose server is connected and no longer offers the tool is withdrawn. func (w *mcpWiring) propose(ctx context.Context) { if w == nil { return } now := time.Now() - fresh := 0 + fresh, changed := 0, 0 + seen := map[string]string{} // local name → "server/tool", for collisions for _, t := range w.mgr.Tools() { name := mcp.LocalName(t.Server, t.Name) + remote := t.Server + "/" + t.Name + // Two different tools can flatten to one local name: server "vik" with + // tool "list_tasks" and server "vik_list" with tool "tasks" both give + // "vik_list_tasks". The store keys rows by name, so the second would + // land on the first one's row. Config-controlled and therefore rare, + // but silently reusing a row is the wrong way to lose that race. + if prev, dup := seen[name]; dup { + log.Printf("mcp: %s and %s both map to the allowlist name %q — skipping the second, rename a server", + prev, remote, name) + continue + } + seen[name] = remote // No readOnlyHint ⇒ assume it mutates ⇒ the confirm turn. Being wrong // in this direction only costs a question. destructive := !t.ReadOnly @@ -83,19 +122,82 @@ func (w *mcpWiring) propose(ctx context.Context) { if t.Description != "" { provenance += ": " + t.Description } + fp := mcp.Fingerprint(t) ok, err := w.st.ProposeMCPTool(ctx, name, mcp.Scope(t.Server), - mcp.Cmd(t.Server, t.Name), destructive, provenance, now) + mcp.Cmd(t.Server, t.Name), destructive, provenance, fp, now) if err != nil { log.Printf("mcp: propose %s: %v", name, err) continue } if ok { fresh++ + continue + } + // The row already existed. Its provenance is whatever the server said + // the first time; reconciling rewrites it, so what /tools shows is what + // the server says now. + ch, err := w.st.ReconcileMCPTool(ctx, name, fp, destructive, provenance, now) + if err != nil { + log.Printf("mcp: reconcile %s: %v", name, err) + continue + } + if !ch.Changed { + continue + } + changed++ + switch { + case ch.Demoted && ch.Escalated: + log.Printf("mcp: %s changed on the server and no longer claims read-only — disabled and marked destructive, re-approve it on /tools", name) + case ch.Demoted: + log.Printf("mcp: %s changed on the server since it was enabled — disabled, re-approve it on /tools", name) + default: + log.Printf("mcp: %s changed on the server; the proposal now shows the new description", name) } } + w.withdrawGone(ctx, seen, now) if fresh > 0 { log.Printf("mcp: %d new tool proposal(s) waiting on /tools", fresh) } + if changed > 0 { + log.Printf("mcp: %d tool(s) changed since approval and need another look", changed) + } +} + +// withdrawGone disarms rows whose tool the server stopped offering. Only +// servers that are CONNECTED are considered: a tool missing because its server +// is down is not a tool that was withdrawn, and disabling a capability every +// time a process restarts would be worse than the problem. +func (w *mcpWiring) withdrawGone(ctx context.Context, seen map[string]string, now time.Time) { + live := map[string]bool{} + for _, name := range w.mgr.Connected() { + live[name] = true + } + if len(live) == 0 { + return + } + rows, err := w.st.ListTools(ctx, "") + if err != nil { + log.Printf("mcp: list tools: %v", err) + return + } + for _, row := range rows { + server, remote, ok := mcp.ParseCmd(row.Cmd) + if !ok || !live[server] { + continue + } + if _, still := seen[row.Name]; still { + continue + } + note := fmt.Sprintf("mcp %s/%s: no longer offered by the server", server, remote) + wasEnabled, err := w.st.WithdrawTool(ctx, row.Name, note, now) + if err != nil { + log.Printf("mcp: withdraw %s: %v", row.Name, err) + continue + } + if wasEnabled { + log.Printf("mcp: %s was enabled but %s no longer offers it — disabled", row.Name, server) + } + } } // run re-dials downed servers and picks up tools that appeared, until ctx is @@ -104,6 +206,9 @@ func (w *mcpWiring) run(ctx context.Context) { if w == nil { return } + // The first dial happens here rather than at wiring time, so boot never + // waits on someone else's process. + w.connect(ctx) t := time.NewTicker(mcpRefreshInterval) defer t.Stop() for { diff --git a/cmd/mavend/mcp_test.go b/cmd/mavend/mcp_test.go index 6339524..b63bdbd 100644 --- a/cmd/mavend/mcp_test.go +++ b/cmd/mavend/mcp_test.go @@ -31,6 +31,23 @@ func TestWireMCPOffWhenUnconfigured(t *testing.T) { } } +// Wiring must not dial. Boot used to block for the whole per-server timeout +// budget on a black-holed endpoint, and on the passkey path that delay landed +// inside the unlock handler. +func TestWireMCPDoesNotDial(t *testing.T) { + st := newTestStore(t) + w := wireMCP(&config.Config{MCP: &config.MCPConfig{Servers: []config.MCPServerConfig{{ + Name: "dead", Command: "/nonexistent/mcp-server", Enabled: true, + }}}}, st) + if w == nil { + t.Fatal("a configured server should wire") + } + defer w.close() + if s := w.status(); len(s) != 1 || s[0].Err != "" { + t.Fatalf("wireMCP dialled: %+v", s) + } +} + // An unreachable server must not stop the daemon, must be reported as down, and // must propose nothing. func TestWireMCPUnreachableServerIsNotFatal(t *testing.T) { @@ -42,6 +59,7 @@ func TestWireMCPUnreachableServerIsNotFatal(t *testing.T) { t.Fatal("a configured server should still wire") } defer w.close() + w.connect(context.Background()) st2 := w.status() if len(st2) != 1 || st2[0].Connected || st2[0].Err == "" { t.Fatalf("status = %+v", st2) @@ -67,6 +85,7 @@ func TestWireMCPPrivateURLRefusedWithoutAllowPrivate(t *testing.T) { t.Fatal("should wire") } defer w.close() + w.connect(context.Background()) s := w.status()[0] if s.Connected { t.Fatal("a loopback server must not connect without allow_private") 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/audio/pcmwav.go b/internal/audio/pcmwav.go index 4889fa3..b9da0f8 100644 --- a/internal/audio/pcmwav.go +++ b/internal/audio/pcmwav.go @@ -94,14 +94,33 @@ func PCMFromWAV(wav []byte) (Format, []byte, error) { // header so the result can be written to disk and played with `aplay`. // Used by the reference client to write the TTS reply; not on the wire. func WAVFromPCM(format Format, pcm []byte) ([]byte, error) { + hdr, err := WAVHeader(format, len(pcm)) + if err != nil { + return nil, err + } + out := make([]byte, wavHeaderSize+len(pcm)) + copy(out, hdr) + copy(out[wavHeaderSize:], pcm) + return out, nil +} + +// WAVHeaderSize is the fixed size of the header WAVHeader writes. A caller +// spooling audio to a file reserves this many bytes up front and rewrites them +// once it knows the length. +const WAVHeaderSize = wavHeaderSize + +// WAVHeader builds just the 44-byte canonical header for n bytes of PCM. It +// exists so a long recording can be written straight to a file: holding the +// whole meeting in memory to prepend 44 bytes is what the streaming path is +// avoiding. +func WAVHeader(format Format, n int) ([]byte, error) { if !format.IsValid() { return nil, fmt.Errorf("audio: WAVFromPCM: %w: %+v", ErrNotCanonicalPCM, format) } - out := make([]byte, wavHeaderSize+len(pcm)) - copy(out[wavHeaderSize:], pcm) + out := make([]byte, wavHeaderSize) // RIFF header copy(out[0:4], []byte("RIFF")) - binary.LittleEndian.PutUint32(out[4:8], uint32(36+len(pcm))) + binary.LittleEndian.PutUint32(out[4:8], uint32(36+n)) copy(out[8:12], []byte("WAVE")) // fmt chunk copy(out[12:16], []byte("fmt ")) @@ -116,6 +135,6 @@ func WAVFromPCM(format Format, pcm []byte) ([]byte, error) { binary.LittleEndian.PutUint16(out[34:36], uint16(format.SampleBits)) // data chunk copy(out[36:40], []byte("data")) - binary.LittleEndian.PutUint32(out[40:44], uint32(len(pcm))) + binary.LittleEndian.PutUint32(out[40:44], uint32(n)) return out, nil } 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 10a7290..7936a52 100644 --- a/internal/auth/policy.go +++ b/internal/auth/policy.go @@ -187,6 +187,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 @@ -223,6 +235,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/capture/capture.go b/internal/capture/capture.go index c9cb852..9f25979 100644 --- a/internal/capture/capture.go +++ b/internal/capture/capture.go @@ -16,8 +16,13 @@ // plan document and is refused: it requires listening in order to notice the // keyword, which is the exact behaviour this capability must not have. // - A session that is not stopped stops itself. MaxDuration is a hard cap -// checked on every Append, not a suggestion; a forgotten recording is a -// recording that ends, not one that runs until the disk is full. +// checked on every Append AND against the wall clock in Start and Status, +// so a client that simply stops sending frames — a browser tab closed, wifi +// gone — does not leave the one session slot occupied until mavend +// restarts. +// - A session belongs to whoever started it. Start returns a token and Append +// and Stop require it, so a second surface at the same authority rung +// cannot feed or harvest a recording it did not begin. // - Audio is stored under internal/media, which means retention prunes it and // it never leaves the box. Both the audio blob and the transcript stay // local; only the summary is written where he will read it. @@ -37,16 +42,20 @@ // // There is exactly one STT in Maven and this package does not add a second: it // takes an stt.Transcriber, which in deploy is the whisper.cpp worker behind -// cmd/mavsttd. Long audio is transcribed in windows too (see chunkAudio), for +// cmd/mavsttd. Long audio is transcribed in windows too (see transcribeFile), for // the same reason whisper itself works in 30s windows — handing a worker an hour // of PCM in one call is a request that either times out or blocks everything -// else for minutes. +// else for minutes. The windows are read back off the stored WAV one at a time, +// so the meeting is never in memory whole. package capture import ( "context" + "crypto/rand" + "encoding/hex" "errors" "fmt" + "os" "strings" "sync" "time" @@ -58,12 +67,17 @@ import ( // DefaultMaxDuration — how long one capture may run before it stops itself. // Two hours covers a long meeting and bounds the damage of a forgotten session: -// at 16 kHz mono that is about 230 MB of PCM, which is over media's default -// per-blob cap, so a session at the limit is stored truncated rather than -// refused. That trade is deliberate — a partial recording of a meeting he asked -// for beats an error after two hours. +// at 16 kHz mono that is about 230 MB of WAV, which is under media's +// DefaultMaxAudioBytes of 512 MiB. The two constants used to disagree — a +// 64 MiB blob cap is 35 minutes of audio against a 120 minute session cap — so +// the meeting that hit the limit was the one that failed to store. const DefaultMaxDuration = 2 * time.Hour +// StaleGrace — how long past MaxDuration a session may sit before Start and +// Status reap it. A frame in flight when the cap fires should not race the +// reaper, and a minute of slack costs nothing against a two-hour cap. +const StaleGrace = time.Minute + // DefaultSTTWindow — how much audio goes to the transcriber in one call. Five // minutes of 16 kHz mono is under 10 MB, transcribes in well under whisper's // own timeout on this box, and keeps the worker responsive to the voice path @@ -87,6 +101,9 @@ var ( // ErrExpired — the session hit MaxDuration and was closed. Returned from // Append so the caller stops sending; the audio collected so far is kept. ErrExpired = errors.New("capture: session reached its time limit") + // ErrWrongSession — the token does not match the running session. The + // recording belongs to the surface that started it. + ErrWrongSession = errors.New("capture: that is not your session") ) // Session — one recording in progress. Not created directly; Recorder.Start @@ -95,11 +112,53 @@ var ( type Session struct { Label string Started time.Time + // Token identifies this session to its owner. Append and Stop need it: the + // rung Append sits on is shared by every writing module, and a rung is not + // an owner. Without it any AuthWrite surface could call capture_stop on a + // meeting it did not start and be handed the verbatim transcript. + Token string mu sync.Mutex - pcm []byte + spool *os.File // the WAV being written, header first + path string + n int64 // PCM bytes written, header excluded format audio.Format expired bool + closed bool +} + +// write appends one frame to the spool file. +func (s *Session) write(b []byte) error { + if s.spool == nil { + return errors.New("capture: session has no spool file") + } + n, err := s.spool.Write(b) + s.n += int64(n) + if err != nil { + return fmt.Errorf("capture: spool write: %w", err) + } + return nil +} + +// finish closes the spool file and stamps the real WAV header over the +// placeholder Start wrote. +func (s *Session) finish() error { + if s.closed { + return nil + } + s.closed = true + if s.spool == nil { + return nil + } + defer s.spool.Close() + hdr, err := audio.WAVHeader(s.format, int(s.n)) + if err != nil { + return err + } + if _, err := s.spool.WriteAt(hdr, 0); err != nil { + return fmt.Errorf("capture: spool header: %w", err) + } + return s.spool.Sync() } // Duration is how much audio has been collected, from the bytes rather than the @@ -112,15 +171,23 @@ func (s *Session) Duration() time.Duration { } func (s *Session) duration() time.Duration { - a := audio.Audio{Format: s.format, Bytes: s.pcm} - return time.Duration(a.Duration() * float64(time.Second)) + return pcmDuration(s.format, s.n) +} + +// pcmDuration is how long n bytes of PCM lasts in the given format. +func pcmDuration(f audio.Format, n int64) time.Duration { + per := int64(f.SampleRate) * int64(f.Channels) * int64(f.SampleBits) / 8 + if per <= 0 { + return 0 + } + return time.Duration(float64(n) / float64(per) * float64(time.Second)) } // Bytes is how much PCM has been collected. For a status line. func (s *Session) Bytes() int { s.mu.Lock() defer s.mu.Unlock() - return len(s.pcm) + return int(s.n) } // Status — what a "что записываешь?" answer needs, and what /dash shows. It is @@ -141,6 +208,7 @@ type Recorder struct { sum *Summarizer maxDuration time.Duration sttWindow time.Duration + staleGrace time.Duration now func() time.Time mu sync.Mutex @@ -180,6 +248,7 @@ func New(blobs *media.Store, tr stt.Transcriber, sum *Summarizer, cfg Config) (* sum: sum, maxDuration: maxDur, sttWindow: window, + staleGrace: StaleGrace, now: time.Now, }, nil } @@ -195,19 +264,84 @@ func (r *Recorder) MaxDuration() time.Duration { return r.maxDuration } func (r *Recorder) Start(label string) (*Session, error) { r.mu.Lock() defer r.mu.Unlock() + r.reapLocked() if r.current != nil { return nil, fmt.Errorf("%w: %q since %s", ErrBusy, r.current.Label, r.current.Started.Format(time.Kitchen)) } + f, err := r.blobs.SpoolFile("capture") + if err != nil { + return nil, err + } + format := audio.PCM16kMono + hdr, err := audio.WAVHeader(format, 0) + if err != nil { + f.Close() + return nil, err + } + // The header is written first and rewritten at Stop with the real length, + // so the spool file is a playable WAV rather than headerless PCM that has + // to be copied to gain 44 bytes. + if _, err := f.Write(hdr); err != nil { + f.Close() + _ = os.Remove(f.Name()) + return nil, fmt.Errorf("capture: spool header: %w", err) + } + token, err := newToken() + if err != nil { + f.Close() + _ = os.Remove(f.Name()) + return nil, err + } s := &Session{ Label: strings.TrimSpace(label), Started: r.now().UTC(), - format: audio.PCM16kMono, + Token: token, + spool: f, + path: f.Name(), + format: format, } r.current = s return s, nil } +// newToken mints a session token. Sixteen random bytes: it is a capability +// handed back over the same socket the call came in on, not a secret at rest. +func newToken() (string, error) { + var b [16]byte + if _, err := rand.Read(b[:]); err != nil { + return "", fmt.Errorf("capture: token: %w", err) + } + return hex.EncodeToString(b[:]), nil +} + +// reapLocked drops a session whose wall clock ran past MaxDuration. The +// frame-driven check in Append only fires while frames arrive, so a client that +// simply stopped sending — a phone whose browser tab was closed, wifi gone — +// left the slot occupied and every later Start answering ErrBusy with a meeting +// from last Tuesday. r.mu must be held. +func (r *Recorder) reapLocked() { + s := r.current + if s == nil { + return + } + if r.now().UTC().Sub(s.Started) < r.maxDuration+r.staleGrace { + return + } + s.mu.Lock() + s.expired = true + _ = s.finish() + path := s.path + s.mu.Unlock() + if path != "" { + // The audio goes with it. A recording nobody stopped is one nobody is + // waiting for, and keeping it would mean storing a meeting on the + // strength of a dropped connection. + _ = os.Remove(path) + } + r.current = nil +} + // Append adds one frame to the running session. ErrNoSession when nothing is // running, which is the guard that makes an ambient path impossible: a stream // arriving at a Recorder nobody started is refused frame by frame. @@ -215,25 +349,39 @@ func (r *Recorder) Start(label string) (*Session, error) { // ErrExpired once the session is at MaxDuration. The audio collected so far is // kept and Stop still works — the cap ends the recording, it does not throw it // away. -func (r *Recorder) Append(a audio.Audio) error { +func (r *Recorder) Append(token string, a audio.Audio) error { if !a.Format.IsValid() { return fmt.Errorf("%w: %+v", ErrBadFormat, a.Format) } r.mu.Lock() + r.reapLocked() s := r.current r.mu.Unlock() if s == nil { return ErrNoSession } + if token != s.Token { + return ErrWrongSession + } s.mu.Lock() defer s.mu.Unlock() if s.expired { return ErrExpired } - s.pcm = append(s.pcm, a.Bytes...) + // The session fixed its format at Start. A client that switches sample rate + // mid-session used to have its frames concatenated into the same buffer: + // duration() then read the whole thing at the original rate, the stored WAV + // header lied, and the cap fired at the wrong length. + if a.Format != s.format { + return fmt.Errorf("%w: session is %+v, frame is %+v", ErrBadFormat, s.format, a.Format) + } + if err := s.write(a.Bytes); err != nil { + return err + } if s.duration() >= r.maxDuration { s.expired = true + _ = s.finish() return ErrExpired } return nil @@ -242,6 +390,7 @@ func (r *Recorder) Append(a audio.Audio) error { // Status reports the running session, or Running=false. func (r *Recorder) Status() Status { r.mu.Lock() + r.reapLocked() s := r.current r.mu.Unlock() if s == nil { @@ -273,6 +422,10 @@ type Result struct { // Chunks — how many windows the transcript was summarised in. 1 means it fit // in one prompt. Reported so a suspiciously vague summary can be explained. Chunks int + // StoreErr — why the audio was not kept, when it was not. The transcript is + // still produced in that case, so this is the difference between "no blob + // because storing failed" and "no blob because nothing was recorded". + StoreErr error } // Stop ends the session and produces the result: store the audio, transcribe it @@ -280,11 +433,18 @@ type Result struct { // the slow work starts, so a stuck model cannot block the next recording. // // The order matters and is the same as vision's: the audio is stored FIRST. If -// transcription or summarisation fails, the recording is still on disk and can -// be run again; a meeting that happened once must not be lost to a model error. -func (r *Recorder) Stop(ctx context.Context) (Result, error) { +// transcription fails, the recording is still on disk under media.retention, so +// the meeting is not lost to a model error. Note that re-running it is a manual +// job today: no method takes a blob id back, unlike vision's Rerun, and the blob +// prunes on the media retention like any other. +func (r *Recorder) Stop(ctx context.Context, token string) (Result, error) { r.mu.Lock() + r.reapLocked() s := r.current + if s != nil && token != s.Token { + r.mu.Unlock() + return Result{}, ErrWrongSession + } r.current = nil r.mu.Unlock() if s == nil { @@ -292,113 +452,168 @@ func (r *Recorder) Stop(ctx context.Context) (Result, error) { } s.mu.Lock() - pcm := s.pcm + err := s.finish() + path := s.path format := s.format + n := s.n s.mu.Unlock() res := Result{Label: s.Label, Started: s.Started} - if len(pcm) == 0 { + if err != nil { + _ = os.Remove(path) + return res, err + } + if n == 0 { + _ = os.Remove(path) return res, ErrEmptyCapture } - full := audio.Audio{Format: format, Bytes: pcm} - res.Duration = time.Duration(full.Duration() * float64(time.Second)) + res.Duration = pcmDuration(format, n) - // Stored as WAV, not headerless PCM: a blob on disk that `aplay` and whisper - // can both open without being told the format is worth 44 bytes. - wav, err := audio.WAVFromPCM(format, pcm) - if err != nil { - return res, fmt.Errorf("capture: wav: %w", err) + // The audio is stored first, as vision does, so a transcription or summary + // failure leaves something to run again. It moves rather than being read + // into memory: a two-hour meeting is a couple of hundred megabytes, and + // this is the process that owns the database and the resident model. + audioPath := path + blob, perr := r.blobs.PutFile(media.KindAudio, "audio/wav", "capture:meeting", path) + if perr == nil { + res.BlobID = blob.ID + audioPath = blob.Path + } else { + // Over the cap, or the store is full. Report it and KEEP GOING: this + // used to return, so the one case the audio cap actually fires on — a + // very long meeting — produced no transcript, no summary and no note, + // which is the whole point of the capability. The spool file stays + // until the transcript has been read off it. + res.StoreErr = perr + defer os.Remove(audioPath) } - blob, err := r.blobs.Put(media.KindAudio, "audio/wav", "capture:meeting", wav) - if err != nil { - // Over the per-blob cap is the expected case for a very long meeting. - // Report it and keep going: a transcript without the audio still beats - // nothing, and the words are what he will read. - return res, fmt.Errorf("capture: store audio: %w", err) - } - res.BlobID = blob.ID - text, err := r.transcribe(ctx, full) - if err != nil { - return res, fmt.Errorf("capture: transcribe: %w", err) - } + text, terr := r.transcribeFile(ctx, audioPath, format, n) res.Transcript = text + if terr != nil { + return res, fmt.Errorf("capture: transcribe: %w", terr) + } if strings.TrimSpace(text) == "" { return res, ErrEmptyCapture } - - if r.sum == nil { - return res, nil + if perr != nil { + return res, fmt.Errorf("capture: store audio: %w", perr) } - summary, chunks, err := r.sum.Summarize(ctx, s.Label, text) - res.Chunks = chunks - if err != nil { - // Degraded success: the transcript is real and stored, only the summary - // is missing. The caller writes the transcript note and says so. - return res, fmt.Errorf("capture: summarize: %w", err) - } - res.Summary = summary return res, nil } +// Summarize runs the map-reduce over a transcript. It is separate from Stop so +// the daemon can answer the stop quickly and do the model work afterwards: a +// full map-reduce is up to forty model calls, and a voice turn that says +// "хватит" should not wait minutes for the reply. +// +// The salvaged text a failed reduce returns is assigned before the error is +// checked. Summarize hands back the per-chunk summaries with its error +// precisely so they are not lost, and the caller used to throw them away. +func (r *Recorder) Summarize(ctx context.Context, res *Result) error { + if r.sum == nil || strings.TrimSpace(res.Transcript) == "" { + return nil + } + summary, chunks, err := r.sum.Summarize(ctx, res.Label, res.Transcript) + res.Chunks = chunks + res.Summary = summary + if err != nil { + return fmt.Errorf("capture: summarize: %w", err) + } + return nil +} + // Abort throws the running session away without transcribing or storing it. // This is what "забудь, не записывай" must map to: a recording someone changed // their mind about leaves nothing behind, not a blob with a note saying it was // abandoned. Returns whether anything was running. -func (r *Recorder) Abort() bool { +func (r *Recorder) Abort(token string) bool { r.mu.Lock() defer r.mu.Unlock() - if r.current == nil { + r.reapLocked() + s := r.current + if s == nil || token != s.Token { return false } r.current = nil + s.mu.Lock() + _ = s.finish() + path := s.path + s.mu.Unlock() + if path != "" { + _ = os.Remove(path) + } return true } -// transcribe runs the transcriber over the audio in windows and joins the text. -// A window that fails is fatal: a summary of a meeting with a silent hole in the -// middle is a summary that misleads. -func (r *Recorder) transcribe(ctx context.Context, a audio.Audio) (string, error) { - windows := chunkAudio(a, r.sttWindow) - parts := make([]string, 0, len(windows)) - for i, w := range windows { - text, _, err := r.tr.Transcribe(ctx, w) +// transcribeFile runs the transcriber over the stored WAV in windows and joins +// the text, reading one window at a time off disk so the meeting is never in +// memory whole. +// +// A window that fails is no longer fatal. It used to be, on the argument that a +// silent hole misleads — but the cost was 24 good windows thrown away for one +// whisper hiccup at minute 100. The hole is marked in the text instead, which +// keeps the words and stays honest about the gap. +func (r *Recorder) transcribeFile(ctx context.Context, path string, format audio.Format, n int64) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", fmt.Errorf("open audio: %w", err) + } + defer f.Close() + + per := windowBytes(format, r.sttWindow) + if per <= 0 || per > n { + per = n + } + total := int((n + per - 1) / per) + buf := make([]byte, per) + parts := make([]string, 0, total) + failed := 0 + for i, off := 0, int64(0); off < n; i, off = i+1, off+per { + size := per + if off+size > n { + size = n - off + } + // Never cut mid-sample: a split inside an int16 shifts every following + // sample by a byte and turns the tail of the window into noise. + if bps := int64(format.SampleBits / 8 * format.Channels); bps > 0 { + size -= size % bps + } + if size <= 0 { + break + } + if _, err := f.ReadAt(buf[:size], int64(audio.WAVHeaderSize)+off); err != nil { + return strings.Join(parts, " "), fmt.Errorf("window %d/%d: %w", i+1, total, err) + } + text, _, err := r.tr.Transcribe(ctx, audio.Audio{Format: format, Bytes: buf[:size]}) if err != nil { - return "", fmt.Errorf("window %d/%d: %w", i+1, len(windows), err) + if ctx.Err() != nil { + return strings.Join(parts, " "), fmt.Errorf("window %d/%d: %w", i+1, total, err) + } + failed++ + parts = append(parts, gapMarker) + continue } if t := strings.TrimSpace(text); t != "" { parts = append(parts, t) } } + if failed == total { + return "", fmt.Errorf("every one of %d window(s) failed", total) + } return strings.Join(parts, " "), nil } -// chunkAudio splits audio into windows of at most window duration, cut on -// sample boundaries. A window shorter than one sample is impossible; audio -// shorter than one window comes back as a single element, so the caller never -// special-cases the short case. -func chunkAudio(a audio.Audio, window time.Duration) []audio.Audio { - bytesPerSample := a.Format.SampleBits / 8 * a.Format.Channels - if bytesPerSample <= 0 || a.Format.SampleRate <= 0 || window <= 0 { - return []audio.Audio{a} +// gapMarker stands in for a window whisper could not read. Russian, because it +// is read by him in a note next to the words around it. +const gapMarker = "[…не разобрала…]" + +// windowBytes is how many PCM bytes one STT window holds. +func windowBytes(f audio.Format, window time.Duration) int64 { + bps := int64(f.SampleBits / 8 * f.Channels) + if bps <= 0 || f.SampleRate <= 0 || window <= 0 { + return 0 } - per := int(window.Seconds()) * a.Format.SampleRate * bytesPerSample - if per <= 0 || len(a.Bytes) <= per { - return []audio.Audio{a} - } - var out []audio.Audio - for off := 0; off < len(a.Bytes); off += per { - end := off + per - if end > len(a.Bytes) { - end = len(a.Bytes) - } - // Never cut mid-sample: a split inside an int16 shifts every following - // sample by a byte and turns the tail of the window into noise. - end -= (end - off) % bytesPerSample - if end <= off { - break - } - out = append(out, audio.Audio{Format: a.Format, Bytes: a.Bytes[off:end]}) - } - return out + per := int64(window.Seconds()) * int64(f.SampleRate) * bps + return per - per%bps } diff --git a/internal/capture/capture_test.go b/internal/capture/capture_test.go index e1c4268..998de21 100644 --- a/internal/capture/capture_test.go +++ b/internal/capture/capture_test.go @@ -90,7 +90,7 @@ func TestNewRequiresStoreAndTranscriber(t *testing.T) { // is refused. There is no ambient path in. func TestAppendWithoutStartIsRefused(t *testing.T) { r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) - if err := r.Append(frame(1)); !errors.Is(err, ErrNoSession) { + if err := r.Append("no-token", frame(1)); !errors.Is(err, ErrNoSession) { t.Fatalf("got %v, want ErrNoSession", err) } if r.Status().Running { @@ -100,20 +100,21 @@ func TestAppendWithoutStartIsRefused(t *testing.T) { func TestStopWithoutStartIsRefused(t *testing.T) { r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) - if _, err := r.Stop(context.Background()); !errors.Is(err, ErrNoSession) { + if _, err := r.Stop(context.Background(), "no-token"); !errors.Is(err, ErrNoSession) { t.Fatalf("got %v, want ErrNoSession", err) } } func TestOneSessionAtATime(t *testing.T) { r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) - if _, err := r.Start("встреча"); err != nil { + s, err := r.Start("встреча") + if err != nil { t.Fatal(err) } if _, err := r.Start("вторая"); !errors.Is(err, ErrBusy) { t.Fatalf("got %v, want ErrBusy", err) } - if _, err := r.Stop(context.Background()); !errors.Is(err, ErrEmptyCapture) { + if _, err := r.Stop(context.Background(), s.Token); !errors.Is(err, ErrEmptyCapture) { t.Fatalf("empty stop: %v", err) } // The slot is free again after a stop, even a failed one. @@ -127,18 +128,22 @@ func TestRoundTripStoresAudioTranscriptAndSummary(t *testing.T) { sum := NewSummarizer(&fakeCompleter{replies: []string{"— решили купить насос"}}, 0, 0, nil) r, blobs := testRecorder(t, tr, sum, Config{}) - if _, err := r.Start("встреча с подрядчиком"); err != nil { + s, err := r.Start("встреча с подрядчиком") + if err != nil { t.Fatal(err) } for i := 0; i < 3; i++ { - if err := r.Append(frame(2)); err != nil { + if err := r.Append(s.Token, frame(2)); err != nil { t.Fatal(err) } } - res, err := r.Stop(context.Background()) + res, err := r.Stop(context.Background(), s.Token) if err != nil { t.Fatalf("stop: %v", err) } + if err := r.Summarize(context.Background(), &res); err != nil { + t.Fatalf("summarize: %v", err) + } if res.BlobID == "" { t.Error("no audio blob stored") } @@ -171,21 +176,22 @@ func TestRoundTripStoresAudioTranscriptAndSummary(t *testing.T) { func TestMaxDurationEndsTheSessionAndKeepsAudio(t *testing.T) { tr := &fakeTranscriber{} r, _ := testRecorder(t, tr, nil, Config{MaxDuration: 4 * time.Second}) - if _, err := r.Start("длинная"); err != nil { + s, err := r.Start("длинная") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(3)); err != nil { + if err := r.Append(s.Token, frame(3)); err != nil { t.Fatalf("first frame: %v", err) } - if err := r.Append(frame(3)); !errors.Is(err, ErrExpired) { + if err := r.Append(s.Token, frame(3)); !errors.Is(err, ErrExpired) { t.Fatalf("got %v, want ErrExpired", err) } // Further frames keep being refused, so a client that ignores the error // cannot grow the recording past the cap. - if err := r.Append(frame(3)); !errors.Is(err, ErrExpired) { + if err := r.Append(s.Token, frame(3)); !errors.Is(err, ErrExpired) { t.Fatalf("post-expiry frame: %v", err) } - res, err := r.Stop(context.Background()) + res, err := r.Stop(context.Background(), s.Token) if err != nil { t.Fatalf("stop after expiry: %v", err) } @@ -196,11 +202,12 @@ func TestMaxDurationEndsTheSessionAndKeepsAudio(t *testing.T) { func TestAppendRejectsWrongFormat(t *testing.T) { r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) - if _, err := r.Start("x"); err != nil { + s, err := r.Start("x") + if err != nil { t.Fatal(err) } bad := audio.Audio{Format: audio.Format{SampleRate: 44100, Channels: 2, SampleBits: 16, Encoding: "pcm_s16le"}, Bytes: make([]byte, 100)} - if err := r.Append(bad); !errors.Is(err, ErrBadFormat) { + if err := r.Append(s.Token, bad); !errors.Is(err, ErrBadFormat) { t.Fatalf("got %v, want ErrBadFormat", err) } } @@ -209,13 +216,14 @@ func TestAppendRejectsWrongFormat(t *testing.T) { func TestAbortLeavesNothing(t *testing.T) { tr := &fakeTranscriber{} r, blobs := testRecorder(t, tr, nil, Config{}) - if _, err := r.Start("зря начали"); err != nil { + s, err := r.Start("зря начали") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(5)); err != nil { + if err := r.Append(s.Token, frame(5)); err != nil { t.Fatal(err) } - if !r.Abort() { + if !r.Abort(s.Token) { t.Fatal("Abort reported nothing running") } if r.Status().Running { @@ -231,7 +239,7 @@ func TestAbortLeavesNothing(t *testing.T) { if tr.calls != 0 { t.Errorf("Abort transcribed anyway (%d calls)", tr.calls) } - if r.Abort() { + if r.Abort(s.Token) { t.Error("second Abort reported a session") } } @@ -241,10 +249,11 @@ func TestStatusReportsTheRunningSession(t *testing.T) { if got := r.Status(); got.Running { t.Error("idle recorder reports running") } - if _, err := r.Start("планёрка"); err != nil { + s, err := r.Start("планёрка") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(10)); err != nil { + if err := r.Append(s.Token, frame(10)); err != nil { t.Fatal(err) } st := r.Status() @@ -264,13 +273,14 @@ func TestStatusReportsTheRunningSession(t *testing.T) { func TestLongAudioIsTranscribedInWindows(t *testing.T) { tr := &fakeTranscriber{} r, _ := testRecorder(t, tr, nil, Config{STTWindow: 2 * time.Second}) - if _, err := r.Start("длинная"); err != nil { + s, err := r.Start("длинная") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(9)); err != nil { + if err := r.Append(s.Token, frame(9)); err != nil { t.Fatal(err) } - res, err := r.Stop(context.Background()) + res, err := r.Stop(context.Background(), s.Token) if err != nil { t.Fatalf("stop: %v", err) } @@ -282,18 +292,19 @@ func TestLongAudioIsTranscribedInWindows(t *testing.T) { } } -// A hole in the middle of a meeting summary would mislead, so a failed window is -// fatal — but the audio is already stored and re-runnable. +// Every window failing is a transcription failure — but the audio is already +// stored and re-runnable. func TestTranscriptionFailureKeepsTheAudio(t *testing.T) { tr := &fakeTranscriber{err: errors.New("whisper is down")} r, blobs := testRecorder(t, tr, nil, Config{}) - if _, err := r.Start("встреча"); err != nil { + s, err := r.Start("встреча") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(2)); err != nil { + if err := r.Append(s.Token, frame(2)); err != nil { t.Fatal(err) } - res, err := r.Stop(context.Background()) + res, err := r.Stop(context.Background(), s.Token) if err == nil { t.Fatal("transcription failure was not reported") } @@ -309,16 +320,20 @@ func TestTranscriptionFailureKeepsTheAudio(t *testing.T) { // error. func TestNoSummarizerStillProducesATranscript(t *testing.T) { r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) - if _, err := r.Start("встреча"); err != nil { + s, err := r.Start("встреча") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(1)); err != nil { + if err := r.Append(s.Token, frame(1)); err != nil { t.Fatal(err) } - res, err := r.Stop(context.Background()) + res, err := r.Stop(context.Background(), s.Token) if err != nil { t.Fatalf("stop: %v", err) } + if err := r.Summarize(context.Background(), &res); err != nil { + t.Fatalf("summarize with no summarizer: %v", err) + } if res.Transcript == "" { t.Error("no transcript") } @@ -331,14 +346,18 @@ func TestNoSummarizerStillProducesATranscript(t *testing.T) { func TestSummaryFailureStillReturnsTheTranscript(t *testing.T) { sum := NewSummarizer(&fakeCompleter{err: errors.New("llama is down")}, 0, 0, nil) r, _ := testRecorder(t, &fakeTranscriber{}, sum, Config{}) - if _, err := r.Start("встреча"); err != nil { + s, err := r.Start("встреча") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(1)); err != nil { + if err := r.Append(s.Token, frame(1)); err != nil { t.Fatal(err) } - res, err := r.Stop(context.Background()) - if err == nil { + res, err := r.Stop(context.Background(), s.Token) + if err != nil { + t.Fatalf("stop: %v", err) + } + if err := r.Summarize(context.Background(), &res); err == nil { t.Fatal("summary failure was not reported") } if res.Transcript == "" { @@ -346,18 +365,12 @@ func TestSummaryFailureStillReturnsTheTranscript(t *testing.T) { } } -func TestChunkAudioNeverCutsMidSample(t *testing.T) { - a := audio.Audio{Format: audio.PCM16kMono, Bytes: make([]byte, 16000*2*5+1)} - for _, w := range chunkAudio(a, 2*time.Second) { - if len(w.Bytes)%2 != 0 { - t.Fatalf("window of %d bytes cuts an int16 in half", len(w.Bytes)) - } - } -} - -func TestChunkAudioShortInputIsOneWindow(t *testing.T) { - a := frame(1) - if got := chunkAudio(a, time.Minute); len(got) != 1 { - t.Errorf("got %d windows, want 1", len(got)) +func TestWindowBytesNeverCutsMidSample(t *testing.T) { + if got := windowBytes(audio.PCM16kMono, 2*time.Second); got%2 != 0 || got != 2*16000*2 { + t.Fatalf("windowBytes = %d", got) + } + odd := audio.Format{SampleRate: 16000, Channels: 1, SampleBits: 16, Encoding: "pcm_s16le"} + if got := windowBytes(odd, 0); got != 0 { + t.Fatalf("a zero window must produce zero, got %d", got) } } diff --git a/internal/capture/session_test.go b/internal/capture/session_test.go new file mode 100644 index 0000000..de31658 --- /dev/null +++ b/internal/capture/session_test.go @@ -0,0 +1,154 @@ +package capture + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/kami/maven/internal/audio" + "github.com/kami/maven/internal/media" +) + +// A frame for a session that already ended must not land in the next one. The +// recorder used to be addressed as "whatever is running now", so a client whose +// session was reaped went on appending its microphone into a meeting somebody +// else had started. +func TestAppendWithTheWrongTokenIsRefused(t *testing.T) { + r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) + s, err := r.Start("первая") + if err != nil { + t.Fatal(err) + } + if err := r.Append("someone-elses-token", frame(1)); !errors.Is(err, ErrWrongSession) { + t.Fatalf("append = %v, want ErrWrongSession", err) + } + if _, err := r.Stop(context.Background(), "someone-elses-token"); !errors.Is(err, ErrWrongSession) { + t.Fatalf("stop = %v, want ErrWrongSession", err) + } + if r.Abort("someone-elses-token") { + t.Fatal("Abort discarded a session it does not own") + } + if err := r.Append(s.Token, frame(1)); err != nil { + t.Fatalf("the owner is still refused: %v", err) + } +} + +// A client that simply stops sending — a phone whose tab was closed — used to +// hold the single session slot forever, and every later Start answered ErrBusy +// with a meeting from last week. +func TestStaleSessionIsReapedByTheWallClock(t *testing.T) { + r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{MaxDuration: time.Minute}) + now := time.Now().UTC() + r.now = func() time.Time { return now } + s, err := r.Start("брошенная") + if err != nil { + t.Fatal(err) + } + if _, err := r.Start("вторая"); !errors.Is(err, ErrBusy) { + t.Fatalf("start = %v, want ErrBusy", err) + } + now = now.Add(time.Minute + StaleGrace + time.Second) + next, err := r.Start("вторая") + if err != nil { + t.Fatalf("a stale session was not reaped: %v", err) + } + if next.Token == s.Token { + t.Fatal("the new session reused the stale token") + } + if err := r.Append(s.Token, frame(1)); !errors.Is(err, ErrWrongSession) { + t.Fatalf("the reaped client can still write: %v", err) + } + // The abandoned recording is not kept: nobody is waiting for it, and storing + // it would mean keeping a meeting on the strength of a dropped connection. + if _, err := os.Stat(s.path); !os.IsNotExist(err) { + t.Fatalf("the reaped spool file survived: %v", err) + } +} + +// One window failing used to fail the whole transcription, which threw away +// every other window of an hour-long meeting. The hole is marked instead, so the +// summary cannot silently read as if nothing was missing. +func TestOneFailedWindowIsMarkedNotFatal(t *testing.T) { + r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{STTWindow: time.Second}) + r.tr = &windowTranscriber{failOn: 2} + s, err := r.Start("встреча") + if err != nil { + t.Fatal(err) + } + if err := r.Append(s.Token, frame(3)); err != nil { + t.Fatal(err) + } + res, err := r.Stop(context.Background(), s.Token) + if err != nil { + t.Fatalf("stop: %v", err) + } + if !strings.Contains(res.Transcript, gapMarker) { + t.Errorf("no gap marker in %q", res.Transcript) + } + if !strings.Contains(res.Transcript, "окно1") || !strings.Contains(res.Transcript, "окно3") { + t.Errorf("the surviving windows were dropped: %q", res.Transcript) + } +} + +// The audio not fitting the store is not a reason to lose the words. Stop used +// to return early on a store failure, so a recording over the blob cap produced +// neither a blob nor a transcript. +func TestStoreFailureStillTranscribes(t *testing.T) { + blobs, err := media.OpenWithBudget(t.TempDir(), 512, 1024, 0) + if err != nil { + t.Fatal(err) + } + tr := &fakeTranscriber{} + r, err := New(blobs, tr, nil, Config{}) + if err != nil { + t.Fatal(err) + } + s, err := r.Start("длинная встреча") + if err != nil { + t.Fatal(err) + } + if err := r.Append(s.Token, frame(2)); err != nil { + t.Fatal(err) + } + // The store failure is reported, but as a degraded success: the Result is + // filled in, and the caller keeps it rather than treating the error as + // nothing having happened. + res, err := r.Stop(context.Background(), s.Token) + if err == nil { + t.Fatal("the store failure was not reported") + } + if res.BlobID != "" { + t.Errorf("blob id = %q, want none", res.BlobID) + } + if !errors.Is(res.StoreErr, media.ErrTooLarge) { + t.Errorf("StoreErr = %v, want ErrTooLarge", res.StoreErr) + } + if res.Transcript == "" { + t.Fatal("the words were lost with the audio") + } + // The spool file is cleaned up even on the failure path. + glob, _ := filepath.Glob(filepath.Join(blobs.Dir(), "spool", "*")) + if len(glob) != 0 { + t.Errorf("spool leaked: %v", glob) + } +} + +// windowTranscriber answers per window and fails a chosen one, which is what a +// whisper timeout in the middle of a meeting looks like. +type windowTranscriber struct { + calls int + failOn int +} + +func (w *windowTranscriber) Transcribe(_ context.Context, _ audio.Audio) (string, float64, error) { + w.calls++ + if w.calls == w.failOn { + return "", 0, errors.New("whisper timed out") + } + return fmt.Sprintf("окно%d", w.calls), 1.0, nil +} diff --git a/internal/config/config.go b/internal/config/config.go index c2d60d7..50da777 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -28,6 +28,7 @@ import ( "github.com/kami/maven/internal/netscan" "github.com/kami/maven/internal/smarthome" "github.com/kami/maven/internal/update" + "github.com/kami/maven/internal/vision" "github.com/robfig/cron/v3" ) @@ -277,8 +278,20 @@ type MCPConfig struct { // MaxBytes — cap on one JSON-RPC response. 0 ⇒ webfetch.DefaultMaxBytes. MaxBytes int64 `json:"max_bytes,omitempty"` + + // HostInterval — minimum spacing between two requests to one MCP server. + // 0 ⇒ DefaultMCPHostInterval (50ms), NOT webfetch's own one-second default. + // That default was sized for a feed poll loop, and this path is in a spoken + // turn: one dial is three requests (initialize, initialized, tools/list), + // so a second of spacing is two seconds of pure sleeping per dial and up to + // another second before every tools/call leaves the box. + HostInterval Duration `json:"host_interval,omitempty"` } +// DefaultMCPHostInterval — see MCPConfig.HostInterval. Enough to stop a +// runaway loop hammering a server, small enough not to be heard. +const DefaultMCPHostInterval = 50 * time.Millisecond + // SmartHomeConfig — the Home Assistant block (Vikunja #256). Dark until // `"enabled": true`, and even then a discovered device is only ever PROPOSED // into the act allowlist: Kami enables it on /tools, behind step-up, exactly as @@ -429,6 +442,13 @@ type MCPServerConfig struct { // Timeout — per-call budget for this server. 0 ⇒ MCPConfig.Timeout. Timeout Duration `json:"timeout,omitempty"` + // Headers — sent verbatim on every request to a url server. This is how a + // bearer token reaches a real remote MCP server: {"Authorization": "Bearer + // ${MCP_TOKEN}"}, with the value in the gitignored env file like the + // telegram credentials. The Vikunja server on homesrv needs none only + // because it is unauthenticated on loopback. + Headers map[string]string `json:"headers,omitempty"` + // Enabled — false (the default) keeps a configured server described but // dark, so a block can be written and reviewed before it is switched on. Enabled bool `json:"enabled,omitempty"` @@ -437,13 +457,31 @@ type MCPServerConfig struct { // MCPServers maps the config blocks onto the mcp package's own type. It lives // here so config validation and daemon wiring cannot drift on the mapping. // Returns nil when nothing is configured or nothing is enabled. +// +// Disabled servers are dropped here, which is why validation does NOT use this +// list — see allMCPServers. func (c *Config) MCPServers() []mcp.ServerConfig { + return c.mcpServers(true) +} + +// allMCPServers is every configured server, enabled or not, for validation. +// +// Validating only the enabled ones meant a block with both command and url, or +// a bare hostname as the url, passed startup validation while it was dark. The +// doc on Enabled says a block can be written and reviewed before it is switched +// on; the review the config layer could give was the one thing skipped. Enabled +// gates the dialing, not the shape check. +func (c *Config) allMCPServers() []mcp.ServerConfig { + return c.mcpServers(false) +} + +func (c *Config) mcpServers(onlyEnabled bool) []mcp.ServerConfig { if c.MCP == nil { return nil } out := make([]mcp.ServerConfig, 0, len(c.MCP.Servers)) for _, s := range c.MCP.Servers { - if !s.Enabled { + if onlyEnabled && !s.Enabled { continue } timeout := time.Duration(s.Timeout) @@ -460,8 +498,9 @@ func (c *Config) MCPServers() []mcp.ServerConfig { AllowPrivate: s.AllowPrivate, AllowTools: s.AllowTools, MaxTools: s.MaxTools, + Headers: s.Headers, Timeout: timeout, - Enabled: true, + Enabled: s.Enabled, }) } if len(out) == 0 { @@ -668,6 +707,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 @@ -755,9 +800,14 @@ type CaptureConfig struct { MaxChunks int `json:"max_chunks,omitempty"` // SaveTranscript — write the full transcript as a note alongside the - // summary. Default false: a verbatim record of what other people said in a - // room is a heavier thing to keep than a four-line summary, so it takes a - // deliberate yes. The audio blob is pruned by media.retention either way. + // summary. Default false, and the cost is not disk: a note is embedded and + // becomes recall corpus, so every later question can surface verbatim words + // other people said in a room. That is the reason it takes a deliberate yes. + // The audio blob is pruned by media.retention either way; the notes are not. + // + // A meeting with no summary writes its transcript regardless. The choice + // here is transcript IN ADDITION to a summary, not whether the meeting is + // remembered at all. SaveTranscript bool `json:"save_transcript,omitempty"` } @@ -1259,11 +1309,18 @@ func (c *Config) applyDefaults() { c.Feeds = nil } - // Same rule for MCP: a block with no server, or none enabled, is the same - // as no block at all. Normalising it to nil keeps "off" in one place. - if c.MCP != nil && len(c.MCPServers()) == 0 { + // Same rule for MCP: a block with no server at all is the same as no block. + // A block whose servers are all disabled is NOT normalised away, because + // validate has to see their shape — a dark block with a typo in it should + // fail at startup, which is the whole reason it can be written before it is + // switched on. wireMCP builds nothing when nothing is enabled, so "off" + // still holds. + if c.MCP != nil && len(c.MCP.Servers) == 0 { c.MCP = nil } + if c.MCP != nil && c.MCP.HostInterval <= 0 { + c.MCP.HostInterval = Duration(DefaultMCPHostInterval) + } // Same rule for the house: a block that is not enabled is the same as no // block at all, so "off" stays in one place. @@ -1389,7 +1446,7 @@ func (c *Config) validate() error { // An MCP block with a typo (no name, both command and url, a bare hostname // as the url) fails here, at startup, rather than at the first turn that // needed the tool. - if err := mcp.Validate(c.MCPServers()); err != nil { + if err := mcp.Validate(c.allMCPServers()); err != nil { return err } // Same for the house: a missing token or a bare hostname fails at startup, @@ -1409,6 +1466,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/mcp_test.go b/internal/config/mcp_test.go index 48e05e2..e11de02 100644 --- a/internal/config/mcp_test.go +++ b/internal/config/mcp_test.go @@ -26,14 +26,55 @@ func TestMCPDisabledServerIsOff(t *testing.T) { if err != nil { t.Fatal(err) } - if c.MCP != nil { - t.Errorf("a block with nothing enabled must normalise to nil, got %+v", c.MCP) - } if got := c.MCPServers(); len(got) != 0 { t.Errorf("MCPServers() = %+v", got) } } +// A block with no servers at all is the same as no block. +func TestMCPEmptyBlockNormalisesToNil(t *testing.T) { + c, err := Load(writeConfig(t, `{"mcp":{"servers":[]}}`)) + if err != nil { + t.Fatal(err) + } + if c.MCP != nil { + t.Errorf("mcp = %+v, want nil", c.MCP) + } +} + +// A server that is written but not switched on is still shape-checked. The +// review the config layer can give is the point of writing a block dark, and +// skipping it meant a typo only surfaced on the day it was enabled. +func TestMCPDisabledServerIsStillValidated(t *testing.T) { + cases := map[string]string{ + "both": `{"mcp":{"servers":[{"name":"a","command":"x","url":"http://a.test"}]}}`, + "bare host": `{"mcp":{"servers":[{"name":"a","url":"a.test"}]}}`, + "no name": `{"mcp":{"servers":[{"command":"x"}]}}`, + "duplicates": `{"mcp":{"servers":[{"name":"a","command":"x"},{"name":"a","command":"y"}]}}`, + } + for name, body := range cases { + t.Run(name, func(t *testing.T) { + if _, err := Load(writeConfig(t, body)); err == nil { + t.Fatal("a dark server with a typo must fail at startup") + } + }) + } +} + +// Headers carry a bearer token to a real remote server. +func TestMCPServerHeaders(t *testing.T) { + c, err := Load(writeConfig(t, `{"mcp":{"servers":[ + {"name":"remote","url":"https://mcp.example.test/mcp","enabled":true, + "headers":{"Authorization":"Bearer sekret"}}]}}`)) + if err != nil { + t.Fatal(err) + } + got := c.MCPServers() + if len(got) != 1 || got[0].Headers["Authorization"] != "Bearer sekret" { + t.Fatalf("headers not mapped: %+v", got) + } +} + func TestMCPEnabledServerMapping(t *testing.T) { c, err := Load(writeConfig(t, `{"mcp":{ "timeout":"5s", diff --git a/internal/config/senses_test.go b/internal/config/senses_test.go index d74b454..44ec12c 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 4494113..1584055 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -227,14 +227,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"` @@ -243,6 +245,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"` } @@ -278,9 +284,14 @@ type CaptureStartReq struct { // which it stops itself; the caller tells him, so a forgotten recording is his // own informed choice rather than a surprise. type CaptureStartResp struct { - Label string `json:"label,omitempty"` - Started time.Time `json:"started"` - MaxSeconds int `json:"max_seconds"` + Label string `json:"label,omitempty"` + Started time.Time `json:"started"` + // Token names THIS session. Every later append, stop and discard has to + // carry it. Without it the recorder is addressed by "whatever is running + // now", and a client whose session already ended on the duration cap goes on + // appending its microphone into the next session someone else started. + Token string `json:"token"` + MaxSeconds int `json:"max_seconds"` } // CaptureAppendReq — one chunk of audio for the running session. Refused with @@ -288,6 +299,9 @@ type CaptureStartResp struct { // makes an ambient path impossible: audio arriving at an idle core is dropped on // the floor, not buffered "just in case". type CaptureAppendReq struct { + // Token from CaptureStartResp. A frame for a session that already ended is + // refused rather than folded into whatever is running now. + Token string `json:"token"` Audio audio.Audio `json:"audio"` } @@ -306,15 +320,22 @@ type CaptureAppendResp struct { // flag rather than a separate method so the client that says "stop" and the // client that says "stop and forget" take the same path to the same session. type CaptureStopReq struct { - Discard bool `json:"discard,omitempty"` + // Token from CaptureStartResp. Stopping by "whatever is running" lets a + // late client end a recording it never started. + Token string `json:"token"` + Discard bool `json:"discard,omitempty"` } // CaptureStopResp — the finished capture. BlobID is the stored WAV, kept under // media.retention like any other blob and pruned with it. // -// A response with a Transcript and an empty Summary is a degraded success: the -// words exist, only the model failed. A response with a BlobID and neither is -// the audio surviving a transcription failure — the same id can be run again. +// A response with a Transcript and an empty Summary is the normal shape, not a +// failure: summarising a long meeting is a map-reduce of minutes, so stop +// answers with the words and the summary note is written afterwards. Summary is +// filled in only when it happened to be ready. A response with a BlobID and no +// transcript is the audio surviving a transcription failure — the same id can be +// run again by hand off the blob before media.retention prunes it — there is no +// capture method that takes a blob id, so this is not a re-run the wire offers. // Discarded is true when nothing was kept. type CaptureStopResp struct { BlobID string `json:"blob_id,omitempty"` diff --git a/internal/ipc/client.go b/internal/ipc/client.go index c3c0433..ed4a3d1 100644 --- a/internal/ipc/client.go +++ b/internal/ipc/client.go @@ -533,9 +533,10 @@ func (c *Client) CaptureAppend(ctx context.Context, req CaptureAppendReq) (Captu return r, nil } -// CaptureStop ends the session. Slow — it transcribes and summarises the whole -// recording — so pass a context with room. Set Discard to throw the recording -// away instead. +// CaptureStop ends the session. It transcribes the whole recording before +// answering, so pass a context with room; the summary is written afterwards by +// the daemon and is usually absent from the response. Set Discard to throw the +// recording away instead. Token comes from CaptureStart. func (c *Client) CaptureStop(ctx context.Context, req CaptureStopReq) (CaptureStopResp, error) { var r CaptureStopResp if err := c.call(ctx, MethodCaptureStop, req, &r); err != nil { diff --git a/internal/ipc/server.go b/internal/ipc/server.go index ba01318..179f697 100644 --- a/internal/ipc/server.go +++ b/internal/ipc/server.go @@ -498,10 +498,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 diff --git a/internal/mcp/allowlist.go b/internal/mcp/allowlist.go index 09c9d06..235b0c5 100644 --- a/internal/mcp/allowlist.go +++ b/internal/mcp/allowlist.go @@ -1,7 +1,11 @@ package mcp import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" "regexp" + "strconv" "strings" ) @@ -59,3 +63,36 @@ func LocalName(server, tool string) string { // Scope is the store scope for a server's rows, so the /tools page can group // them and a human can tell at a glance where a capability came from. func Scope(server string) string { return "mcp:" + server } + +// Fingerprint is the declared shape of a discovered tool: its name, its +// description, its input schema and its readOnlyHint, hashed. +// +// It exists because an allowlist row cannot pin an MCP tool's behaviour. The +// row's cmd is ["mcp", server, tool], a reference to a name the REMOTE server +// owns and may redefine — the row does not have to change for the tool to +// become something else. The fingerprint is what Kami actually approved, so a +// later discovery can tell "same tool" from "same name". +// +// The schema is canonicalised through a decode and re-encode, so a server that +// reorders its JSON keys or changes its whitespace does not read as a +// redefinition. Unparseable schema bytes are hashed as they arrived. +func Fingerprint(t Tool) string { + schema := "" + if len(t.InputSchema) > 0 { + var any any + if json.Unmarshal(t.InputSchema, &any) == nil { + if raw, err := json.Marshal(any); err == nil { + schema = string(raw) + } + } + if schema == "" { + schema = string(t.InputSchema) + } + } + h := sha256.New() + for _, part := range []string{t.Name, t.Description, schema, strconv.FormatBool(t.ReadOnly)} { + h.Write([]byte(part)) + h.Write([]byte{0}) + } + return hex.EncodeToString(h.Sum(nil)) +} diff --git a/internal/mcp/allowlist_test.go b/internal/mcp/allowlist_test.go new file mode 100644 index 0000000..fd74ae2 --- /dev/null +++ b/internal/mcp/allowlist_test.go @@ -0,0 +1,32 @@ +package mcp + +import ( + "encoding/json" + "testing" +) + +// The fingerprint must cover everything the approval was given for, and must +// not move when only the JSON spelling of the schema does. +func TestFingerprintCoversTheDeclaredShape(t *testing.T) { + base := Tool{Name: "list_tasks", Description: "list them", ReadOnly: true, + InputSchema: json.RawMessage(`{"type":"object","properties":{}}`)} + same := base + same.InputSchema = json.RawMessage("{\n \"properties\": {},\n \"type\": \"object\"\n}") + if Fingerprint(base) != Fingerprint(same) { + t.Error("reformatting the schema must not read as a redefinition") + } + for name, mut := range map[string]func(*Tool){ + "description": func(x *Tool) { x.Description = "delete them" }, + "schema": func(x *Tool) { x.InputSchema = json.RawMessage(`{"required":["id"]}`) }, + "readonly": func(x *Tool) { x.ReadOnly = false }, + "name": func(x *Tool) { x.Name = "delete_tasks" }, + } { + t.Run(name, func(t *testing.T) { + got := base + mut(&got) + if Fingerprint(got) == Fingerprint(base) { + t.Error("a redefinition must change the fingerprint") + } + }) + } +} diff --git a/internal/mcp/http.go b/internal/mcp/http.go index a717266..c4d96e4 100644 --- a/internal/mcp/http.go +++ b/internal/mcp/http.go @@ -35,13 +35,14 @@ type PostResponse struct { type httpTransport struct { poster Poster url string + extra map[string]string // static headers, e.g. an Authorization bearer mu sync.Mutex session string // Mcp-Session-Id, echoed back when the server issues one } -func newHTTPTransport(post Poster, endpoint string) *httpTransport { - return &httpTransport{poster: post, url: endpoint} +func newHTTPTransport(post Poster, endpoint string, extra map[string]string) *httpTransport { + return &httpTransport{poster: post, url: endpoint, extra: extra} } func (t *httpTransport) Call(ctx context.Context, req *rpcRequest) (*rpcResponse, error) { @@ -49,7 +50,7 @@ func (t *httpTransport) Call(ctx context.Context, req *rpcRequest) (*rpcResponse if err != nil { return nil, err } - frame, err := decodeFrame(body) + frame, err := decodeFrame(body, req.ID) if err != nil { return nil, err } @@ -57,6 +58,18 @@ func (t *httpTransport) Call(ctx context.Context, req *rpcRequest) (*rpcResponse if err := json.Unmarshal(frame, &resp); err != nil { return nil, fmt.Errorf("mcp: decode response: %w", err) } + // The id check the stdio transport already did. Without it a server that + // sends a request of its own (sampling/createMessage, roots/list) mid-stream + // has that request accepted as the answer: it unmarshals into an rpcResponse + // with neither result nor error, and the call reports success with nothing + // in it. An empty string and no error is the one answer that lies — the act + // is logged as done and the tool never ran. + if resp.ID == nil || *resp.ID != req.ID { + return nil, fmt.Errorf("mcp: response id mismatch (wanted %d)", req.ID) + } + if resp.Error == nil && len(resp.Result) == 0 { + return nil, errors.New("mcp: response carries neither result nor error") + } return &resp, nil } @@ -71,7 +84,14 @@ func (t *httpTransport) send(ctx context.Context, req *rpcRequest) ([]byte, erro if err != nil { return nil, err } - hdr := map[string]string{"Accept": "application/json, text/event-stream"} + hdr := map[string]string{} + // Configured headers first, so nothing here can be overwritten by them: + // a real remote server needs a bearer token, and the Vikunja one on + // loopback is only reachable without one because it is unauthenticated. + for k, v := range t.extra { + hdr[k] = v + } + hdr["Accept"] = "application/json, text/event-stream" t.mu.Lock() if t.session != "" { hdr["Mcp-Session-Id"] = t.session @@ -114,9 +134,10 @@ func headerGet(h map[string]string, key string) string { } // decodeFrame pulls the JSON object out of a body that is either raw JSON or -// SSE. For SSE we take the LAST data: payload that parses, which is the -// response — earlier frames on the stream are progress notifications. -func decodeFrame(body []byte) ([]byte, error) { +// SSE. For SSE we take the last data: payload that parses AND carries our own +// id with a result or an error in it. Matching on the presence of an "id" key +// alone is not enough: a JSON-RPC request from the server has one too. +func decodeFrame(body []byte, id int64) ([]byte, error) { trimmed := bytes.TrimSpace(body) if len(trimmed) == 0 { return nil, errors.New("mcp: empty response body") @@ -136,19 +157,28 @@ func decodeFrame(body []byte) ([]byte, error) { if payload == "" { continue } - var probe map[string]json.RawMessage + var probe struct { + ID *int64 `json:"id"` + Result json.RawMessage `json:"result"` + Error json.RawMessage `json:"error"` + Method string `json:"method"` + } if json.Unmarshal([]byte(payload), &probe) != nil { continue } - if _, isResp := probe["id"]; isResp { - last = []byte(payload) + if probe.Method != "" || probe.ID == nil || *probe.ID != id { + continue } + if len(probe.Result) == 0 && len(probe.Error) == 0 { + continue + } + last = []byte(payload) } if err := sc.Err(); err != nil { return nil, fmt.Errorf("mcp: read event stream: %w", err) } if last == nil { - return nil, errors.New("mcp: no JSON-RPC response in event stream") + return nil, fmt.Errorf("mcp: no JSON-RPC response for id %d in event stream", id) } return last, nil } diff --git a/internal/mcp/manager.go b/internal/mcp/manager.go index cec97e9..d8a7dc4 100644 --- a/internal/mcp/manager.go +++ b/internal/mcp/manager.go @@ -24,12 +24,31 @@ const ( // wrong act. Twelve per server is already generous. DefaultMaxTools = 12 // DefaultReconnectEvery is how long the manager waits before re-dialing a - // server whose connection died. + // server whose connection died. It is the FIRST wait: every consecutive + // failure doubles it, up to MaxReconnectEvery. DefaultReconnectEvery = 30 * time.Second + // MaxReconnectEvery caps the backoff. Without one, a permanently + // misconfigured stdio server is exec'd once a minute forever, which is a + // process spawn per minute in the logs and nothing that ever gets better. + MaxReconnectEvery = 30 * time.Minute + // DefaultMaxDescription bounds one tool description. It is written by a + // server Maven does not control and it lands in two places that cannot + // absorb an arbitrary blob: the resident model's 4096-token context, and a + // table cell on /tools. + DefaultMaxDescription = 400 ) -// ErrNoServer — the named server is not configured or not connected. -var ErrNoServer = errors.New("mcp: no such server") +var ( + // ErrNoServer — the named server is not configured. + ErrNoServer = errors.New("mcp: no such server") + // ErrNotConnected — the server is configured but nothing is dialed. Held + // apart from ErrNoServer so a caller can say "that tool is not connected" + // instead of drafting a proposal for a tool that already exists. + ErrNotConnected = errors.New("mcp: server is not connected") + // ErrToolGone — the server no longer offers this tool. An enabled row can + // outlive the tool it names; this is what the act path sees when it does. + ErrToolGone = errors.New("mcp: server no longer offers this tool") +) // ServerConfig is one configured MCP server. Off unless present. // @@ -61,6 +80,10 @@ type ServerConfig struct { AllowTools []string `json:"allow_tools,omitempty"` // MaxTools caps the contribution (0 ⇒ DefaultMaxTools). MaxTools int `json:"max_tools,omitempty"` + // Headers are sent verbatim on every request to a url server. This is how + // a bearer token reaches a real remote server; the Vikunja one on loopback + // needs none only because it is unauthenticated. + Headers map[string]string `json:"-"` // Timeout bounds one call (0 ⇒ DefaultTimeout). Timeout time.Duration `json:"-"` // Enabled=false keeps a configured server described but dark. @@ -90,6 +113,20 @@ type conn struct { lastErr error lastTry time.Time dialedAt time.Time + fails int // consecutive dial failures, for the backoff +} + +// backoff is how long this connection waits before the next dial attempt: +// DefaultReconnectEvery doubled per consecutive failure, capped. +func (c *conn) backoff() time.Duration { + d := DefaultReconnectEvery + for i := 1; i < c.fails && d < MaxReconnectEvery; i++ { + d *= 2 + } + if d > MaxReconnectEvery { + d = MaxReconnectEvery + } + return d } // NewManager builds a manager for the enabled servers in cfgs. newPoster is @@ -202,7 +239,7 @@ func (m *Manager) dial(ctx context.Context, name string) error { } else { var poster Poster if poster, err = m.newPoster(cfg); err == nil { - tr = newHTTPTransport(poster, cfg.URL) + tr = newHTTPTransport(poster, cfg.URL, cfg.Headers) } } if err != nil { @@ -233,6 +270,7 @@ func (m *Manager) dial(ctx context.Context, name string) error { m.conns[name].client = cl m.conns[name].tools = tools m.conns[name].lastErr = nil + m.conns[name].fails = 0 m.conns[name].dialedAt = time.Now() m.mu.Unlock() log.Printf("mcp: %s connected (%s %s), %d tool(s)", name, cl.Info().Name, cl.Info().Version, len(tools)) @@ -246,12 +284,19 @@ func (m *Manager) fail(name string, err error) { c.lastErr = err c.client = nil c.tools = nil + c.fails++ } } -// filterTools applies AllowTools and MaxTools, and drops nameless entries. -// Sorted first, so the cap is deterministic rather than "whatever order the -// server felt like". +// filterTools applies AllowTools and MaxTools, drops nameless entries and +// truncates descriptions. +// +// Over the cap WITHOUT allow_tools, the whole contribution is dropped. Taking +// the first N of a sorted list was deterministic but it handed the choice of +// which N to the server: a thirteenth tool named "aaa_" would push a tool that +// had already been discovered, proposed and maybe enabled out of the +// catalogue. Determinism was not the property worth buying. With allow_tools +// set, Kami named the tools, so the cap trims a list he chose. func filterTools(cfg ServerConfig, in []Tool) []Tool { sort.Slice(in, func(i, j int) bool { return in[i].Name < in[j].Name }) out := make([]Tool, 0, len(in)) @@ -259,16 +304,32 @@ func filterTools(cfg ServerConfig, in []Tool) []Tool { if len(cfg.AllowTools) > 0 && !contains(cfg.AllowTools, t.Name) { continue } + t.Description = truncate(t.Description, DefaultMaxDescription) out = append(out, t) } if cfg.MaxTools > 0 && len(out) > cfg.MaxTools { - log.Printf("mcp: %s offers %d tools, taking the first %d (raise max_tools or set allow_tools)", - cfg.Name, len(out), cfg.MaxTools) + if len(cfg.AllowTools) == 0 { + log.Printf("mcp: %s offers %d tools, over the cap of %d — taking NONE of them, set allow_tools to choose or raise max_tools", + cfg.Name, len(out), cfg.MaxTools) + return nil + } + log.Printf("mcp: %s: allow_tools names %d tools, over the cap of %d — taking the first %d", + cfg.Name, len(out), cfg.MaxTools, cfg.MaxTools) out = out[:cfg.MaxTools] } return out } +// truncate bounds a server-written string. The ellipsis is there so a reader +// on /tools can tell the text was cut rather than written that way. +func truncate(s string, max int) string { + r := []rune(s) + if len(r) <= max { + return s + } + return strings.TrimSpace(string(r[:max])) + "…" +} + func contains(hay []string, needle string) bool { for _, h := range hay { if h == needle { @@ -281,18 +342,33 @@ func contains(hay []string, needle string) bool { // Refresh re-dials any server that is down, if enough time has passed since the // last attempt. Call it from the daemon's periodic tick — it is cheap when // everything is up. +// The health check itself is done OUTSIDE m.mu, the way Resources already +// does it. alive() reaches into the transport, and a transport waiting on a +// silent subprocess would otherwise hold m.mu for as long as it waits, which +// blocks Tools, Status and Call for every other server too. func (m *Manager) Refresh(ctx context.Context) { now := time.Now() - var stale []string + type candidate struct { + name string + cl *Client + ready bool + } + var cands []candidate m.mu.Lock() for _, name := range m.order { c := m.conns[name] - down := c.client == nil || !c.client.alive() - if down && now.Sub(c.lastTry) >= DefaultReconnectEvery { - stale = append(stale, name) - } + cands = append(cands, candidate{name: name, cl: c.client, ready: now.Sub(c.lastTry) >= c.backoff()}) } m.mu.Unlock() + var stale []string + for _, c := range cands { + if !c.ready { + continue + } + if c.cl == nil || !c.cl.alive() { + stale = append(stale, c.name) + } + } for _, name := range stale { if err := m.dial(ctx, name); err != nil { log.Printf("mcp: %s: reconnect: %v", name, err) @@ -312,6 +388,21 @@ func (m *Manager) Tools() []Tool { return out } +// Connected — the names of servers that are dialed right now. A caller that +// wants to act on a tool's ABSENCE needs this: a tool missing from Tools() +// because its server is down is not a tool the server withdrew. +func (m *Manager) Connected() []string { + m.mu.Lock() + defer m.mu.Unlock() + var out []string + for _, name := range m.order { + if m.conns[name].client != nil { + out = append(out, name) + } + } + return out +} + // Status is one server's health, for the web surface. type Status struct { Name string @@ -367,13 +458,13 @@ func (m *Manager) Call(ctx context.Context, server, tool string, args map[string } m.mu.Unlock() if cl == nil { - return "", fmt.Errorf("mcp: %s is not connected", server) + return "", fmt.Errorf("%w: %s", ErrNotConnected, server) } // The discovered-and-filtered set is the second allowlist: even an enabled // store row cannot reach a tool the server stopped offering, or one // allow_tools excludes. if !known { - return "", fmt.Errorf("mcp: %s offers no tool %q", server, tool) + return "", fmt.Errorf("%w: %s/%s", ErrToolGone, server, tool) } cctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() @@ -442,8 +533,8 @@ var ErrNeedsArgs = errors.New("mcp: tool needs named arguments") // // - a tool with no required properties runs with no arguments (a spare tail // is ignored — "покажи проекты пожалуйста" should still list projects); -// - a READ-ONLY tool with exactly one required property, of type string or -// integer/number, gets the tail bound to it; +// - a READ-ONLY tool NAMED IN allow_tools, with exactly one required +// property, of type string or integer/number, gets the tail bound to it; // - anything else is refused with ErrNeedsArgs. Such a tool is still callable // with explicit arguments from the authed surface, where a human types // them. @@ -456,32 +547,54 @@ var ErrNeedsArgs = errors.New("mcp: tool needs named arguments") // partially-filled write can do is destroy what it did not mention. A mutating // tool with nothing required is still fine: nothing was guessed, and it still // goes through the confirm turn. +// +// The allow_tools condition is the second half, and it is there because +// readOnlyHint is the SERVER's claim about itself. It already buys one +// exemption (destructive=false, so no confirm turn); letting it buy argument +// binding as well means one lie converts a spoken utterance into an +// unconfirmed, argument-carrying write. A server advertising delete_project +// with readOnlyHint true and the description "show a project and its tasks" +// would be enough. So the binding half rests on something local instead: a +// name Kami typed into mavend.json. The tool name is not a defence — the +// router picks tools by name similarity and the description a human reads is +// server-written too. func (m *Manager) CallPositional(ctx context.Context, server, tool string, args []string) (string, error) { m.mu.Lock() c := m.conns[server] var schema json.RawMessage - found, readOnly := false, false + found, readOnly, bindable := false, false, false + configured, connected := c != nil, false if c != nil { + connected = c.client != nil for _, t := range c.tools { if t.Name == tool { schema, readOnly, found = t.InputSchema, t.ReadOnly, true + bindable = contains(c.cfg.AllowTools, tool) break } } } m.mu.Unlock() if !found { - return "", fmt.Errorf("mcp: %s offers no tool %q", server, tool) + if !configured { + return "", fmt.Errorf("%w: %s", ErrNoServer, server) + } + if !connected { + return "", fmt.Errorf("%w: %s", ErrNotConnected, server) + } + return "", fmt.Errorf("%w: %s/%s", ErrToolGone, server, tool) } - named, err := bindPositional(schema, args, readOnly) + named, err := bindPositional(schema, args, readOnly && bindable) if err != nil { return "", err } return m.Call(ctx, server, tool, named) } -// bindPositional implements the rule documented on CallPositional. -func bindPositional(schema json.RawMessage, args []string, readOnly bool) (map[string]any, error) { +// bindPositional implements the rule documented on CallPositional. bind is the +// caller's verdict on whether a guessed argument is allowed at all: read-only +// AND named in allow_tools. +func bindPositional(schema json.RawMessage, args []string, bind bool) (map[string]any, error) { var s struct { Required []string `json:"required"` Properties map[string]struct { @@ -498,14 +611,20 @@ func bindPositional(schema json.RawMessage, args []string, readOnly bool) (map[s return map[string]any{}, nil case 1: name := s.Required[0] - if !readOnly { - return nil, fmt.Errorf("%w: %q, and a tool that writes never gets a guessed one", ErrNeedsArgs, name) + prop, described := s.Properties[name] + if !described { + // required names it, properties does not describe it. The zero + // value would make it a string, which is a guess about a guess. + return nil, fmt.Errorf("%w: %q, which the schema never describes", ErrNeedsArgs, name) + } + if !bind { + return nil, fmt.Errorf("%w: %q, and a guessed argument goes only to a read-only tool named in allow_tools", ErrNeedsArgs, name) } tail := strings.TrimSpace(strings.Join(args, " ")) if tail == "" { return nil, fmt.Errorf("%w: %q", ErrNeedsArgs, name) } - switch s.Properties[name].Type { + switch prop.Type { case "string", "": return map[string]any{name: tail}, nil case "integer", "number": @@ -515,7 +634,7 @@ func bindPositional(schema json.RawMessage, args []string, readOnly bool) (map[s } return map[string]any{name: n}, nil default: - return nil, fmt.Errorf("%w: %q is a %s", ErrNeedsArgs, name, s.Properties[name].Type) + return nil, fmt.Errorf("%w: %q is a %s", ErrNeedsArgs, name, prop.Type) } default: return nil, fmt.Errorf("%w: %s", ErrNeedsArgs, strings.Join(s.Required, ", ")) diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index 0888e25..9c8c966 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -3,6 +3,7 @@ package mcp import ( "context" "encoding/json" + "errors" "fmt" "strings" "sync" @@ -102,7 +103,7 @@ func echoServer() func(string, json.RawMessage) (any, *rpcError) { func dialFake(t *testing.T, p *fakePoster) *Client { t.Helper() - c := newClient("fake", newHTTPTransport(p, "http://example.test/mcp")) + c := newClient("fake", newHTTPTransport(p, "http://example.test/mcp", nil)) if err := c.Initialize(context.Background()); err != nil { t.Fatalf("initialize: %v", err) } @@ -190,7 +191,7 @@ func TestResources(t *testing.T) { } func TestCallBeforeInitializeRefused(t *testing.T) { - c := newClient("fake", newHTTPTransport(&fakePoster{handler: echoServer()}, "http://example.test/mcp")) + c := newClient("fake", newHTTPTransport(&fakePoster{handler: echoServer()}, "http://example.test/mcp", nil)) if _, err := c.CallTool(context.Background(), "read_thing", nil); err != ErrNotInitialized { t.Fatalf("err = %v, want ErrNotInitialized", err) } @@ -217,7 +218,7 @@ func TestHandshakeWithoutProtocolVersionRefused(t *testing.T) { p := &fakePoster{handler: func(m string, _ json.RawMessage) (any, *rpcError) { return map[string]any{"serverInfo": map[string]any{"name": "not-mcp"}}, nil }} - c := newClient("x", newHTTPTransport(p, "http://example.test/mcp")) + c := newClient("x", newHTTPTransport(p, "http://example.test/mcp", nil)) if err := c.Initialize(context.Background()); err == nil { t.Fatal("a reply with no protocolVersion is not an MCP server") } @@ -245,8 +246,8 @@ func TestDecodeFrame(t *testing.T) { name, in, want string wantErr bool }{ - {name: "plain json", in: `{"id":1,"result":{}}`, want: `{"id":1,"result":{}}`}, - {name: "sse single", in: "event: message\ndata: {\"id\":1,\"result\":1}\n\n", want: `{"id":1,"result":1}`}, + {name: "plain json", in: `{"id":2,"result":{}}`, want: `{"id":2,"result":{}}`}, + {name: "sse single", in: "event: message\ndata: {\"id\":2,\"result\":1}\n\n", want: `{"id":2,"result":1}`}, { name: "sse picks the response not the notification", in: "data: {\"method\":\"notifications/progress\"}\n\ndata: {\"id\":2,\"result\":2}\n\n", @@ -254,10 +255,24 @@ func TestDecodeFrame(t *testing.T) { }, {name: "empty", in: " ", wantErr: true}, {name: "sse with no response", in: "data: {\"method\":\"x\"}\n\n", wantErr: true}, + { + // A JSON-RPC REQUEST from the server has an id too. Taking it as + // the response gave a frame with neither result nor error, which + // the client reported as an empty success: the act logged as done + // and the tool never run. + name: "sse server request is not a response", + in: "data: {\"id\":2,\"method\":\"sampling/createMessage\",\"params\":{}}\n\n", + wantErr: true, + }, + { + name: "sse response for another id", + in: "data: {\"id\":9,\"result\":1}\n\n", + wantErr: true, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got, err := decodeFrame([]byte(tc.in)) + got, err := decodeFrame([]byte(tc.in), 2) if tc.wantErr { if err == nil { t.Fatalf("want error, got %q", got) @@ -368,10 +383,19 @@ func TestManagerAllowToolsAndMaxTools(t *testing.T) { if _, err := m.Call(context.Background(), "fake", "break_thing", nil); err == nil { t.Error("a tool excluded by allow_tools must be unreachable") } + // Over the cap with no allow_tools: NOTHING is taken. Trimming a sorted + // list handed the server the choice of which tools survive — a new tool + // named "aaa_" would push an already-approved one out of the catalogue. m2 := mk(ServerConfig{MaxTools: 1}) defer m2.Close() - if got := m2.Tools(); len(got) != 1 || got[0].Name != "break_thing" { - t.Fatalf("max_tools should keep the first name-sorted tool: %+v", got) + if got := m2.Tools(); len(got) != 0 { + t.Fatalf("over the cap without allow_tools must contribute nothing, got %+v", got) + } + // With allow_tools, Kami chose the list, so the cap trims his list. + m3 := mk(ServerConfig{MaxTools: 1, AllowTools: []string{"break_thing", "read_thing"}}) + defer m3.Close() + if got := m3.Tools(); len(got) != 1 || got[0].Name != "break_thing" { + t.Fatalf("max_tools over allow_tools: %+v", got) } } @@ -563,3 +587,57 @@ func TestCallPositionalThroughManager(t *testing.T) { t.Error("an unknown tool must be refused") } } + +// A guessed argument may only be bound for a tool Kami named in allow_tools. +// readOnlyHint alone was the old rule, and readOnlyHint is written by the same +// server that named the tool: a server advertising delete_project as read-only +// got an unconfirmed argument-carrying call. +func TestBindPositionalNeedsAllowTools(t *testing.T) { + schema := json.RawMessage(`{"required":["query"],"properties":{"query":{"type":"string"}}}`) + if _, err := bindPositional(schema, []string{"tea"}, false); !errors.Is(err, ErrNeedsArgs) { + t.Fatalf("err = %v, want ErrNeedsArgs when the tool is not in allow_tools", err) + } + got, err := bindPositional(schema, []string{"tea"}, true) + if err != nil || got["query"] != "tea" { + t.Fatalf("bind = %v, %v", got, err) + } +} + +// required names a property the schema never describes. Falling through to the +// zero value made it a string, which is a guess about a guess. +func TestBindPositionalRefusesUndescribedProperty(t *testing.T) { + schema := json.RawMessage(`{"required":["query"],"properties":{}}`) + _, err := bindPositional(schema, []string{"tea"}, true) + if !errors.Is(err, ErrNeedsArgs) { + t.Fatalf("err = %v, want ErrNeedsArgs", err) + } + if !strings.Contains(err.Error(), "never describes") { + t.Fatalf("err = %v, want it to name the schema gap", err) + } +} + +// A server that cannot be dialled must be retried more and more slowly. At a +// flat one minute a permanently misconfigured stdio server was re-exec'd 1440 +// times a day forever. +func TestReconnectBackoffGrows(t *testing.T) { + c := &conn{} + prev := time.Duration(0) + for i := 1; i <= 12; i++ { + c.fails = i + d := c.backoff() + if d < prev { + t.Fatalf("backoff shrank at %d failures: %v after %v", i, d, prev) + } + if d > MaxReconnectEvery { + t.Fatalf("backoff %v exceeds the cap %v", d, MaxReconnectEvery) + } + prev = d + } + if prev != MaxReconnectEvery { + t.Fatalf("backoff never reached the cap: %v", prev) + } + c.fails = 1 + if c.backoff() != DefaultReconnectEvery { + t.Fatalf("first retry = %v, want %v", c.backoff(), DefaultReconnectEvery) + } +} diff --git a/internal/mcp/stdio.go b/internal/mcp/stdio.go index d597875..86e60e3 100644 --- a/internal/mcp/stdio.go +++ b/internal/mcp/stdio.go @@ -2,6 +2,7 @@ package mcp import ( "bufio" + "bytes" "context" "encoding/json" "errors" @@ -9,12 +10,17 @@ import ( "io" "os" "os/exec" - "strings" "sync" ) // maxLine bounds one JSON-RPC frame from a subprocess. A tool result bigger // than this is a misbehaving server, not something to buffer. +// +// The bound is enforced by bufio.Scanner's own buffer limit, not by measuring +// the line after it was assembled. Measuring afterwards is not a bound: a +// server that emits 500 MB with no newline would have all 500 MB in mavend's +// heap before the check could reject it, which on the deploy target is an OOM +// kill of the core daemon. const maxLine = 1 << 20 // 1 MiB // stdioTransport speaks newline-delimited JSON-RPC to a child process. This is @@ -23,12 +29,27 @@ const maxLine = 1 << 20 // 1 MiB // // Args are argv, never a shell string — the same discipline internal/tool // keeps, for the same reason. +// +// Reading happens on its own goroutine, feeding frames down a channel. That is +// what makes a call abandonable: bufio never observes a context, so a server +// that accepts a request and then writes nothing at all would otherwise block +// the reader forever with the transport lock held, and every other server in +// the manager behind it. type stdioTransport struct { - mu sync.Mutex - cmd *exec.Cmd - in io.WriteCloser - out *bufio.Reader - dead bool + cmd *exec.Cmd + in io.WriteCloser + lines chan []byte + stop chan struct{} // closed by Close, so the reader can give up + + // callMu serialises whole calls, so two callers cannot consume each + // other's frames off the shared channel. It is deliberately NOT the lock + // alive() takes: a hung call must not make the manager's health check + // block on it. + callMu sync.Mutex + + mu sync.Mutex + dead bool + readErr error } func newStdioTransport(ctx context.Context, argv []string, env []string, dir string) (*stdioTransport, error) { @@ -52,38 +73,92 @@ func newStdioTransport(ctx context.Context, argv []string, env []string, dir str if err := cmd.Start(); err != nil { return nil, fmt.Errorf("mcp: start %q: %w", argv[0], err) } - return &stdioTransport{cmd: cmd, in: in, out: bufio.NewReaderSize(out, 64<<10)}, nil + t := &stdioTransport{ + cmd: cmd, + in: in, + lines: make(chan []byte), + stop: make(chan struct{}), + } + go t.readLoop(out) + return t, nil +} + +// readLoop pushes one frame per line onto t.lines until the pipe ends. The +// scanner's own buffer limit is the frame bound: a line longer than maxLine +// ends the scan with bufio.ErrTooLong having buffered at most maxLine, rather +// than assembling the whole thing first and rejecting it afterwards. +func (t *stdioTransport) readLoop(out io.Reader) { + defer close(t.lines) + sc := bufio.NewScanner(out) + sc.Buffer(make([]byte, 0, 64<<10), maxLine) + for sc.Scan() { + line := bytes.TrimSpace(sc.Bytes()) + if len(line) == 0 { + continue + } + frame := append([]byte(nil), line...) + select { + case t.lines <- frame: + case <-t.stop: + return + } + } + err := sc.Err() + switch { + case errors.Is(err, bufio.ErrTooLong): + err = fmt.Errorf("mcp: frame exceeds %d bytes", maxLine) + case err == nil: + err = io.EOF + } + t.mu.Lock() + t.readErr = err + t.mu.Unlock() } func (t *stdioTransport) Call(ctx context.Context, req *rpcRequest) (*rpcResponse, error) { - t.mu.Lock() - defer t.mu.Unlock() - if t.dead { + t.callMu.Lock() + defer t.callMu.Unlock() + if !t.alive() { return nil, ErrClosed } - if err := t.write(req); err != nil { - t.dead = true + t.mu.Lock() + err := t.write(req) + t.mu.Unlock() + if err != nil { + _ = t.Close() return nil, err } // Read until the frame with our id turns up; anything else on the pipe is // a notification or a server-initiated request we do not answer. for { - if err := ctx.Err(); err != nil { - return nil, err + select { + case <-ctx.Done(): + // A server that took the request and answered nothing is not a + // server this connection can be reused with: the next call would + // read into a pipe whose state we no longer know. Drop it and let + // the manager re-dial. + _ = t.Close() + return nil, ctx.Err() + case line, ok := <-t.lines: + if !ok { + t.mu.Lock() + rerr := t.readErr + t.mu.Unlock() + _ = t.Close() + if rerr == nil { + rerr = ErrClosed + } + return nil, fmt.Errorf("mcp: read: %w", rerr) + } + var resp rpcResponse + if err := json.Unmarshal(line, &resp); err != nil { + continue // not a response frame; ignore rather than break the turn + } + if resp.ID == nil || *resp.ID != req.ID { + continue + } + return &resp, nil } - line, err := t.readLine() - if err != nil { - t.dead = true - return nil, err - } - var resp rpcResponse - if err := json.Unmarshal(line, &resp); err != nil { - continue // not a response frame; ignore rather than break the turn - } - if resp.ID == nil || *resp.ID != req.ID { - continue - } - return &resp, nil } } @@ -96,6 +171,7 @@ func (t *stdioTransport) Notify(ctx context.Context, method string, params any) return t.write(&rpcRequest{JSONRPC: "2.0", Method: method, Params: params}) } +// write must be called with t.mu held. func (t *stdioTransport) write(req *rpcRequest) error { req.JSONRPC = "2.0" raw, err := json.Marshal(req) @@ -108,31 +184,20 @@ func (t *stdioTransport) write(req *rpcRequest) error { return nil } -func (t *stdioTransport) readLine() ([]byte, error) { - for { - line, err := t.out.ReadString('\n') - if err != nil { - if len(strings.TrimSpace(line)) == 0 { - return nil, fmt.Errorf("mcp: read: %w", err) - } - return []byte(line), nil - } - if len(line) > maxLine { - return nil, fmt.Errorf("mcp: frame exceeds %d bytes", maxLine) - } - if s := strings.TrimSpace(line); s != "" { - return []byte(s), nil - } - } -} - +// Close is idempotent: a call that abandoned a silent pipe calls it, and so +// does the manager. func (t *stdioTransport) Close() error { t.mu.Lock() - defer t.mu.Unlock() + if t.dead { + t.mu.Unlock() + return nil + } t.dead = true + close(t.stop) if t.in != nil { _ = t.in.Close() } + t.mu.Unlock() if t.cmd.Process != nil { _ = t.cmd.Process.Kill() _ = t.cmd.Wait() @@ -145,5 +210,8 @@ func (t *stdioTransport) Close() error { func (t *stdioTransport) alive() bool { t.mu.Lock() defer t.mu.Unlock() - return !t.dead + if t.dead { + return false + } + return t.readErr == nil } diff --git a/internal/mcp/stdio_test.go b/internal/mcp/stdio_test.go index 8e43b59..b75b47d 100644 --- a/internal/mcp/stdio_test.go +++ b/internal/mcp/stdio_test.go @@ -9,6 +9,7 @@ import ( "os/exec" "strings" "testing" + "time" ) // The stdio transport is tested against a real subprocess — this test binary, @@ -47,6 +48,20 @@ func fakeStdioServer() { _ = out.Flush() continue } + // "mute" answers the handshake and then goes silent: a python server + // that hit an unhandled exception in its own read loop but did not + // exit is the ordinary way to get here. + if os.Getenv("MAVEN_MCP_FAKE") == "mute" && req.Method == "tools/call" { + select {} // never answer, never exit + } + // "flood" writes one enormous line with no newline in it. + if os.Getenv("MAVEN_MCP_FAKE") == "flood" && req.Method == "tools/call" { + for i := 0; i < 64; i++ { + _, _ = out.Write(make([]byte, 1<<20)) + } + _ = out.Flush() + continue + } result, rerr := h(req.Method, req.Params) resp := map[string]any{"jsonrpc": "2.0", "id": *req.ID} if rerr != nil { @@ -147,3 +162,55 @@ func TestStdioMissingCommand(t *testing.T) { t.Logf("err = %q", st[0].Err) } } + +// A stdio server that accepts a call and then answers nothing must not wedge +// the manager. Before the read moved onto its own goroutine, the read held the +// transport lock, Refresh took that lock through alive() while holding the +// manager lock, and from then on Tools, Status and Call blocked for EVERY +// server — including turns that touch no MCP tool at all. +func TestStdioSilentServerDoesNotWedgeTheManager(t *testing.T) { + m := stdioManager(t, "mute") + defer m.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) + defer cancel() + done := make(chan error, 1) + go func() { + _, err := m.Call(ctx, "fake", "read_thing", nil) + done <- err + }() + select { + case err := <-done: + if err == nil { + t.Fatal("a call into a silent server must fail, not succeed") + } + case <-time.After(5 * time.Second): + t.Fatal("the call never returned: the context is not observed during the read") + } + + // The manager must still answer while (and after) that call was stuck. + ready := make(chan struct{}) + go func() { + m.Refresh(context.Background()) + m.Tools() + m.Status() + close(ready) + }() + select { + case <-ready: + case <-time.After(5 * time.Second): + t.Fatal("Refresh/Tools/Status deadlocked behind the hung call") + } +} + +// One frame is bounded by the reader's buffer, not measured after the whole +// thing has already been assembled in mavend's heap. +func TestStdioOversizedFrameIsRefused(t *testing.T) { + m := stdioManager(t, "flood") + defer m.Close() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if _, err := m.Call(ctx, "fake", "read_thing", nil); err == nil { + t.Fatal("a 64 MiB frame must be refused") + } +} diff --git a/internal/mcp/webfetchdoor.go b/internal/mcp/webfetchdoor.go index 3a892a2..2623592 100644 --- a/internal/mcp/webfetchdoor.go +++ b/internal/mcp/webfetchdoor.go @@ -16,10 +16,20 @@ import ( // must not become a hole for some public endpoint that happens to redirect at // the LAN. Rate limiting is per fetcher too, which is the right shape here: // separate servers are separate hosts. +// +// A server WITH allow_private also gets redirects switched off. Across servers +// the per-fetcher split holds the line; within the one server that has the +// flag it did not, because allow_private disables the dialer guard on every +// hop: http://localhost:9100/mcp answering 302 to +// http://169.254.169.254/latest/meta-data/ was followed, up to MaxRedirects. A +// local MCP endpoint has no business redirecting, so refusing costs nothing. func WebfetchDoor(limits webfetch.Config) PosterFactory { return func(cfg ServerConfig) (Poster, error) { c := limits c.AllowPrivate = cfg.AllowPrivate + if cfg.AllowPrivate { + c.MaxRedirects = -1 // negative ⇒ no redirects followed + } if c.Timeout <= 0 && cfg.Timeout > 0 { c.Timeout = cfg.Timeout } diff --git a/internal/media/image.go b/internal/media/image.go index b798c3a..905eccc 100644 --- a/internal/media/image.go +++ b/internal/media/image.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "image" - "image/draw" "image/gif" "image/jpeg" "image/png" @@ -25,6 +24,19 @@ const DefaultMaxDim = 896 // original bytes stay in the blob store untouched. const JPEGQuality = 85 +// DefaultMaxPixels — the largest source image this build will decode, counted +// in pixels rather than in compressed bytes. A byte cap is not a memory bound +// for an image: a 20000x20000 PNG of flat colour compresses to a few hundred +// kilobytes and decodes to 400 million pixels, which is 1.6 GB of heap in the +// process that owns the database and the socket. 40 megapixels is well past any +// phone camera and two orders of magnitude short of an OOM. +const DefaultMaxPixels = 40 << 20 + +// ErrTooManyPixels — the image header declares more pixels than this build +// will decode. Separate from ErrUnsupportedImage because the format is fine and +// the size is not, and the log line should say which. +var ErrTooManyPixels = errors.New("media: image has too many pixels") + // ErrUnsupportedImage — the bytes are not an image format this build can // decode. Notably webp: the stdlib has no webp decoder and this repo takes no // new dependencies, so a webp arriving from Telegram is refused here with a @@ -91,6 +103,16 @@ func PrepareImage(data []byte, source string, maxDim int) (Image, error) { if err != nil { return Image{}, err } + // The header is read before the pixels. Deciding after the decode is not a + // decision: by then the whole bitmap is already in the heap. + cfg, err := decodeConfig(data, mime) + if err != nil { + return Image{}, fmt.Errorf("media: read %s header: %w", mime, err) + } + if px := int64(cfg.Width) * int64(cfg.Height); px > DefaultMaxPixels { + return Image{}, fmt.Errorf("%w: %dx%d is %d, cap is %d", + ErrTooManyPixels, cfg.Width, cfg.Height, px, int64(DefaultMaxPixels)) + } src, err := decode(data, mime) if err != nil { return Image{}, fmt.Errorf("media: decode %s: %w", mime, err) @@ -105,6 +127,19 @@ func PrepareImage(data []byte, source string, maxDim int) (Image, error) { return Image{JPEG: buf.Bytes(), Width: b.Dx(), Height: b.Dy(), Source: source}, nil } +func decodeConfig(data []byte, mime string) (image.Config, error) { + r := bytes.NewReader(data) + switch strings.ToLower(mime) { + case "image/jpeg": + return jpeg.DecodeConfig(r) + case "image/png": + return png.DecodeConfig(r) + case "image/gif": + return gif.DecodeConfig(r) + } + return image.Config{}, ErrUnsupportedImage +} + func decode(data []byte, mime string) (image.Image, error) { r := bytes.NewReader(data) switch strings.ToLower(mime) { @@ -123,18 +158,16 @@ func decode(data []byte, mime string) (image.Image, error) { // destination pixel — nearest-neighbour would alias small text into noise, // which defeats the point of reading a screenshot, and an area average is a // dozen lines against pulling in golang.org/x/image on an offline box. +// +// It reads the source through At and allocates only the destination. Flattening +// into a full-size RGBA first doubled the peak: a 40-megapixel photo already +// costs 160 MB decoded, and the intermediate made it 320 MB before MaxDim had +// any chance to help. func flattenAndScale(src image.Image, maxDim int) *image.RGBA { sb := src.Bounds() sw, sh := sb.Dx(), sb.Dy() dw, dh := fit(sw, sh, maxDim) - flat := image.NewRGBA(image.Rect(0, 0, sw, sh)) - draw.Draw(flat, flat.Bounds(), image.NewUniform(image.White), image.Point{}, draw.Src) - draw.Draw(flat, flat.Bounds(), src, sb.Min, draw.Over) - if dw == sw && dh == sh { - return flat - } - dst := image.NewRGBA(image.Rect(0, 0, dw, dh)) for y := 0; y < dh; y++ { y0, y1 := y*sh/dh, (y+1)*sh/dh @@ -146,20 +179,24 @@ func flattenAndScale(src image.Image, maxDim int) *image.RGBA { if x1 <= x0 { x1 = x0 + 1 } - var r, g, b, n uint32 + var r, g, b, n uint64 for sy := y0; sy < y1; sy++ { for sx := x0; sx < x1; sx++ { - i := flat.PixOffset(sx, sy) - r += uint32(flat.Pix[i]) - g += uint32(flat.Pix[i+1]) - b += uint32(flat.Pix[i+2]) + // At returns premultiplied 16-bit. Compositing over white + // is then c + (1-alpha), which is the same answer the + // draw.Over pass used to give, one pixel at a time. + cr, cg, cb, ca := src.At(sb.Min.X+sx, sb.Min.Y+sy).RGBA() + inv := uint64(0xFFFF - ca) + r += uint64(cr) + inv + g += uint64(cg) + inv + b += uint64(cb) + inv n++ } } o := dst.PixOffset(x, y) - dst.Pix[o] = uint8(r / n) - dst.Pix[o+1] = uint8(g / n) - dst.Pix[o+2] = uint8(b / n) + dst.Pix[o] = uint8(r / n >> 8) + dst.Pix[o+1] = uint8(g / n >> 8) + dst.Pix[o+2] = uint8(b / n >> 8) dst.Pix[o+3] = 0xFF } } diff --git a/internal/media/image_test.go b/internal/media/image_test.go index 4355d0c..fe2802b 100644 --- a/internal/media/image_test.go +++ b/internal/media/image_test.go @@ -2,7 +2,9 @@ package media import ( "bytes" + "encoding/binary" "errors" + "hash/crc32" "image" "image/color" "image/gif" @@ -189,3 +191,70 @@ func gifBytes(t *testing.T, w, h int) []byte { } return buf.Bytes() } + +// A decode bomb is a small file. Nothing bounded pixels before decoding, so a +// 20000x20000 PNG of flat colour — a few hundred kilobytes on the wire, well +// under the byte cap — decoded to 1.6 GB and then allocated another 1.6 GB to +// flatten, in the process that owns the database and the socket. +func TestPrepareImageRefusesADecodeBomb(t *testing.T) { + // The header is what is checked, so the test writes a real header and + // truncated pixel data: reaching the decode at all is the failure. + var buf bytes.Buffer + if err := png.Encode(&buf, image.NewGray(image.Rect(0, 0, 1, 1))); err != nil { + t.Fatal(err) + } + bomb := forgePNGSize(t, buf.Bytes(), 20000, 20000) + _, err := PrepareImage(bomb, "telegram", 0) + if !errors.Is(err, ErrTooManyPixels) { + t.Fatalf("err = %v, want ErrTooManyPixels", err) + } + // A phone photo is not a bomb. + if _, err := PrepareImage(pngBytes(t, 64, 48), "telegram", 0); err != nil { + t.Fatalf("an ordinary image was refused: %v", err) + } +} + +// forgePNGSize rewrites the IHDR width and height (and its CRC) of a valid PNG, +// which is how a header claiming 400 megapixels is produced without writing +// 400 megapixels. +func forgePNGSize(t *testing.T, src []byte, w, h uint32) []byte { + t.Helper() + out := append([]byte(nil), src...) + // 8 byte signature, 4 byte length, 4 byte "IHDR", then width and height. + const ihdr = 8 + 4 + 4 + binary.BigEndian.PutUint32(out[ihdr:], w) + binary.BigEndian.PutUint32(out[ihdr+4:], h) + crc := crc32.ChecksumIEEE(out[8+4 : ihdr+13]) + binary.BigEndian.PutUint32(out[ihdr+13:], crc) + return out +} + +// Transparency still composites onto white, which is what makes a screenshot +// readable. The old code did that with a full-size intermediate; the scaler +// walks the source instead and must give the same answer. +func TestPrepareImageFlattensOntoWhite(t *testing.T) { + img := image.NewRGBA(image.Rect(0, 0, 8, 8)) + // Fully transparent everywhere: over white, that is white. + data := encodePNG(t, img) + out, err := PrepareImage(data, "test", 4) + if err != nil { + t.Fatal(err) + } + dec, err := jpeg.Decode(bytes.NewReader(out.JPEG)) + if err != nil { + t.Fatal(err) + } + r, g, b, _ := dec.At(2, 2).RGBA() + if r>>8 < 240 || g>>8 < 240 || b>>8 < 240 { + t.Fatalf("transparent pixel came out %d,%d,%d, want white", r>>8, g>>8, b>>8) + } +} + +func encodePNG(t *testing.T, img image.Image) []byte { + t.Helper() + var buf bytes.Buffer + if err := png.Encode(&buf, img); err != nil { + t.Fatal(err) + } + return buf.Bytes() +} diff --git a/internal/media/store.go b/internal/media/store.go index c41897e..32d3fc4 100644 --- a/internal/media/store.go +++ b/internal/media/store.go @@ -6,11 +6,13 @@ import ( "encoding/json" "errors" "fmt" + "io" "io/fs" "os" "path/filepath" "sort" "strings" + "sync" "time" ) @@ -19,11 +21,32 @@ import ( // ceiling; a single item bigger than that is a mistake, not a meeting. const DefaultMaxBytes int64 = 64 << 20 +// DefaultMaxAudioBytes — the per-blob cap for audio. Separate from +// DefaultMaxBytes because the two kinds are not the same size of thing: an +// image over 64 MiB is a mistake, and a two-hour meeting at 16 kHz mono is +// about 230 MB of PCM by design. With one shared cap, capture's own +// DefaultMaxDuration of two hours and this store's 64 MiB contradicted each +// other, and the meeting that hit the limit was the one that failed to store. +const DefaultMaxAudioBytes int64 = 512 << 20 + // DefaultRetention — how long a blob is kept when no retention is configured. // Seven days is long enough to re-run a transcription that came out wrong and // short enough that "she has a month of my meetings on disk" is never true. const DefaultRetention = 7 * 24 * time.Hour +// DefaultMaxTotalBytes — the whole-store budget when one is not configured. The +// per-blob cap bounds one call and nothing bounded the sum of them: 64 MiB per +// call, an unlimited number of calls, and a seven-day window fills the disk +// mavend's database lives on. Content addressing does not help, because one +// flipped pixel is a different digest. 4 GiB is roughly sixty meetings or a few +// thousand photos inside the window. +const DefaultMaxTotalBytes int64 = 4 << 30 + +// ErrStoreFull — the store is at its total-bytes budget. Distinct from +// ErrTooLarge: the payload is a reasonable size and there is no room for it, so +// the answer is to prune or raise the budget, not to send something smaller. +var ErrStoreFull = errors.New("media: store is full") + // Store — a content-addressed blob directory. Zero value is not usable; build // one with Open, which creates the directory 0700. The store holds no lock and // no cache: every operation is a filesystem call, and two writers of the same @@ -31,8 +54,17 @@ const DefaultRetention = 7 * 24 * time.Hour type Store struct { dir string maxBytes int64 + maxAudio int64 + maxTotal int64 retention time.Duration now func() time.Time + + // total is the running sum of stored blob bytes, seeded by Open with a + // directory walk and kept up to date by Put, Delete and Prune. It is a + // cache of something the filesystem already knows: re-walking on every Put + // would be correct too and would make an image intake O(store size). + totalMu sync.Mutex + total int64 } // Open prepares a blob store rooted at dir. maxBytes ≤ 0 ⇒ DefaultMaxBytes; @@ -40,6 +72,12 @@ type Store struct { // created later) is 0700: these are recordings of people, and the daemon's user // is the only reader. func Open(dir string, maxBytes int64, retention time.Duration) (*Store, error) { + return OpenWithBudget(dir, maxBytes, 0, retention) +} + +// OpenWithBudget is Open with the whole-store budget spelled out. maxTotal ≤ 0 +// ⇒ DefaultMaxTotalBytes. +func OpenWithBudget(dir string, maxBytes, maxTotal int64, retention time.Duration) (*Store, error) { if strings.TrimSpace(dir) == "" { return nil, errors.New("media: empty dir") } @@ -53,12 +91,61 @@ func Open(dir string, maxBytes int64, retention time.Duration) (*Store, error) { if maxBytes <= 0 { maxBytes = DefaultMaxBytes } + maxAudio := DefaultMaxAudioBytes + if maxBytes > maxAudio { + maxAudio = maxBytes + } + if maxTotal <= 0 { + maxTotal = DefaultMaxTotalBytes + } + if maxTotal < maxAudio { + maxAudio = maxTotal + } + if maxTotal < maxBytes { + return nil, fmt.Errorf("media: max_total_bytes %d is below the per-blob cap %d", maxTotal, maxBytes) + } if retention <= 0 { retention = DefaultRetention } - return &Store{dir: abs, maxBytes: maxBytes, retention: retention, now: time.Now}, nil + s := &Store{dir: abs, maxBytes: maxBytes, maxAudio: maxAudio, maxTotal: maxTotal, + retention: retention, now: time.Now} + s.total = s.measure() + return s, nil } +// measure sums what is already on disk, so a restart does not start the budget +// over at zero. +func (s *Store) measure() int64 { + var total int64 + spool := filepath.Join(s.dir, "spool") + _ = filepath.WalkDir(s.dir, func(path string, d fs.DirEntry, err error) error { + if err == nil && d.IsDir() && path == spool { + // Spool files are not blobs yet and PutFile counts them when they + // become one. Counting them here too would double them. + return filepath.SkipDir + } + if err != nil || d.IsDir() || strings.HasSuffix(path, ".json") { + return nil //nolint:nilerr // an unreadable corner is not worth refusing to boot over + } + if info, err := d.Info(); err == nil { + total += info.Size() + } + return nil + }) + return total +} + +// Total is the number of blob bytes currently stored, and Budget the cap Put +// checks it against. Both are exported so the daemon can log how close it is. +func (s *Store) Total() int64 { + s.totalMu.Lock() + defer s.totalMu.Unlock() + return s.total +} + +// Budget is the whole-store cap. +func (s *Store) Budget() int64 { return s.maxTotal } + // Dir is the store root. Exported for logs and for pointing a subprocess at a // path under it. func (s *Store) Dir() string { return s.dir } @@ -81,8 +168,8 @@ func (s *Store) Put(kind Kind, mime, source string, data []byte) (Blob, error) { if len(data) == 0 { return Blob{}, ErrEmpty } - if int64(len(data)) > s.maxBytes { - return Blob{}, fmt.Errorf("%w: %d > %d", ErrTooLarge, len(data), s.maxBytes) + if cap := s.capFor(kind); int64(len(data)) > cap { + return Blob{}, fmt.Errorf("%w: %d > %d", ErrTooLarge, len(data), cap) } sum := sha256.Sum256(data) id := hex.EncodeToString(sum[:]) @@ -104,15 +191,156 @@ func (s *Store) Put(kind Kind, mime, source string, data []byte) (Blob, error) { b.Created = prev.Created } - if err := writeFile(blobPath, data); err != nil { + // A blob already on disk costs nothing more, so dedupe is checked before + // the budget rather than after it. + _, already := os.Stat(blobPath) + if already != nil { + s.totalMu.Lock() + room := s.total+b.Size <= s.maxTotal + if room { + s.total += b.Size + } + s.totalMu.Unlock() + if !room { + return Blob{}, fmt.Errorf("%w: %d stored, %d budget, %d more asked for", + ErrStoreFull, s.Total(), s.maxTotal, b.Size) + } + } + + // The sidecar goes first. Written second, a full disk or a crash between + // the two left the bytes on disk with no sidecar, and List only sees + // sidecars, so Prune could never collect them: Put returned an error and an + // image nobody knew about became permanent. + if err := writeMeta(metaPath, b); err != nil { return Blob{}, err } + if err := writeFile(blobPath, data); err != nil { + _ = os.Remove(metaPath) + if already != nil { + s.totalMu.Lock() + s.total -= b.Size + s.totalMu.Unlock() + } + return Blob{}, err + } + return b, nil +} + +// capFor is the per-blob cap for a kind. Audio has its own, larger one. +func (s *Store) capFor(kind Kind) int64 { + if kind == KindAudio { + return s.maxAudio + } + return s.maxBytes +} + +// PutFile stores a file that is already on disk, by moving it into place rather +// than reading it into memory. It exists for meeting audio: a two-hour capture +// is a couple of hundred megabytes, and Put's []byte means that much heap in +// the process that owns the database, twice over while the WAV is built. +// +// src is consumed: on success it has been renamed into the store, and on a +// duplicate it is removed. On failure it is left where it is, so a caller that +// still needs the bytes can fall back to reading them. +func (s *Store) PutFile(kind Kind, mime, source, src string) (Blob, error) { + if !kind.Valid() { + return Blob{}, ErrBadKind + } + info, err := os.Stat(src) + if err != nil { + return Blob{}, fmt.Errorf("media: stat spool: %w", err) + } + if info.Size() == 0 { + return Blob{}, ErrEmpty + } + if cap := s.capFor(kind); info.Size() > cap { + return Blob{}, fmt.Errorf("%w: %d > %d", ErrTooLarge, info.Size(), cap) + } + id, err := hashFile(src) + if err != nil { + return Blob{}, err + } + blobPath, metaPath, err := s.paths(kind, id, mime) + if err != nil { + return Blob{}, err + } + if err := os.MkdirAll(filepath.Dir(blobPath), 0o700); err != nil { + return Blob{}, fmt.Errorf("media: create bucket: %w", err) + } + b := Blob{ID: id, Kind: kind, MIME: mime, Size: info.Size(), Source: source, + Created: s.now().UTC(), Path: blobPath} + if prev, err := readMeta(metaPath); err == nil && !prev.Created.IsZero() { + b.Created = prev.Created + } + _, already := os.Stat(blobPath) + if already != nil { + s.totalMu.Lock() + room := s.total+b.Size <= s.maxTotal + if room { + s.total += b.Size + } + s.totalMu.Unlock() + if !room { + return Blob{}, fmt.Errorf("%w: %d stored, %d budget, %d more asked for", + ErrStoreFull, s.Total(), s.maxTotal, b.Size) + } + } if err := writeMeta(metaPath, b); err != nil { return Blob{}, err } + if already == nil { + // Same bytes already here. Drop the spool copy. + _ = os.Remove(src) + return b, nil + } + if err := os.Chmod(src, 0o600); err != nil { + return Blob{}, fmt.Errorf("media: chmod spool: %w", err) + } + if err := os.Rename(src, blobPath); err != nil { + _ = os.Remove(metaPath) + s.totalMu.Lock() + s.total -= b.Size + s.totalMu.Unlock() + return Blob{}, fmt.Errorf("media: move spool: %w", err) + } return b, nil } +// SpoolFile creates an empty file under the store, outside the kind +// directories, for a caller that is writing a blob incrementally. Prune never +// looks at it and List never reports it; PutFile is what turns it into a blob. +// The caller owns removing it if it never gets that far. +func (s *Store) SpoolFile(prefix string) (*os.File, error) { + dir := filepath.Join(s.dir, "spool") + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("media: create spool: %w", err) + } + f, err := os.CreateTemp(dir, prefix+"-*") + if err != nil { + return nil, fmt.Errorf("media: spool: %w", err) + } + if err := f.Chmod(0o600); err != nil { + f.Close() + return nil, fmt.Errorf("media: chmod spool: %w", err) + } + return f, nil +} + +// hashFile streams the digest so the id costs one buffer rather than the whole +// file. +func hashFile(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", fmt.Errorf("media: open spool: %w", err) + } + defer f.Close() + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return "", fmt.Errorf("media: hash spool: %w", err) + } + return hex.EncodeToString(h.Sum(nil)), nil +} + // Get returns the blob's metadata without reading its bytes. func (s *Store) Get(id string) (Blob, error) { if !validID(id) { @@ -210,10 +438,24 @@ func (s *Store) Delete(id string) error { continue } for _, e := range entries { - if strings.HasPrefix(e.Name(), id) { - if err := os.Remove(filepath.Join(bucket, e.Name())); err != nil && !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("media: delete %s: %w", shortID(id), err) + if !strings.HasPrefix(e.Name(), id) { + continue + } + path := filepath.Join(bucket, e.Name()) + var size int64 + if info, err := e.Info(); err == nil && !strings.HasSuffix(e.Name(), ".json") { + size = info.Size() + } + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("media: delete %s: %w", shortID(id), err) + } + if size > 0 { + s.totalMu.Lock() + s.total -= size + if s.total < 0 { + s.total = 0 } + s.totalMu.Unlock() } } } @@ -232,15 +474,70 @@ func (s *Store) Prune() (int, error) { } now := s.now() deleted := 0 + known := map[string]bool{} for _, b := range blobs { + known[b.ID] = true if b.Age(now) <= s.retention { continue } if err := s.Delete(b.ID); err != nil { return deleted, err } + delete(known, b.ID) deleted++ } + n, err := s.pruneOrphans(known, now) + return deleted + n, err +} + +// pruneOrphans collects blob files with no readable sidecar. List walks +// sidecars, so those files were invisible to retention and stayed on disk +// forever: audio of people accumulating is the exact failure this package +// exists to prevent, and a half-finished Put from an older build is enough to +// produce one. They are only collected once they are older than retention, so a +// Put racing a Prune does not lose its bytes. +func (s *Store) pruneOrphans(known map[string]bool, now time.Time) (int, error) { + deleted := 0 + for _, kind := range []Kind{KindImage, KindAudio} { + root := filepath.Join(s.dir, string(kind)) + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return err + } + if d.IsDir() || strings.HasSuffix(path, ".json") { + return nil + } + name := d.Name() + id, _, _ := strings.Cut(name, ".") + if known[id] { + return nil + } + info, err := d.Info() + if err != nil { + return nil //nolint:nilerr // gone underneath us is the outcome we wanted + } + if now.Sub(info.ModTime()) <= s.retention { + return nil + } + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + s.totalMu.Lock() + s.total -= info.Size() + if s.total < 0 { + s.total = 0 + } + s.totalMu.Unlock() + deleted++ + return nil + }) + if err != nil { + return deleted, fmt.Errorf("media: prune %s: %w", kind, err) + } + } return deleted, nil } @@ -300,6 +597,9 @@ func extFor(mime string, kind Kind) string { case "image/gif": return ".gif" case "image/webp": + // Unreachable for images today: SniffImage refuses webp before + // anything reaches Put, because this build has no webp decoder. Kept + // so the mapping is right on the day one arrives. return ".webp" case "audio/wav", "audio/x-wav", "audio/wave": return ".wav" diff --git a/internal/media/store_test.go b/internal/media/store_test.go index c983f0b..fccb67f 100644 --- a/internal/media/store_test.go +++ b/internal/media/store_test.go @@ -1,6 +1,8 @@ package media import ( + "crypto/sha256" + "encoding/hex" "errors" "os" "path/filepath" @@ -212,3 +214,129 @@ func TestOpenRejectsEmptyDir(t *testing.T) { t.Error("empty dir accepted") } } + +// A blob whose sidecar is missing was invisible to List, so Prune never saw it +// and the bytes stayed on disk forever. Put produced exactly that state, by +// writing the blob first and the sidecar second. +func TestPruneCollectsASidecarlessBlob(t *testing.T) { + s := testStore(t) + b, err := s.Put(KindImage, "image/png", "web:upload", []byte("orphan")) + if err != nil { + t.Fatal(err) + } + meta := filepath.Join(s.dir, string(KindImage), b.ID[:2], b.ID+".json") + if err := os.Remove(meta); err != nil { + t.Fatal(err) + } + // Age the file past retention, the same way a real orphan gets there. + old := time.Now().Add(-2 * DefaultRetention) + if err := os.Chtimes(b.Path, old, old); err != nil { + t.Fatal(err) + } + n, err := s.Prune() + if err != nil { + t.Fatal(err) + } + if n != 1 { + t.Fatalf("pruned %d, want the orphan collected", n) + } + if _, err := os.Stat(b.Path); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("the orphaned bytes are still on disk: %v", err) + } +} + +// A young orphan is left alone: a Put racing a Prune must not lose its bytes. +func TestPruneLeavesAYoungOrphan(t *testing.T) { + s := testStore(t) + b, err := s.Put(KindImage, "image/png", "web:upload", []byte("fresh")) + if err != nil { + t.Fatal(err) + } + if err := os.Remove(filepath.Join(s.dir, string(KindImage), b.ID[:2], b.ID+".json")); err != nil { + t.Fatal(err) + } + if n, err := s.Prune(); err != nil || n != 0 { + t.Fatalf("prune = %d, %v; want the fresh orphan kept", n, err) + } +} + +// Put writes the sidecar first, so a failure writing the bytes leaves nothing +// at all rather than an uncollectable blob. +func TestPutLeavesNothingWhenTheBytesCannotBeWritten(t *testing.T) { + s := testStore(t) + data := []byte("will not land") + sum := sha256.Sum256(data) + id := hex.EncodeToString(sum[:]) + bucket := filepath.Join(s.dir, string(KindImage), id[:2]) + if err := os.MkdirAll(bucket, 0o700); err != nil { + t.Fatal(err) + } + // A directory where the blob file needs to be: rename onto it fails, which + // is the same shape as a full disk one step later. + if err := os.Mkdir(filepath.Join(bucket, id+".png"), 0o700); err != nil { + t.Fatal(err) + } + if _, err := s.Put(KindImage, "image/png", "web:upload", data); err == nil { + t.Fatal("put must fail") + } + if _, err := os.Stat(filepath.Join(bucket, id+".json")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("a sidecar was left behind claiming a blob that does not exist: %v", err) + } + if s.Total() != 0 { + t.Errorf("total = %d, want the failed put not counted", s.Total()) + } +} + +// The per-blob cap bounds one call and nothing bounded their sum. 64 MiB per +// call times unlimited calls inside a seven-day window fills the disk mavend's +// database lives on. +func TestPutRefusesPastTheStoreBudget(t *testing.T) { + s, err := OpenWithBudget(t.TempDir(), 16, 48, 0) + if err != nil { + t.Fatal(err) + } + for i, want := range []bool{true, true, true, false} { + data := []byte(strings.Repeat(string(rune('a'+i)), 16)) + _, err := s.Put(KindImage, "image/png", "web:upload", data) + if ok := err == nil; ok != want { + t.Fatalf("put %d: err = %v, want ok=%v", i, err, want) + } + if !want && !errors.Is(err, ErrStoreFull) { + t.Fatalf("put %d: err = %v, want ErrStoreFull", i, err) + } + } + // The same bytes again cost nothing, so they are not refused. + if _, err := s.Put(KindImage, "image/png", "web:upload", []byte(strings.Repeat("a", 16))); err != nil { + t.Fatalf("a re-send of stored bytes was refused: %v", err) + } + // Deleting frees the budget again. + blobs, err := s.List(KindImage) + if err != nil { + t.Fatal(err) + } + if err := s.Delete(blobs[0].ID); err != nil { + t.Fatal(err) + } + if _, err := s.Put(KindImage, "image/png", "web:upload", []byte(strings.Repeat("z", 16))); err != nil { + t.Fatalf("budget was not released on delete: %v", err) + } +} + +// A restart must not start the budget over at zero. +func TestOpenSeedsTheBudgetFromDisk(t *testing.T) { + dir := t.TempDir() + s, err := OpenWithBudget(dir, 16, 48, 0) + if err != nil { + t.Fatal(err) + } + if _, err := s.Put(KindImage, "image/png", "web:upload", []byte(strings.Repeat("a", 16))); err != nil { + t.Fatal(err) + } + again, err := OpenWithBudget(dir, 16, 48, 0) + if err != nil { + t.Fatal(err) + } + if again.Total() != 16 { + t.Fatalf("total after reopen = %d, want 16", again.Total()) + } +} diff --git a/internal/store/migrations.go b/internal/store/migrations.go index 3414d9b..a266efe 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -200,6 +200,14 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 ); CREATE INDEX IF NOT EXISTS idx_eco_traces_ts ON ecosystem_traces (ts DESC); CREATE INDEX IF NOT EXISTS idx_eco_traces_correlation ON ecosystem_traces (correlation_id);`, + `ALTER TABLE tools ADD COLUMN fingerprint TEXT NOT NULL DEFAULT '';`, + // #17 — what a discovered tool WAS when it was approved (Vikunja #251). + // An MCP row's cmd is ["mcp", server, tool], which is a late-bound + // reference: it names a tool on a server the remote end owns and it pins + // no behaviour at all. A server upgraded, or taken over, can redefine + // list_tasks into something that writes without the row changing by one + // byte. The fingerprint is the declared shape at approval time, so a + // redefinition is a re-approval instead of a silent upgrade. } // migrate applies every migration with a number greater than the DB's current diff --git a/internal/store/tools.go b/internal/store/tools.go index 10f9c37..bf34901 100644 --- a/internal/store/tools.go +++ b/internal/store/tools.go @@ -65,7 +65,16 @@ func (s *Store) ProposeTool(ctx context.Context, name, utterance, scope string, // Like ProposeTool it never touches an existing row, so re-discovery on every // restart is idempotent and cannot silently re-arm a tool that was disabled or // change the cmd of one already enabled. -func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []string, destructive bool, utterance string, ts time.Time) (bool, error) { +// +// The row is NOT what protects him, and it is worth being exact about that. +// cmd is ["mcp", server, tool]: a late-bound reference to a name the remote +// server owns. The tool it points at can be redefined on the far end without +// the row changing at all, so "the cmd cannot change" is true and beside the +// point. fingerprint is what closes that: it records the declared shape (name, +// description, input schema, readOnlyHint) at the time the proposal was +// written, and ReconcileMCPTool compares against it on every later discovery. +// Pass "" for a row with nothing to fingerprint (a Home Assistant device). +func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []string, destructive bool, utterance, fingerprint string, ts time.Time) (bool, error) { if len(cmd) == 0 { return false, ErrToolCmd } @@ -81,10 +90,10 @@ func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []st d = 1 } res, err := s.db.ExecContext(ctx, ` - INSERT INTO tools (name, scope, cmd, destructive, status, utterance, created_ts, updated_ts) - VALUES (?, ?, ?, ?, 'proposed', ?, ?, ?) + INSERT INTO tools (name, scope, cmd, destructive, status, utterance, fingerprint, created_ts, updated_ts) + VALUES (?, ?, ?, ?, 'proposed', ?, ?, ?, ?) ON CONFLICT(name) DO NOTHING`, - name, scope, string(raw), d, utterance, ts.UnixMilli(), ts.UnixMilli()) + name, scope, string(raw), d, utterance, fingerprint, ts.UnixMilli(), ts.UnixMilli()) if err != nil { return false, fmt.Errorf("propose mcp tool: %w", err) } @@ -105,7 +114,111 @@ func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []st // turn. Re-discovery on every refresh is idempotent — an existing row is never // touched, so a device he disabled stays disabled. func (s *Store) ProposeSmartHomeTool(ctx context.Context, name, scope string, cmd []string, utterance string, ts time.Time) (bool, error) { - return s.ProposeMCPTool(ctx, name, scope, cmd, true, utterance, ts) + return s.ProposeMCPTool(ctx, name, scope, cmd, true, utterance, "", ts) +} + +// ToolChange — what ReconcileMCPTool did to an existing row. +type ToolChange struct { + // Changed — the discovered shape differs from the approved one. + Changed bool + // Demoted — the row was enabled and is now 'proposed' again, so the + // capability is off until a human looks at it a second time. + Demoted bool + // Escalated — destructive went from 0 to 1. It never goes the other way. + Escalated bool +} + +// ReconcileMCPTool compares a freshly discovered tool against the row that was +// approved, and escalates when they disagree. +// +// The failure this exists for: day 1 the server offers list_tasks with +// readOnlyHint true, so the row is proposed non-destructive and Kami enables +// it. Day 30 the server is upgraded, or taken over, and list_tasks now writes. +// Insert-or-skip does nothing on that discovery — the row is still enabled, +// still destructive=0 — and the confirm turn never fires, because the flag was +// frozen against a claim the server has since withdrawn. +// +// So: a differing fingerprint drops the row back to 'proposed' and rewrites the +// provenance, and a tool that stopped claiming read-only gets destructive=1. +// destructive is only ever raised, never lowered: relaxing it on the say-so of +// the same server that changed underneath us would undo the point. +// +// A row with an empty stored fingerprint predates this and simply adopts the +// discovered one — an upgrade is not a redefinition. +func (s *Store) ReconcileMCPTool(ctx context.Context, name, fingerprint string, destructive bool, utterance string, ts time.Time) (ToolChange, error) { + var ( + stored string + status string + wasDest int + ) + err := s.db.QueryRowContext(ctx, + `SELECT fingerprint, status, destructive FROM tools WHERE name = ?`, name). + Scan(&stored, &status, &wasDest) + if errors.Is(err, sql.ErrNoRows) { + return ToolChange{}, ErrToolNotFound + } + if err != nil { + return ToolChange{}, fmt.Errorf("reconcile mcp tool: %w", err) + } + var ch ToolChange + if stored == "" { + if _, err := s.db.ExecContext(ctx, + `UPDATE tools SET fingerprint = ?, updated_ts = ? WHERE name = ?`, + fingerprint, ts.UnixMilli(), name); err != nil { + return ToolChange{}, fmt.Errorf("reconcile mcp tool: %w", err) + } + return ch, nil + } + if stored == fingerprint { + return ch, nil + } + ch.Changed = true + ch.Demoted = status == "enabled" + d := wasDest + if destructive && wasDest == 0 { + d, ch.Escalated = 1, true + } + if _, err := s.db.ExecContext(ctx, ` + UPDATE tools + SET fingerprint = ?, destructive = ?, status = 'proposed', utterance = ?, updated_ts = ? + WHERE name = ?`, + fingerprint, d, utterance, ts.UnixMilli(), name); err != nil { + return ToolChange{}, fmt.Errorf("reconcile mcp tool: %w", err) + } + return ch, nil +} + +// WithdrawTool disarms a row whose remote tool no longer exists: it drops back +// to 'proposed' and its provenance says why. +// +// Nothing else retracted a proposal, so a tool a server stopped offering kept +// its row forever, and an ENABLED one stayed enabled and failed at call time +// with an internal string the act path does not match. /tools is where he would +// go to find out and it was the one place that did not say. Returns whether the +// row was still enabled. +func (s *Store) WithdrawTool(ctx context.Context, name, utterance string, ts time.Time) (bool, error) { + res, err := s.db.ExecContext(ctx, ` + UPDATE tools SET status = 'proposed', utterance = ?, updated_ts = ? + WHERE name = ? AND status = 'enabled'`, + utterance, ts.UnixMilli(), name) + if err != nil { + return false, fmt.Errorf("withdraw tool: %w", err) + } + n, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("withdraw tool: rows affected: %w", err) + } + if n > 0 { + return true, nil + } + // Not enabled: still refresh the provenance so the proposed row says it. + _, err = s.db.ExecContext(ctx, + `UPDATE tools SET utterance = ?, updated_ts = ? WHERE name = ?`, + utterance, ts.UnixMilli(), name) + if err != nil { + return false, fmt.Errorf("withdraw tool: %w", err) + } + return false, nil } // EnableTool fills cmd + destructive and flips status to 'enabled'. This is the diff --git a/internal/store/tools_test.go b/internal/store/tools_test.go index 0ee973e..375d5b0 100644 --- a/internal/store/tools_test.go +++ b/internal/store/tools_test.go @@ -70,7 +70,7 @@ func TestProposeMCPTool(t *testing.T) { now := time.Now() cmd := []string{"mcp", "vikunja", "list_tasks"} - fresh, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "mcp vikunja/list_tasks: List tasks", now) + fresh, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "mcp vikunja/list_tasks: List tasks", "fp1", now) if err != nil { t.Fatal(err) } @@ -92,7 +92,7 @@ func TestProposeMCPTool(t *testing.T) { } // Re-discovery on the next boot is idempotent. - fresh, err = s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, true, "changed", now) + fresh, err = s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, true, "changed", "fp1", now) if err != nil { t.Fatal(err) } @@ -104,7 +104,7 @@ func TestProposeMCPTool(t *testing.T) { if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { t.Fatal(err) } - if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", []string{"mcp", "vikunja", "delete_task"}, true, "x", now); err != nil { + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", []string{"mcp", "vikunja", "delete_task"}, true, "x", "fp2", now); err != nil { t.Fatal(err) } got, err = s.LookupTool(ctx, "vikunja_list_tasks") @@ -118,7 +118,142 @@ func TestProposeMCPTool(t *testing.T) { func TestProposeMCPToolNeedsCmd(t *testing.T) { s := newTestStore(t) - if _, err := s.ProposeMCPTool(context.Background(), "x", "mcp:y", nil, false, "", time.Now()); !errors.Is(err, ErrToolCmd) { + if _, err := s.ProposeMCPTool(context.Background(), "x", "mcp:y", nil, false, "", "", time.Now()); !errors.Is(err, ErrToolCmd) { t.Fatalf("err = %v, want ErrToolCmd", err) } } + +// A server that redefines a tool Kami already approved must have to ask again. +// The row stores cmd ["mcp", server, tool], a late-bound reference to a name +// the far end owns, so before the fingerprint a server could turn an enabled +// read-only list_tasks into something that writes and Maven would keep running +// it without a confirm turn. +func TestReconcileMCPToolDemotesARedefinedTool(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + cmd := []string{"mcp", "vikunja", "list_tasks"} + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "read only", "fp1", now); err != nil { + t.Fatal(err) + } + if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { + t.Fatal(err) + } + + // Same shape ⇒ nothing happens. Discovery runs every minute and must be + // idempotent. + ch, err := s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp1", false, "read only", now) + if err != nil { + t.Fatal(err) + } + if ch.Changed { + t.Fatalf("an unchanged tool must not be touched: %+v", ch) + } + if got, _ := s.LookupTool(ctx, "vikunja_list_tasks"); got.Status != "enabled" { + t.Fatalf("status = %q, want it left enabled", got.Status) + } + + // It stopped claiming read-only and its schema moved. + ch, err = s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp2", true, "now writes", now) + if err != nil { + t.Fatal(err) + } + if !ch.Changed || !ch.Demoted || !ch.Escalated { + t.Fatalf("change = %+v, want changed+demoted+escalated", ch) + } + got, err := s.LookupTool(ctx, "vikunja_list_tasks") + if err != nil { + t.Fatal(err) + } + if got.Status != "proposed" { + t.Errorf("status = %q, want a redefined tool back in the queue", got.Status) + } + if !got.Destructive { + t.Error("a tool that stopped claiming read-only must gain the confirm turn") + } + if got.Utterance != "now writes" { + t.Errorf("utterance = %q, want what the server says today", got.Utterance) + } + + // destructive is only ever raised. The server that changed underneath us + // does not get to relax it by claiming read-only next time. + if _, err := s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp3", false, "read only again", now); err != nil { + t.Fatal(err) + } + if got, _ = s.LookupTool(ctx, "vikunja_list_tasks"); !got.Destructive { + t.Error("destructive was relaxed by the server") + } +} + +// A row written before fingerprints exist simply adopts one. An upgrade is not +// a redefinition and must not disable everything Kami approved. +func TestReconcileMCPToolAdoptsAnEmptyFingerprint(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + cmd := []string{"mcp", "vikunja", "list_tasks"} + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "x", "", now); err != nil { + t.Fatal(err) + } + if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { + t.Fatal(err) + } + ch, err := s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp1", false, "x", now) + if err != nil { + t.Fatal(err) + } + if ch.Changed { + t.Fatalf("adopting must be silent: %+v", ch) + } + if got, _ := s.LookupTool(ctx, "vikunja_list_tasks"); got.Status != "enabled" { + t.Fatalf("status = %q, want still enabled after the upgrade", got.Status) + } + // And now it is pinned. + if ch, _ = s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp2", false, "y", now); !ch.Changed { + t.Fatal("the adopted fingerprint must be enforced on the next pass") + } +} + +func TestReconcileMCPToolUnknownRow(t *testing.T) { + s := newTestStore(t) + if _, err := s.ReconcileMCPTool(context.Background(), "nope", "fp", false, "", time.Now()); !errors.Is(err, ErrToolNotFound) { + t.Fatalf("err = %v, want ErrToolNotFound", err) + } +} + +// A tool the server stopped offering must be disarmed and must say why. It used +// to stay enabled and fail at call time with an internal string, and /tools — +// the one place he would look — did not mention it. +func TestWithdrawTool(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + cmd := []string{"mcp", "vikunja", "list_tasks"} + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "x", "fp1", now); err != nil { + t.Fatal(err) + } + if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { + t.Fatal(err) + } + was, err := s.WithdrawTool(ctx, "vikunja_list_tasks", "gone", now) + if err != nil { + t.Fatal(err) + } + if !was { + t.Error("withdrawing an enabled tool must report that it was enabled") + } + got, err := s.LookupTool(ctx, "vikunja_list_tasks") + if err != nil { + t.Fatal(err) + } + if got.Status != "proposed" || got.Utterance != "gone" { + t.Fatalf("row = %+v, want proposed and saying why", got) + } + // Withdrawing again is not an error and does not claim it was enabled. + if was, err = s.WithdrawTool(ctx, "vikunja_list_tasks", "still gone", now); err != nil || was { + t.Fatalf("second withdraw = %v, %v", was, err) + } + if got, _ = s.LookupTool(ctx, "vikunja_list_tasks"); got.Utterance != "still gone" { + t.Errorf("utterance = %q, want the provenance refreshed anyway", got.Utterance) + } +} diff --git a/internal/tool/tool.go b/internal/tool/tool.go index c2ffdae..4a1b017 100644 --- a/internal/tool/tool.go +++ b/internal/tool/tool.go @@ -60,6 +60,13 @@ var ( ErrNotEnabled = errors.New("tool not on the enabled allowlist") // ErrNeedsConfirm — the fn is enabled but destructive; needs a confirm turn. ErrNeedsConfirm = errors.New("destructive tool needs confirmation") + // ErrNotConnected — the row is enabled and well formed, but the thing it + // dispatches to is not wired: the mcp block was dropped from the config + // while enabled MCP rows remained, or the same for the house. Held apart + // from ErrNotEnabled because the act path turns that one into a fresh + // proposal, and drafting a new proposal for a tool that already exists and + // is enabled is a lie about what is wrong. + ErrNotConnected = errors.New("tool is enabled but its backend is not connected") ) // MCPCaller is the seam for an act that is an MCP tool call rather than a @@ -134,7 +141,7 @@ func (e *Executor) Exec(ctx context.Context, name string, args []string, confirm // confirmed. Only the dispatch differs. if server, remote, ok := mcp.ParseCmd(t.Cmd); ok { if e.mcp == nil { - return "", ErrNotEnabled + return "", fmt.Errorf("%w: %s is an MCP tool and no mcp block is configured", ErrNotConnected, name) } ctx, cancel := context.WithTimeout(ctx, e.timeout) defer cancel() @@ -148,7 +155,7 @@ func (e *Executor) Exec(ctx context.Context, name string, args []string, confirm // row but can never compose a target of its own. if entityID, service, ok := smarthome.ParseCmd(t.Cmd); ok { if e.home == nil { - return "", ErrNotEnabled + return "", fmt.Errorf("%w: %s is a house tool and no smarthome block is configured", ErrNotConnected, name) } // The confirm turn on a house row is structural, not a column. The // proposal is written destructive=true, but /tools writes the checkbox diff --git a/internal/tool/tool_test.go b/internal/tool/tool_test.go index b6f8049..5b63a64 100644 --- a/internal/tool/tool_test.go +++ b/internal/tool/tool_test.go @@ -178,8 +178,15 @@ func TestExecMCPRowWithoutCallerRefuses(t *testing.T) { ran := false e := NewExecutor(api, time.Second) e.run = func(context.Context, []string) (string, error) { ran = true; return "", nil } - if _, err := e.Exec(context.Background(), "vikunja_list_tasks", nil, false); !errors.Is(err, ErrNotEnabled) { - t.Fatalf("err = %v, want ErrNotEnabled", err) + err := func() error { _, e2 := e.Exec(context.Background(), "vikunja_list_tasks", nil, false); return e2 }() + // ErrNotConnected, NOT ErrNotEnabled: the act path turns ErrNotEnabled into + // a fresh proposal, and drafting a proposal for a row that already exists + // and is enabled answers the wrong question. + if !errors.Is(err, ErrNotConnected) { + t.Fatalf("err = %v, want ErrNotConnected", err) + } + if errors.Is(err, ErrNotEnabled) { + t.Fatal("an enabled row with a missing backend must not read as not-enabled") } if ran { t.Fatal(`"mcp" must never be run as a binary`) @@ -222,8 +229,8 @@ func TestExecSmartHomeRow(t *testing.T) { } // No house configured ⇒ the row refuses rather than being exec'd. - if _, err := newExec(nil).Exec(context.Background(), "home_light_x_off", nil, true); !errors.Is(err, ErrNotEnabled) { - t.Fatalf("unconfigured house: err = %v, want ErrNotEnabled", err) + if _, err := newExec(nil).Exec(context.Background(), "home_light_x_off", nil, true); !errors.Is(err, ErrNotConnected) { + t.Fatalf("unconfigured house: err = %v, want ErrNotConnected", err) } if ran { t.Fatal(`"smarthome" was run as a binary`) diff --git a/internal/vision/vision.go b/internal/vision/vision.go index eda0ad8..a1a4667 100644 --- a/internal/vision/vision.go +++ b/internal/vision/vision.go @@ -37,6 +37,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "net" "net/http" "net/url" @@ -93,6 +94,10 @@ func (Disabled) Describe(context.Context, media.Image, string) (string, error) { return "", ErrDisabled } +// MaxReplyBytes bounds what is read back from the vision server. A description +// is words; anything past a megabyte is a broken endpoint. +const MaxReplyBytes = 1 << 20 + // Config — how to reach the local vision server. Built from // config.VisionConfig by the daemon; kept separate so this package does not // import internal/config. @@ -154,13 +159,31 @@ func NewLocal(cfg Config) (*LocalProvider, error) { model: cfg.Model, prompt: prompt, maxTokens: maxTokens, - http: &http.Client{Timeout: timeout}, + http: &http.Client{ + Timeout: timeout, + // No redirects. checkPrivate validates the configured literal and + // nothing validated a hop, so a 302 from the local llama-server + // would send the photo, as a data URI in the POST body, wherever + // the redirect named. "No provider in this repo may upload a blob" + // has to be true of the second request as well as the first. + CheckRedirect: func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + }, + }, }, nil } // Endpoint is the server this provider talks to. For logs and /dash. func (p *LocalProvider) Endpoint() string { return p.endpoint } +// ValidateEndpoint reports whether a configured endpoint is one this package +// would accept. Exported so config validation fails at startup on a typo, +// rather than logging once at wiring time and leaving the capability quietly +// off. +func ValidateEndpoint(raw string) error { + return checkPrivate(strings.TrimRight(strings.TrimSpace(raw), "/")) +} + // checkPrivate refuses any endpoint that is not on this box or its LAN. A // hostname that is not an IP literal is refused too: "vision.example.com" could // resolve anywhere, and resolving it here would be trusting DNS with his photos. @@ -260,7 +283,10 @@ func (p *LocalProvider) Describe(ctx context.Context, im media.Image, prompt str return "", fmt.Errorf("vision: status %d", resp.StatusCode) } var out chatResp - if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + // Capped: the decoder would otherwise read whatever the endpoint sends, and + // a local server answering with a stuck stream should not cost the daemon + // its memory. A description is a few hundred tokens. + if err := json.NewDecoder(io.LimitReader(resp.Body, MaxReplyBytes)).Decode(&out); err != nil { return "", fmt.Errorf("vision: decode: %w", err) } if len(out.Choices) == 0 { diff --git a/internal/vision/vision_test.go b/internal/vision/vision_test.go index 71787fc..a65cfc5 100644 --- a/internal/vision/vision_test.go +++ b/internal/vision/vision_test.go @@ -11,6 +11,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" @@ -196,3 +197,72 @@ func TestDescribeErrors(t *testing.T) { } }) } + +// checkPrivate validates the configured literal and used to validate nothing +// else. A 302 from the local llama-server would have sent the photo, as a data +// URI in the POST body, wherever the redirect named. +func TestLocalProviderDoesNotFollowARedirect(t *testing.T) { + var elsewhere int32 + away := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&elsewhere, 1) + _, _ = io.WriteString(w, `{"choices":[{"message":{"content":"leaked"}}]}`) + })) + defer away.Close() + local := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, away.URL+"/v1/chat/completions", http.StatusFound) + })) + defer local.Close() + + p, err := NewLocal(Config{Endpoint: local.URL}) + if err != nil { + t.Fatal(err) + } + im := media.Image{JPEG: []byte{0xFF, 0xD8, 0xFF}} + if _, err := p.Describe(context.Background(), im, "что это"); err == nil { + t.Fatal("a redirected describe must fail, not follow") + } + if n := atomic.LoadInt32(&elsewhere); n != 0 { + t.Fatalf("the image was sent to the redirect target %d time(s)", n) + } +} + +// The reply is read through a cap. A stuck endpoint should not cost the daemon +// its memory. +func TestLocalProviderCapsTheReply(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"choices":[{"message":{"content":"`) + for written := 0; written < MaxReplyBytes+(1<<20); written += 1 << 16 { + if _, err := io.WriteString(w, strings.Repeat("a", 1<<16)); err != nil { + return + } + } + })) + defer srv.Close() + p, err := NewLocal(Config{Endpoint: srv.URL}) + if err != nil { + t.Fatal(err) + } + im := media.Image{JPEG: []byte{0xFF, 0xD8, 0xFF}} + if _, err := p.Describe(context.Background(), im, ""); err == nil { + t.Fatal("an unbounded reply must fail rather than being read whole") + } +} + +// ValidateEndpoint is what config calls at startup, and it must agree with the +// constructor. +func TestValidateEndpointMatchesTheConstructor(t *testing.T) { + for _, raw := range []string{"http://127.0.0.1:8081", "http://localhost:8081/"} { + if err := ValidateEndpoint(raw); err != nil { + t.Errorf("ValidateEndpoint(%q) = %v", raw, err) + } + } + for _, raw := range []string{"http://8.8.8.8:8081", "http://vision.example.com", "ftp://127.0.0.1"} { + if err := ValidateEndpoint(raw); err == nil { + t.Errorf("ValidateEndpoint(%q) accepted a non-private endpoint", raw) + } + if _, err := NewLocal(Config{Endpoint: raw}); err == nil { + t.Errorf("NewLocal(%q) accepted what ValidateEndpoint should refuse", raw) + } + } +}