Skip to content

fix(store): persist not_conflict verdicts in mem_compare and ScanProject #491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
goutham80808 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(store): persist not_conflict verdicts in mem_compare and ScanProject #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions cmd/engram/conflicts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,11 @@ func cmdConflictsList(cfg store.Config) {
defer s.Close()

opts := store.ListRelationsOptions{
Project: proj,
Status: statusFlag,
SinceTime: sinceTime,
Limit: limit,
Project: proj,
Status: statusFlag,
ExcludeNotConflict: true,
SinceTime: sinceTime,
Limit: limit,
}

items, err := s.ListRelations(opts)
Expand Down
5 changes: 2 additions & 3 deletions internal/mcp/mcp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,11 +881,11 @@ PARAMS:

BEHAVIOR:
- Persists the verdict via JudgeBySemantic with system provenance (marked_by_actor="engram").
- not_conflict: no row is inserted; tool returns success with empty sync_id (the verdict is recorded but not stored — it means "we evaluated these and they do not conflict").
- not_conflict: inserts a row tracking that the pair was evaluated and found non-conflicting. This resolves any pending relation annotation and prevents ScanProject from re-inserting the pair.
- Idempotent: calling again for the same pair updates the existing row.
- Cross-project pairs are rejected.

SUCCESS: Returns { "sync_id": "rel-..." } on persist, { "sync_id": "" } on not_conflict.
SUCCESS: Returns { "sync_id": "rel-..." } on persist.
ERROR: Returns IsError=true if IDs are unknown, relation is invalid, or cross-project pair.`),
mcp.WithTitleAnnotation("Compare Memory Pair (Persist Semantic Verdict)"),
mcp.WithReadOnlyHintAnnotation(false),
Expand Down Expand Up @@ -2143,7 +2143,6 @@ func handleCompare(s *store.Store, _ *SessionActivity) server.ToolHandlerFunc {
return mcp.NewToolResultError(err.Error()), nil
}

// syncID is "" when relation == "not_conflict" (JudgeBySemantic no-op).
envelope := map[string]any{
"sync_id": syncID,
}
Expand Down
12 changes: 6 additions & 6 deletions internal/mcp/mcp_compare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,9 @@ func TestHandleCompare_HappyPath(t *testing.T) {
}
}

// TestHandleCompare_NotConflict_NoRow — not_conflict returns success without inserting a row.
// REQ-011 | Design §9 (not_conflict is still persisted but JudgeBySemantic handles it as no-op)
func TestHandleCompare_NotConflict_NoRow(t *testing.T) {
// TestHandleCompare_NotConflict_Persists — not_conflict inserts a row and returns non-empty sync_id.
// REQ-011 | Design §9 (not_conflict is persisted to track evaluated pairs)
func TestHandleCompare_NotConflict_Persists(t *testing.T) {
s := newMCPTestStore(t)
idA, idB := seedCompareFixture(t, s)

Expand All @@ -115,10 +115,10 @@ func TestHandleCompare_NotConflict_NoRow(t *testing.T) {
t.Fatalf("response not valid JSON: %v", err)
}

// not_conflict is a no-op — sync_id should be empty string
// not_conflict should now persist — sync_id must be non-empty
syncID, _ := envelope["sync_id"].(string)
if syncID != "" {
t.Fatalf("expected empty sync_id for not_conflict, got %q", syncID)
if syncID == "" {
t.Fatalf("expected non-empty sync_id for not_conflict, got empty")
}
Comment on lines +118 to 122

@coderabbitai coderabbitai Bot Jun 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Strengthen the not_conflict handler test with a persistence assertion.

Line 118 through Line 122 currently validates only non-empty sync_id. Add an assertion that the returned ID resolves to a stored relation row with relation == not_conflict to fully cover the behavior change.

Suggested test hardening 
  syncID, _ := envelope["sync_id"].(string)
  if syncID == "" {
      t.Fatalf("expected non-empty sync_id for not_conflict, got empty")
  }
+ rel, err := s.GetRelation(syncID)
+ if err != nil {
+     t.Fatalf("expected persisted relation row for sync_id=%q: %v", syncID, err)
+ }
+ if rel.Relation != store.RelationNotConflict {
+     t.Fatalf("expected relation=%q, got %q", store.RelationNotConflict, rel.Relation)
+ }

As per coding guidelines, “**/*_test.go: Verify coverage of happy path, error paths, and edge cases… Behavior changes without tests should be blocked.”

📝 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
// not_conflict should now persist — sync_id must be non-empty
syncID, _ := envelope["sync_id"].(string)
if syncID != "" {
t.Fatalf("expected empty sync_id for not_conflict, got %q", syncID)
if syncID == "" {
t.Fatalf("expected non-empty sync_id for not_conflict, got empty")
}
// not_conflict should now persist — sync_id must be non-empty
syncID, _ := envelope["sync_id"].(string)
if syncID == "" {
t.Fatalf("expected non-empty sync_id for not_conflict, got empty")
}
rel, err := s.GetRelation(syncID)
if err != nil {
t.Fatalf("expected persisted relation row for sync_id=%q: %v", syncID, err)
}
if rel.Relation != store.RelationNotConflict {
t.Fatalf("expected relation=%q, got %q", store.RelationNotConflict, rel.Relation)
}
🤖 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/mcp/mcp_compare_test.go` around lines 118 - 122, The test currently
only asserts syncID is non-empty; after the existing syncID check add a lookup
of the persisted relation row for that sync ID and assert its relation field
equals "not_conflict". Concretely, immediately after the syncID, use the test's
DB/test helper (or perform a direct query) to fetch the persisted row by sync_id
(e.g., call a helper like getRelationBySyncID(syncID) or DB.QueryRow("SELECT
relation FROM relations WHERE sync_id = ?", syncID).Scan(&relation)) and then
t.Fatalf/t.Errorf if relation != "not_conflict". Ensure this lookup runs in the
same test context and uses existing DB/test fixtures so the assertion validates
persistence of the "not_conflict" relation.

Source: Coding guidelines

}

Expand Down
16 changes: 9 additions & 7 deletions internal/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,10 +979,11 @@ func (s *Server) handleListConflicts(w http.ResponseWriter, r *http.Request) {
offset := queryInt(r, "offset", 0)

opts := store.ListRelationsOptions{
Project: project,
Status: status,
Limit: limit,
Offset: offset,
Project: project,
Status: status,
ExcludeNotConflict: true,
Limit: limit,
Offset: offset,
}

sinceStr := r.URL.Query().Get("since")
Expand All @@ -1003,9 +1004,10 @@ func (s *Server) handleListConflicts(w http.ResponseWriter, r *http.Request) {

// Count without limit/offset for the total field.
countOpts := store.ListRelationsOptions{
Project: project,
Status: status,
SinceTime: opts.SinceTime,
Project: project,
Status: status,
ExcludeNotConflict: true,
SinceTime: opts.SinceTime,
}
total, err := s.store.CountRelations(countOpts)
if err != nil {
Expand Down
27 changes: 14 additions & 13 deletions internal/store/judge_by_semantic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,11 @@ func TestJudgeBySemantic_UpsertIdempotency(t *testing.T) {
}
}

// ─── C.2c — TestJudgeBySemantic_NotConflictIsNoOp ────────────────────────────
// ─── C.2c — TestJudgeBySemantic_NotConflictPersists ─────────────────────────

// TestJudgeBySemantic_NotConflictIsNoOp verifies that passing Relation="not_conflict"
// inserts no row and returns an empty sync_id without error.
func TestJudgeBySemantic_NotConflictIsNoOp(t *testing.T) {
// TestJudgeBySemantic_NotConflictPersists verifies that passing Relation="not_conflict"
// inserts a row and returns a non-empty sync_id, so the pair is tracked as evaluated.
func TestJudgeBySemantic_NotConflictPersists(t *testing.T) {
s := setupRelationsStore(t)

_, syncA := addTestObs(t, s, "Unrelated auth decision", "decision", "testproject", "project")
Expand All @@ -160,20 +160,20 @@ func TestJudgeBySemantic_NotConflictIsNoOp(t *testing.T) {
if err != nil {
t.Fatalf("JudgeBySemantic(not_conflict): unexpected error: %v", err)
}
if syncID != "" {
t.Errorf("JudgeBySemantic(not_conflict): expected empty sync_id; got %q", syncID)
if syncID == "" {
t.Errorf("JudgeBySemantic(not_conflict): expected non-empty sync_id; got empty")
}

// No row must exist for this pair.
// A row must exist for this pair with relation = 'not_conflict'.
var count int
if err := s.db.QueryRow(
`SELECT count(*) FROM memory_relations WHERE source_id = ? OR target_id = ?`,
syncA, syncA,
`SELECT count(*) FROM memory_relations WHERE source_id = ? AND target_id = ? AND relation = 'not_conflict'`,
syncA, syncB,
).Scan(&count); err != nil {
t.Fatalf("count query: %v", err)
}
if count != 0 {
t.Errorf("expected 0 rows for not_conflict pair; got %d", count)
if count != 1 {
t.Errorf("expected 1 not_conflict row for pair; got %d", count)
}
}

Expand Down Expand Up @@ -312,15 +312,16 @@ func TestJudgeBySemantic_CrossProjectRejected(t *testing.T) {

// ─── C.2f — TestJudgeBySemantic_AllValidRelations ────────────────────────────

// TestJudgeBySemantic_AllValidRelations verifies that all 5 non-not_conflict
// relation verbs are accepted by JudgeBySemantic.
// TestJudgeBySemantic_AllValidRelations verifies that all 6 relation verbs
// (including not_conflict) are accepted and persisted by JudgeBySemantic.
func TestJudgeBySemantic_AllValidRelations(t *testing.T) {
validRelations := []string{
RelationRelated,
RelationCompatible,
RelationScoped,
RelationConflictsWith,
RelationSupersedes,
RelationNotConflict,
}

for _, rel := range validRelations {
Expand Down
Loading