fix(store): reject empty observation titles before persistence

[ceeesar13]

🔗 Linked Issue

Closes #459

Related: #629 (doctor/repair does not scan the observations table for the same empty fields) and #340 (umbrella for legacy required-field repair). This PR is the write-side half only — it stops new malformed rows being created. It does not repair rows that already exist; that remains #629/#340 territory.

---

🏷️ PR Type

• type:bug — Bug fix

---

📝 Summary

• Rejects observations with an empty or whitespace-only title before persistence, at every first-party write entrypoint.
• Reuses the rule ValidateSyncMutationPayload already owned instead of adding a second title check, per @ricardoarz-dev's review note on the issue — the invariant now has exactly one definition.
• Turns a silent write that freezes a project's cloud sync days later into an immediate, actionable error at the call site.

📂 Changes

File	Change
internal/store/diagnostic.go	New ErrObservationTitleRequired, ValidateObservationTitle, and the private predicate observationTitleIsPresent. ValidateSyncMutationPayload's observation-upsert branch now calls that predicate instead of its inline require("title").
internal/store/store.go	AddObservation validates the post-stripPrivateTags title before opening the transaction, so nothing is inserted and no sync_mutations row is enqueued.
internal/mcp/mcp.go	mem_save returns a tool error on a blank title, before project resolution or session creation. Placed after the existing content is required guard so error ordering is unchanged.
cmd/engram/main.go	engram save fails with a non-zero exit and the actionable message, before opening the store.
internal/server/server.go	POST /observations maps ErrObservationTitleRequired to 400 instead of 500.
DOCS.md, CHANGELOG.md	Documented the 400 and the cross-entrypoint rule; Unreleased entry.

🧪 Test Plan

New tests:

• internal/store — empty / whitespace-only / empty-after-strip all rejected, asserting no observation row and no sync_mutations row; valid titles still accepted with exactly one enqueued mutation; plus TestValidateObservationTitleMatchesSyncPayloadRule pinning that the write-side and payload-side rules agree.
• internal/mcp — mem_save rejects missing / empty / whitespace titles, nothing persisted, no mutation queued.
• internal/server — 400 for "" and " ", onWrite never fires.
• cmd/engram — non-zero exit, stderr carries the message and the cloud-sync explanation.

Commands:

go test -tags e2e ./internal/server/...
ok  github.com/Gentleman-Programming/engram/internal/server  8.294s

go test ./...

All new tests pass. go test ./... is not fully green on Windows, and is equally not green at main — I verified by stashing this change and re-running on the clean tree, where every failure reproduces identically. Pre-existing failures: cmd/engram (TestCmdExportDefaultAndCmdImportErrors path-string mismatch, TestPrintUsage hang), internal/setup (tests touching the real home config), internal/store (TestMigrate_Idempotent — SQLite file lock on TempDir cleanup), internal/sync (TestManifestReadWrite error-string mismatch), internal/obsidian (watcher timing flake). All are Windows-environment artifacts unrelated to this change. Every package this PR touches passes apart from those.

📌 Notes for reviewers

stripPrivateTags does not produce an empty title. It substitutes the literal [REDACTED], so a title consisting only of a private tag is non-empty and is accepted. I pinned that existing behavior in a test rather than changing it — rejecting [REDACTED] titles is a separate product decision.

Inbound paths are deliberately not guarded, and do not need to be. Cloud pull (applyPulledMutationTx → applyObservationUpsertTx) and engram import (Store.Import) write observations with direct SQL and never reach AddObservation, so no legitimate inbound record can be blocked by this change.

Store.PassiveCapture does route through AddObservation. ExtractLearnings returns trimmed non-empty strings so it is unaffected; a blank learning would now error instead of poisoning the queue, which is the intended direction.

Known remaining gap, intentionally out of scope. Store.UpdateObservation also enqueues an observation upsert from a stripPrivateTags'd title, so PATCH /observations/{id} with {"title":""} can still produce the same head-of-line block. It is the identical one-line guard, but it is a different entrypoint than #459 enumerates, so I kept this PR to one logical change. Happy to open a follow-up issue.

Why this matters

Verified in production on v1.20.0: 19 of 1565 observations (1.2%) carried title = '', and that was enough to keep cloud sync dead for 3 days across two projects. Because the mutation queue is an ordered log and the pusher stops at the first invalid row, one bad record blocks every later one — including valid records queued behind it. Full measurements in the issue thread.

Summary by CodeRabbit

• Bug Fixes
  • Observation saves, API creates, and MCP mem_save now reject missing, empty, or whitespace-only titles before writing or syncing.
  • The CLI and MCP return clear “observation title is required” errors.
  • POST /observations responds with 400 Bad Request, with title validation taking precedence over session/project issues.
  • Prevents invalid upsert attempts from blocking later project mutations during cloud sync.
• Documentation
  • Updated the changelog and API reference to reflect the stricter title validation and sync impact.

[coderabbitai]

📝 Walkthrough

Walkthrough

Observation title validation is centralized in the store and enforced before persistence across CLI, MCP, and HTTP write paths. Invalid titles return path-specific errors, create no observation or sync mutation, and are documented and tested.

Changes

Observation title validation

