From c402d93cea18d8406ed050644da9c529ae9d01b6 Mon Sep 17 00:00:00 2001 From: Steve Pelech Date: Thu, 12 Nov 2026 16:28:20 -0600 Subject: [PATCH] fix(trace-path): follow own-class calls and ignore test doubles A trace from ToolsController.PublishInstance stopped before the repository call that caused a real bug, for three reasons: - A C# call without a receiver (`GetByInstanceId(...)`, `this.Method()`) was stored as the bare method name, so the walk could not tell which class it meant. It is now stored as `OwnClass.Method` when the enclosing type declares that method. - A fake that implements an interface in a test project counted as a second implementation, so `IToolInstanceReader.GetByInstanceId` did not map to `ToolInstanceReader.GetByInstanceId`. Test classes no longer count as implementations. - Production code was linked to test code: `transaction.Commit()` resolved to a fake `RecordingTransaction.Commit` in a test project. When the caller is production code, test candidates are dropped; with only test candidates, the call stays unresolved. Test-code detection now also covers .NET test projects (`App.Tests`) and folders such as `Unit Tests`. C# repositories must be indexed again to get the own-class targets. Co-Authored-By: Claude Opus 5.5 --- REQUIREMENTS.md | 8 +- .../chunking/csharp_receiver_types.py | 45 +++++- app/services/chunking/route_specificity.py | 5 +- app/services/graph_traversal.py | 17 ++- ...t_trace_path_own_class_and_test_doubles.py | 138 ++++++++++++++++++ 5 files changed, 202 insertions(+), 11 deletions(-) create mode 100644 tests/test_trace_path_own_class_and_test_doubles.py diff --git a/REQUIREMENTS.md b/REQUIREMENTS.md index 4adb39c..01e6379 100644 --- a/REQUIREMENTS.md +++ b/REQUIREMENTS.md @@ -2,7 +2,7 @@ > **Note:** This document is automatically generated and verified against the live test suite by `scripts/generate_requirements.py` and `tests/backend/test_requirements_sync.py`. -**Test Verification Baseline:** **1244 Automated Tests** (880 Pytest Backend + 314 Vitest Frontend + 50 Playwright E2E). +**Test Verification Baseline:** **1248 Automated Tests** (884 Pytest Backend + 314 Vitest Frontend + 50 Playwright E2E). --- @@ -1483,6 +1483,12 @@ over the PostgresConnectionAdapter using an initialized test engine._ - `test_same_named_methods_get_only_their_injected_callers` - `test_undecidable_calls_are_kept` +#### `tests/test_trace_path_own_class_and_test_doubles.py` (4 tests) +- `test_test_projects_and_folders_count_as_test_code` +- `test_calls_without_a_receiver_get_the_own_class` +- `test_a_method_the_class_does_not_declare_stays_bare` +- `test_trace_reaches_the_repository_through_own_class_calls_and_the_real_implementation` + #### `tests/test_utf16_file_reads.py` (4 tests) - `test_text_samples_are_not_binary` - `test_real_binary_sample_is_binary` diff --git a/app/services/chunking/csharp_receiver_types.py b/app/services/chunking/csharp_receiver_types.py index 67f95b3..d1d99a5 100644 --- a/app/services/chunking/csharp_receiver_types.py +++ b/app/services/chunking/csharp_receiver_types.py @@ -9,8 +9,9 @@ The receiver must be a simple name (`x` or `this.x`). Its type comes from the nearest scope that declares it: a parameter, local variable, `foreach` variable, or lambda parameter of an enclosing method; or a field, property, or primary-constructor parameter of the enclosing type. `var x = new T()` -gives `T`. When no declaration is found (static calls, `var` without `new`, inherited members), -the call keeps the bare name. +gives `T`. A call without a receiver (`Method(...)` or `this.Method(...)`) gets the enclosing type when that +type declares the method. When no declaration is found (static calls, `var` without `new`, inherited +members), the call keeps the bare name. """ import re @@ -162,9 +163,45 @@ def receiver_type(call_node, source_bytes: bytes, cache: TypeCache) -> Optional[ return None +def _declared_methods(type_decl, source_bytes: bytes) -> ScopeTypes: + """Method names declared directly in a type (name -> type name), for calls without a receiver.""" + name_node = type_decl.child_by_field_name("name") + type_name = _text(name_node, source_bytes) if name_node is not None else "" + out: ScopeTypes = {} + for child in type_decl.children: + if child.type == "declaration_list" and type_name: + for member in child.children: + m_name = member.child_by_field_name("name") if member.type == "method_declaration" else None + if m_name is not None: + out.setdefault(_text(m_name, source_bytes), type_name) + return out + + +def own_class_type(call_node, source_bytes: bytes, target: str, cache: TypeCache) -> Optional[str]: + """The enclosing type for `Method(...)` or `this.Method(...)` when that type declares `Method`, else None.""" + fn_node = call_node.child_by_field_name("function") + if fn_node is None: + return None + if fn_node.type == "member_access_expression": + receiver = fn_node.child_by_field_name("expression") + if receiver is None or receiver.type != "this": + return None + elif fn_node.type not in ("identifier", "generic_name"): + return None + node = call_node.parent + while node is not None and node.type not in _TYPE_DECLARATIONS: + node = node.parent + if node is None: + return None + key = (node.start_byte, node.end_byte, "declared_methods") + if key not in cache: + cache[key] = _declared_methods(node, source_bytes) + return cache[key].get(target) + + def qualify_csharp_call(call_node, source_bytes: bytes, target: str, cache: TypeCache) -> str: - """`Type.target` when the receiver's declared type is known, else `target` unchanged.""" + """`Type.target` when the receiver's declared type (or, without a receiver, the own class) is known.""" if call_node.type != "invocation_expression" or not target or "." in target: return target - r_type = receiver_type(call_node, source_bytes, cache) + r_type = receiver_type(call_node, source_bytes, cache) or own_class_type(call_node, source_bytes, target, cache) return f"{r_type}.{target}" if r_type else target diff --git a/app/services/chunking/route_specificity.py b/app/services/chunking/route_specificity.py index dc6c39e..f723660 100644 --- a/app/services/chunking/route_specificity.py +++ b/app/services/chunking/route_specificity.py @@ -49,9 +49,10 @@ def is_test_route_file(rel_path: str) -> bool: path_parts = [p.lower() for p in (rel_path or "").replace("\\", "/").strip("/").split("/") if p] bname = path_parts[-1] if path_parts else "" return ( - any(p in ("tests", "test", "__tests__") for p in path_parts[:-1]) + # test folders, including .NET test projects (`App.Tests`) and folders such as `Unit Tests` + any(p in ("tests", "test", "__tests__") or p.endswith((".tests", ".test", " tests")) for p in path_parts[:-1]) or bname.startswith("test_") - or bname.endswith(("_test.py", ".test.ts", ".spec.ts", "tests.cs")) + or bname.endswith(("_test.py", ".test.ts", ".spec.ts", "tests.cs", "test.cs")) ) diff --git a/app/services/graph_traversal.py b/app/services/graph_traversal.py index 6c203d7..6c7f559 100644 --- a/app/services/graph_traversal.py +++ b/app/services/graph_traversal.py @@ -139,7 +139,7 @@ def get_permalink(r_repo: str, r_filepath: str, start_l: Optional[int], end_l: O ).fetchall() ] impl_rows = conn.execute( - "SELECT source_symbol, target_symbol FROM ast_relationships WHERE relationship_type = 'IMPLEMENTS'" + (" AND repo = ?" if bfs_repo else ""), + "SELECT source_symbol, source_filepath, target_symbol FROM ast_relationships WHERE relationship_type = 'IMPLEMENTS'" + (" AND repo = ?" if bfs_repo else ""), bfs_args, ).fetchall() @@ -154,6 +154,7 @@ def get_permalink(r_repo: str, r_filepath: str, start_l: Optional[int], end_l: O for s in repo_symbols: s["_clean_fp"] = _rel_path(s.get("filepath")) + s["_test"] = is_test_route_file(s["_clean_fp"]) full_s, name_s = s.get("full_symbol"), s.get("name") if full_s: syms_by_full.setdefault(full_s, []).append(s) @@ -178,7 +179,9 @@ def get_permalink(r_repo: str, r_filepath: str, start_l: Optional[int], end_l: O tgt_iface = (ir["target_symbol"] or "").split(".")[-1] if src_cls and tgt_iface: concrete_to_ifaces.setdefault(src_cls, set()).add(tgt_iface) - iface_to_concretes.setdefault(tgt_iface, set()).add(src_cls) + # Test doubles do not count as implementations: one real class plus fakes is still one implementation. + if not is_test_route_file(_rel_path(ir["source_filepath"])): + iface_to_concretes.setdefault(tgt_iface, set()).add(src_cls) def _resolve_symbol_candidates(sym_str: str) -> List[dict]: if not sym_str: @@ -204,7 +207,13 @@ def _single_implementation(full: str) -> str: return impl_cands[0]["full_symbol"] return full - def _resolve_di_callee(raw_target: str, caller_sym: str, candidates: List[dict]) -> str: + def _resolve_di_callee(raw_target: str, caller_sym: str, candidates: List[dict], caller_file: Optional[str] = None) -> str: + if caller_file and candidates and not is_test_route_file(_rel_path(caller_file)): + # Production code never calls into test code: drop test candidates, and keep the call unresolved + # when only test candidates match (for example `transaction.Commit()` and a fake transaction class). + candidates = [c for c in candidates if not c.get("_test")] + if not candidates: + return raw_target if not candidates: if "." in raw_target: owner, member = raw_target.rsplit(".", 1) @@ -461,7 +470,7 @@ def _make_edge_node(display_sym: str, row_obj: object, child_key: str) -> dict: continue raw_target = r["target_symbol"] callee_cands = [c for c in _resolve_symbol_candidates(raw_target) if c.get("full_symbol") != curr_sym] - callee_sym = _resolve_di_callee(raw_target, caller_sym, callee_cands) + callee_sym = _resolve_di_callee(raw_target, caller_sym, callee_cands, r["source_filepath"]) edge_key = (curr_sym, callee_sym, r["relationship_type"], "callee", r["line_number"]) if edge_key in visited_edges: continue diff --git a/tests/test_trace_path_own_class_and_test_doubles.py b/tests/test_trace_path_own_class_and_test_doubles.py new file mode 100644 index 0000000..a86c71d --- /dev/null +++ b/tests/test_trace_path_own_class_and_test_doubles.py @@ -0,0 +1,138 @@ +"""trace_path follows calls to the own class and ignores test doubles (fakes in test projects).""" + +import json +from unittest.mock import patch + +import pytest + +from app.services.chunking import extract_symbols_and_chunks +from app.services.chunking.route_specificity import is_test_route_file +from app.services.chunking.tree_sitter_loader import get_tree_sitter_parser +from app.services.search import trace_symbol_path +from tests.test_multilanguage_cs_ts_py import _conn_factory, _setup_sqlite_repo + +pytestmark = pytest.mark.skipif(get_tree_sitter_parser("c_sharp") is None, reason="needs the C# tree-sitter grammar") + +CONTROLLER = """\ +public class ToolsController +{ + private readonly ToolService _toolService; + public async Task PublishInstance(Guid id) { return Ok(await _toolService.PublishInstance(id)); } +} +""" +SERVICE = """\ +public class ToolService +{ + private readonly IToolInstanceReader _reader; + private readonly ToolSuperRepository _toolRepository; + + public async Task PublishInstance(Guid id) + { + var transaction = _toolRepository.SqlConnection.BeginTransaction(); + var tool = await GetByInstanceId(id, transaction); + transaction.Commit(); + return tool; + } + + public async Task GetByInstanceId(Guid id, IDbTransaction transaction) + { + return await _reader.GetByInstanceId(id, transaction); + } +} +""" +READER = """\ +public interface IToolInstanceReader +{ + Task GetByInstanceId(Guid id, IDbTransaction transaction); +} + +public sealed class ToolInstanceReader(ToolSuperRepository toolRepository) : IToolInstanceReader +{ + public async Task GetAccessPreflightAsync(Guid id) + { + return await toolRepository.GetToolAccessPreflight(id); + } + + public async Task GetByInstanceId(Guid id, IDbTransaction transaction) + { + var access = await this.GetAccessPreflightAsync(id); + return access; + } +} +""" +REPOSITORY = """\ +public class ToolSuperRepository +{ + public Task GetToolAccessPreflight(Guid id) => Task.FromResult(1); +} +""" +TEST_DOUBLES = """\ +public sealed class FakeToolInstanceReader : IToolInstanceReader +{ + public Task GetByInstanceId(Guid id, IDbTransaction transaction) => Task.FromResult(0); +} + +public sealed class RecordingTransaction +{ + public void Commit() { } +} +""" + + +def test_test_projects_and_folders_count_as_test_code(): + assert is_test_route_file("server/App.Tests/Services_V2/FakeToolInstanceReader.cs") + assert is_test_route_file("server/Orchestrator/Unit Tests/Complex/Routing_Tests.cs") + assert is_test_route_file("server/App/Services/ToolServiceTest.cs") + assert not is_test_route_file("server/App/Services/ToolService.cs") + assert not is_test_route_file("server/App/Contests/Scoreboard.cs") + + +def test_calls_without_a_receiver_get_the_own_class(): + targets = {r.target_symbol for r in extract_symbols_and_chunks(SERVICE, "ToolService.cs", repo="r").relationships + if r.relationship_type == "CALLS"} + assert "ToolService.GetByInstanceId" in targets + reader_targets = {r.target_symbol for r in extract_symbols_and_chunks(READER, "ToolInstanceReader.cs", repo="r").relationships + if r.relationship_type == "CALLS"} + assert "ToolInstanceReader.GetAccessPreflightAsync" in reader_targets # this.Method() + assert "ToolSuperRepository.GetToolAccessPreflight" in reader_targets # primary-constructor parameter + + +def test_a_method_the_class_does_not_declare_stays_bare(): + source = "public class A : Base { public void Run() { Inherited(); Local(); void Local() { } } }" + targets = {r.target_symbol for r in extract_symbols_and_chunks(source, "A.cs", repo="r").relationships + if r.relationship_type == "CALLS"} + assert "Inherited" in targets and "A.Inherited" not in targets + + +@pytest.fixture +def tool_publish_db(tmp_path): + db_path = str(tmp_path / "tool_publish.db") + _setup_sqlite_repo(db_path, "hub", [ + ("server/App/Controllers/ToolsController.cs", CONTROLLER), + ("server/App/Services/ToolService.cs", SERVICE), + ("server/App/Services/ToolInstanceReader.cs", READER), + ("server/Data/ToolSuperRepository.cs", REPOSITORY), + ("server/App.Tests/Services/TestDoubles.cs", TEST_DOUBLES), + ]) + return _conn_factory(db_path) + + +def _walk(nodes, key): + for node in nodes: + yield node["symbol"] + yield from _walk(node.get(key) or [], key) + + +def test_trace_reaches_the_repository_through_own_class_calls_and_the_real_implementation(tool_publish_db): + with patch("app.services.search.get_db_connection", side_effect=tool_publish_db): + down = json.loads(trace_symbol_path("ToolsController.PublishInstance", repo="hub", direction="callees", depth=5)) + up = json.loads(trace_symbol_path("ToolSuperRepository.GetToolAccessPreflight", repo="hub", direction="callers", depth=5)) + reached = list(_walk(down["callees"], "callees")) + assert "ToolService.GetByInstanceId" in reached + assert "ToolInstanceReader.GetByInstanceId" in reached + assert "ToolInstanceReader.GetAccessPreflightAsync" in reached + assert "ToolSuperRepository.GetToolAccessPreflight" in reached + assert not [s for s in reached if "Fake" in s or "RecordingTransaction" in s], reached + callers = list(_walk(up["callers"], "callers")) + assert callers[:2] == ["ToolInstanceReader.GetAccessPreflightAsync", "ToolInstanceReader.GetByInstanceId"] + assert "ToolsController.PublishInstance" in callers