From f459eff0c18fdd14a22c3d7daf93791ae31a1393 Mon Sep 17 00:00:00 2001 From: Cesar Rivas Date: Tue, 28 Jul 2026 10:31:36 -0500 Subject: [PATCH 1/2] fix(store): reject empty observation titles before persistence An observation saved with an empty or whitespace-only title was persisted as `observations.title = ''` and enqueued a cloud observation upsert with `payload.title = ''`. The cloud, doctor and chunk validators reject that payload, and because the mutation queue is an ordered log processed by seq, the rejected row blocked every later mutation for the project. The rule already existed on the pull/doctor side inside ValidateSyncMutationPayload. Rather than duplicating it, the underlying required-field check is now a single helper that both the payload validator and the new write-time guard call: - store.ValidateObservationTitle / ErrObservationTitleRequired in internal/store/diagnostic.go, next to the validator that owned the rule - store.AddObservation validates the post-strip title, so a title that survives only as whitespace is rejected before any INSERT or enqueue - engram save, mem_save and POST /observations reject the write with an actionable message (non-zero exit, MCP tool error, HTTP 400) Inbound paths are unaffected: cloud pull (applyPulledMutationTx) and `engram import` write observations with direct SQL and never call AddObservation, so a legitimate inbound record is not blocked. Closes #459 --- CHANGELOG.md | 4 + DOCS.md | 1 + cmd/engram/main.go | 7 ++ cmd/engram/main_extra_test.go | 21 ++++ internal/mcp/mcp.go | 5 + internal/mcp/mcp_test.go | 57 +++++++++++ internal/server/server.go | 5 + internal/server/server_test.go | 33 +++++++ internal/store/diagnostic.go | 30 +++++- internal/store/store.go | 9 ++ internal/store/store_test.go | 170 +++++++++++++++++++++++++++++++++ 11 files changed, 341 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f6002b1..f2077d1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ Breaking changes are always marked with a `type:breaking-change` label and docum +### Memory core + +- **fix(store):** reject empty or whitespace-only observation titles at write time (`engram save`, `mem_save`, `POST /observations`, `store.AddObservation`). Persisting a titleless observation also enqueued a cloud upsert that sync validators reject, which blocked every later mutation for the project. + ### Cloud sync - **fix(cloud):** make chunk and mutation push payload limits configurable with `ENGRAM_CLOUD_MAX_PUSH_BYTES` while preserving the 8 MiB default. diff --git a/DOCS.md b/DOCS.md index a0971d24..638314cd 100644 --- a/DOCS.md +++ b/DOCS.md @@ -134,6 +134,7 @@ Engram is local-first: local SQLite is authoritative; cloud features are optiona ### Observations - `POST /observations` — Add observation. Body: `{session_id, type, title, content, tool_name?, project?, scope?, topic_key?}` + - `400` when `title` is missing, empty, or whitespace-only. The same rule applies to every write path (`engram save`, `mem_save`, `POST /observations`): cloud sync rejects observation upserts without a title, and one rejected mutation blocks every later mutation for the project - `GET /observations` — Recent observations compatibility endpoint. Query: `?project=X&scope=project|personal|global&limit=N&sort=created_at:desc` - `GET /observations/recent` — Recent observations. Query: `?project=X&scope=project|personal|global&limit=N` - `GET /observations/{id}` — Get single observation by ID diff --git a/cmd/engram/main.go b/cmd/engram/main.go index 730d783f..9276aeb3 100644 --- a/cmd/engram/main.go +++ b/cmd/engram/main.go @@ -1032,6 +1032,13 @@ func cmdSave(cfg store.Config) { } } + // Reject titleless saves before opening the store or creating a session + // (#459). The store applies the same rule as a backstop. + if err := store.ValidateObservationTitle(title); err != nil { + fatal(err) + return + } + s, err := storeNew(cfg) if err != nil { fatal(err) diff --git a/cmd/engram/main_extra_test.go b/cmd/engram/main_extra_test.go index f16d06b4..f42581f8 100644 --- a/cmd/engram/main_extra_test.go +++ b/cmd/engram/main_extra_test.go @@ -4325,3 +4325,24 @@ func TestCmdMCPAutosyncPollTickerPullsDuringServe(t *testing.T) { t.Fatalf("expected MCP autosync poll ticker proof to complete cleanly, panic=%v stderr=%q", recovered, stderr) } } + +// TestCmdSaveRejectsEmptyTitle pins that `engram save` exits non-zero with an +// actionable message instead of persisting a titleless observation (#459). +func TestCmdSaveRejectsEmptyTitle(t *testing.T) { + cfg := testConfig(t) + stubExitWithPanic(t) + + for _, title := range []string{"", " "} { + withArgs(t, "engram", "save", title, "content body") + _, stderr, recovered := captureOutputAndRecover(t, func() { cmdSave(cfg) }) + if _, ok := recovered.(exitCode); !ok { + t.Fatalf("title %q: expected exit panic, got %v", title, recovered) + } + if !strings.Contains(stderr, "observation title is required") { + t.Fatalf("title %q: stderr missing title guard message: %q", title, stderr) + } + if !strings.Contains(stderr, "cloud sync") { + t.Fatalf("title %q: stderr should explain the cloud sync impact: %q", title, stderr) + } + } +} diff --git a/internal/mcp/mcp.go b/internal/mcp/mcp.go index e1fb4d16..5f257fd8 100644 --- a/internal/mcp/mcp.go +++ b/internal/mcp/mcp.go @@ -1195,6 +1195,11 @@ func handleSave(s *store.Store, cfg MCPConfig, activity *SessionActivity) server if strings.TrimSpace(content) == "" { return mcp.NewToolResultError("content is required for mem_save (use content, or observation for backward-compatible clients)"), nil } + // Reject titleless saves before any project resolution or session + // creation (#459). The store applies the same rule as a backstop. + if err := store.ValidateObservationTitle(title); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } typ, _ := req.GetArguments()["type"].(string) sessionID, _ := req.GetArguments()["session_id"].(string) scope, _ := req.GetArguments()["scope"].(string) diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index 89fc21ed..21305c3b 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -7452,3 +7452,60 @@ func TestHandleSearch_MatchModeInvalidError(t *testing.T) { t.Fatalf("parameter-validation error must not contain query-advice suffix \"Try simpler keywords\", got: %s", text) } } + +// TestHandleSaveRejectsEmptyTitle pins that mem_save refuses a titleless save +// (#459) instead of persisting an observation whose cloud upsert would block +// the project's mutation queue. +func TestHandleSaveRejectsEmptyTitle(t *testing.T) { + for _, tc := range []struct { + name string + title any + }{ + {"missing title", nil}, + {"empty title", ""}, + {"whitespace only title", " "}, + } { + t.Run(tc.name, func(t *testing.T) { + s := newMCPTestStore(t) + h := handleSave(s, MCPConfig{}, NewSessionActivity(10*time.Minute)) + + args := map[string]any{ + "content": "Body that would otherwise be saved", + "type": "note", + "project": "engram", + } + if tc.title != nil { + args["title"] = tc.title + } + + res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: args}}) + if err != nil { + t.Fatalf("handler error: %v", err) + } + if !res.IsError { + t.Fatalf("expected tool error, got %q", callResultText(t, res)) + } + if !strings.Contains(callResultText(t, res), "observation title is required") { + t.Fatalf("unexpected error text: %q", callResultText(t, res)) + } + + obs, err := s.RecentObservations("engram", "project", 5) + if err != nil { + t.Fatalf("recent observations: %v", err) + } + if len(obs) != 0 { + t.Fatalf("expected no observation persisted, got %#v", obs) + } + + mutations, err := s.ListPendingSyncMutations(store.DefaultSyncTargetKey, 100) + if err != nil { + t.Fatalf("list pending sync mutations: %v", err) + } + for _, mutation := range mutations { + if mutation.Entity == store.SyncEntityObservation { + t.Fatalf("expected no observation mutation enqueued, got %#v", mutation) + } + } + }) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index c30f66a1..27646d16 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -337,6 +337,11 @@ func (s *Server) handleAddObservation(w http.ResponseWriter, r *http.Request) { id, err := s.store.AddObservation(body) if err != nil { + // A titleless observation is a client mistake, not a server failure. + if errors.Is(err, store.ErrObservationTitleRequired) { + jsonError(w, http.StatusBadRequest, err.Error()) + return + } jsonError(w, http.StatusInternalServerError, err.Error()) return } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index dd6d9736..1dc2e9e2 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -2168,3 +2168,36 @@ func TestMigrateProjectCaseOnlySkipped(t *testing.T) { t.Fatalf("expected status=skipped for case-only difference, got %v (full response: %#v)", resp["status"], resp) } } + +// TestHandleAddObservationRejectsBlankTitle pins that POST /observations answers +// 400 (client mistake) rather than 500 or 201 when the title is blank (#459). +func TestHandleAddObservationRejectsBlankTitle(t *testing.T) { + st := newServerTestStore(t) + srv := New(st, 0) + h := srv.Handler() + + var writeCount atomic.Int32 + srv.SetOnWrite(func() { writeCount.Add(1) }) + + if err := st.CreateSession("s-blank-title", "engram", t.TempDir()); err != nil { + t.Fatalf("create session: %v", err) + } + + for _, body := range []string{ + `{"session_id":"s-blank-title","type":"note","title":"","content":"body","project":"engram"}`, + `{"session_id":"s-blank-title","type":"note","title":" ","content":"body","project":"engram"}`, + } { + req := httptest.NewRequest(http.MethodPost, "/observations", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("body %s: expected 400, got %d (%s)", body, rec.Code, rec.Body.String()) + } + } + + if writeCount.Load() != 0 { + t.Fatalf("expected 0 onWrite calls for rejected writes, got %d", writeCount.Load()) + } +} diff --git a/internal/store/diagnostic.go b/internal/store/diagnostic.go index 9392485e..d2227683 100644 --- a/internal/store/diagnostic.go +++ b/internal/store/diagnostic.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -129,6 +130,31 @@ func (s *Store) listPendingProjectMutationsTxLike(q rowQuerier, project string) return mutations, rows.Err() } +// ErrObservationTitleRequired is returned by write paths that would otherwise +// persist an observation without a usable title. Cloud sync rejects observation +// upserts whose payload carries an empty title, and because the mutation queue +// is an ordered log, a single rejected row blocks every later mutation for the +// same project. +var ErrObservationTitleRequired = errors.New("observation title is required: an empty or whitespace-only title is rejected by cloud sync and would block every later mutation for the project") + +// observationTitleIsPresent is the single definition of "an observation upsert +// has a title". Both ValidateSyncMutationPayload (pull/doctor side) and +// ValidateObservationTitle (write side) call it so the rule lives in one place. +func observationTitleIsPresent(title string) bool { + return strings.TrimSpace(title) != "" +} + +// ValidateObservationTitle enforces, before persistence, the same non-empty +// title rule that ValidateSyncMutationPayload enforces for observation upserts. +// It returns ErrObservationTitleRequired when the title is empty or +// whitespace-only. +func ValidateObservationTitle(title string) error { + if !observationTitleIsPresent(title) { + return ErrObservationTitleRequired + } + return nil +} + // ValidateSyncMutationPayload performs pure required-field validation for sync // payloads. It is intentionally conservative: malformed/empty/unsupported // payloads are reported as manual blocks, while complete payloads return an @@ -184,7 +210,9 @@ func ValidateSyncMutationPayload(entity, op, payload, entityKey string) SyncMuta if op == SyncOpUpsert { require("session_id") require("type") - require("title") + if !observationTitleIsPresent(field("title")) { + missing = append(missing, "title") + } require("content") require("scope") } diff --git a/internal/store/store.go b/internal/store/store.go index 9c6537b9..ff97f54f 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -2256,6 +2256,15 @@ func (s *Store) AddObservation(p AddObservationParams) (int64, error) { title := stripPrivateTags(p.Title) content := stripPrivateTags(p.Content) + // Reject titleless observations before any persistence. The check runs on + // the post-strip title so redaction cannot turn a valid title into an empty + // one behind our back. Persisting an empty title also enqueues a cloud + // observation upsert that the sync validators reject, which blocks every + // later mutation for the project. + if err := ValidateObservationTitle(title); err != nil { + return 0, err + } + if len(content) > s.cfg.MaxObservationLength { content = content[:s.cfg.MaxObservationLength] + "... [truncated]" } diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 5ed55ca6..e49d6c75 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -8830,3 +8830,173 @@ func TestSanitizeFTS(t *testing.T) { }) } } + +// TestAddObservationRejectsEmptyTitle pins the write-time guard for #459: an +// observation without a usable title must never reach the observations table, +// because it also enqueues a cloud upsert the sync validators reject, which +// blocks every later mutation for the project. +func TestAddObservationRejectsEmptyTitle(t *testing.T) { + s := newTestStore(t) + if err := s.CreateSession("s-title-guard", "engram", "/tmp/engram"); err != nil { + t.Fatalf("create session: %v", err) + } + + countMutations := func() int { + t.Helper() + var n int + if err := s.db.QueryRow(`SELECT COUNT(*) FROM sync_mutations WHERE entity = ?`, SyncEntityObservation).Scan(&n); err != nil { + t.Fatalf("count sync mutations: %v", err) + } + return n + } + countObservations := func() int { + t.Helper() + var n int + if err := s.db.QueryRow(`SELECT COUNT(*) FROM observations`).Scan(&n); err != nil { + t.Fatalf("count observations: %v", err) + } + return n + } + + cases := []struct { + name string + title string + }{ + {"empty title", ""}, + {"whitespace only title", " "}, + // stripPrivateTags trims the value, so a title made only of blank + // characters is still empty after stripping. The guard runs on the + // post-strip title precisely so redaction cannot smuggle one through. + {"title empty after stripping private tags", " \t\n "}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mutationsBefore := countMutations() + observationsBefore := countObservations() + + id, err := s.AddObservation(AddObservationParams{ + SessionID: "s-title-guard", + Type: "note", + Title: tc.title, + Content: "content that is perfectly valid", + Project: "engram", + Scope: "project", + }) + if !errors.Is(err, ErrObservationTitleRequired) { + t.Fatalf("expected ErrObservationTitleRequired, got id=%d err=%v", id, err) + } + if id != 0 { + t.Fatalf("expected no observation id, got %d", id) + } + if got := countObservations(); got != observationsBefore { + t.Fatalf("expected no observation persisted, count went %d → %d", observationsBefore, got) + } + if got := countMutations(); got != mutationsBefore { + t.Fatalf("expected no sync mutation enqueued, count went %d → %d", mutationsBefore, got) + } + }) + } +} + +// TestAddObservationAcceptsValidTitle pins that the #459 guard does not change +// behaviour for observations that already carry a usable title, including a +// title whose private tags collapse into the redaction marker. +func TestAddObservationAcceptsValidTitle(t *testing.T) { + s := newTestStore(t) + if err := s.CreateSession("s-title-ok", "engram", "/tmp/engram"); err != nil { + t.Fatalf("create session: %v", err) + } + + cases := []struct { + name string + title string + wantTitle string + }{ + {"plain title", " Reject empty titles ", "Reject empty titles"}, + {"title reduced to redaction marker", "secret", "[REDACTED]"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + id, err := s.AddObservation(AddObservationParams{ + SessionID: "s-title-ok", + Type: "note", + Title: tc.title, + Content: "content for " + tc.name, + Project: "engram", + Scope: "project", + }) + if err != nil { + t.Fatalf("add observation: %v", err) + } + obs, err := s.GetObservation(id) + if err != nil { + t.Fatalf("get observation: %v", err) + } + if obs.Title != tc.wantTitle { + t.Fatalf("expected title %q, got %q", tc.wantTitle, obs.Title) + } + + var mutations int + if err := s.db.QueryRow( + `SELECT COUNT(*) FROM sync_mutations WHERE entity = ? AND entity_key = ?`, + SyncEntityObservation, obs.SyncID, + ).Scan(&mutations); err != nil { + t.Fatalf("count sync mutations: %v", err) + } + if mutations != 1 { + t.Fatalf("expected 1 sync mutation for %s, got %d", obs.SyncID, mutations) + } + }) + } +} + +// TestValidateObservationTitleMatchesSyncPayloadRule pins that the write-time +// guard and the sync payload validator agree on what a missing title is, so the +// rule keeps living in exactly one place. +func TestValidateObservationTitleMatchesSyncPayloadRule(t *testing.T) { + cases := []struct { + name string + title string + valid bool + }{ + {"empty", "", false}, + {"whitespace", " ", false}, + {"present", "Real title", true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateObservationTitle(tc.title) + if tc.valid && err != nil { + t.Fatalf("expected title %q to be accepted, got %v", tc.title, err) + } + if !tc.valid && !errors.Is(err, ErrObservationTitleRequired) { + t.Fatalf("expected ErrObservationTitleRequired for %q, got %v", tc.title, err) + } + + payload, marshalErr := json.Marshal(map[string]string{ + "sync_id": "obs-1", + "session_id": "s-1", + "type": "note", + "title": tc.title, + "content": "content", + "scope": "project", + }) + if marshalErr != nil { + t.Fatalf("marshal payload: %v", marshalErr) + } + result := ValidateSyncMutationPayload(SyncEntityObservation, SyncOpUpsert, string(payload), "obs-1") + missingTitle := false + for _, field := range result.MissingFields { + if field == "title" { + missingTitle = true + } + } + if missingTitle == tc.valid { + t.Fatalf("validator disagrees with write guard for %q: missing_fields=%v", tc.title, result.MissingFields) + } + }) + } +} From b10276c3d7c419bb7608cd6e756940ebe85d3b9c Mon Sep 17 00:00:00 2001 From: Cesar Rivas Date: Tue, 28 Jul 2026 12:10:45 -0500 Subject: [PATCH 2/2] fix(server): validate observation title before session lookup Review follow-up on #678. POST /observations checked `title == ""` on the raw body, so a whitespace-only title passed that check and the request fell through to validateSessionProject. A nonexistent or mismatched session then answered first, masking the documented title-validation 400 behind a session error. The handler now calls store.ValidateObservationTitle right after decoding the body and before the session lookup, so the title rule is reported on its own terms. The AddObservation error mapping stays as a backstop for any caller that reaches the store directly, and session_id/content keep their combined required-fields 400. DOCS.md previously claimed the title rule applies to "every write path", which overstates it: Store.UpdateObservation is deliberately out of scope. The sentence is now scoped to the observation-create paths. --- DOCS.md | 2 +- internal/server/server.go | 11 +++++-- internal/server/server_test.go | 56 ++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/DOCS.md b/DOCS.md index 638314cd..de941119 100644 --- a/DOCS.md +++ b/DOCS.md @@ -134,7 +134,7 @@ Engram is local-first: local SQLite is authoritative; cloud features are optiona ### Observations - `POST /observations` — Add observation. Body: `{session_id, type, title, content, tool_name?, project?, scope?, topic_key?}` - - `400` when `title` is missing, empty, or whitespace-only. The same rule applies to every write path (`engram save`, `mem_save`, `POST /observations`): cloud sync rejects observation upserts without a title, and one rejected mutation blocks every later mutation for the project + - `400` when `title` is missing, empty, or whitespace-only. The same rule applies to the observation-create paths (`engram save`, `mem_save`, `POST /observations`), not to updates via `PATCH /observations/{id}`: cloud sync rejects observation upserts without a title, and one rejected mutation blocks every later mutation for the project - `GET /observations` — Recent observations compatibility endpoint. Query: `?project=X&scope=project|personal|global&limit=N&sort=created_at:desc` - `GET /observations/recent` — Recent observations. Query: `?project=X&scope=project|personal|global&limit=N` - `GET /observations/{id}` — Get single observation by ID diff --git a/internal/server/server.go b/internal/server/server.go index 27646d16..3d9ef9fe 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -327,8 +327,15 @@ func (s *Server) handleAddObservation(w http.ResponseWriter, r *http.Request) { jsonError(w, http.StatusBadRequest, "invalid json: "+err.Error()) return } - if body.SessionID == "" || body.Title == "" || body.Content == "" { - jsonError(w, http.StatusBadRequest, "session_id, title, and content are required") + // Validate the title before the session lookup so a bad session or project + // cannot mask the documented title-validation 400 (#459). A whitespace-only + // title survives a raw `== ""` check, so it needs the shared predicate. + if err := store.ValidateObservationTitle(body.Title); err != nil { + jsonError(w, http.StatusBadRequest, err.Error()) + return + } + if body.SessionID == "" || body.Content == "" { + jsonError(w, http.StatusBadRequest, "session_id and content are required") return } if !s.validateSessionProject(w, body.SessionID, body.Project) { diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 1dc2e9e2..d9193e2b 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -2201,3 +2201,59 @@ func TestHandleAddObservationRejectsBlankTitle(t *testing.T) { t.Fatalf("expected 0 onWrite calls for rejected writes, got %d", writeCount.Load()) } } + +// TestHandleAddObservationBlankTitleNotMaskedBySessionError pins that the title +// check runs before the session/project lookup. A whitespace-only title passes +// the raw required-fields check, so before #459's follow-up the request was +// answered with the session error instead of the documented title 400. +func TestHandleAddObservationBlankTitleNotMaskedBySessionError(t *testing.T) { + st := newServerTestStore(t) + srv := New(st, 0) + h := srv.Handler() + + var writeCount atomic.Int32 + srv.SetOnWrite(func() { writeCount.Add(1) }) + + // Exists, but bound to a different project than the request claims. + if err := st.CreateSession("s-mismatched", "engram", t.TempDir()); err != nil { + t.Fatalf("create session: %v", err) + } + + for _, tc := range []struct { + name string + body string + }{ + { + name: "nonexistent session", + body: `{"session_id":"s-does-not-exist","type":"note","title":" ","content":"body","project":"engram"}`, + }, + { + name: "mismatched project", + body: `{"session_id":"s-mismatched","type":"note","title":" ","content":"body","project":"other"}`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/observations", strings.NewReader(tc.body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d (%s)", rec.Code, rec.Body.String()) + } + + var resp map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v (body %s)", err, rec.Body.String()) + } + msg, _ := resp["error"].(string) + if msg != store.ErrObservationTitleRequired.Error() { + t.Fatalf("expected the title error, got %q — the session lookup masked it", msg) + } + }) + } + + if writeCount.Load() != 0 { + t.Fatalf("expected 0 onWrite calls for rejected writes, got %d", writeCount.Load()) + } +}