Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds Relation API support to trace call paths between two symbols, answering "how does A reach B?". The implementation uses LSP Call Hierarchy requests with a BFS algorithm to discover all chains connecting a source symbol to a target symbol.
Changes:
- Added Pydantic schemas (
ChainNode,RelationRequest,RelationResponse) with markdown template for response formatting - Implemented
RelationCapabilityusing LSP Call Hierarchy with BFS path-finding algorithm - Added comprehensive schema/model tests covering request creation, response handling, and markdown formatting
- Updated documentation with API specification, examples, and design decisions
- Registered the new capability in the schema and capability modules
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/lsap/schema/relation.py |
Defines Pydantic models for relation requests/responses and markdown template |
src/lsap/capability/relation.py |
Implements BFS-based path-finding using LSP Call Hierarchy |
tests/test_relation.py |
Adds unit tests for schema validation and data models |
src/lsap/schema/__init__.py |
Registers relation schema in capability_schemas dictionary |
src/lsap/capability/__init__.py |
Adds RelationCapability to Capabilities TypedDict |
docs/schemas/draft/relation.md |
Comprehensive API documentation with examples and design rationale |
src/lsap/schema/draft/relation.py |
Removes old draft implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/schemas/draft/relation.md
Outdated
| | `max_depth` | `number` | The maximum depth used for the search. | | ||
|
|
There was a problem hiding this comment.
The documentation incorrectly lists max_depth as a field of RelationResponse. This field does not exist in the actual schema implementation. The max_depth value is accessible through request.max_depth since the response includes the original request.
| | `max_depth` | `number` | The maximum depth used for the search. | | |
| The maximum depth used for the search is available as `request.max_depth`, since the response includes the original `RelationRequest`. |
src/lsap/capability/relation.py
Outdated
| # Skip if already visited (cycle detection) | ||
| if current_key in visited: | ||
| continue | ||
| visited.add(current_key) | ||
|
|
||
| # Check if we've reached the target | ||
| if current_key == target_key: | ||
| found_chains.append(path) | ||
| continue | ||
|
|
There was a problem hiding this comment.
The BFS implementation has a bug that prevents finding all paths to the target. When the target is reached for the first time, it's marked as visited (line 118), which prevents subsequent paths from being discovered.
Consider this scenario:
- Path 1: Source → A → Target
- Path 2: Source → B → Target
When the first path reaches Target, it's marked as visited. When the second path tries to reach Target, it's skipped because it's already in the visited set.
To fix this, the target check should happen before marking the node as visited, or the target should never be added to the visited set. For example:
Move the target check before the visited.add() call:
# Check if we've reached the target
if current_key == target_key:
found_chains.append(path)
continue
# Skip if already visited (cycle detection)
if current_key in visited:
continue
visited.add(current_key)This way, the target can be reached multiple times via different paths, while still preventing cycles in non-target nodes.
| # Skip if already visited (cycle detection) | |
| if current_key in visited: | |
| continue | |
| visited.add(current_key) | |
| # Check if we've reached the target | |
| if current_key == target_key: | |
| found_chains.append(path) | |
| continue | |
| # Check if we've reached the target | |
| if current_key == target_key: | |
| found_chains.append(path) | |
| continue | |
| # Skip if already visited (cycle detection) | |
| if current_key in visited: | |
| continue | |
| visited.add(current_key) |
| """ | ||
| Functional tests for Relation API. | ||
|
|
||
| Tests the call chain discovery capability that answers "how does A reach B?" | ||
| """ | ||
|
|
||
| from pathlib import Path | ||
|
|
||
| from lsap.schema.locate import Locate, SymbolScope | ||
| from lsap.schema.relation import ChainNode, RelationRequest, RelationResponse | ||
|
|
||
|
|
||
| def test_chain_node_creation(): | ||
| """Test creating a ChainNode.""" | ||
| node = ChainNode( | ||
| name="handle_request", | ||
| kind="Function", | ||
| file_path=Path("src/controllers.py"), | ||
| ) | ||
| assert node.name == "handle_request" | ||
| assert node.kind == "Function" | ||
| assert node.file_path == Path("src/controllers.py") | ||
| assert node.detail is None | ||
|
|
||
|
|
||
| def test_chain_node_with_detail(): | ||
| """Test creating a ChainNode with detail.""" | ||
| node = ChainNode( | ||
| name="UserService.get_user", | ||
| kind="Method", | ||
| file_path=Path("src/services/user.py"), | ||
| detail="(user_id: int) -> User", | ||
| ) | ||
| assert node.name == "UserService.get_user" | ||
| assert node.kind == "Method" | ||
| assert node.detail == "(user_id: int) -> User" | ||
|
|
||
|
|
||
| def test_relation_request_creation(): | ||
| """Test creating a RelationRequest.""" | ||
| req = RelationRequest( | ||
| source=Locate( | ||
| file_path=Path("src/controllers.py"), | ||
| scope=SymbolScope(symbol_path=["handle_request"]), | ||
| ), | ||
| target=Locate( | ||
| file_path=Path("src/db.py"), | ||
| scope=SymbolScope(symbol_path=["query"]), | ||
| ), | ||
| ) | ||
| assert req.source.file_path == Path("src/controllers.py") | ||
| assert req.target.file_path == Path("src/db.py") | ||
| assert req.max_depth == 10 # default | ||
|
|
||
|
|
||
| def test_relation_request_with_custom_max_depth(): | ||
| """Test creating a RelationRequest with custom max_depth.""" | ||
| req = RelationRequest( | ||
| source=Locate( | ||
| file_path=Path("src/controllers.py"), | ||
| scope=SymbolScope(symbol_path=["handle_request"]), | ||
| ), | ||
| target=Locate( | ||
| file_path=Path("src/db.py"), | ||
| scope=SymbolScope(symbol_path=["query"]), | ||
| ), | ||
| max_depth=5, | ||
| ) | ||
| assert req.max_depth == 5 | ||
|
|
||
|
|
||
| def test_relation_response_with_single_chain(): | ||
| """Test RelationResponse with a single call chain.""" | ||
| source = ChainNode( | ||
| name="handle_request", | ||
| kind="Function", | ||
| file_path=Path("src/controllers.py"), | ||
| ) | ||
| target = ChainNode( | ||
| name="query", | ||
| kind="Function", | ||
| file_path=Path("src/db.py"), | ||
| ) | ||
|
|
||
| chain = [ | ||
| source, | ||
| ChainNode( | ||
| name="UserService.get_user", | ||
| kind="Method", | ||
| file_path=Path("src/services/user.py"), | ||
| ), | ||
| target, | ||
| ] | ||
|
|
||
| req = RelationRequest( | ||
| source=Locate( | ||
| file_path=Path("src/controllers.py"), | ||
| scope=SymbolScope(symbol_path=["handle_request"]), | ||
| ), | ||
| target=Locate( | ||
| file_path=Path("src/db.py"), | ||
| scope=SymbolScope(symbol_path=["query"]), | ||
| ), | ||
| ) | ||
|
|
||
| resp = RelationResponse( | ||
| request=req, | ||
| source=source, | ||
| target=target, | ||
| chains=[chain], | ||
| ) | ||
|
|
||
| assert len(resp.chains) == 1 | ||
| assert len(resp.chains[0]) == 3 | ||
| assert resp.chains[0][0].name == "handle_request" | ||
| assert resp.chains[0][1].name == "UserService.get_user" | ||
| assert resp.chains[0][2].name == "query" | ||
|
|
||
|
|
||
| def test_relation_response_with_multiple_chains(): | ||
| """Test RelationResponse with multiple call chains (example from docs).""" | ||
| source = ChainNode( | ||
| name="handle_request", | ||
| kind="Function", | ||
| file_path=Path("src/controllers.py"), | ||
| ) | ||
| target = ChainNode( | ||
| name="query", | ||
| kind="Function", | ||
| file_path=Path("src/db.py"), | ||
| ) | ||
|
|
||
| # Chain 1: handle_request -> UserService.get_user -> db.query | ||
| chain1 = [ | ||
| source, | ||
| ChainNode( | ||
| name="UserService.get_user", | ||
| kind="Method", | ||
| file_path=Path("src/services/user.py"), | ||
| ), | ||
| target, | ||
| ] | ||
|
|
||
| # Chain 2: handle_request -> AuthService.validate_token -> SessionManager.get_session -> db.query | ||
| chain2 = [ | ||
| source, | ||
| ChainNode( | ||
| name="AuthService.validate_token", | ||
| kind="Method", | ||
| file_path=Path("src/services/auth.py"), | ||
| ), | ||
| ChainNode( | ||
| name="SessionManager.get_session", | ||
| kind="Method", | ||
| file_path=Path("src/services/session.py"), | ||
| ), | ||
| target, | ||
| ] | ||
|
|
||
| req = RelationRequest( | ||
| source=Locate( | ||
| file_path=Path("src/controllers.py"), | ||
| scope=SymbolScope(symbol_path=["handle_request"]), | ||
| ), | ||
| target=Locate( | ||
| file_path=Path("src/db.py"), | ||
| scope=SymbolScope(symbol_path=["query"]), | ||
| ), | ||
| max_depth=5, | ||
| ) | ||
|
|
||
| resp = RelationResponse( | ||
| request=req, | ||
| source=source, | ||
| target=target, | ||
| chains=[chain1, chain2], | ||
| ) | ||
|
|
||
| assert len(resp.chains) == 2 | ||
| assert len(resp.chains[0]) == 3 | ||
| assert len(resp.chains[1]) == 4 | ||
| assert resp.chains[0][1].name == "UserService.get_user" | ||
| assert resp.chains[1][1].name == "AuthService.validate_token" | ||
| assert resp.chains[1][2].name == "SessionManager.get_session" | ||
|
|
||
|
|
||
| def test_relation_response_with_no_chains(): | ||
| """Test RelationResponse when no connection found.""" | ||
| source = ChainNode( | ||
| name="handle_request", | ||
| kind="Function", | ||
| file_path=Path("src/controllers.py"), | ||
| ) | ||
| target = ChainNode( | ||
| name="unrelated_function", | ||
| kind="Function", | ||
| file_path=Path("src/utils.py"), | ||
| ) | ||
|
|
||
| req = RelationRequest( | ||
| source=Locate( | ||
| file_path=Path("src/controllers.py"), | ||
| scope=SymbolScope(symbol_path=["handle_request"]), | ||
| ), | ||
| target=Locate( | ||
| file_path=Path("src/utils.py"), | ||
| scope=SymbolScope(symbol_path=["unrelated_function"]), | ||
| ), | ||
| ) | ||
|
|
||
| resp = RelationResponse( | ||
| request=req, | ||
| source=source, | ||
| target=target, | ||
| chains=[], | ||
| ) | ||
|
|
||
| assert len(resp.chains) == 0 | ||
|
|
||
|
|
||
| def test_relation_response_markdown_format_with_chains(): | ||
| """Test markdown formatting of RelationResponse with chains.""" | ||
| source = ChainNode( | ||
| name="handle_request", | ||
| kind="Function", | ||
| file_path=Path("src/controllers.py"), | ||
| ) | ||
| target = ChainNode( | ||
| name="query", | ||
| kind="Function", | ||
| file_path=Path("src/db.py"), | ||
| ) | ||
|
|
||
| chain = [ | ||
| source, | ||
| ChainNode( | ||
| name="UserService.get_user", | ||
| kind="Method", | ||
| file_path=Path("src/services/user.py"), | ||
| detail="(user_id: int) -> User", | ||
| ), | ||
| target, | ||
| ] | ||
|
|
||
| req = RelationRequest( | ||
| source=Locate( | ||
| file_path=Path("src/controllers.py"), | ||
| scope=SymbolScope(symbol_path=["handle_request"]), | ||
| ), | ||
| target=Locate( | ||
| file_path=Path("src/db.py"), | ||
| scope=SymbolScope(symbol_path=["query"]), | ||
| ), | ||
| ) | ||
|
|
||
| resp = RelationResponse( | ||
| request=req, | ||
| source=source, | ||
| target=target, | ||
| chains=[chain], | ||
| ) | ||
|
|
||
| markdown = resp.format() | ||
|
|
||
| # Check that markdown contains key elements | ||
| assert "handle_request" in markdown | ||
| assert "query" in markdown | ||
| assert "Found 1 call chain(s)" in markdown | ||
| assert "Chain 1" in markdown | ||
| assert "UserService.get_user" in markdown | ||
| assert "Method" in markdown | ||
| assert "(user_id: int) -> User" in markdown | ||
|
|
||
|
|
||
| def test_relation_response_markdown_format_no_chains(): | ||
| """Test markdown formatting of RelationResponse with no chains.""" | ||
| source = ChainNode( | ||
| name="handle_request", | ||
| kind="Function", | ||
| file_path=Path("src/controllers.py"), | ||
| ) | ||
| target = ChainNode( | ||
| name="unrelated_function", | ||
| kind="Function", | ||
| file_path=Path("src/utils.py"), | ||
| ) | ||
|
|
||
| req = RelationRequest( | ||
| source=Locate( | ||
| file_path=Path("src/controllers.py"), | ||
| scope=SymbolScope(symbol_path=["handle_request"]), | ||
| ), | ||
| target=Locate( | ||
| file_path=Path("src/utils.py"), | ||
| scope=SymbolScope(symbol_path=["unrelated_function"]), | ||
| ), | ||
| max_depth=5, | ||
| ) | ||
|
|
||
| resp = RelationResponse( | ||
| request=req, | ||
| source=source, | ||
| target=target, | ||
| chains=[], | ||
| ) | ||
|
|
||
| markdown = resp.format() | ||
|
|
||
| # Check that markdown indicates no connection | ||
| assert "handle_request" in markdown | ||
| assert "unrelated_function" in markdown | ||
| assert "No connection found" in markdown | ||
| assert "within depth 5" in markdown | ||
|
|
||
|
|
||
| def test_chain_node_equality(): | ||
| """Test that ChainNodes with same data are equal.""" | ||
| node1 = ChainNode( | ||
| name="foo", | ||
| kind="Function", | ||
| file_path=Path("test.py"), | ||
| detail="detail", | ||
| ) | ||
| node2 = ChainNode( | ||
| name="foo", | ||
| kind="Function", | ||
| file_path=Path("test.py"), | ||
| detail="detail", | ||
| ) | ||
| assert node1 == node2 | ||
|
|
||
|
|
||
| def test_relation_response_chain_order(): | ||
| """Test that chains preserve order.""" | ||
| source = ChainNode( | ||
| name="a", | ||
| kind="Function", | ||
| file_path=Path("a.py"), | ||
| ) | ||
| middle1 = ChainNode( | ||
| name="b", | ||
| kind="Function", | ||
| file_path=Path("b.py"), | ||
| ) | ||
| middle2 = ChainNode( | ||
| name="c", | ||
| kind="Function", | ||
| file_path=Path("c.py"), | ||
| ) | ||
| target = ChainNode( | ||
| name="d", | ||
| kind="Function", | ||
| file_path=Path("d.py"), | ||
| ) | ||
|
|
||
| chain = [source, middle1, middle2, target] | ||
|
|
||
| req = RelationRequest( | ||
| source=Locate( | ||
| file_path=Path("a.py"), | ||
| scope=SymbolScope(symbol_path=["a"]), | ||
| ), | ||
| target=Locate( | ||
| file_path=Path("d.py"), | ||
| scope=SymbolScope(symbol_path=["d"]), | ||
| ), | ||
| ) | ||
|
|
||
| resp = RelationResponse( | ||
| request=req, | ||
| source=source, | ||
| target=target, | ||
| chains=[chain], | ||
| ) | ||
|
|
||
| # Verify order is preserved | ||
| assert resp.chains[0][0].name == "a" | ||
| assert resp.chains[0][1].name == "b" | ||
| assert resp.chains[0][2].name == "c" | ||
| assert resp.chains[0][3].name == "d" | ||
|
|
||
|
|
||
| def test_relation_request_with_nested_symbol_path(): | ||
| """Test RelationRequest with nested symbol paths.""" | ||
| req = RelationRequest( | ||
| source=Locate( | ||
| file_path=Path("src/services/user.py"), | ||
| scope=SymbolScope(symbol_path=["UserService", "get_user"]), | ||
| ), | ||
| target=Locate( | ||
| file_path=Path("src/db.py"), | ||
| scope=SymbolScope(symbol_path=["Database", "query"]), | ||
| ), | ||
| max_depth=3, | ||
| ) | ||
|
|
||
| assert req.source.scope.symbol_path == ["UserService", "get_user"] | ||
| assert req.target.scope.symbol_path == ["Database", "query"] | ||
| assert req.max_depth == 3 |
There was a problem hiding this comment.
The test file only covers schema validation and data model tests, but lacks integration tests for the RelationCapability implementation. Consider adding tests that:
- Mock the LSP call hierarchy requests to test the BFS path-finding algorithm
- Test cycle detection with recursive function calls
- Test max_depth boundary conditions
- Test the scenario where no path exists between source and target
- Test multiple paths of different lengths
These tests would ensure the capability implementation works correctly, similar to how test_reference.py tests ReferenceCapability.
src/lsap/capability/relation.py
Outdated
| # Skip if already visited (cycle detection) | ||
| if current_key in visited: | ||
| continue | ||
| visited.add(current_key) | ||
|
|
||
| # Check if we've reached the target | ||
| if current_key == target_key: | ||
| found_chains.append(path) | ||
| continue | ||
|
|
||
| # Get outgoing calls from current item | ||
| outgoing_calls = ( | ||
| await call_hierarchy._request_call_hierarchy_outgoing_calls( | ||
| CallHierarchyOutgoingCallsParams(item=current_item) | ||
| ) | ||
| ) | ||
|
|
||
| if not outgoing_calls: | ||
| continue | ||
|
|
||
| # Add each outgoing call to the queue | ||
| for call in outgoing_calls: | ||
| next_item = call.to | ||
| next_node = self._to_chain_node(next_item) | ||
| next_path = path + [next_node] | ||
| queue.append((next_item, next_path, depth + 1)) |
There was a problem hiding this comment.
The BFS implementation has a subtle issue with the visited tracking. Currently, a node is marked as visited after being dequeued, which means the same node can be added to the queue multiple times before being processed. This could lead to memory issues with large call graphs.
Consider checking if a node is already visited before adding it to the queue. This would prevent redundant queue entries. For example:
Move the visited check to line 140, before appending to the queue:
next_key = self._item_key(next_item)
if next_key not in visited:
next_node = self._to_chain_node(next_item)
next_path = path + [next_node]
queue.append((next_item, next_path, depth + 1))
Note: The current implementation will still produce correct results, but is less efficient.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@observerw I've opened a new pull request, #17, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ts (#17) * Initial plan * fix: address PR review comments for Relation API - Fix documentation: Remove incorrect max_depth field from RelationResponse - Fix BFS bug: Move target check before visited.add() to find all paths - Fix BFS optimization: Check visited before adding to queue Co-authored-by: observerw <20661574+observerw@users.noreply.github.com> * test: add comprehensive integration tests for RelationCapability - Add mock LSP client with call hierarchy and document symbol support - Test single path discovery - Test multiple paths between symbols - Test scenario with no path - Test max_depth boundary conditions - Test cycle detection with recursive calls - Test direct calls - Test paths of different lengths Co-authored-by: observerw <20661574+observerw@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: observerw <20661574+observerw@users.noreply.github.com>
- Add inspect.md: documents usage-oriented symbol inspection API - Add explore.md: documents relationship-oriented code exploration API - Both docs include overview, use cases, schema, and examples
Summary
RelationCapabilityusing LSP Call Hierarchy.RelationRequestandRelationResponse.