Layer / File(s)	Summary
Shared store validation internal/store/diagnostic.go, internal/store/store.go, internal/store/store_test.go	Adds a shared required-title error and validation rule, rejects invalid titles before persistence, aligns sync payload validation, and tests valid and invalid cases.
CLI and MCP write guards cmd/engram/main.go, cmd/engram/main_extra_test.go, internal/mcp/mcp.go, internal/mcp/mcp_test.go	CLI and MCP save paths reject missing, empty, or whitespace-only titles before creating sessions or mutations, with regression tests.
HTTP response handling and documentation internal/server/server.go, internal/server/server_test.go, DOCS.md, CHANGELOG.md	Maps invalid observation titles to HTTP 400, verifies rejected callbacks, and documents the validation and sync behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: alan-thegentleman, gentleman-programming

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: rejecting empty observation titles before persistence.
Linked Issues check	✅ Passed	The changes address #459 by rejecting empty or whitespace-only titles across CLI, MCP, store writes, and related validation paths.
Out of Scope Changes check	✅ Passed	No clearly unrelated code changes are present; the docs and HTTP validation updates stay within the title-validation fix.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[ceeesar13]

Could a maintainer apply the type:bug label? CONTRIBUTING requires exactly one type:* label and the Check PR Has type:* Label check will stay red until then — as an external contributor I get does not have the correct permissions to execute AddLabelsToLabelable when I try to set it myself.

[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 `@DOCS.md`:
- Line 137: Update the documentation sentence describing title validation to
scope it only to the listed observation-create paths: engram save, mem_save, and
POST /observations. Remove the claim that the rule applies to every write path,
while preserving the cloud-sync rejection behavior for observation upserts
without a title.

In `@internal/server/server.go`:
- Around line 340-344: Validate the decoded observation title with
store.ValidateObservationTitle immediately after JSON decoding and before
validateSessionProject, while retaining AddObservation validation as a backstop.
In internal/server/server.go lines 340-344, preserve the existing title-specific
400 mapping. In internal/server/server_test.go lines 2068-2080, add a
whitespace-only title request using a nonexistent or mismatched session and
assert the title-validation 400 response.

🪄 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: f0c73d33-fa34-4bbe-a7fd-00de4de533d1

📥 Commits

Reviewing files that changed from the base of the PR and between 763a6ba and 4b4fc9e.

📒 Files selected for processing (11)

• CHANGELOG.md
• DOCS.md
• cmd/engram/main.go
• cmd/engram/main_extra_test.go
• internal/mcp/mcp.go
• internal/mcp/mcp_test.go
• internal/server/server.go
• internal/server/server_test.go
• internal/store/diagnostic.go
• internal/store/store.go
• internal/store/store_test.go

[ceeesar13]

Rebased onto main (the branch was 36 commits behind and conflicting — now mergeable) and addressed both review comments. Pushed as a separate commit so the delta is reviewable on its own.

internal/server/server.go — valid, and the failure mode was real. store.ValidateObservationTitle(body.Title) now runs immediately after JSON decoding, before validateSessionProject, so a whitespace-only title can no longer be masked by a session/project error. The AddObservation mapping is retained as the backstop, and the existing combined session_id, title, and content are required 400 is untouched. New test TestHandleAddObservationBlankTitleNotMaskedBySessionError sends " " with a nonexistent session and asserts the title-specific 400.

DOCS.md — valid, the sentence overstated the guarantee. It now scopes the rule to the observation-create paths and says explicitly that PATCH /observations/{id} is not covered, which matches the out-of-scope note in the PR description.

Verification after the rebase:

go test ./internal/store/... ./internal/server/... ./internal/mcp/...
  ok  internal/server, internal/mcp
  FAIL internal/store — TestMigrate_Idempotent, TestNewErrorBranches

go test -count=1 -tags e2e ./internal/server/...
  ok  9.246s

Both internal/store failures are pre-existing and unrelated. I confirmed by checking out upstream/main in a separate worktree and running those two tests on the clean tree, where they reproduce identically:

--- FAIL: TestMigrate_Idempotent
    TempDir RemoveAll cleanup: unlinkat ...\engram.db:
    The process cannot access the file because it is being used by another process.

A Windows SQLite file-lock artifact on TempDir cleanup, not an assertion failure. All tests added by this PR pass.

Also confirmed against upstream's newer commits: Store.Import (#616) writes observations with direct SQL inside its own transaction and never calls AddObservation, so the import-dedup work does not interact with this guard and inbound cloud-pull writes remain unaffected.

[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 `@cmd/engram/main.go`:
- Around line 1035-1040: Apply title redaction before validation at every
observation entry point, reusing the store’s private-tag stripping behavior
before ValidateObservationTitle: update cmd/engram/main.go:1035-1040 before
storeNew/CreateSession, internal/mcp/mcp.go:1198-1202 before project resolution
and implicit-session creation, and internal/server/server.go:330-336 before
validateSessionProject. Add private-tag-only title cases in
cmd/engram/main_extra_test.go:4331-4348, internal/mcp/mcp_test.go:7459-7511, and
internal/server/server_test.go:2174-2259, asserting respectively no storeNew
call, no session/observation/sync mutation, and a title-specific 400 even with
an invalid session.

🪄 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: b7b376e9-4a05-42ca-8705-469b7a0c4b41

📥 Commits

Reviewing files that changed from the base of the PR and between 4b4fc9e and b10276c.

📒 Files selected for processing (11)

• CHANGELOG.md
• DOCS.md
• cmd/engram/main.go
• cmd/engram/main_extra_test.go
• internal/mcp/mcp.go
• internal/mcp/mcp_test.go
• internal/server/server.go
• internal/server/server_test.go
• internal/store/diagnostic.go
• internal/store/store.go
• internal/store/store_test.go