fix(sync): include memory_relations in local chunk sync#489
Merged
Conversation
Local 'engram sync' wrote chunks containing only sessions, observations, and prompts, silently omitting memory_relations rows. On import the relation graph was lost, violating the 'no silent drops' invariant. Add ExportRelationMutations to query non-orphaned relations per project and append them as explicit relation upsert mutations to the chunk. On import the existing applyRelationUpsertTx restores them idempotently, with observation upserts ordered before relation upserts so FK preconditions hold. The export call runs only in local (non-cloud) mode and propagates errors loudly. Closes #353
📝 WalkthroughWalkthroughThis PR extends local chunk sync to include ChangesLocal chunk sync includes memory_relations via relation mutations
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.go`:
- Around line 3378-3423: ExportRelationMutations currently only enforces
source/target project parity when normalizedProject is provided, causing
unscoped exports to inherit the source project even for cross-project relations;
update the query construction in ExportRelationMutations (the query string,
normalizedProject conditional, and args slice) so that when normalizedProject ==
"" you still add a WHERE clause enforcing coalesce(nullif(src.project,''),
src_s.project,'') = coalesce(nullif(tgt.project,''), tgt_s.project,'') (and keep
the SELECT column for Project as-is), and when normalizedProject != "" keep the
existing equality checks; ensure the args slice is updated only when needed and
that the parity check is included in the final query before executing
s.queryItHook.
🪄 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: 0156b14b-2fe2-4102-a1c8-899f3ed399ac
📒 Files selected for processing (6)
DOCS.mddocs/codebase/sync-and-cloud.mdinternal/store/relations_test.gointernal/store/store.gointernal/sync/sync.gointernal/sync/sync_test.go
Comment on lines
+3378
to
+3423
| query := ` | ||
| SELECT r.sync_id, r.source_id, r.target_id, r.relation, r.reason, r.evidence, r.confidence, | ||
| r.judgment_status, r.marked_by_actor, r.marked_by_kind, r.marked_by_model, | ||
| r.session_id, coalesce(nullif(src.project, ''), src_s.project, ''), r.created_at, r.updated_at | ||
| FROM memory_relations r | ||
| JOIN observations src ON src.sync_id = r.source_id AND src.deleted_at IS NULL | ||
| JOIN observations tgt ON tgt.sync_id = r.target_id AND tgt.deleted_at IS NULL | ||
| LEFT JOIN sessions src_s ON src_s.id = src.session_id | ||
| LEFT JOIN sessions tgt_s ON tgt_s.id = tgt.session_id | ||
| WHERE r.judgment_status != ?` | ||
| args := []any{JudgmentStatusOrphaned} | ||
| if normalizedProject != "" { | ||
| query += ` AND coalesce(nullif(src.project, ''), src_s.project, '') = ? | ||
| AND coalesce(nullif(tgt.project, ''), tgt_s.project, '') = ?` | ||
| args = append(args, normalizedProject, normalizedProject) | ||
| } | ||
| query += ` ORDER BY r.created_at, r.sync_id` | ||
|
|
||
| rows, err := s.queryItHook(s.db, query, args...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("export relation mutations: %w", err) | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| mutations := []SyncMutation{} | ||
| for rows.Next() { | ||
| var p syncRelationPayload | ||
| if err := rows.Scan( | ||
| &p.SyncID, &p.SourceID, &p.TargetID, &p.Relation, &p.Reason, &p.Evidence, &p.Confidence, | ||
| &p.JudgmentStatus, &p.MarkedByActor, &p.MarkedByKind, &p.MarkedByModel, | ||
| &p.SessionID, &p.Project, &p.CreatedAt, &p.UpdatedAt, | ||
| ); err != nil { | ||
| return nil, fmt.Errorf("export relation mutations: scan: %w", err) | ||
| } | ||
| payload, err := json.Marshal(p) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("export relation mutations: marshal %s: %w", p.SyncID, err) | ||
| } | ||
| mutations = append(mutations, SyncMutation{ | ||
| Entity: SyncEntityRelation, | ||
| EntityKey: strings.TrimSpace(p.SyncID), | ||
| Op: SyncOpUpsert, | ||
| Payload: string(payload), | ||
| Project: strings.TrimSpace(p.Project), | ||
| OccurredAt: p.UpdatedAt, | ||
| }) |
There was a problem hiding this comment.
Enforce source/target project parity in unscoped relation export.
ExportRelationMutations only enforces project equality when a project filter is provided. In the unscoped path, Project is derived from the source side only, so a cross-project relation row would be exported with mislabeled project metadata and can break downstream project-scoped sync semantics.
Suggested fix
- query := `
- SELECT r.sync_id, r.source_id, r.target_id, r.relation, r.reason, r.evidence, r.confidence,
- r.judgment_status, r.marked_by_actor, r.marked_by_kind, r.marked_by_model,
- r.session_id, coalesce(nullif(src.project, ''), src_s.project, ''), r.created_at, r.updated_at
+ query := `
+ SELECT r.sync_id, r.source_id, r.target_id, r.relation, r.reason, r.evidence, r.confidence,
+ r.judgment_status, r.marked_by_actor, r.marked_by_kind, r.marked_by_model,
+ r.session_id, coalesce(nullif(src.project, ''), src_s.project, ''), r.created_at, r.updated_at
FROM memory_relations r
JOIN observations src ON src.sync_id = r.source_id AND src.deleted_at IS NULL
JOIN observations tgt ON tgt.sync_id = r.target_id AND tgt.deleted_at IS NULL
LEFT JOIN sessions src_s ON src_s.id = src.session_id
LEFT JOIN sessions tgt_s ON tgt_s.id = tgt.session_id
- WHERE r.judgment_status != ?`
+ WHERE r.judgment_status != ?
+ AND coalesce(nullif(src.project, ''), src_s.project, '') =
+ coalesce(nullif(tgt.project, ''), tgt_s.project, '')`🤖 Prompt for 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.
In `@internal/store/store.go` around lines 3378 - 3423, ExportRelationMutations
currently only enforces source/target project parity when normalizedProject is
provided, causing unscoped exports to inherit the source project even for
cross-project relations; update the query construction in
ExportRelationMutations (the query string, normalizedProject conditional, and
args slice) so that when normalizedProject == "" you still add a WHERE clause
enforcing coalesce(nullif(src.project,''), src_s.project,'') =
coalesce(nullif(tgt.project,''), tgt_s.project,'') (and keep the SELECT column
for Project as-is), and when normalizedProject != "" keep the existing equality
checks; ensure the args slice is updated only when needed and that the parity
check is included in the final query before executing s.queryItHook.
There was a problem hiding this comment.
Pull request overview
Fixes local (non-cloud) engram sync exports that previously omitted memory_relations, causing relation graphs to be dropped on import and violating the “no silent drops” invariant.
Changes:
- Add
Store.ExportRelationMutations(project)to export non-orphaned relation rows as explicitrelationupsert sync mutations. - Extend local chunk export to append relation mutations (and treat mutation-only chunks as non-empty); import reuses the existing mutation apply pipeline with ordering that keeps FK prerequisites intact.
- Add focused store + sync round-trip/idempotency/backward-compat tests and update docs to reflect relations being included in local chunks.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/store/store.go | Adds ExportRelationMutations SQL export of relation upsert mutations scoped by project. |
| internal/sync/sync.go | Local export now includes relation mutations in chunk payload; incremental filtering for relation mutations. |
| internal/store/relations_test.go | Unit tests for relation mutation export: project-scoped, unscoped, orphan-excluded. |
| internal/sync/sync_test.go | End-to-end tests for local chunk export/import with relations, idempotency, backward compatibility, isolation, and error propagation. |
| docs/codebase/sync-and-cloud.md | Documents that local project chunks include relations via sync mutations. |
| DOCS.md | Updates DB/reference docs to reflect relations syncing via local chunks as well as cloud. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| JOIN observations tgt ON tgt.sync_id = r.target_id AND tgt.deleted_at IS NULL | ||
| LEFT JOIN sessions src_s ON src_s.id = src.session_id | ||
| LEFT JOIN sessions tgt_s ON tgt_s.id = tgt.session_id | ||
| WHERE r.judgment_status != ?` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #353
Type
Summary
engram syncsilently omittedmemory_relationsfrom.engram/chunks/*.jsonl.gz, dropping the relation graph on import and violating the documented "no silent drops" invariant.ExportRelationMutationsto query non-orphaned relations per project and append them as explicitrelationupsert mutations to the chunk; import restores them idempotently via the existingapplyRelationUpsertTx, with observation upserts ordered before relation upserts so FK preconditions hold.Changes
internal/store/store.goExportRelationMutations— queries non-orphaned relations for a project with JOIN-verified FK integrityinternal/sync/sync.goIsEmptyaccounts for mutation-only chunksinternal/store/relations_test.gointernal/sync/sync_test.godocs/codebase/sync-and-cloud.mdDOCS.mdTest Plan
go build ./...passesgo vet ./internal/...passesgo test ./internal/sync/... ./internal/store/...passes (sync 89.9%, store 78.7% coverage)Contributor Checklist
status:approved)type:*labelCo-Authored-BytrailersSummary by CodeRabbit
New Features
Documentation
Tests