From c8e0288f94f421dc878d1cb87672e0395c9fc94c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Manzur?= Date: Fri, 5 Jun 2026 20:11:22 -0300 Subject: [PATCH 1/7] fix(store): reject empty observation titles in AddObservation Empty (or whitespace-only) titles passed to AddObservation were accepted locally but rejected by the cloud sync server (ValidateSyncMutationPayload), silently blocking the mutation queue. Validate at write time so the failure surfaces immediately and the caller gets actionable feedback. Closes #459 --- internal/store/store.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/internal/store/store.go b/internal/store/store.go index 8000bee9..17c212ca 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -2230,6 +2230,17 @@ func (s *Store) AddObservation(p AddObservationParams) (int64, error) { title := stripPrivateTags(p.Title) content := stripPrivateTags(p.Content) + // Reject empty titles before touching the DB. This mirrors the rule + // ValidateSyncMutationPayload enforces on the sync path (observation + // upserts require a non-empty title). We validate the post-strip value + // because that is exactly what gets persisted and later pushed to the + // cloud server. Without this, an empty title is accepted locally and the + // failure only surfaces later when sync rejects the mutation, silently + // blocking the queue with no feedback to the caller. See issue #459. + if strings.TrimSpace(title) == "" { + return 0, fmt.Errorf("observation title is required (non-empty after trimming whitespace)") + } + if len(content) > s.cfg.MaxObservationLength { content = content[:s.cfg.MaxObservationLength] + "... [truncated]" } From 897cbc9f602c0f1fb30aa60a3434145b5be9df80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Manzur?= Date: Fri, 5 Jun 2026 20:11:22 -0300 Subject: [PATCH 2/7] fix(mcp): return clear error from handleSave when title is empty Mirror the existing content check. The error message instructs the agent to provide a short, searchable title, since the previous silent success caused stuck cloud sync queues with no feedback loop. Refs #459 --- internal/mcp/mcp.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/mcp/mcp.go b/internal/mcp/mcp.go index e4958c29..6e91bbe7 100644 --- a/internal/mcp/mcp.go +++ b/internal/mcp/mcp.go @@ -1064,6 +1064,14 @@ 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 empty titles early with an agent-actionable message. The store + // enforces the same rule (single source of truth), but returning here + // lets us tell the model exactly what to provide on retry instead of + // surfacing a generic store error. An empty title would otherwise be + // accepted and silently block cloud sync downstream. See issue #459. + if strings.TrimSpace(title) == "" { + return mcp.NewToolResultError("title is required for mem_save — pass a short, searchable title (e.g. 'Fixed N+1 query in UserList', 'Decision: use OpenTofu over Terraform'). Empty titles silently block cloud sync (issue #459)."), nil + } typ, _ := req.GetArguments()["type"].(string) sessionID, _ := req.GetArguments()["session_id"].(string) scope, _ := req.GetArguments()["scope"].(string) From ac8ae00dd27feefea6898ec1b99d88cb807cb4c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Manzur?= Date: Fri, 5 Jun 2026 20:11:22 -0300 Subject: [PATCH 3/7] test(store): cover empty and whitespace title rejection --- internal/store/store_test.go | 51 ++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/internal/store/store_test.go b/internal/store/store_test.go index e64bd8bd..cf45e8fd 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -8289,3 +8289,54 @@ func TestMostRecentActiveSessionScopedByProject(t *testing.T) { t.Fatalf("expected no active session for engram when only 'other' has one, got ok=%v", ok) } } + +func TestAddObservationRejectsEmptyTitle(t *testing.T) { + cases := []struct { + name string + title string + }{ + {"empty string", ""}, + {"only spaces", " "}, + {"only tabs and newlines", "\t\n \n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := newTestStore(t) + _, err := s.AddObservation(AddObservationParams{ + SessionID: "s1", + Type: "decision", + Title: tc.title, + Content: "some content", + Project: "engram", + Scope: "project", + }) + if err == nil { + t.Fatal("expected error for empty title, got nil") + } + if !strings.Contains(err.Error(), "title is required") { + t.Errorf("expected 'title is required' in error, got: %v", err) + } + }) + } +} + +func TestAddObservationAcceptsNonEmptyTitle(t *testing.T) { + s := newTestStore(t) + if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil { + t.Fatalf("create session: %v", err) + } + id, err := s.AddObservation(AddObservationParams{ + SessionID: "s1", + Type: "decision", + Title: "Valid title", + Content: "some content", + Project: "engram", + Scope: "project", + }) + if err != nil { + t.Fatalf("unexpected error for valid title: %v", err) + } + if id == 0 { + t.Fatal("expected a non-zero observation id") + } +} From b861b2060bcacff8f01df54dc1a2d91843e2fd84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Manzur?= Date: Fri, 5 Jun 2026 20:11:22 -0300 Subject: [PATCH 4/7] test(mcp): handleSave rejects empty/missing title with clear error --- internal/mcp/mcp_test.go | 47 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index c063f54c..6d848ed6 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -7088,3 +7088,50 @@ func TestHandleSearchLegacyMixedCaseProject(t *testing.T) { t.Fatalf("expected search results for legacy project, got: %s", text) } } + +func TestHandleSaveRejectsEmptyTitle(t *testing.T) { + cases := []struct { + name string + title any + }{ + {"missing title", nil}, + {"empty string", ""}, + {"only whitespace", " \t\n"}, + } + for _, tc := range cases { + 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 with no usable title", + "type": "bugfix", + "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 an error result for empty title, got success") + } + text := callResultText(t, res) + if !strings.Contains(text, "title is required") { + t.Fatalf("expected 'title is required' in error, got %q", text) + } + + // Nothing should have been persisted. + obs, err := s.RecentObservations("engram", "project", 5) + if err != nil { + t.Fatalf("recent observations: %v", err) + } + if len(obs) != 0 { + t.Fatalf("expected no persisted observations, got %d", len(obs)) + } + }) + } +} From 8a67216a8bbf7f1a46f2ab81b614d05172f92391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Manzur?= Date: Tue, 16 Jun 2026 18:14:21 -0300 Subject: [PATCH 5/7] test(store): pin post-strip title validation contract Add a test documenting that AddObservation validates the title AFTER stripPrivateTags: a private-only title becomes "[REDACTED]" (non-empty) and is accepted/persisted as such. Closes the coverage gap on the post-strip path raised in review (CodeRabbit), without the incorrect assumption that stripping yields an empty, rejected title. --- internal/store/store_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 9bddca9f..af0849d5 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -8627,3 +8627,35 @@ func TestAddObservationAcceptsNonEmptyTitle(t *testing.T) { t.Fatal("expected a non-zero observation id") } } + +// TestAddObservationValidatesPostStripTitle pins the contract that the +// non-empty check runs on the value *after* stripPrivateTags, which is what +// actually gets persisted and pushed to the cloud (see issue #459). A title +// made only of private tags is NOT empty post-strip: stripPrivateTags replaces +// the tags with "[REDACTED]", so the observation is accepted and persisted with +// that title. This guards against a refactor that moves validation before the +// strip and silently changes the persisted value. +func TestAddObservationValidatesPostStripTitle(t *testing.T) { + s := newTestStore(t) + if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil { + t.Fatalf("create session: %v", err) + } + id, err := s.AddObservation(AddObservationParams{ + SessionID: "s1", + Type: "decision", + Title: "secret", + Content: "some content", + Project: "engram", + Scope: "project", + }) + if err != nil { + t.Fatalf("private-only title strips to a non-empty placeholder; expected accept, got: %v", err) + } + obs, err := s.GetObservation(id) + if err != nil { + t.Fatalf("get observation: %v", err) + } + if obs.Title != "[REDACTED]" { + t.Errorf("expected persisted title %q, got %q", "[REDACTED]", obs.Title) + } +} From 1a0e7a4509ce3f7db81b42da90b6e21170cb9328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Manzur?= Date: Wed, 17 Jun 2026 08:55:43 -0300 Subject: [PATCH 6/7] fix(store): reject empty observation titles on the update path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UpdateObservation accepted empty/whitespace titles, persisted them, and enqueued a sync upsert the cloud server rejects — the same stuck-queue class AddObservation already guards (issue #459), reachable via mem_update and PATCH /observations/{id} (the path the gentle-engram/opencode plugin forwards to). Validate the final post-strip title in UpdateObservation, introduce the ErrEmptyObservationTitle sentinel shared by both add and update paths, and map it to 400 in the HTTP handlers. Cover the store, MCP and HTTP update surfaces with tests. --- internal/mcp/mcp_test.go | 56 +++++++++++++++ internal/server/server.go | 13 +++- internal/server/server_test.go | 64 ++++++++++++++++++ internal/store/store.go | 20 +++++- internal/store/store_test.go | 120 +++++++++++++++++++++++++++++++++ 5 files changed, 271 insertions(+), 2 deletions(-) diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index eee89f65..a0c045dc 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -1478,6 +1478,62 @@ func TestHandleUpdateAcceptsAllOptionalFields(t *testing.T) { } } +// TestHandleUpdateRejectsEmptyTitle pins that mem_update surfaces an +// agent-actionable error when the new title is empty or whitespace-only, and +// that the original title is left untouched. This closes the same stuck-sync +// class as the save path (issue #459) on the update surface. +func TestHandleUpdateRejectsEmptyTitle(t *testing.T) { + cases := []struct { + name string + title string + }{ + {"empty string", ""}, + {"only whitespace", " \t\n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := newMCPTestStore(t) + if err := s.CreateSession("s-empty-update", "engram", "/tmp/engram"); err != nil { + t.Fatalf("create session: %v", err) + } + id, err := s.AddObservation(store.AddObservationParams{ + SessionID: "s-empty-update", + Type: "decision", + Title: "Original", + Content: "Original content", + Project: "engram", + Scope: "project", + }) + if err != nil { + t.Fatalf("add observation: %v", err) + } + + res, err := handleUpdate(s)(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{ + "id": float64(id), + "title": tc.title, + }}}) + if err != nil { + t.Fatalf("update handler error: %v", err) + } + if !res.IsError { + t.Fatalf("expected an error result for empty title, got success") + } + if !strings.Contains(callResultText(t, res), "title is required") { + t.Fatalf("expected 'title is required' in error, got %q", callResultText(t, res)) + } + + // The original title must survive the rejected update. + obs, err := s.GetObservation(id) + if err != nil { + t.Fatalf("get observation: %v", err) + } + if obs.Title != "Original" { + t.Fatalf("expected title unchanged %q, got %q", "Original", obs.Title) + } + }) + } +} + func TestHandleContextWithSessionOnlyUsesNoneProjects(t *testing.T) { s := newMCPTestStore(t) if err := s.CreateSession("s-context-none", "engram", "/tmp/engram"); err != nil { diff --git a/internal/server/server.go b/internal/server/server.go index 244d3b38..d0ca9951 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -337,6 +337,10 @@ func (s *Server) handleAddObservation(w http.ResponseWriter, r *http.Request) { id, err := s.store.AddObservation(body) if err != nil { + if errors.Is(err, store.ErrEmptyObservationTitle) { + jsonError(w, http.StatusBadRequest, err.Error()) + return + } jsonError(w, http.StatusInternalServerError, err.Error()) return } @@ -452,7 +456,14 @@ func (s *Server) handleUpdateObservation(w http.ResponseWriter, r *http.Request) obs, err := s.store.UpdateObservation(id, body) if err != nil { - jsonError(w, http.StatusNotFound, err.Error()) + switch { + case errors.Is(err, store.ErrEmptyObservationTitle): + jsonError(w, http.StatusBadRequest, err.Error()) + case errors.Is(err, store.ErrObservationNotFound): + jsonError(w, http.StatusNotFound, err.Error()) + default: + jsonError(w, http.StatusNotFound, err.Error()) + } return } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 81f0e157..def6b66a 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -169,6 +169,70 @@ func TestClaudeSaveNudgeCompatibilityRoutes(t *testing.T) { } } +// TestUpdateObservationRejectsEmptyTitle covers the HTTP PATCH path used by the +// gentle-engram (opencode) plugin, which forwards mem_update to +// PATCH /observations/{id}. An empty or whitespace-only title must be rejected +// with 400 (not persisted, not enqueued) and the original title preserved — +// closing the stuck-sync class from issue #459 on the supported HTTP surface. +func TestUpdateObservationRejectsEmptyTitle(t *testing.T) { + st := newServerTestStore(t) + srv := New(st, 0) + h := srv.Handler() + + createReq := httptest.NewRequest(http.MethodPost, "/sessions", strings.NewReader(`{"id":"s-empty","project":"engram"}`)) + createReq.Header.Set("Content-Type", "application/json") + createRec := httptest.NewRecorder() + h.ServeHTTP(createRec, createReq) + if createRec.Code != http.StatusCreated { + t.Fatalf("expected session create 201, got %d", createRec.Code) + } + + obsReq := httptest.NewRequest(http.MethodPost, "/observations", strings.NewReader(`{"session_id":"s-empty","type":"note","title":"Original","content":"body","project":"engram"}`)) + obsReq.Header.Set("Content-Type", "application/json") + obsRec := httptest.NewRecorder() + h.ServeHTTP(obsRec, obsReq) + if obsRec.Code != http.StatusCreated { + t.Fatalf("expected observation create 201, got %d body=%s", obsRec.Code, obsRec.Body.String()) + } + var created map[string]any + if err := json.Unmarshal(obsRec.Body.Bytes(), &created); err != nil { + t.Fatalf("decode created observation: %v", err) + } + id := int64(created["id"].(float64)) + + for _, tc := range []struct { + name string + body string + }{ + {"empty string", `{"title":""}`}, + {"only whitespace", `{"title":" \t\n"}`}, + } { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPatch, fmt.Sprintf("/observations/%d", id), 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 for empty title, got %d body=%s", rec.Code, rec.Body.String()) + } + + getReq := httptest.NewRequest(http.MethodGet, fmt.Sprintf("/observations/%d", id), nil) + getRec := httptest.NewRecorder() + h.ServeHTTP(getRec, getReq) + if getRec.Code != http.StatusOK { + t.Fatalf("expected get 200, got %d", getRec.Code) + } + var got map[string]any + if err := json.Unmarshal(getRec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode observation: %v", err) + } + if got["title"] != "Original" { + t.Fatalf("expected title unchanged %q, got %v", "Original", got["title"]) + } + }) + } +} + func TestAdditionalServerErrorBranches(t *testing.T) { st := newServerTestStore(t) srv := New(st, 0) diff --git a/internal/store/store.go b/internal/store/store.go index dfab800a..ee7e2648 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -51,6 +51,14 @@ var ( ErrObservationNotFound = errors.New("observation not found") ErrPromptNotFound = errors.New("prompt not found") ErrProjectNotFound = errors.New("project not found") + + // ErrEmptyObservationTitle is returned by AddObservation and + // UpdateObservation when the post-stripPrivateTags title is empty after + // trimming whitespace. The check runs on the persisted value because that is + // exactly what gets pushed to the cloud, where ValidateSyncMutationPayload + // rejects empty titles and silently stalls the sync queue (issue #459). + // Callers use errors.Is to map this to a 400-class response. + ErrEmptyObservationTitle = errors.New("observation title is required (non-empty after trimming whitespace)") ) // Sentinel errors for relation sync apply path (Phase 2). @@ -2263,7 +2271,7 @@ func (s *Store) AddObservation(p AddObservationParams) (int64, error) { // failure only surfaces later when sync rejects the mutation, silently // blocking the queue with no feedback to the caller. See issue #459. if strings.TrimSpace(title) == "" { - return 0, fmt.Errorf("observation title is required (non-empty after trimming whitespace)") + return 0, ErrEmptyObservationTitle } if len(content) > s.cfg.MaxObservationLength { @@ -2908,6 +2916,16 @@ func (s *Store) UpdateObservation(id int64, p UpdateObservationParams) (*Observa topicKey = normalizeTopicKey(*p.TopicKey) } + // Reject empty titles before persisting. We validate the final post-strip + // value because that is what gets written and enqueued as a sync upsert. + // Without this, mem_update / PATCH /observations/{id} could persist an + // empty title and queue a mutation the cloud server rejects, silently + // stalling the sync queue — the same class AddObservation guards against + // (issue #459). + if strings.TrimSpace(title) == "" { + return ErrEmptyObservationTitle + } + if _, err := s.execHook(tx, `UPDATE observations SET type = ?, diff --git a/internal/store/store_test.go b/internal/store/store_test.go index af0849d5..382925c7 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -8659,3 +8659,123 @@ func TestAddObservationValidatesPostStripTitle(t *testing.T) { t.Errorf("expected persisted title %q, got %q", "[REDACTED]", obs.Title) } } + +// TestUpdateObservationRejectsEmptyTitle pins the same invariant as +// TestAddObservationRejectsEmptyTitle for the update path. mem_update and +// PATCH /observations/{id} funnel through UpdateObservation; an explicit empty +// or whitespace-only title must be rejected before it is persisted and enqueued +// as a sync upsert the cloud server rejects (issue #459). The original title +// must be left untouched. +func TestUpdateObservationRejectsEmptyTitle(t *testing.T) { + cases := []struct { + name string + title string + }{ + {"empty string", ""}, + {"only spaces", " "}, + {"only tabs and newlines", "\t\n \n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := newTestStore(t) + if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil { + t.Fatalf("create session: %v", err) + } + id, err := s.AddObservation(AddObservationParams{ + SessionID: "s1", + Type: "decision", + Title: "Original title", + Content: "some content", + Project: "engram", + Scope: "project", + }) + if err != nil { + t.Fatalf("seed observation: %v", err) + } + + _, err = s.UpdateObservation(id, UpdateObservationParams{Title: &tc.title}) + if err == nil { + t.Fatal("expected error for empty title, got nil") + } + if !errors.Is(err, ErrEmptyObservationTitle) { + t.Errorf("expected ErrEmptyObservationTitle, got: %v", err) + } + + // The original title must survive the rejected update. + obs, err := s.GetObservation(id) + if err != nil { + t.Fatalf("get observation: %v", err) + } + if obs.Title != "Original title" { + t.Errorf("expected title unchanged %q, got %q", "Original title", obs.Title) + } + }) + } +} + +// TestUpdateObservationValidatesPostStripTitle mirrors the AddObservation +// post-strip contract on the update path: a title made only of private tags +// strips to "[REDACTED]" (not empty), so the update is accepted and persisted +// with that placeholder. +func TestUpdateObservationValidatesPostStripTitle(t *testing.T) { + s := newTestStore(t) + if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil { + t.Fatalf("create session: %v", err) + } + id, err := s.AddObservation(AddObservationParams{ + SessionID: "s1", + Type: "decision", + Title: "Original title", + Content: "some content", + Project: "engram", + Scope: "project", + }) + if err != nil { + t.Fatalf("seed observation: %v", err) + } + + newTitle := "secret" + if _, err := s.UpdateObservation(id, UpdateObservationParams{Title: &newTitle}); err != nil { + t.Fatalf("private-only title strips to a non-empty placeholder; expected accept, got: %v", err) + } + obs, err := s.GetObservation(id) + if err != nil { + t.Fatalf("get observation: %v", err) + } + if obs.Title != "[REDACTED]" { + t.Errorf("expected persisted title %q, got %q", "[REDACTED]", obs.Title) + } +} + +// TestUpdateObservationAcceptsContentOnlyUpdate guards that the title check does +// not regress content-only updates: when Title is nil the existing (valid) +// title is preserved and the update succeeds. +func TestUpdateObservationAcceptsContentOnlyUpdate(t *testing.T) { + s := newTestStore(t) + if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil { + t.Fatalf("create session: %v", err) + } + id, err := s.AddObservation(AddObservationParams{ + SessionID: "s1", + Type: "decision", + Title: "Original title", + Content: "some content", + Project: "engram", + Scope: "project", + }) + if err != nil { + t.Fatalf("seed observation: %v", err) + } + + newContent := "updated content" + updated, err := s.UpdateObservation(id, UpdateObservationParams{Content: &newContent}) + if err != nil { + t.Fatalf("content-only update should succeed, got: %v", err) + } + if updated.Title != "Original title" { + t.Errorf("expected title preserved %q, got %q", "Original title", updated.Title) + } + if updated.Content != newContent { + t.Errorf("expected content %q, got %q", newContent, updated.Content) + } +} From 6909e334372b34fc86863419fbd2c42ff9ad61f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Manzur?= Date: Wed, 17 Jun 2026 09:47:55 -0300 Subject: [PATCH 7/7] fix(server): 500 for unexpected update errors; cover POST empty-title route Address CodeRabbit on PR #467: - handleUpdateObservation default branch now returns 500 for unexpected failures instead of 404; ErrObservationNotFound and sql.ErrNoRows stay 404 so the not-found contract is preserved. - Add route+response test for POST /observations rejecting empty and whitespace-only titles with 400 and no persistence. --- internal/server/server.go | 4 +-- internal/server/server_test.go | 51 ++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/internal/server/server.go b/internal/server/server.go index d0ca9951..1d2ce2ea 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -459,10 +459,10 @@ func (s *Server) handleUpdateObservation(w http.ResponseWriter, r *http.Request) switch { case errors.Is(err, store.ErrEmptyObservationTitle): jsonError(w, http.StatusBadRequest, err.Error()) - case errors.Is(err, store.ErrObservationNotFound): + case errors.Is(err, store.ErrObservationNotFound), errors.Is(err, sql.ErrNoRows): jsonError(w, http.StatusNotFound, err.Error()) default: - jsonError(w, http.StatusNotFound, err.Error()) + jsonError(w, http.StatusInternalServerError, err.Error()) } return } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index def6b66a..4358f24d 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -233,6 +233,57 @@ func TestUpdateObservationRejectsEmptyTitle(t *testing.T) { } } +// TestAddObservationRejectsEmptyTitle covers the POST /observations route after +// the store-level empty-title validation change: a whitespace-only title (which +// passes the raw `title == ""` pre-check) must be rejected with 400 by the +// store sentinel, and an empty string by the pre-check — neither may persist. +func TestAddObservationRejectsEmptyTitle(t *testing.T) { + st := newServerTestStore(t) + srv := New(st, 0) + h := srv.Handler() + + createReq := httptest.NewRequest(http.MethodPost, "/sessions", strings.NewReader(`{"id":"s-add-empty","project":"engram"}`)) + createReq.Header.Set("Content-Type", "application/json") + createRec := httptest.NewRecorder() + h.ServeHTTP(createRec, createReq) + if createRec.Code != http.StatusCreated { + t.Fatalf("expected session create 201, got %d", createRec.Code) + } + + for _, tc := range []struct { + name string + body string + }{ + {"empty string", `{"session_id":"s-add-empty","type":"note","title":"","content":"body","project":"engram"}`}, + {"only whitespace", `{"session_id":"s-add-empty","type":"note","title":" \t\n","content":"body","project":"engram"}`}, + } { + 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 for empty title, got %d body=%s", rec.Code, rec.Body.String()) + } + }) + } + + // Nothing should have been persisted by the rejected requests. + listReq := httptest.NewRequest(http.MethodGet, "/observations?project=engram&limit=10", nil) + listRec := httptest.NewRecorder() + h.ServeHTTP(listRec, listReq) + if listRec.Code != http.StatusOK { + t.Fatalf("expected list 200, got %d", listRec.Code) + } + var obs []map[string]any + if err := json.Unmarshal(listRec.Body.Bytes(), &obs); err != nil { + t.Fatalf("decode observations: %v", err) + } + if len(obs) != 0 { + t.Fatalf("expected no persisted observations, got %d", len(obs)) + } +} + func TestAdditionalServerErrorBranches(t *testing.T) { st := newServerTestStore(t) srv := New(st, 0)