fix(store): reject empty observation titles before persistence

[tommanzur]

Summary

Closes #459.

Empty (or whitespace-only) observation titles were accepted by mem_save and
store.AddObservation, persisted locally, and queued for cloud sync. The cloud
server rejects them via ValidateSyncMutationPayload, blocking the entire
mutation queue silently — the caller receives success at write time and has no
feedback to retry with a valid title.

This PR pushes the same validation up to the write path, so the failure
surfaces immediately and can be corrected.

Changes

• internal/store/store.go: AddObservation rejects empty/whitespace titles
  with a clear error, before any DB access. Validated after stripPrivateTags
  so it matches exactly the value that gets persisted and later pushed to the
  cloud server.
• internal/mcp/mcp.go: handleSave returns an agent-actionable
  ToolResultError before reaching the store, with an example title format so
  the model knows what to provide on retry.
• Tests added for both layers (empty / whitespace / missing title).

Why this approach

Per the suggestion on #459, reusing the rule already enforced by
ValidateSyncMutationPayload (diagnostic.go) keeps validation in one logical
place. The fix is defensive at two layers — MCP for UX, store as the single
source of truth — so the CLI, MCP, and any future entrypoint inherit it.

Test plan

• go test ./internal/store/... — new tests for empty/whitespace titles + a positive case
• go test ./internal/mcp/... — handler returns ToolResultError, nothing persisted
• go build ./cmd/engram — binary builds clean
• Existing tests pass (no regression in dedupe, topic_key upsert, normalization)

Notes

• This is technically a behavior change for callers passing an empty title
  today, but those cases are already broken downstream (cloud sync blocks them).
  The change turns a silent failure into a loud, immediate one.
• stripPrivateTags replaces <private>…</private> with [REDACTED], not an
  empty string, so a title made only of private tags is not rejected — that
  is intended.

Out of scope

• Backfilling existing observations with empty titles in users' local DBs. The
  repair tooling and the SQLite workaround documented in Reject empty observation titles before persistence #459's comments handle
  that path. This PR prevents new occurrences.
• Title length / format constraints beyond non-empty. Can be a follow-up.

Summary by CodeRabbit

• Bug Fixes
  • Prevented saving or updating observations with empty or whitespace-only titles (including titles that become empty after redaction).
  • Improved error handling so invalid requests return 400 Bad Request, and rejected updates no longer change existing titles.
• Tests
  • Expanded negative test coverage for empty-title validation across MCP operations, storage behavior, and the observation create/update HTTP endpoints.

[tommanzur]

Thanks @ricardoarz-dev for the pointer to ValidateSyncMutationPayload — this PR reuses exactly that rule at write time, in store.AddObservation (single source of truth) plus an early agent-actionable error in handleSave. cc @jorocoimbra since you confirmed the repro and the stuck-queue recovery path.

One note for a maintainer: I couldn't add the type:bug label myself (external contributor — GitHub blocks labeling), so it'll need to be applied on triage.

Happy to adjust the approach or split anything further if it helps review.

[Alan-TheGentleman]

#459 is now approved and this PR has the type:bug label. The problem is valid: empty titles should fail before persistence instead of poisoning sync later. Next step is code review after required checks are green.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 89b2ffdc-661d-47e5-bf70-89d0b938ead3

📥 Commits

Reviewing files that changed from the base of the PR and between 1a0e7a4 and 6909e33.

📒 Files selected for processing (2)

• internal/server/server.go
• internal/server/server_test.go

---

📝 Walkthrough

Walkthrough

Empty/whitespace-only observation title validation is added at three enforcement points: AddObservation and UpdateObservation in the store reject blank titles before any database writes, the server maps that error to HTTP 400 Bad Request, and handleSave and handleUpdate in the MCP layer reject them at the tool interface. All paths are covered by comprehensive table-driven tests.

Changes

Empty Title Validation

