Skip to content

fix(context): make ContextGraph.add_edge idempotent by deduping on edge_id - #926

Open
pravit-amp wants to merge 1 commit into
semantica-agi:mainfrom
pravit-amp:fix/922-context-graph-edge-dedupe
Open

fix(context): make ContextGraph.add_edge idempotent by deduping on edge_id#926
pravit-amp wants to merge 1 commit into
semantica-agi:mainfrom
pravit-amp:fix/922-context-graph-edge-dedupe

Conversation

@pravit-amp

Copy link
Copy Markdown
Contributor

Fixes #922

ContextGraph._add_internal_edge appended every edge unconditionally, so adding the same edge twice stored two copies sharing one content-derived edge_id. This inflated stats()["edge_count"], pushed density() past 1.0, and made re-ingest double the edge set on every run.

Changes

  • Added an edge_id -> ContextEdge index (_edge_index), mirroring how self.nodes dedupes by node ID.
  • _add_internal_edge now returns False when the edge_id already exists, before touching edges, edge_type_index, or _adjacency, and before firing the mutation callback (no phantom ADD_EDGE audit events).
  • The two state-reset paths (load and clear()) also clear the new index.
  • Four regression tests: repeat add_edge is a no-op, parallel edges with distinct attributes are preserved, re-ingest via build_from_entities_and_relationships stays at one edge, and clear() resets the dedupe index.

Notes

  • This implements the silent no-op option from the issue discussion (option 1). Happy to switch to update-in-place if maintainers prefer.
  • This also resolves the interaction with feat(context): add ContextGraph Markdown round-trip #852: ordinary re-ingested graphs no longer trip that PR's duplicate-edge-ID export guard.

Testing

@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Make ContextGraph.add_edge idempotent by deduping edges on edge_id

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Add an internal edge_id index to prevent duplicate edge storage.
• Make re-adding an existing edge_id a silent no-op (no index/audit mutations).
• Add regression tests covering idempotency, parallel edges, re-ingest, and clear().
Diagram

graph TD
  A["ContextGraph.add_edge"] --> B["Resolve edge_id"] --> C["_add_internal_edge"] --> D{ "edge_id exists?" }
  D -->|Yes| I["Return False"]
  D -->|No| E[("Edge storage")]
  E --> F["edge_type_index + adjacency"]
  E --> G["mutation_callback"]
  H["load_from_file / clear"] --> E
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Update-in-place on duplicate edge_id (upsert semantics)
  • ➕ Supports refreshing metadata/weight on re-add without creating new edges
  • ➕ Can simplify callers that naturally re-submit edges during re-ingest
  • ➖ Ambiguous behavior for an API named add_edge; re-add would mutate existing state
  • ➖ Harder to define and audit (ADD vs UPDATE) and can mask upstream duplication bugs
2. Deduplicate by (source_id, target_id, edge_type) instead of edge_id
  • ➕ Aligns with a common mental model of 'one relationship per pair/type'
  • ➖ Would collapse legitimate parallel edges that differ by attributes (e.g., confidence, validity window)
  • ➖ Requires defining equality rules across optional fields; higher risk of breaking behavior
3. Make duplicate-edge behavior configurable (no-op vs upsert vs error)
  • ➕ Allows different downstream expectations without changing core logic later
  • ➖ Adds configuration complexity and expands the test matrix and documentation surface

Recommendation: Keep the current silent no-op dedupe keyed by the content-derived edge_id. It directly addresses the reported duplication/density/stat inflation and prevents phantom audit events, while still allowing parallel edges when attributes differ (validated by tests). If update semantics are needed later, add an explicit update_edge/upsert_edge API rather than overloading add_edge.

Files changed (2) +64 / -0

Bug fix (1) +9 / -0
context_graph.pyAdd _edge_index and short-circuit duplicate internal edge inserts +9/-0

Add _edge_index and short-circuit duplicate internal edge inserts

• Introduces an internal _edge_index (edge_id -> ContextEdge) and uses it in _add_internal_edge to return False before mutating edges, edge_type_index, adjacency, or emitting mutation callbacks when an existing edge_id is re-added. Ensures load_from_file() and clear() also clear _edge_index to avoid stale dedupe state after resets.

semantica/context/context_graph.py

Tests (1) +55 / -0
test_context.pyAdd regression tests for edge dedupe and reset behavior +55/-0

Add regression tests for edge dedupe and reset behavior

• Adds tests asserting add_edge is idempotent for identical edges, parallel edges with distinct attributes remain distinct, repeated build_from_entities_and_relationships doesn’t duplicate edges, and clear() resets the dedupe index.

tests/context/test_context.py

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(context): ContextGraph.add_edge has no dedupe — identical edges are stored repeatedly under one shared edge ID, and re-ingest doubles the edge set

1 participant