fix(store): preserve UTF-8 truncation boundaries by dnlrsls · Pull Request #809 · Gentleman-Programming/engram

dnlrsls · 2026-08-26T20:54:30Z

🔗 Linked Issue

Closes #697

🏷️ PR Type

type:bug — Bug fix
type:feature — New feature
type:docs — Documentation only
type:refactor — Code refactoring (no behavior change)
type:chore — Maintenance, dependencies, tooling
type:breaking-change — Breaking change

📝 Summary

Preserve valid UTF-8 when oversized observation or prompt content is truncated.
Keep the existing byte-prefix cap and truncation marker unchanged.
Cover 2-, 3-, and 4-byte rune boundaries across every affected persistence path.

📂 Changes

File	Change
`internal/store/store.go`	Add a shared UTF-8-safe byte-boundary truncation helper and use it for observation add, prompt preparation, and observation update.
`internal/store/store_test.go`	Add boundary and persistence-route regression coverage.

🧪 Test Plan

Focused regression tests pass: go test ./internal/store -run 'Test(TruncateContentPreservesUTF8BytePrefix|PersistenceRoutesTruncateContentAtUTF8Boundary)$'
Full local store package: go test ./internal/store (Windows SQLite TempDir cleanup locks remain in TestMigrate_Idempotent and TestNewErrorBranches; new tests pass)
E2E tests: not run; this change is isolated to store truncation
git diff --check passes
Gentle AI RDD four-lens review approved the frozen candidate

🤖 Automated Checks

These run automatically and all must pass before merge:

Check	What it verifies	Status
Check Issue Reference	PR body contains `Closes #N` / `Fixes #N` / `Resolves #N`	⏳
Check Issue Has status:approved	Linked issue has `status:approved` label	⏳
Check PR Has type: Label*	PR has exactly one `type:*` label	⏳
Unit Tests	`go test ./...` passes	⏳
E2E Tests	`go test -tags e2e ./internal/server/...` passes	⏳

✅ Contributor Checklist