Layer / File(s)	Summary
Store-level title validation in AddObservation and UpdateObservation internal/store/store.go, internal/store/store_test.go	A new sentinel error ErrEmptyObservationTitle is exported. Both AddObservation and UpdateObservation strip <private> tags from title, trim whitespace, and return the error before any DB operations when the result is empty. Tests assert error messages contain "title is required" for empty and whitespace inputs, that valid titles succeed with non-zero observation IDs, that validation occurs after private-tag stripping (so private-only titles become "[REDACTED]"), and that content-only updates preserve existing titles while applying new content.
Server HTTP error mapping internal/server/server.go, internal/server/server_test.go	handleAddObservation maps store.ErrEmptyObservationTitle to HTTP 400 Bad Request. handleUpdateObservation uses a switch statement to return 400 for empty title errors and 404 for not-found errors. Server integration tests validate POST and PATCH requests with empty/whitespace titles are rejected with 400 and do not modify or create observations (verified by follow-up GET and list operations).
MCP tool-level title validation internal/mcp/mcp.go, internal/mcp/mcp_test.go	handleSave adds an early validation guard returning an MCP tool error for blank titles with an agent-actionable message and cloud sync consequence. handleUpdate similarly validates early. Table-driven tests cover missing, empty, and whitespace-only title cases, verifying error text and confirming no observations are persisted.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 47.06% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: adding validation to reject empty observation titles before database persistence, which is the core fix.
Linked Issues check	✅ Passed	The PR fully addresses all primary coding objectives from issue #459: validation rejects empty/whitespace titles at store layer, returns actionable error messages, prevents persistence, and centralizes validation logic consistently across CLI, MCP, and HTTP APIs.
Out of Scope Changes check	✅ Passed	All changes are directly in scope: store validation layer, MCP handler error mapping, HTTP status code handling, and comprehensive test coverage for the implemented validation. No backfilling of existing observations or additional constraints are introduced.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/store/store_test.go`:
- Around line 8580-8608: The TestAddObservationRejectsEmptyTitle test only
covers titles that are already empty in their raw form, but misses the critical
edge case where a title is non-empty initially but becomes empty after private
tags are stripped (for example, a title containing only
"<private>secret</private>"). Add a new test case to the cases slice in
TestAddObservationRejectsEmptyTitle that covers this scenario by providing a
title that contains only private tags, ensuring this edge path is properly
tested for regression.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f796fa13-2689-4ba2-89f6-f1f7ca11e7ac

📥 Commits

Reviewing files that changed from the base of the PR and between 36c0819 and b4661ac.

📒 Files selected for processing (4)

• internal/mcp/mcp.go
• internal/mcp/mcp_test.go
• internal/store/store.go
• internal/store/store_test.go

[Alan-TheGentleman]

Thanks for tightening the create path. The direction is right, but I am not merging this yet because the invariant is not applied across the full supported surface.

UpdateObservation still accepts empty or whitespace titles, persists them, and enqueues an observation upsert, so mem_update and PATCH /observations/{id} can still create the same stuck sync queue class this PR is meant to prevent. Also, gentle-engram support lives in this repo and needs to stay aligned with Engram behavior, so please verify and cover that supported path as part of this fix rather than treating it as external follow-up.

Once the update path and gentle-engram-supported path are covered with tests, this should be good to re-review.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/server/server_test.go`:
- Around line 172-234: Add a new test function to verify that the POST
/observations endpoint rejects whitespace-only titles with a 400 Bad Request
response and does not persist the observation. Create a test similar to the
existing TestUpdateObservationRejectsEmptyTitle pattern: first establish a
session, then attempt to create an observation with empty string and
whitespace-only title values, asserting that both return HTTP 400 and that no
observation is created (verify by querying the created observation ID or
confirming the count remains zero). This ensures handleAddObservation
store-level empty-title validation is covered across both POST and PATCH routes
as per the PR requirements.

In `@internal/server/server.go`:
- Around line 459-466: The default case in the error handling switch statement
for UpdateObservation currently returns http.StatusNotFound for all unexpected
errors, which incorrectly classifies internal failures as not-found errors.
Change the default branch in this switch statement to return
http.StatusInternalServerError instead of http.StatusNotFound when using the
jsonError function, so that unexpected or unknown errors are properly
communicated as internal server errors rather than resource not-found errors.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bdc76d8f-2928-471a-84a7-6b3b3b93d631

📥 Commits

Reviewing files that changed from the base of the PR and between 8a67216 and 1a0e7a4.

📒 Files selected for processing (5)

• internal/mcp/mcp_test.go
• internal/server/server.go
• internal/server/server_test.go
• internal/store/store.go
• internal/store/store_test.go

[Alan-TheGentleman]

Update path, MCP update, HTTP POST/PATCH status handling, and the gentle-engram/opencode-supported route are now covered. Focused tests pass locally: go test -count=1 ./internal/store ./internal/mcp ./internal/server ./internal/setup. Good to merge.