perf(graph): swap BFS for Dijkstra over edge-weighted graph (#328)#463
Conversation
Memory graph edges carry weights from 0.1 to 1.0 (encoding relation
strength). BFS visits nodes in edge-count order regardless of weight,
so a one-hop weak edge ranked the same as a two-hop chain of strong
edges to the same node — which is the wrong semantics for the
weighted graph the retrieval surface actually queries.
Replace bfsTraversal with dijkstraTraversal:
cost(edge) = 1 / max(edge.weight, 0.01)
Higher-weight edges cost less, so Dijkstra prefers strong-edge paths.
The clamp floor at 0.01 protects against malformed edges with
weight=0 dividing by zero (the documented weight floor is 0.1, but
imports / older snapshots may carry zeros).
Same swap tightens the perf profile:
- Adjacency built once in O(V+E) (was: filter allEdges per visited
node, O(V·E) overall).
- Min-heap dequeue is O(log V) per pop (was: Array.shift() which is
O(n) — the dominant cost on graphs above ~200 nodes per the
contributor's benchmark linked in #328).
Min-heap is a 50-LOC inline class so the file doesn't take a new
dependency for the perf-critical inner loop.
Semantics preserved:
- maxDepth still bounds edge-count, not accumulated cost. So a deep
chain of strong edges still terminates at maxDepth hops.
- One path per reachable node within depth (matches what the
existing scoring loop consumes).
- score = avgWeight × (1/pathLength) formula unchanged.
- src/functions/graph-retrieval.ts: dijkstraTraversal + MinHeap
- test/graph-retrieval.test.ts: 4 new cases — weight-optimal path
selection (the core Dijkstra promise), disconnected-node safety,
near-zero-weight clamp, maxDepth bound preserved
1011/1011 tests pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughGraph traversal switches from breadth-first search to a Dijkstra-style weighted traversal using an inline MinHeap; two search entry points call the new traversal and tests are added for weight-based path choice, zero-weight handling, disconnected nodes, start-node scoring, and maxDepth. ChangesDijkstra-Based Graph Traversal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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.
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 `@src/functions/graph-retrieval.ts`:
- Around line 286-296: The returned path list includes an entry for the
startNode (populated in pathTo) which causes start node observations to get
averaged and scored as 0.5; update the graph traversal in
src/functions/graph-retrieval.ts so the startNode is excluded from the returned
paths: either avoid inserting the initial startNode entry into pathTo or remove
pathTo.get(startNode) (the startNode key) before the function returns (the line
returning Array.from(pathTo.values())). Ensure you reference and preserve
existing symbols pathTo and startNode so other consumers still receive only
non-start-node 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 355eff49-0bb4-4389-acc2-42f0ab536143
📒 Files selected for processing (2)
src/functions/graph-retrieval.tstest/graph-retrieval.test.ts
Inline review on #463 surfaced a pre-existing bug carried forward through the BFS-to-Dijkstra refactor: the returned path list included the startNode's own length-1 path, so the generic path-scoring loop in searchByEntities ran first — pathLength=1 with no edges → avgWeight fell to 0.5 (empty-edgeWeights fallback) → score=0.5 → startNode obs marked visited. The dedicated score=1.0 fallback loop for startNode obs was then skipped by the visitedObs guard, making it dead code. So startNode observations were silently scoring 0.5 instead of the intended 1.0. Drop the startNode entry from pathTo before returning. The caller's fallback loop now fires as designed. - src/functions/graph-retrieval.ts: pathTo.delete(startNode.id) immediately before Array.from(pathTo.values()); inline comment explains why the fallback would otherwise be dead. - test/graph-retrieval.test.ts: regression asserts obs_root carries score=1.0 and pathLength=0 when its node is the matched start. 1012/1012 tests pass.
Brings 10 days of main into the OpenCode plugin branch so the PR no longer conflicts on README + carries the new surfaces that shipped between v0.9.2 (when the branch opened) and v0.9.20: - v0.9.19 commit linking (rohitg00#498): KV.commits + Session.commitShas + memory_commit_lookup/memory_commits MCP tools (53 total now, plugin badge bumped from 51) - v0.9.19 Azure OpenAI v1 URL pattern (rohitg00#462) + Dijkstra graph retrieval (rohitg00#463) - v0.9.19 env passthrough on MCP server entries (rohitg00#460): ${VAR} expansion for AGENTMEMORY_URL / AGENTMEMORY_SECRET so one wired entry covers local + remote - v0.9.20 Codex Stop revert (rohitg00#501) README conflict resolution kept main's richer "Other agents" table shape (env-passthrough block + per-host config-file column + programmatic-access section) and re-added the OpenCode entry as two rows: "OpenCode (MCP only)" for the bare MCP wiring + "OpenCode (full plugin)" pointing at this plugin's 22-hook capture surface. src/triggers/api.ts auto-merged: PR's 3-line title->summary/firstPrompt addition (lines 535, 543, 544) survived alongside main's other api.ts churn since. plugin/opencode/plugin.json bumped 0.9.4 -> 0.9.20 to match the canonical version everything else ships on. plugin/opencode/README.md MCP-tool badge bumped 51 -> 53.
Closes #328.
Problem
Memory graph edges carry weights from 0.1 to 1.0 encoding relation strength.
GraphRetrieval.bfsTraversalvisited nodes in edge-count order regardless of weight, so a one-hop weak edge (e.g.weight: 0.15) ranked the same as a two-hop chain of strong edges to the same node (e.g.0.9 + 0.9). For the weighted graph the retrieval surface actually queries, that's the wrong semantics — high-confidence relations should pull memories in first.Also: the BFS implementation had a sub-optimal perf profile that showed up in @Tanmay-008's benchmark in #328 —
Array.shift()is O(n) per dequeue, andallEdges.filter(...)ran per visited node (O(V·E) overall).Fix
Replace
bfsTraversalwithdijkstraTraversal:Higher-weight edges cost less, so Dijkstra prefers strong-edge paths. The clamp floor at
0.01protects against malformed edges withweight: 0from imports / older snapshots without crashing (documented weight floor is 0.1).Same swap tightens the perf profile:
O(V+E)via aMap<nodeId, [{neighborId, edge}]>. Was:allEdges.filter(...)per visited node,O(V·E)overall.O(log V)per pop. Was:Array.shift()atO(n). The dominant cost on graphs above ~200 nodes per perf: improve graph searching by replacing BFS with Dijkstra's algorithm #328's benchmark.Min-heap is a 50-LOC inline class —
graph-retrievaldoesn't take a new dependency for the perf-critical inner loop.Semantics preserved
maxDepthstill bounds edge-count (not accumulated cost). A deep chain of strong edges terminates atmaxDepthhops, same as before.score = avgWeight × (1/pathLength)formula unchanged. Only the which path changed.Tests
4 new cases in
test/graph-retrieval.test.ts:n1 → n3via weight 0.15 vs chainn1 → n2 → n3via 0.9 + 0.9. Dijkstra picks the chain (pathLength: 3, context containsMid).maxDepthbound preserved: chain of 3 hops withmaxDepth: 2reaches the 2-hop node but not the 3-hop one.9 existing cases continue to pass.
1011/1011 tests pass locally.
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests