fix(store): backfill relation sync mutations so relations reach cloud#497
Merged
Conversation
Relations in memory_relations could lack a sync_mutations row and never replicate to Engram Cloud (journal-based), silently. Cloud-journal sibling of #353 (local chunk path). - Add backfillRelationSyncMutationsTx mirroring the observation backfill (two-phase collect-then-insert, non-orphaned, NOT EXISTS guard). - Call it from backfillProjectSyncMutationsTx (startup repair, enroll, rename, merge). - Count non-orphaned relations missing a mutation in projectNeedsBackfill. - Enqueue from JudgeBySemantic on the enrolled path (mem_compare/ScanProject verdicts previously never enqueued). Closes #496
📝 WalkthroughWalkthroughAdds cloud sync journal coverage for ChangesCloud Relation Backfill (Issue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@forNerzul concrete review request: this is the #496 fix ready for your eyes. GitHub will not let me add you as a formal reviewer (external/fork contributor), so flagging it here directly. Could you review the diff and validate it against your cloud-enrolled git fetch origin fix/cloud-relation-backfill
git checkout fix/cloud-relation-backfill
go build -o ./engram ./cmd/engramWhat to confirm:
If your own intended fix diverged in approach (you described mirroring the observation/session/prompt backfill — which is what this does), call it out and we will reconcile before merge. Trying to land this soon, so your validation is the last gate. |
![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/relations.go`:
- Around line 818-865: The JudgeBySemantic function is missing a warning log
that exists in JudgeRelation (lines 660-662) for the case when the source
observation is absent locally. After determining that srcProject is empty and
setting enrollCheckProject to tgtProject as a fallback, add a warning log to
document this edge case so the server's rejection of the mutation is properly
recorded in observability logs. This ensures consistent logging behavior between
JudgeRelation and JudgeBySemantic for the same missing source observation
scenario.
🪄 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: a0a75df1-ed7d-4537-8f83-72e7a75033d3
📒 Files selected for processing (4)
internal/mcp/mcp_conflict_loop_test.gointernal/store/relation_backfill_test.gointernal/store/relations.gointernal/store/store.go
There was a problem hiding this comment.
Pull request overview
This PR fixes a cloud-sync replication gap where memory_relations rows could exist without corresponding sync_mutations entries, preventing relations from reaching Engram Cloud’s journal-based sync pipeline.
Changes:
- Add relation backfill (
backfillRelationSyncMutationsTx) and wire it intobackfillProjectSyncMutationsTx; extendprojectNeedsBackfillto detect missing relation mutations. - Update
JudgeBySemanticto enqueue relation sync mutations when the project is enrolled (mirroringJudgeRelation’s enrollment gate). - Add/extend tests to cover relation backfill behavior, enrollment gating, and end-to-end enrollment backfill; update MCP conflict-loop test comments to reflect the enrollment-gate invariant.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/store/store.go | Adds relation mutation backfill and updates startup “needs backfill” detection; updates helper comment describing the intended enrollment-gate invariant. |
| internal/store/relations.go | Enqueues relation sync mutations from JudgeBySemantic when enrolled (previously never enqueued). |
| internal/store/relation_backfill_test.go | Adds regression tests for relation backfill + enrollment gating + enrollment-triggered backfill. |
| internal/mcp/mcp_conflict_loop_test.go | Updates REQ-009-era comments to clarify the invariant is “unenrolled projects must not enqueue relation mutations.” |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+5048
to
+5064
| { | ||
| // Count non-orphaned relations whose source and target observations are | ||
| // locally available and that have no local upsert sync_mutations row. | ||
| // Mirrors the SELECT in backfillRelationSyncMutationsTx. | ||
| q: `SELECT COUNT(*) | ||
| 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 | ||
| WHERE r.judgment_status != ? | ||
| AND coalesce(nullif(src.project, ''), src_s.project, '') = ? | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM sync_mutations sm | ||
| WHERE sm.target_key = ? AND sm.entity = ? AND sm.entity_key = r.sync_id AND sm.source = ? | ||
| )`, | ||
| args: []any{JudgmentStatusOrphaned, project, DefaultSyncTargetKey, SyncEntityRelation, SyncSourceLocal}, | ||
| }, |
Comment on lines
+5409
to
+5423
| 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 | ||
| WHERE r.judgment_status != ? | ||
| AND coalesce(nullif(src.project, ''), src_s.project, '') = ? | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM sync_mutations sm | ||
| WHERE sm.target_key = ? | ||
| AND sm.entity = ? | ||
| AND sm.entity_key = r.sync_id | ||
| AND sm.source = ? | ||
| ) | ||
| ORDER BY r.created_at ASC, r.sync_id ASC`, | ||
| JudgmentStatusOrphaned, | ||
| project, | ||
| DefaultSyncTargetKey, SyncEntityRelation, SyncSourceLocal, |
Comment on lines
+821
to
+827
| var srcProject, tgtProject string | ||
| _ = tx.QueryRow( | ||
| `SELECT ifnull(project,'') FROM observations WHERE sync_id = ?`, p.SourceID, | ||
| ).Scan(&srcProject) | ||
| _ = tx.QueryRow( | ||
| `SELECT ifnull(project,'') FROM observations WHERE sync_id = ?`, p.TargetID, | ||
| ).Scan(&tgtProject) |
Comment on lines
+43
to
+49
| if _, err := s.db.Exec(` | ||
| INSERT INTO memory_relations | ||
| (sync_id, source_id, target_id, relation, judgment_status, created_at, updated_at) | ||
| VALUES (?, ?, ?, 'related', ?, datetime('now'), datetime('now')) | ||
| `, syncID, sourceID, targetID, judgmentStatus); err != nil { | ||
| t.Fatalf("insertRelationDirect: %v", err) | ||
| } |
…ject via session - Tighten backfillRelationSyncMutationsTx and projectNeedsBackfill to exclude pending/orphaned rows and any row missing marked_by_actor or marked_by_kind. Both predicates are now identical to prevent the fast-path check from desyncing with the write path. - JudgeBySemantic now derives srcProject via session-fallback (coalesce(obs.project, session.project)) matching the backfill SQL, so enqueued payloads carry a non-empty project when obs.project is blank. - Add REQ-011 WARNING log in JudgeBySemantic when srcProject is empty, matching the wording in JudgeRelation. - Update insertRelationDirect fixture usages to insertJudgedRelationDirect (with marked_by_actor/kind) for judged-relation test cases. - Add tests: pending row not backfilled, projectNeedsBackfill not triggered by pending row, JudgeBySemantic session-fallback populates payload project.
Collaborator
Author
|
Addressed all CodeRabbit + Copilot findings in
Added two regression tests: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/store/relations.go (1)
843-857:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove the REQ-011 warning below the enrollment gate.
When
enrollCheckProjectis not enrolled, Line 857 returns before any sync mutation is queued, but this warning has already logged"enqueueing relation ...". That creates false-positive breadcrumbs for the intentionally skipped path.Suggested fix
- // REQ-011: log at WARNING level when source observation is missing locally - // (project='' race condition). The server will reject with 400; this log - // is the local breadcrumb so the gap is not silently swallowed. - if srcProject == "" { - log.Printf("[store] WARNING: JudgeBySemantic enqueueing relation %s with project='' (source observation missing locally); server will reject", existingSyncID) - } - var enrolled int if err := tx.QueryRow( `SELECT 1 FROM sync_enrolled_projects WHERE project = ? LIMIT 1`, enrollCheckProject, ).Scan(&enrolled); err != nil && err != sql.ErrNoRows { return fmt.Errorf("JudgeBySemantic: check enrollment: %w", err) @@ if enrolled == 0 { return nil // not enrolled — backfill will cover it on enrollment } + + // REQ-011: log at WARNING level when source observation is missing locally + // (project='' race condition). The server will reject with 400; this log + // is the local breadcrumb so the gap is not silently swallowed. + if srcProject == "" { + log.Printf("[store] WARNING: JudgeBySemantic enqueueing relation %s with project='' (source observation missing locally); server will reject", existingSyncID) + }🤖 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/relations.go` around lines 843 - 857, The REQ-011 warning logs that a relation is being enqueued even when the enrollment check at the enrolled == 0 gate will cause the function to return early without actually enqueueing anything, creating false-positive breadcrumbs. Move the warning log block (the if srcProject == "" conditional that logs the WARNING message) to execute after the enrollment check, specifically after the if enrolled == 0 return statement, so the warning only logs when the relation will actually be enqueued and not for intentionally skipped paths.internal/store/relation_backfill_test.go (1)
371-393: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider mirroring the full backfill predicate in the manual SQL query.
The manual SQL verification query (lines 374-383) simplifies the real
projectNeedsBackfillrelation predicate by omitting:
- Pending status exclusion (
r.judgment_status NOT IN (?, ?))marked_by_actorandmarked_by_kindchecks- Project filter via
coalesce(nullif(src.project, ''), src_s.project, '') = ?- Session LEFT JOIN for project derivation
While the test passes correctly for the current scenario (only an orphaned relation is inserted), the simplified query could produce false positives if test data changes or if a future maintainer adds non-orphaned, unmarked relations. Mirroring the exact predicate from context snippet line 5046–5061 would improve clarity and robustness.
♻️ Proposed alignment with real predicate
var n int err := tx.QueryRow(` SELECT COUNT(*) 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 - WHERE r.judgment_status != ? + LEFT JOIN sessions src_s ON src_s.id = src.session_id + WHERE r.judgment_status NOT IN (?, ?) + AND ifnull(r.marked_by_actor, '') != '' + AND ifnull(r.marked_by_kind, '') != '' + AND coalesce(nullif(src.project, ''), src_s.project, '') = ? AND NOT EXISTS ( SELECT 1 FROM sync_mutations sm WHERE sm.target_key = ? AND sm.entity = ? AND sm.entity_key = r.sync_id AND sm.source = ? ) - `, JudgmentStatusOrphaned, DefaultSyncTargetKey, SyncEntityRelation, SyncSourceLocal).Scan(&n) + `, JudgmentStatusOrphaned, JudgmentStatusPending, "proj-orph", DefaultSyncTargetKey, SyncEntityRelation, SyncSourceLocal).Scan(&n)🤖 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/relation_backfill_test.go` around lines 371 - 393, The manual SQL verification query in the test does not fully match the real projectNeedsBackfill relation predicate. Update the SQL query in the withTx block to include all the filtering conditions from the actual predicate: add the pending status exclusion check (judgment_status NOT IN), include the marked_by_actor and marked_by_kind conditions, add the project filter with the coalesce/nullif logic, and perform the necessary LEFT JOIN with the session table for project derivation. This ensures the test validates against the exact same logic as the production code and prevents false positives if test data changes in the future.
🤖 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/relations.go`:
- Around line 824-840: The validateCrossProjectGuard check is performed against
only the observation.project columns, but the srcProject and tgtProject
resolution here includes session fallback logic when observations.project is
blank. This creates a gap where relations with blank project values in
observations but different projects in their associated sessions will
incorrectly pass the guard. Update the validateCrossProjectGuard to apply the
same session-fallback logic used for resolving srcProject and tgtProject,
ensuring it queries both observation and session project columns with the same
coalesce/nullif pattern before comparing the resolved projects.
---
Outside diff comments:
In `@internal/store/relation_backfill_test.go`:
- Around line 371-393: The manual SQL verification query in the test does not
fully match the real projectNeedsBackfill relation predicate. Update the SQL
query in the withTx block to include all the filtering conditions from the
actual predicate: add the pending status exclusion check (judgment_status NOT
IN), include the marked_by_actor and marked_by_kind conditions, add the project
filter with the coalesce/nullif logic, and perform the necessary LEFT JOIN with
the session table for project derivation. This ensures the test validates
against the exact same logic as the production code and prevents false positives
if test data changes in the future.
In `@internal/store/relations.go`:
- Around line 843-857: The REQ-011 warning logs that a relation is being
enqueued even when the enrollment check at the enrolled == 0 gate will cause the
function to return early without actually enqueueing anything, creating
false-positive breadcrumbs. Move the warning log block (the if srcProject == ""
conditional that logs the WARNING message) to execute after the enrollment
check, specifically after the if enrolled == 0 return statement, so the warning
only logs when the relation will actually be enqueued and not for intentionally
skipped paths.
🪄 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: f77bee98-66b3-42c6-a8fa-99ac1ca23363
📒 Files selected for processing (3)
internal/store/relation_backfill_test.gointernal/store/relations.gointernal/store/store.go
…-011 log placement - validateCrossProjectGuard now uses coalesce(nullif(o.project,''), s.project, '') with LEFT JOIN sessions for both source and target, matching the JudgeBySemantic project-derivation query so blank observations.project cannot bypass the guard when session projects differ (closes cross-project guard bypass via session fallback) - Move REQ-011 warning log in JudgeBySemantic to after the enrollment gate so it only fires when a mutation will actually be enqueued, matching JudgeRelation ordering - Add regression tests: TestCrossProjectGuard_SessionFallback_RejectsDifferentSessionProjects and TestCrossProjectGuard_SessionFallback_AllowsSameSessionProject
Collaborator
Author
|
Both CodeRabbit findings addressed in
Added |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
internal/store/store.go:6762
- CountRelationSyncMutations currently counts all sync_mutations whose entity is not session/observation/prompt. That’s broader than "relation" and will start failing the enrollment-gate test if any new non-relation entity is added to the journal in the future. Since the helper is now specifically about relation enrollment gating, it should count only entity = SyncEntityRelation.
err := s.db.QueryRow(`
SELECT count(*)
FROM sync_mutations
WHERE entity NOT IN ('session', 'observation', 'prompt')
`).Scan(&count)
internal/store/store.go:5012
- The doc comment for projectNeedsBackfill is now outdated: it says it covers sessions/live observations/prompts and runs three COUNT queries, but relations are now included and the function runs four COUNTs. Updating the comment will keep future readers from missing the relation backfill fast-path dependency.
// projectNeedsBackfill returns true when a project has any sessions, live observations,
// or prompts that are missing a corresponding sync_mutation row.
// It runs three lightweight COUNT queries — no cursor is held open.
func (s *Store) projectNeedsBackfill(project string) (bool, error) {
Comment on lines
701
to
+705
| func validateCrossProjectGuard(tx *sql.Tx, sourceID, targetID string) error { | ||
| var srcProject, tgtProject string | ||
| _ = tx.QueryRow( | ||
| `SELECT ifnull(project,'') FROM observations WHERE sync_id = ?`, sourceID, | ||
| `SELECT coalesce(nullif(o.project,''), s.project, '') | ||
| FROM observations o |
Comment on lines
+830
to
+842
| var srcProject, tgtProject string | ||
| _ = tx.QueryRow( | ||
| `SELECT coalesce(nullif(o.project,''), s.project, '') | ||
| FROM observations o | ||
| LEFT JOIN sessions s ON s.id = o.session_id | ||
| WHERE o.sync_id = ?`, p.SourceID, | ||
| ).Scan(&srcProject) | ||
| _ = tx.QueryRow( | ||
| `SELECT coalesce(nullif(o.project,''), s.project, '') | ||
| FROM observations o | ||
| LEFT JOIN sessions s ON s.id = o.session_id | ||
| WHERE o.sync_id = ?`, p.TargetID, | ||
| ).Scan(&tgtProject) |
Comment on lines
+375
to
+384
| err := tx.QueryRow(` | ||
| SELECT COUNT(*) 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 | ||
| WHERE r.judgment_status != ? | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM sync_mutations sm | ||
| WHERE sm.target_key = ? AND sm.entity = ? AND sm.entity_key = r.sync_id AND sm.source = ? | ||
| ) | ||
| `, JudgmentStatusOrphaned, DefaultSyncTargetKey, SyncEntityRelation, SyncSourceLocal).Scan(&n) |
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/relations.go`:
- Around line 704-713: The JudgeRelation function currently derives srcProject
and tgtProject from observations.project alone, which doesn't account for the
session fallback logic. This creates inconsistency with the session-fallback
project lookup pattern shown in the diff that uses
coalesce(nullif(o.project,''), s.project, ''). Update the JudgeRelation function
to use the same session-fallback query pattern when determining srcProject and
tgtProject before the enrollment check, replacing the direct
observations.project lookups with queries that LEFT JOIN to sessions and apply
the coalesce-nullif logic to properly handle blank observation projects that
should fall back to the session project value.
🪄 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: b9afc659-4dbe-467b-8ea6-bb2b6115ac8b
📒 Files selected for processing (2)
internal/store/relation_backfill_test.gointernal/store/relations.go
Replace the plain `SELECT ifnull(project,'') FROM observations` lookups
for srcProject/tgtProject in JudgeRelation with the session-fallback
form already used by JudgeBySemantic:
SELECT coalesce(nullif(o.project,''), s.project, '')
FROM observations o
LEFT JOIN sessions s ON s.id = o.session_id
WHERE o.sync_id = ?
Without this, enrolled-project relations whose observations carry a blank
observations.project (project lives only on the session) resolve
srcProject="" → enrollCheckProject="" → enrolled=0 → return nil with no
sync_mutations row, recreating the #496 replication gap for the
JudgeRelation path. The fix makes enqueue behaviour consistent with
JudgeBySemantic and ensures the payload Project is non-empty.
Adds TestJudgeRelation_UsesSessionFallback_ForProject (TDD: red before
fix, green after) which covers both the missing mutation row and the
empty payload.project assertions.
Collaborator
Author
|
Good catch — addressed in Fixed by giving SELECT coalesce(nullif(o.project,""), s.project, "") FROM observations o LEFT JOIN sessions s ON s.id = o.session_id WHERE o.sync_id = ?Now Added |
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/relations.go`:
- Around line 599-610: The code at the two QueryRow().Scan() calls for
srcProject and tgtProject is ignoring errors by using underscore assignments
instead of capturing and handling them explicitly. This allows database errors
to silently result in empty project values, which can cause incorrect
cross-project behavior or skip important operations. Replace the underscore with
an error variable for both Scan calls and add explicit error handling that
checks if the error is not nil and either returns the error, logs it, or takes
appropriate action to prevent the query from silently failing and leaving
srcProject and tgtProject as empty strings.
🪄 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: a91b5927-28b9-4328-9da2-143ecacbac14
📒 Files selected for processing (2)
internal/store/relation_backfill_test.gointernal/store/relations.go
Comment on lines
599
to
610
| _ = tx.QueryRow( | ||
| `SELECT ifnull(project,'') FROM observations WHERE sync_id = ?`, sourceID, | ||
| `SELECT coalesce(nullif(o.project,''), s.project, '') | ||
| FROM observations o | ||
| LEFT JOIN sessions s ON s.id = o.session_id | ||
| WHERE o.sync_id = ?`, sourceID, | ||
| ).Scan(&srcProject) | ||
| _ = tx.QueryRow( | ||
| `SELECT ifnull(project,'') FROM observations WHERE sync_id = ?`, targetID, | ||
| `SELECT coalesce(nullif(o.project,''), s.project, '') | ||
| FROM observations o | ||
| LEFT JOIN sessions s ON s.id = o.session_id | ||
| WHERE o.sync_id = ?`, targetID, | ||
| ).Scan(&tgtProject) |
There was a problem hiding this comment.
Handle project-lookup query errors explicitly instead of silently treating failures as empty project values.
At Line 599 and Line 605, Scan errors are ignored. A real DB error then becomes srcProject/tgtProject == "", which can silently skip enqueue or alter cross-project behavior.
Suggested fix
var srcProject, tgtProject string
- _ = tx.QueryRow(
+ if err := tx.QueryRow(
`SELECT coalesce(nullif(o.project,''), s.project, '')
FROM observations o
LEFT JOIN sessions s ON s.id = o.session_id
WHERE o.sync_id = ?`, sourceID,
- ).Scan(&srcProject)
- _ = tx.QueryRow(
+ ).Scan(&srcProject); err != nil && err != sql.ErrNoRows {
+ return fmt.Errorf("JudgeRelation: resolve source project: %w", err)
+ }
+ if err := tx.QueryRow(
`SELECT coalesce(nullif(o.project,''), s.project, '')
FROM observations o
LEFT JOIN sessions s ON s.id = o.session_id
WHERE o.sync_id = ?`, targetID,
- ).Scan(&tgtProject)
+ ).Scan(&tgtProject); err != nil && err != sql.ErrNoRows {
+ return fmt.Errorf("JudgeRelation: resolve target project: %w", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _ = tx.QueryRow( | |
| `SELECT ifnull(project,'') FROM observations WHERE sync_id = ?`, sourceID, | |
| `SELECT coalesce(nullif(o.project,''), s.project, '') | |
| FROM observations o | |
| LEFT JOIN sessions s ON s.id = o.session_id | |
| WHERE o.sync_id = ?`, sourceID, | |
| ).Scan(&srcProject) | |
| _ = tx.QueryRow( | |
| `SELECT ifnull(project,'') FROM observations WHERE sync_id = ?`, targetID, | |
| `SELECT coalesce(nullif(o.project,''), s.project, '') | |
| FROM observations o | |
| LEFT JOIN sessions s ON s.id = o.session_id | |
| WHERE o.sync_id = ?`, targetID, | |
| ).Scan(&tgtProject) | |
| if err := tx.QueryRow( | |
| `SELECT coalesce(nullif(o.project,''), s.project, '') | |
| FROM observations o | |
| LEFT JOIN sessions s ON s.id = o.session_id | |
| WHERE o.sync_id = ?`, sourceID, | |
| ).Scan(&srcProject); err != nil && err != sql.ErrNoRows { | |
| return fmt.Errorf("JudgeRelation: resolve source project: %w", err) | |
| } | |
| if err := tx.QueryRow( | |
| `SELECT coalesce(nullif(o.project,''), s.project, '') | |
| FROM observations o | |
| LEFT JOIN sessions s ON s.id = o.session_id | |
| WHERE o.sync_id = ?`, targetID, | |
| ).Scan(&tgtProject); err != nil && err != sql.ErrNoRows { | |
| return fmt.Errorf("JudgeRelation: resolve target project: %w", err) | |
| } |
🤖 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/relations.go` around lines 599 - 610, The code at the two
QueryRow().Scan() calls for srcProject and tgtProject is ignoring errors by
using underscore assignments instead of capturing and handling them explicitly.
This allows database errors to silently result in empty project values, which
can cause incorrect cross-project behavior or skip important operations. Replace
the underscore with an error variable for both Scan calls and add explicit error
handling that checks if the error is not nil and either returns the error, logs
it, or takes appropriate action to prevent the query from silently failing and
leaving srcProject and tgtProject as empty strings.
This was referenced Jun 14, 2026
Closed
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 #496
PR Type
type:bugSummary
Cloud-journal sibling of #353 (PR #494, which fixed the local chunk path). A relation in
memory_relationscould exist with nosync_mutationsrow, with no mechanism to ever create one, so it never replicated to Engram Cloud (journal-based) and failed silently. Three confluent gaps, all addressed:backfillProjectSyncMutationsTxonly backfilled sessions/observations/prompts. AddedbackfillRelationSyncMutationsTx, mirroringbackfillObservationSyncMutationsTx(two-phase collect-then-insert, non-orphaned only,NOT EXISTSguard against the cloud target key). Wired intobackfillProjectSyncMutationsTx, so it runs on startup repair, enrollment, rename, and merge.projectNeedsBackfillignored relations. Added a 4th COUNT matching the backfill SELECT exactly, so the startup fast-path detects the gap.JudgeBySemanticnever enqueued. It backsmem_compareandScanProject; every semantic verdict landed inmemory_relationswith no journal row. It now enqueues on the enrolled path, mirroringJudgeRelations enrollment gate. Unenrolled stays a no-op and is caught by the backfill on later enrollment.REQ-009 intent (the open question in the issue)
Confirmed superseded.
#313/#379/#383already enabled relation cloud sync (JudgeRelationalready enqueued). TheTestConflictLoop_SyncRegression/assertNoRelationSyncMutationsguard still passes and is kept — but its comment was updated: it guards the enrollment gate (an unenrolled project must never enqueue), not a blanket "local-only" rule.Changes
internal/store/store.gobackfillRelationSyncMutationsTx(new), call frombackfillProjectSyncMutationsTx, relation COUNT inprojectNeedsBackfill, accurate doc comment onCountRelationSyncMutationsinternal/store/relations.goJudgeBySemanticenrollment-gated enqueueinternal/store/relation_backfill_test.goprojectNeedsBackfillcases,JudgeBySemanticenrolled/unenrolled/upsert, andTestEnrollProject_BackfillsPreExistingRelations(real pre-enrollment → EnrollProject → backfill end-to-end)internal/mcp/mcp_conflict_loop_test.goTest Plan
go build ./...,go vet ./...go test ./...— all packages green (Strict TDD: tests written red-first)Note for @forNerzul
You filed this and described the exact fix direction (mirror the observation/session/prompt backfill for relations) plus the REQ-009 intent question. This implements that. Since you have the real cloud-enrolled
cognicion-modularsetup that reproduces the silent drop, could you validate it against your scenario? Build from this branch:Then on an enrolled project: trigger a
JudgeBySemanticverdict (mem_compare/ScanProject) or enroll a project that already has relations, and confirm arelationrow appears insync_mutationsand reaches cloud (before this it was 0). Backfill path:engramstartup on an enrolled project with pre-existing relations should now enqueue them.If your own fix diverged in approach, flag it here and we will reconcile before merge.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests