fix(store): persist not_conflict verdicts in mem_compare and ScanProject#491
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR persists Changesnot_conflict persistence and conflict-view alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant mem_compare
participant JudgeBySemantic
participant memory_relations
Client->>mem_compare: submit not_conflict verdict
mem_compare->>JudgeBySemantic: evaluate relation
JudgeBySemantic->>memory_relations: upsert relation row
memory_relations-->>mem_compare: return sync_id
mem_compare-->>Client: return persisted sync_id
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR changes semantic scanning behavior so "not_conflict" verdicts are persisted (instead of treated as a no-op), and updates conflict-focused views to exclude "not_conflict" rows.
Changes:
- Persist
"not_conflict"viaJudgeBySemanticandScanProject, and update related tests/docs. - Add
ExcludeNotConflicttoListRelationsOptionsand use it in server/CLI conflict listings. - Exclude
"not_conflict"from relation stats queries to keep stats conflict-focused.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/store/scan_semantic_test.go | Updates semantic scan test to expect persisted not_conflict rows. |
| internal/store/relations.go | Persists not_conflict, adds ExcludeNotConflict, updates stats/query behavior and comments. |
| internal/store/judge_by_semantic_test.go | Updates semantic judge tests to assert not_conflict persistence. |
| internal/server/server.go | Ensures conflicts endpoint excludes not_conflict rows. |
| internal/mcp/mcp_compare_test.go | Updates MCP compare test to expect a persisted not_conflict sync_id. |
| internal/mcp/mcp.go | Updates MCP tool docs/output expectations for not_conflict persistence. |
| cmd/engram/conflicts.go | Ensures CLI conflicts list excludes not_conflict rows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var count int | ||
| _ = s.db.QueryRow( | ||
| `SELECT count(*) FROM memory_relations WHERE marked_by_actor = 'engram'`, | ||
| `SELECT count(*) FROM memory_relations | ||
| WHERE marked_by_actor = 'engram' AND relation = 'not_conflict'`, | ||
| ).Scan(&count) |
| if opts.ExcludeNotConflict { | ||
| query += ` AND r.relation != 'not_conflict'` | ||
| } |
There was a problem hiding this comment.
Fixed in d8019a9 — both buildRelationsQuery and GetRelationStats now use ? with args = append(args, RelationNotConflict). Removes the drift risk you flagged.
| @@ -1099,14 +1104,16 @@ func (s *Store) GetRelationStats(project string) (RelationStats, error) { | |||
| FROM memory_relations r | |||
| LEFT JOIN observations src ON src.sync_id = r.source_id AND src.deleted_at IS NULL | |||
| LEFT JOIN observations tgt ON tgt.sync_id = r.target_id AND tgt.deleted_at IS NULL | |||
| WHERE ifnull(src.project,'') = ? OR ifnull(tgt.project,'') = ? | |||
| WHERE (ifnull(src.project,'') = ? OR ifnull(tgt.project,'') = ?) | |||
| AND r.relation != 'not_conflict' | |||
| GROUP BY r.relation, r.judgment_status | |||
| ` | |||
| args = []any{project, project} | |||
| } else { | |||
| q = ` | |||
| SELECT relation, judgment_status, count(*) AS cnt | |||
| FROM memory_relations | |||
| WHERE relation != 'not_conflict' | |||
| GROUP BY relation, judgment_status | |||
| ` | |||
| } | |||
There was a problem hiding this comment.
Partial address in d8019a9. Renaming the function or splitting into GetConflictStats vs GetRelationStatsAll would be a breaking API change across CLI (cmd/engram/conflicts.go:262) and server (server.go:1053) — out of scope for this bugfix PR.
Updated the doc comment to state explicitly that not_conflict is excluded by design and that callers wanting inclusive enumeration should use ListRelations. Both current callers are conflict-focused (conflicts dashboard + CLI conflicts list), so no consumer needs inclusive stats today. A proper rename can ship in a dedicated refactor PR if/needed.
|
hey @Alan-TheGentleman , i've made the changes. and below are the design decisions i've took. Would you please take a look on this. Design Decisions — Reviewer Please ConfirmDuring implementation, 4 design decisions emerged that I want to flag for reviewer confirmation. All are implemented in this PR. 1. ScanProject now persists not_conflict (was previously skipped)Decision: Removed the short-circuit at the old Why: ScanProject Phase 3 creates Impact: 2. Dashboard / CLI filter not_conflict from conflict viewsDecision: Added Why: Without filtering, Scope note: This touches 3. FindCandidates left unchanged (no relation filtering)Decision: Why: The fix works indirectly — when 4. Sync payload includes not_conflict rows (no exclusion)Decision: Why: Syncing the All feedback welcome. If any decision needs a different approach, I'll adjust before merge. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/server.go (1)
981-1011:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd endpoint tests for the new
/conflictsfiltering behavior.Line 981 through Line 1011 changes endpoint semantics (
ExcludeNotConflicton both list and total), but no matching handler/route response test updates are included in this PR slice. Please add tests that verify parse/validation and that bothrelationsandtotalexcludenot_conflict.As per coding guidelines, “
internal/server/**/*.go: New or modified endpoints need handler tests (parsing/validation) and route+response tests.” and “**/*_test.go: Behavior changes without tests should be blocked.”🤖 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/server/server.go` around lines 981 - 1011, This change adds ExcludeNotConflict to both the list call and the count call but no tests were updated; add handler and route/response tests that exercise the new /conflicts filtering: update or create tests that call the HTTP handler that builds ListRelationsOptions (verify parsing/validation of the since query parameter using time.RFC3339 and that invalid values return 400), and add assertions that s.store.ListRelations is invoked (or the HTTP response contains) only relations excluding status "not_conflict" and that the returned JSON "total" field is the count computed with the same ExcludeNotConflict behavior (mirror countOpts.SinceTime usage). Target the code paths that construct opts and countOpts and the call sites s.store.ListRelations and ensure both relations and total exclude not_conflict.Source: Coding guidelines
🤖 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/mcp/mcp_compare_test.go`:
- Around line 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.
---
Outside diff comments:
In `@internal/server/server.go`:
- Around line 981-1011: This change adds ExcludeNotConflict to both the list
call and the count call but no tests were updated; add handler and
route/response tests that exercise the new /conflicts filtering: update or
create tests that call the HTTP handler that builds ListRelationsOptions (verify
parsing/validation of the since query parameter using time.RFC3339 and that
invalid values return 400), and add assertions that s.store.ListRelations is
invoked (or the HTTP response contains) only relations excluding status
"not_conflict" and that the returned JSON "total" field is the count computed
with the same ExcludeNotConflict behavior (mirror countOpts.SinceTime usage).
Target the code paths that construct opts and countOpts and the call sites
s.store.ListRelations and ensure both relations and total exclude not_conflict.
🪄 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: 3c9de56b-c61d-469f-a63b-b1779c6af634
📒 Files selected for processing (7)
cmd/engram/conflicts.gointernal/mcp/mcp.gointernal/mcp/mcp_compare_test.gointernal/server/server.gointernal/store/judge_by_semantic_test.gointernal/store/relations.gointernal/store/scan_semantic_test.go
|
This PR has been inactive for 30 days. Please rebase, update it, or leave a comment — otherwise it will be closed in 14 days. |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Thanks for this. The insert path is right and I verified the annotation builder only renders supersedes/conflicts_with/pending, so no not_conflict clutter reaches the agent.
But this only closes half of #490. The symptom actually reported there — "FindCandidates re-surfaces the same pair every session" — survives this change:
-
FindCandidatesinserts a freshpendingrow per candidate with no existence check, andmemory_relationshas no UNIQUE on(source_id, target_id)(only onsync_id). So the nextmem_savere-inserts the pair and the agent re-evaluates it. Please add the existing-relation filter — without it the user-visible bug is still there. - Removing the
ScanProjectskip leavesSemanticSkippednever incremented in production code, sosemantic_skippedis now permanently0in JSON output while the tests still sum it. Either remove the field or repurpose it — an always-zero field is worse than none.
type:bug applied. Repo CI hasn't run on this branch; a rebase should trigger it.
JudgeBySemantic returned success without inserting a row for not_conflict, causing FindCandidates to re-surface the same pair on every session. Now all verdicts (including not_conflict) are persisted via the existing UPSERT logic. - Remove no-op early return in JudgeBySemantic for not_conflict - Remove ScanProject skip that prevented not_conflict from being persisted - Add ExcludeNotConflict filter to ListRelationsOptions for conflict views - Filter not_conflict from GetRelationStats (conflict stats view) - Set ExcludeNotConflict in dashboard and CLI conflict list handlers - Update tests to assert not_conflict rows are created - Update tool description and doc comments Closes Gentleman-Programming#490
Address maintainer review feedback on Gentleman-Programming#491: 1. FindCandidates now skips pairs that already have a relation row in memory_relations (either direction). Without this check, every save re-inserted a fresh pending row for the same pair, re-surfacing it for agent re-evaluation -- the core user-visible bug from Gentleman-Programming#490. 2. Removed SemanticSkipped field from ScanResult, CLI output, server JSON, and all tests. The ScanProject skip that incremented it was removed in the previous commit, leaving the field permanently zero in production. An always-zero field is worse than none. 3. Removed dead pairResult struct (was defined but never used). 4. Hardened tests per review feedback: - TestHandleCompare_NotConflict_Persists: assert stored relation row - TestScanProject_Semantic_NotConflictPersisted: assert Scan error - Added TestFindCandidates_SkipsExistingRelation
bb002b0 to
2a21160
Compare
|
Thanks for the thorough review @Alan-TheGentleman. Both items are addressed in 1. FindCandidates existing-relation filter ✓Added a pre-check before each INSERT that mirrors the one in var exists int
if err := s.db.QueryRow(
`SELECT 1 FROM memory_relations
WHERE (source_id = ? AND target_id = ?)
OR (source_id = ? AND target_id = ?)
LIMIT 1`,
sourceSyncID, rc.syncID, rc.syncID, sourceSyncID,
).Scan(&exists); err == nil {
continue // pair already has a relation row
}If a row exists in either direction, the candidate is skipped entirely — no new Added 2. Removed
|
| // Semantic counters — always present; zero when semantic=false. | ||
| "semantic_judged": result.SemanticJudged, | ||
| "semantic_skipped": result.SemanticSkipped, | ||
| "semantic_errors": result.SemanticErrors, | ||
| } |
There was a problem hiding this comment.
Pushing back here. Maintainer @Alan-TheGentleman explicitly requested removal in his review:
Removing the
ScanProjectskip leavesSemanticSkippednever incremented in production code, sosemantic_skippedis now permanently0in JSON output while the tests still sum it. Either remove the field or repurpose it — an always-zero field is worse than none.
The "deprecate with always-0" alternative was the path he rejected. I went with removal per his directive. Happy to revisit if you and Alan disagree, but AFAICT the breaker is intentional.
| // Semantic counters — populated only when ScanOptions.Semantic is true. | ||
| // Zero-value is safe for existing JSON consumers. | ||
| SemanticJudged int `json:"semantic_judged"` | ||
| SemanticSkipped int `json:"semantic_skipped"` | ||
| SemanticErrors int `json:"semantic_errors"` | ||
| SemanticJudged int `json:"semantic_judged"` | ||
| SemanticErrors int `json:"semantic_errors"` |
There was a problem hiding this comment.
Same answer as the sibling comment on server.go:1222 — removal was the maintainer's explicit ask (see quote there). Struct field, server JSON key, CLI output line, and all test sums updated together in one pass.
| // Both semantic counters must be present and zero. | ||
| for _, field := range []string{"semantic_judged", "semantic_errors"} { |
There was a problem hiding this comment.
Fixed in d8019a9 — comment now reads "the response includes semantic_judged=0 and semantic_errors=0", which matches the assertion loop on line 1540.
| // semantic_judged + semantic_errors should be present (values depend on FTS). | ||
| for _, field := range []string{"semantic_judged", "semantic_errors"} { |
There was a problem hiding this comment.
No change needed here — the comment on line 1636 already reads semantic_judged + semantic_errors should be present (values depend on FTS), which accurately describes the assertion loop immediately below it. The stale comment was on the sibling test SemanticFalse_CountersZero (still mentioned semantic_skipped=0); that one is fixed in d8019a9.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/store/relations.go (1)
1371-1376: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winFilter out already-evaluated pairs before queueing them for the semantic worker pool.
When
Semantic=true,ScanProjectpassesSkipInsert=truetoFindCandidates, which intentionally bypasses the DB existence check to avoid unwanted DB writes during candidate gathering. However, because the candidates are gathered blindly, theSemantic=trueblock must perform its own DB pre-check (just like theSemantic=falseblock does) to prevent wasting expensive LLM calls on already-judged pairs.Additionally, because the worker pool evaluates pairs concurrently, you should track queued pairs in memory to avoid evaluating reciprocal pairs (e.g.,
A->BandB->A) within the same scan.⚡ Proposed fix
Define a tracking map before the outer
observationsloop (around line 1339):var semanticPairs []candidatePair + seenPairs := make(map[string]bool)Then, apply this pre-check and dedup logic inside the candidate loop:
for _, c := range candidates { + // Avoid queuing reciprocal pairs (A->B and B->A) in the same run. + k1, k2 := obs.syncID, c.SyncID + if k1 > k2 { + k1, k2 = k2, k1 + } + pairKey := k1 + "|" + k2 + if seenPairs[pairKey] { + continue + } + + // Pre-check: skip pairs that already have a relation row from previous runs. + var exists int + if err := s.db.QueryRow( + `SELECT 1 FROM memory_relations + WHERE (source_id = ? AND target_id = ?) + OR (source_id = ? AND target_id = ?) + LIMIT 1`, + obs.syncID, c.SyncID, c.SyncID, obs.syncID, + ).Scan(&exists); err == nil { + result.AlreadyRelated++ + continue + } else if err != sql.ErrNoRows { + log.Printf("[store] ScanProject semantic pre-check obs=%s cand=%s: %v", obs.syncID, c.SyncID, err) + continue + } + if len(semanticPairs) >= maxSemantic { // Cap reached — stop adding pairs. result.Capped = true break } + + seenPairs[pairKey] = true🤖 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 1371 - 1376, Update the Semantic=true candidate-processing path in ScanProject around semanticPairs to pre-check each candidate against the database before queueing it, matching the Semantic=false behavior while preserving SkipInsert=true. Add an in-memory seenPairs map before the observations loop and use a canonical pair key so reciprocal candidates are skipped during the same scan; only unseen, not-already-evaluated pairs should count toward maxSemantic and be queued for the worker pool.
🤖 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.
Outside diff comments:
In `@internal/store/relations.go`:
- Around line 1371-1376: Update the Semantic=true candidate-processing path in
ScanProject around semanticPairs to pre-check each candidate against the
database before queueing it, matching the Semantic=false behavior while
preserving SkipInsert=true. Add an in-memory seenPairs map before the
observations loop and use a canonical pair key so reciprocal candidates are
skipped during the same scan; only unseen, not-already-evaluated pairs should
count toward maxSemantic and be queued for the worker pool.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a9eefbed-19ba-4dfc-8969-582734d06147
📒 Files selected for processing (9)
cmd/engram/conflicts.gointernal/mcp/mcp.gointernal/mcp/mcp_compare_test.gointernal/server/server.gointernal/server/server_test.gointernal/store/judge_by_semantic_test.gointernal/store/relations.gointernal/store/relations_test.gointernal/store/scan_semantic_test.go
Address CodeRabbit/Copilot review feedback on Gentleman-Programming#491: - ScanProject Semantic=true path now mirrors Phase 3's existence pre-check before queueing pairs for LLM evaluation. SkipInsert=true bypasses FindCandidates' dedup, so without this check pairs with existing relation rows would still be sent to the LLM (wasted API spend). - Parameterize the 'not_conflict' SQL literal in buildRelationsQuery and GetRelationStats via the RelationNotConflict constant — removes drift risk if the constant value ever changes. - GetRelationStats doc comment now states explicitly that not_conflict is excluded by design (conflicts-dashboard view; issue Gentleman-Programming#490). - Add TestHandleListConflicts_ExcludesNotConflict: seeds a pending and a not_conflict pair in the same project; asserts both relations array and total exclude not_conflict. - Add TestHandleListConflicts_InvalidSince_400: covers the RFC3339 parse /validation gap on the since query parameter. - Fix stale doc comment on TestHandleScanConflicts_SemanticFalse_CountersZero that still referenced the removed semantic_skipped field.
|
@coderabbitai addressing your two outside-diff comments from review 1. ✅ DB pre-check: applied. The Semantic=true path uses ❌ 2. ✅ Applied. Added two tests in
Both pass locally along with the full 22-package suite. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Targeted commit diff =='
git show --format=fuller --stat d8019a9 --
git show --format= --unified=40 d8019a9 -- internal/store/relations.go internal/server/server_test.go
echo '== Store file outline =='
ast-grep outline internal/store/relations.go --items all --type function,struct --match 'FindCandidates|ScanProject|buildRelationsQuery|ListRelationsOptions|ScanResult' --view expanded
echo '== Relevant relation-code contexts =='
rg -n -C 12 'SkipInsert|AlreadyRelated|seenPairs|SELECT 1 FROM memory_relations|func \(s \*Store\) (FindCandidates|ScanProject)' internal/store/relations.go
echo '== New server tests and handler context =='
ast-grep outline internal/server/server_test.go --items all --type function --match 'TestHandleListConflicts_(ExcludesNotConflict|InvalidSince_400)' --view expanded
rg -n -C 12 'TestHandleListConflicts_(ExcludesNotConflict|InvalidSince_400)|func \(s \*Server\) handleListConflicts|ExcludeNotConflict' internal/server/server.go internal/server/server_test.goLength of output: 41081
One correction on the reciprocal-dedup rationale: the current Phase 3 pre-check at So I agree that a semantic-only
The pre-check and endpoint-test findings are addressed; the directional-pair explanation remains unresolved. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/server/server.go:1222
- POST /conflicts/scan response no longer includes the previously documented
semantic_skippedfield. Repo docs/spec currently state thatsemantic_judged,semantic_skipped, andsemantic_errorsare always present (and describenot_conflictas skipped), so this is an API/contract change that should be reflected in documentation and any external consumers (or keepsemantic_skippedas a deprecated always-0 field for backward compatibility).
resp := map[string]any{
"project": result.Project,
"inspected": result.Inspected,
"candidates_found": result.CandidatesFound,
"already_related": result.AlreadyRelated,
"inserted": result.RelationsInserted,
"capped": result.Capped,
"dry_run": result.DryRun,
// Semantic counters — always present; zero when semantic=false.
"semantic_judged": result.SemanticJudged,
"semantic_errors": result.SemanticErrors,
}
internal/store/relations.go:158
- The ScanResult semantic-counter comment still implies JSON consumer compatibility, but this PR removes the
semantic_skippedcounter entirely and changes hownot_conflictis handled. Consider either keeping a deprecatedsemantic_skippedfield (always 0) for backwards compatibility, or update the surrounding documentation/specs to explicitly reflect the new counter set and persistence semantics.
// Semantic counters — populated only when ScanOptions.Semantic is true.
// Zero-value is safe for existing JSON consumers.
SemanticJudged int `json:"semantic_judged"`
SemanticErrors int `json:"semantic_errors"`
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 (1)
internal/store/relations.go (1)
832-846: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winUpdate
source_idandtarget_idduring UPSERT to preserve relation direction.When
JudgeBySemanticupdates an existing relation, it setsrelation = ?but leavessource_idandtarget_idas they were in the database. Because the existence check queries both directions ((source, target) OR (target, source)), if an LLM or user judges that "A supersedes B" but the pending row was originally created as(B, A), the row is updated torelation = supersedeswithout swapping the IDs. This corrupts the data by persisting "B supersedes A".To ensure directional relations (
supersedes,scoped, etc.) maintain the correct polarity, overwritesource_idandtarget_idwith the requested ones during the update.🐛 Proposed fix
// Update existing row. if _, execErr := tx.Exec(` UPDATE memory_relations SET relation = ?, + source_id = ?, + target_id = ?, judgment_status = 'judged', confidence = ?, reason = ?, marked_by_actor = ?, marked_by_kind = ?, marked_by_model = ?, updated_at = datetime('now') WHERE sync_id = ? - `, p.Relation, confidence, p.Reasoning, + `, p.Relation, p.SourceID, p.TargetID, confidence, p.Reasoning, actor, kind, modelPtr, existingSyncID, ); execErr != nil {🤖 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 832 - 846, Update the existing-relation UPDATE statement in JudgeBySemantic to also set source_id and target_id from the requested relation endpoints before updating relation and judgment fields. Keep the bidirectional existence check, but ensure the persisted row uses the requested source/target polarity when an existing relation is judged.
🤖 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/server/server_test.go`:
- Around line 1290-1292: Update the relation assertion in the test around the
existing len(relations) check to use t.Fatalf when the count is not exactly one,
then explicitly assert that relations[0].sync_id matches the expected pending
relation and thus confirms not_conflict was filtered out.
---
Outside diff comments:
In `@internal/store/relations.go`:
- Around line 832-846: Update the existing-relation UPDATE statement in
JudgeBySemantic to also set source_id and target_id from the requested relation
endpoints before updating relation and judgment fields. Keep the bidirectional
existence check, but ensure the persisted row uses the requested source/target
polarity when an existing relation is judged.
🪄 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: b10c4cd6-e73e-40ca-80a9-a8997dc372c4
📒 Files selected for processing (2)
internal/server/server_test.gointernal/store/relations.go
…-scan reciprocal pairs CodeRabbit's review on d8019a9 caught two real issues. Both fixed: 1. JudgeBySemantic UPSERT was not rewriting source_id/target_id when the bidirectional existence check matched a row in the reversed direction. Consequence: a directional verb like 'supersedes' would be persisted with the opposite polarity (B supersedes A stored when caller asked for A supersedes B, because the existing pending row was (B, A)). Fix: SET clause now includes source_id = ?, target_id = ? so the row reflects the caller's intended direction. Sync enqueue path reads the row after the update, so the replicated payload is consistent. 2. My earlier rationale for rejecting the seenPairs map was wrong. Phase 3 deduplicates reciprocal pairs (A→B and B→A) via synchronous INSERT — when obs B's loop runs, the pre-check finds the row just inserted by obs A's loop. The Semantic path defers work to goroutines, so neither row exists when both candidates pass the pre-check, and both get queued for the worker pool — wasting one LLM call (the second UPSERT overwrites the first). Fix: track unordered pair keys in seenPairs and skip the second sighting within the same scan. Also tightens TestHandleListConflicts_ExcludesNotConflict per CodeRabbit nit (t.Fatalf on count, positive verification that the kept sync_id is the pending one). Tests: - New TestJudgeBySemantic_UpsertRewritesDirection covers fix Gentleman-Programming#1 — seeds a (B, A, pending) row, calls JudgeBySemantic(A, B, supersedes), asserts source/target swap and single-row invariant. - Existing TestScanProject_Semantic_HappyPath covers fix Gentleman-Programming#2 implicitly (asserts SemanticJudged > 0, still true with one pair instead of two). - All 22 packages pass.
|
@coderabbitai you were right on both counts — pushing back was incorrect. Fixed in Re: Phase 3 reciprocal-pair rationale You correctly pointed out that Phase 3's pre-check at The Semantic path queues work to goroutines, so neither row exists when both candidates pass the pre-check. Without an in-memory guard, both directions were getting queued and the second UPSERT was overwriting the first. Added the Re: JudgeBySemantic direction corruption This was the more important catch. The bidirectional existence check at Added Both fixes verified with full 22-package suite. The other minor test nit (t.Fatalf on count + verify kept sync_id) is also applied. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Commit summary and focused diff =='
git show --format=fuller --stat c97c617 --
git show --format= --unified=35 c97c617 -- internal/store/relations.go internal/store/judge_by_semantic_test.go internal/store/scan_semantic_test.go
echo '== Relevant declarations =='
ast-grep outline internal/store/relations.go --items all --type function,struct --match 'JudgeBySemantic|ScanProject|ScanResult' --view expanded
ast-grep outline internal/store/judge_by_semantic_test.go --items all --type function --match 'TestJudgeBySemantic_UpsertRewritesDirection' --view expanded
echo '== Focused implementation and test contexts =='
rg -n -C 18 'seenPairs|canonical|JudgeBySemantic|source_id = \?|target_id = \?|TestJudgeBySemantic_UpsertRewritesDirection' \
internal/store/relations.go internal/store/judge_by_semantic_test.go internal/store/scan_semantic_test.goLength of output: 91897
The remaining finding is addressed. Nicely corrected. 🐇 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/store/relations.go (1)
1395-1439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix indentation and optimize
seenPairsmarking.The
semanticPairs = append(...)block is indented by an extra tab.Additionally, consider moving
seenPairs[pairKey] = trueto immediately after theseenPairscheck. This prevents redundant database queries for reciprocal pairs that already exist in the database (since bothA->BandB->Aare emitted as candidates) and avoids double-countingresult.AlreadyRelatedwithin the same scan.♻️ Proposed refactor
pairKey := k1 + "|" + k2 if seenPairs[pairKey] { continue } + seenPairs[pairKey] = true // Pre-check: skip pairs that already have any relation row in either // direction. SkipInsert=true above bypasses FindCandidates' built-in // existence check, so we mirror the Phase 3 pre-check here to avoid // wasting LLM calls on already-judged pairs from PREVIOUS scans. var exists int if err := s.db.QueryRow( `SELECT 1 FROM memory_relations WHERE (source_id = ? AND target_id = ?) OR (source_id = ? AND target_id = ?) LIMIT 1`, obs.syncID, c.SyncID, c.SyncID, obs.syncID, ).Scan(&exists); err == nil { result.AlreadyRelated++ continue } else if err != sql.ErrNoRows { log.Printf("[store] ScanProject: semantic pre-check obs=%s cand=%s: %v", obs.syncID, c.SyncID, err) continue } if len(semanticPairs) >= maxSemantic { // Cap reached — stop adding pairs. result.Capped = true break } var candTitle, candType, candContent string _ = s.db.QueryRow( `SELECT title, type, ifnull(content,'') FROM observations WHERE sync_id = ?`, c.SyncID, ).Scan(&candTitle, &candType, &candContent) - semanticPairs = append(semanticPairs, candidatePair{ - sourceSnippet: srcSnippet, - candidateSnippet: ObservationSnippet{ - ID: c.ID, - SyncID: c.SyncID, - Title: candTitle, - Type: candType, - Content: candContent, - }, - }) - seenPairs[pairKey] = true - } + semanticPairs = append(semanticPairs, candidatePair{ + sourceSnippet: srcSnippet, + candidateSnippet: ObservationSnippet{ + ID: c.ID, + SyncID: c.SyncID, + Title: candTitle, + Type: candType, + Content: candContent, + }, + }) + }🤖 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 1395 - 1439, Fix the extra indentation in the semanticPairs append block within the ScanProject candidate loop. Move seenPairs[pairKey] = true to immediately after the existing seenPairs check, before the database pre-check, while preserving the continue behavior for previously seen pairs and preventing reciprocal candidates from repeating database checks or incrementing result.AlreadyRelated twice.
🤖 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.
Outside diff comments:
In `@internal/store/relations.go`:
- Around line 1395-1439: Fix the extra indentation in the semanticPairs append
block within the ScanProject candidate loop. Move seenPairs[pairKey] = true to
immediately after the existing seenPairs check, before the database pre-check,
while preserving the continue behavior for previously seen pairs and preventing
reciprocal candidates from repeating database checks or incrementing
result.AlreadyRelated twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 752eea86-681b-485f-9bbf-0d85be6f8545
📒 Files selected for processing (3)
internal/server/server_test.gointernal/store/judge_by_semantic_test.gointernal/store/relations.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/store/relations.go:1432
- This block is mis-indented (extra leading tab), which suggests the file wasn’t gofmt’d after the edits and reduces readability in a dense control-flow section. Reformat the append+seenPairs lines to match surrounding indentation (gofmt will do this automatically).
semanticPairs = append(semanticPairs, candidatePair{
sourceSnippet: srcSnippet,
candidateSnippet: ObservationSnippet{
ID: c.ID,
SyncID: c.SyncID,
| if err := s.db.QueryRow( | ||
| `SELECT 1 FROM memory_relations | ||
| WHERE (source_id = ? AND target_id = ?) | ||
| OR (source_id = ? AND target_id = ?) | ||
| LIMIT 1`, |
There was a problem hiding this comment.
Both addressed in 7167e9a.
Indentation: confirmed — gofmt -l flagged relations.go. Applied gofmt -w to the whole file; the candidate loop is now at the correct nesting depth.
seenPairs reordering: applied exactly as you suggested. seenPairs[pairKey] = true now sits immediately after the if seenPairs[pairKey] { continue } check, before the DB pre-check. Saves one DB query when the first direction of an unordered pair fails the DB pre-check (existing relation row) — previously the reciprocal would have hit the DB again before being skipped. End behavior unchanged: each unordered pair is still processed at most once per scan.
There was a problem hiding this comment.
Sorry — my previous reply was for the sibling CodeRabbit indentation thread; let me address your actual N+1 question.
Pushing back on this one. Considered carefully, but I don't think the change is worth it here:
-
Magnitude: engram ships with SQLite in-process. Each query is a B-tree lookup, ~10–100μs. With
Limit=3(default) the "N+1" is 3 extra round trips per save — single-digit microseconds. Not a real bottleneck. -
Consistency with Phase 3:
ScanProject's Phase 3 path atrelations.go:1452+uses the same SELECT-then-INSERT pattern with the same justification. Changing only FindCandidates would create an inconsistency; changing both expands scope beyond what @Alan-TheGentleman asked for. -
Race window argument cuts both ways: The combined
INSERT...SELECT...WHERE NOT EXISTSwould close a theoretical TOCTOU race between SELECT and INSERT. But in this codebase the worst case is two pending rows in opposite directions (A→B and B→A as distinct sync_ids), andJudgeBySemantic's bidirectional UPSERT consolidates them on the next verdict. So the race is self-healing — not data corruption. -
SQL complexity cost: The combined form is harder to read, harder to log per-candidate outcomes, and harder to debug when something goes wrong. The current form mirrors Phase 3 exactly and is easier for the next maintainer to reason about.
If we ever migrate off SQLite (e.g., to PostgreSQL with real network round trips), this becomes worth revisiting — and at that point we'd want to refactor Phase 3 the same way. Happy to do that in a dedicated perf PR if/when it matters. For this bugfix I'd rather keep the dedup logic uniform across both paths.
CodeRabbit/Copilot flagged indentation on the semantic candidate loop left over from c97c617. gofmt -w applied. Also hoisted seenPairs[pairKey] = true to immediately after the seenPairs check (CodeRabbit suggestion). Previously it was marked only after queue append, so if a pair's first direction failed the DB pre-check (existing row from previous scan), the reciprocal direction would still trigger a redundant DB query. Marking on first encounter closes that small gap. Behavior unchanged — each unordered pair is still processed at most once per scan, just with one less DB query in the edge case.
There was a problem hiding this comment.
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 (2)
451-471: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winBidirectional exists-check SQL is duplicated three times in this file.
The same
SELECT 1 FROM memory_relations WHERE (source_id=? AND target_id=?) OR (source_id=? AND target_id=?) LIMIT 1pre-check appears here, again inScanProject's semantic pre-check (lines ~1407-1420), and again inScanProject's Phase 3 apply loop (lines ~1460-1474). Extracting a single helper (e.g.relationExists(sourceSyncID, targetSyncID string) (bool, error)) would remove the triplication and ensure any future change to the dedup predicate (e.g. accounting for soft-deletes) only needs to be made once.♻️ Proposed helper extraction
+func (s *Store) relationExists(sourceSyncID, targetSyncID string) (bool, error) { + var exists int + err := s.db.QueryRow( + `SELECT 1 FROM memory_relations + WHERE (source_id = ? AND target_id = ?) + OR (source_id = ? AND target_id = ?) + LIMIT 1`, + sourceSyncID, targetSyncID, targetSyncID, sourceSyncID, + ).Scan(&exists) + if err == nil { + return true, nil + } + if err == sql.ErrNoRows { + return false, nil + } + return false, err +}Then each of the three call sites becomes a two-line
if exists, err := s.relationExists(a, b); err != nil { ... } else if exists { ... }.🤖 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 451 - 471, Extract the duplicated bidirectional existence query into a Store helper such as relationExists(sourceSyncID, targetSyncID string) (bool, error), preserving its current error behavior and predicate. Replace the checks in FindCandidates and both ScanProject locations with calls to this helper, handling returned errors and skipping existing relations as before.
451-471: 🗄️ Data Integrity & Integration | 🔵 TrivialExists-check-then-insert is not atomic; no DB constraint backs the dedup guard.
The comment here correctly notes
memory_relations has no UNIQUE on (source_id, target_id), so this SELECT-then-write pattern only guards against duplicates created within the same sequential code path (which is fine for the in-scan cases handled by this PR), but not against a concurrent second scan, or a concurrent directmem_comparecall, racing between the check and the write for the same pair. Consider adding a SQLite expression-based unique index over a normalized column pair (e.g.min(source_id,target_id), max(source_id,target_id), supported since SQLite 3.9) so duplicates are rejected at the DB layer regardless of caller concurrency, rather than relying solely on this application-level check.🤖 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 451 - 471, Add a SQLite unique expression index for memory_relations that normalizes source_id and target_id with min/max, enforcing one undirected relation per pair at the database layer. Update the relation insertion flow around FindCandidates and its write path to handle unique-constraint conflicts safely while preserving existing duplicate-skipping behavior.
🤖 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.
Outside diff comments:
In `@internal/store/relations.go`:
- Around line 451-471: Extract the duplicated bidirectional existence query into
a Store helper such as relationExists(sourceSyncID, targetSyncID string) (bool,
error), preserving its current error behavior and predicate. Replace the checks
in FindCandidates and both ScanProject locations with calls to this helper,
handling returned errors and skipping existing relations as before.
- Around line 451-471: Add a SQLite unique expression index for memory_relations
that normalizes source_id and target_id with min/max, enforcing one undirected
relation per pair at the database layer. Update the relation insertion flow
around FindCandidates and its write path to handle unique-constraint conflicts
safely while preserving existing duplicate-skipping behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 58ce14fa-13de-4c7e-b238-c94f4b704b7c
📒 Files selected for processing (1)
internal/store/relations.go
Fixes #490.
Summary
JudgeBySemanticreturned success without inserting a row fornot_conflict, causingFindCandidatesto re-surface the same pair on every session. This PR makesnot_conflictverdicts persist end-to-end and plugs the dedup gap inFindCandidates.Changes
Core fix (commit 1 —
050f288):JudgeBySemantic—not_conflictnow persists via existing UPSERTScanProjectskip that preventednot_conflictfrom being persistedExcludeNotConflicttoListRelationsOptions; dashboard and CLI conflict views set itGetRelationStatsexcludesnot_conflict(it's a conflict stats view)Reviewer feedback (commit 2 —
2a21160, rebased onto main):FindCandidatesnow skips pairs that already have a relation row in either direction — closes the user-visible symptom from fix(store): mem_compare not_conflict returns success but persists nothing #490 (every save re-inserted a freshpendingrow for the same pair). Same existence check asScanProject's Phase 3 pre-check.SemanticSkippedfield fromScanResult, CLI output, server JSON, and all tests (the skip that incremented it was removed in commit 1, leaving the field permanently zero).pairResultstruct (defined but never instantiated).TestHandleCompare_NotConflict_Persistsasserts the stored relation row;TestScanProject_Semantic_NotConflictPersistedcaptures the count-query error instead of ignoring it.TestFindCandidates_SkipsExistingRelation— two calls with the same pair; second must return 0 candidates.internal/store/relations.goJudgeBySemanticpersistsnot_conflict;FindCandidatesdedup check; removedSemanticSkipped+ deadpairResult;ExcludeNotConflictinListRelationsOptions;GetRelationStatsfilterinternal/store/relations_test.goTestFindCandidates_SkipsExistingRelationinternal/store/scan_semantic_test.goSemanticSkippedfrom sums; assert count-query errorinternal/store/judge_by_semantic_test.gonot_conflictpersistence assertionsinternal/mcp/mcp.gointernal/mcp/mcp_compare_test.gonot_conflictinternal/server/server.gohandleListConflicts+handleScanConflictsfilter; removedsemantic_skippedfrom JSONinternal/server/server_test.gosemantic_skippedfrom field checkscmd/engram/conflicts.gocmdConflictsListfilter; removedsemantic_skippedfrom CLI outputDesign decisions
FindCandidates filter is a query-level check, not a schema UNIQUE.
memory_relationsis sync-replicated and direction-sensitive; a DB constraint on(source_id, target_id)would reject legitimate re-judgments viaJudgeBySemantic's UPSERT. Same approach as the existingScanProjectpre-check.SemanticSkippedremoved, not repurposed. No production code increments it after commit 1. An always-zero JSON field is worse than none.Sync payload includes
not_conflictrows. Syncing the verdict prevents other nodes from re-evaluating the same pair and wasting LLM calls.Test plan
go test ./...(22 packages, 0 failures)go test -tags e2e ./internal/server/...go build ./...+go vet ./...cleanTestFindCandidates_SkipsExistingRelationfails without the dedup checkSummary by CodeRabbit
/conflictsnow consistently returns only conflict relations and reports a matchingtotal.semantic_skipped; semantic counters are aligned to judged/error metrics.not_conflictverdicts are now persisted and reused during semantic evaluation and scanning.not_conflictpersistence, conflict-only listing, and candidate skipping behavior.