Revert "Holla"#23
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Structural Memory Protocol (SMP) V2, a graph-based memory system for AI agents that utilizes static AST analysis to model complex codebases. The implementation includes a FastAPI-based JSON-RPC server, a Python SDK, and backends for Neo4j and ChromaDB. The code review identifies several critical issues, including a TypeError in the server initialization due to a missing argument in the MerkleIndex constructor, a potential UnboundLocalError in the linker, and inefficient O(N^2) complexity in the community detection algorithm. Additionally, the Merkle index synchronization logic is incomplete, the CLI's node ID generation is unreliable, and the codebase contains redundant routing logic. Feedback also suggests improving exception handling in the query engine and replacing inline imports with standard top-level imports for better readability.
| engine = SeedWalkEngine(graph_store=graph, vector_store=vector, enricher=enricher) | ||
| builder = DefaultGraphBuilder(graph) | ||
| registry = ParserRegistry() | ||
| merkle_index = MerkleIndex() |
There was a problem hiding this comment.
The MerkleIndex constructor requires a MerkleTree instance as an argument, but it is being called here without any arguments. This will result in a TypeError when the application starts.
| merkle_index = MerkleIndex() | |
| tree = MerkleTree() | |
| merkle_index = MerkleIndex(tree) |
| continue | ||
|
|
||
| if entity_name in import_map: | ||
| module_path, original_name = import_map[entity_name] |
There was a problem hiding this comment.
The variable entity_name is defined inside the conditional block starting at line 98. If that condition is not met, accessing entity_name here will raise an UnboundLocalError. This entire block (lines 108-134) should likely be nested within the if statement at line 98.
| ki = len(adjacency.get(nid, set())) | ||
|
|
||
| for comm, ki_comm in neighbor_comms.items(): | ||
| sigma_tot = sum(1 for n, c in community.items() if c == comm and n in adjacency) |
There was a problem hiding this comment.
This line performs a full iteration over all nodes in the graph (community.items()) inside a loop that already iterates over nodes and their neighbors. This leads to
| if self._tree.hash() == remote_hash: | ||
| return None | ||
|
|
||
| log.info("merkle_sync_diff_triggered", local=self._tree.hash(), remote=remote_hash) |
There was a problem hiding this comment.
The sync method currently returns None even when a hash mismatch is detected and logged. It should return the result of the diff or a signal to the caller that synchronization is required, as per the method's intended purpose.
| language=doc.language, | ||
| nodes=[ | ||
| type(n)( | ||
| id=n.id.replace(str(file_path), rel_path), |
There was a problem hiding this comment.
Using replace(str(file_path), rel_path) on the node ID is risky. If the absolute path string happens to be part of another component of the ID (e.g., a name or a signature that coincidentally contains the path), it will be incorrectly replaced. It is safer to construct the ID using the relative path directly or use more robust path manipulation utilities.
| @@ -0,0 +1,653 @@ | |||
| """JSON-RPC 2.0 dispatcher for the Structural Memory Protocol (SMP(3)). | |||
There was a problem hiding this comment.
This file appears to be a duplicate implementation of the JSON-RPC dispatching logic found in smp/protocol/dispatcher.py. Since smp/protocol/server.py is configured to use the modular handler pattern from dispatcher.py, this file is redundant and should be removed to avoid confusion and maintenance overhead.
| edges_out = await self._graph.get_edges(current_id, direction="outgoing") | ||
| except Exception: | ||
| edges_out = [] | ||
| try: | ||
| edges_in = await self._graph.get_edges(current_id, direction="incoming") | ||
| except Exception: | ||
| edges_in = [] | ||
| all_edges = edges_out + edges_in |
There was a problem hiding this comment.
Catching all exceptions and returning an empty list here masks potential issues with the graph store (e.g., connection timeouts or authentication failures). It is better to let the exception propagate or log it with a stack trace so that system failures are visible.
| id=comm.id, | ||
| type=NodeType("Community") if "Community" in NodeType._value2member_map_ else NodeType.FILE, | ||
| file_path=comm.majority_path_prefix, | ||
| structural=__import__("smp.core.models", fromlist=["StructuralProperties"]).StructuralProperties( |
There was a problem hiding this comment.
Using __import__ inline here is unnecessary and less readable than a standard import at the top of the file. Since smp.core.models is already used elsewhere, it should be imported normally.
1 participant
Reverts #22