Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions DOCS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions cmd/engram/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

s, err := storeNew(cfg)
if err != nil {
fatal(err)
Expand Down
21 changes: 21 additions & 0 deletions cmd/engram/main_extra_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
5 changes: 5 additions & 0 deletions internal/mcp/mcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
123 changes: 123 additions & 0 deletions internal/mcp/mcp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7452,3 +7452,126 @@ 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)
}
}
})
}
}

func TestHandleUpdateRejectsBlankTitleWithoutSideEffects(t *testing.T) {
s := newMCPTestStore(t)
if err := s.CreateSession("s-update-title-guard", "engram", t.TempDir()); err != nil {
t.Fatalf("create session: %v", err)
}
id, err := s.AddObservation(store.AddObservationParams{
SessionID: "s-update-title-guard",
Type: "note",
Title: "Original title",
Content: "Original content",
Project: "engram",
Scope: "project",
})
if err != nil {
t.Fatalf("add observation: %v", err)
}
before, err := s.GetObservation(id)
if err != nil {
t.Fatalf("get original observation: %v", err)
}
countMutations := func() int {
t.Helper()
mutations, err := s.ListPendingSyncMutations(store.DefaultSyncTargetKey, 10)
if err != nil {
t.Fatalf("list pending mutations: %v", err)
}
count := 0
for _, mutation := range mutations {
if mutation.Entity == store.SyncEntityObservation && mutation.EntityKey == before.SyncID {
count++
}
}
return count
}
mutationsBefore := countMutations()

for _, title := range []string{"", " \t\n "} {
title := title
t.Run("blank title", func(t *testing.T) {
res, err := handleUpdate(s)(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{
"id": float64(id),
"title": title,
}}})
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))
}
after, err := s.GetObservation(id)
if err != nil {
t.Fatalf("get observation after rejected update: %v", err)
}
if after.Title != before.Title || after.Content != before.Content || after.RevisionCount != before.RevisionCount {
t.Fatalf("rejected update changed observation: before=%#v after=%#v", before, after)
}
if got := countMutations(); got != mutationsBefore {
t.Fatalf("rejected update enqueued a mutation: got %d, want %d", got, mutationsBefore)
}
})
}
}
22 changes: 19 additions & 3 deletions internal/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
jsonError(w, http.StatusInternalServerError, err.Error())
return
}
Expand Down Expand Up @@ -465,7 +477,11 @@ 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())
if errors.Is(err, store.ErrObservationTitleRequired) {
jsonError(w, http.StatusBadRequest, err.Error())
} else {
jsonError(w, http.StatusNotFound, err.Error())
}
return
}

Expand Down
Loading