fix(store): reject empty observation titles before persistence

CHANGELOG.md

@@ -21,6 +21,10 @@ Breaking changes are always marked with a `type:breaking-change` label and docum
 
 <!-- Changes that are merged but not yet released are tracked here until the next tag. -->
 
+### 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.

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 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

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)

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)
+		}
+	}
+}

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)

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)
+				}
+			}
+		})
+	}
+}

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) {
@@ -337,6 +344,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
 	}

internal/server/server_test.go

@@ -2168,3 +2168,92 @@ 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())
+	}
+}
+
+// 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())
+	}
+}

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")
 		}

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]"
 	}