I linked an approved issue above (Closes #697)
I added exactly one type:* label to this PR
I ran unit tests locally: go test ./...
I ran e2e tests locally: go test -tags e2e ./internal/server/...
Docs are not required because the documented byte-limit semantics are unchanged
Commits follow conventional commits format
No Co-Authored-By trailers in commits

💬 Notes for Reviewers

The correction intentionally preserves the existing byte cap instead of converting it to a rune-count cap. When the cap lands inside a rune, the prefix backs up to the rune start before appending the existing marker.

Summary by CodeRabbit

Bug Fixes
- Fixed content truncation to preserve valid UTF-8 characters.
- Prevented multibyte characters from being split at the configured byte limit.
- Ensured truncation markers remain within the limit for saved observations and prompts.
- Applied limits after redaction and reported accurate byte counts.
New Features
- Added truncation metadata and warnings to save, update, and prompt responses.
Documentation
- Clarified byte-based limits, redaction behavior, truncation metadata, and warnings.
Tests
- Added coverage for UTF-8-safe truncation, metadata, warnings, and persistence scenarios.

coderabbitai · 2026-08-26T20:55:29Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ef06ceca-4142-44f1-ae8e-537799443080

📥 Commits

Reviewing files that changed from the base of the PR and between fe25544 and b7ef630.

📒 Files selected for processing (5)

DOCS.md
internal/mcp/mcp.go
internal/mcp/mcp_test.go
internal/store/store.go
internal/store/store_test.go

📝 Walkthrough

Walkthrough

The store now applies shared, redaction-aware UTF-8-safe truncation to observations and prompts. MCP write tools return byte-based truncation metadata and warnings. Tests cover persistence paths, multibyte content, and redacted content.

Changes

Content truncation and reporting

Layer / File(s)	Summary
Shared storage truncation `internal/store/store.go`, `internal/store/store_test.go`	`prepareStoredContent` redacts content, reports byte metadata, and truncates at a valid UTF-8 boundary. Observation and prompt persistence paths use the shared helper. Tests validate byte limits and persisted UTF-8 content.
MCP response metadata `internal/mcp/mcp.go`, `internal/mcp/mcp_test.go`	`mem_save`, `mem_update`, and `mem_save_prompt` return truncation metadata and byte-based warnings. Tests cover multibyte content, redacted byte counts, and normal responses.
Documentation contract `DOCS.md`	The save and prompt documentation describes post-redaction UTF-8 byte limits, metadata, and truncation warnings.

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

Suggested reviewers: gentleman-programming, alan-thegentleman

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR fixes UTF-8 boundary corruption across the three storage truncation paths and adds regression tests [`#697`]. It does not implement the linked issue's prompt truncation warning or resolve the doc…	Add prompt-path truncation signaling and emit the warning from mem_save_prompt. Also align the cap, reported counts, warning text, and documentation on one unit, or update the linked issue scope if those requirements are intentionally defer…
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: preserving UTF-8 boundaries during store truncation.
Out of Scope Changes check	✅ Passed	All production and test changes support UTF-8-safe truncation in the affected store persistence paths. No unrelated changes are present.

Full details: Linked Issues check

Explanation

The PR fixes UTF-8 boundary corruption across the three storage truncation paths and adds regression tests [#697]. It does not implement the linked issue's prompt truncation warning or resolve the documented byte-versus-character reporting requirements [#697].

Resolution

Add prompt-path truncation signaling and emit the warning from mem_save_prompt. Also align the cap, reported counts, warning text, and documentation on one unit, or update the linked issue scope if those requirements are intentionally deferred.

Fix all pre-merge checks with AI

✨ 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

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

Alan-TheGentleman

Requesting changes. mem_save_prompt still truncates silently, and its documented character limit is implemented in bytes. Expose post-redaction truncation metadata and provide warning parity for callers. Add unit coverage for the unit contract and truncation behavior before treating #697 as closed.

Alan-TheGentleman

The requested prompt warning, post-redaction byte metadata, and regression coverage are present. The exact correction passed RDD, full local tests, real MCP process validation, and all required CI checks. Approved for merge.

…le-guard Current main already landed the observation title/content admission sentinels (ErrObservationTitleRequired, ErrObservationContentRequired), their HTTP 400 mapping and UTF-8-safe content preparation. This merge keeps all of that and reduces this branch to the part main does not cover: rejecting a titleless write before it produces a side effect. Conflict resolutions: - internal/store/store.go, AddObservation: kept main's prepareStoredContent (UTF-8 truncation boundaries, Gentleman-Programming#809) and the content guard; the branch's inline truncation and duplicate title check were dropped in favour of ValidateObservationTitle. - internal/store/store.go, UpdateObservation: kept main's pre-transaction admission and removed the branch's duplicate in-transaction title check, so a rejected update still opens no transaction. - internal/server/server.go, both handlers: kept main's switch mapping title and content errors to 400, and kept this branch's reordering so POST /observations validates the title before the session lookup. - internal/store/diagnostic.go: dropped the branch's second ErrObservationTitleRequired declaration, which collided with main's sentinel. ValidateObservationTitle now lives beside the write paths in store.go, and ValidateSyncMutationPayload keeps main's require("title") because its field() accessor already trims. Behaviour kept from this branch: engram save and mem_save reject a titleless write before opening the store or creating a session, and the HTTP create path validates the title before the session lookup so a bad session or project can no longer mask the documented 400.

fix(store): preserve UTF-8 truncation boundaries

fe25544

dnlrsls added the type:bug Bug fix label Aug 26, 2026

dpaul20 mentioned this pull request Aug 26, 2026

feat(store,mcp): add partial reads to mem_get_observation (offset/limit + find/context) #812

Open

2 tasks

Alan-TheGentleman requested changes Aug 27, 2026

View reviewed changes

Alan-TheGentleman added 2 commits August 27, 2026 15:28

fix(mcp): report post-redaction byte truncation

1744bb8

Merge branch 'main' into fix/utf8-truncation-boundary

b7ef630

Alan-TheGentleman approved these changes Aug 27, 2026

View reviewed changes

Alan-TheGentleman merged commit afe7011 into Gentleman-Programming:main Aug 27, 2026
5 checks passed

Alan-TheGentleman mentioned this pull request Aug 27, 2026

fix(store): reject empty observation titles on create and update #802

Merged

12 tasks

Navigation Menu

Uh oh!

fix(store): preserve UTF-8 truncation boundaries - #809

fix(store): preserve UTF-8 truncation boundaries#809
Alan-TheGentleman merged 3 commits into
Gentleman-Programming:mainfrom
dnlrsls:fix/utf8-truncation-boundary

dnlrsls commented Aug 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 26, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

Alan-TheGentleman left a comment

Uh oh!

Alan-TheGentleman left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dnlrsls commented Aug 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Linked Issue

🏷️ PR Type

📝 Summary

📂 Changes

🧪 Test Plan

🤖 Automated Checks

✅ Contributor Checklist

💬 Notes for Reviewers

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

Alan-TheGentleman left a comment

Choose a reason for hiding this comment

Uh oh!

Alan-TheGentleman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dnlrsls commented Aug 26, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 26, 2026 •

edited

Loading