fix(mcp): address PR #702 find_symbol review comments

DvirDukhan · Copilot · DvirDukhan · commit a5abde0f7f15 · 2026-06-08T23:17:39.000+03:00
- Validate/coerce limit: reject non-integers, accept stringified ints, and
  clamp to 1..FIND_SYMBOL_DB_LIMIT so a negative value no longer flips
  rows[:limit] into a surprising tail slice (_clamp_find_symbol_limit).
- Reject empty symbol names up front (empty, whitespace, or a trailing-dot
  qualname) instead of silently querying for "".
- Always include file_match in the response (False when no file filter is
  requested) so the documented shape is stable, and sort deterministically
  (in-file matches first, then file/line/name/id) so ordering no longer
  depends on FalkorDB row order.
- Add tests/mcp/test_find_symbol.py covering simple + dotted-name resolution,
  file disambiguation + ordering, response-shape stability, deterministic
  order, and limit/empty-name validation.

70/70 mcp tests pass against live FalkorDB; ruff clean.

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;
diff --git a/api/mcp/tools/structural.py b/api/mcp/tools/structural.py
@@ -739,6 +739,27 @@ async def _neighbors_payload(
 FIND_SYMBOL_DB_LIMIT = 200
 
 
+def _clamp_find_symbol_limit(limit: Any) -> int:
+    """Coerce ``limit`` to ``1..FIND_SYMBOL_DB_LIMIT``.
+
+    Agents may hand back a stringified or out-of-range ``limit``; a negative
+    value would otherwise flip ``rows[:limit]`` into a surprising tail slice and
+    a non-integer would fail deep in the slice with an opaque error. Strings of
+    digits are accepted; anything else is rejected up front.
+    """
+    if isinstance(limit, bool):
+        raise ValueError(f"limit must be an integer, got bool: {limit!r}")
+    if isinstance(limit, str) and limit.lstrip("-").isdigit():
+        limit = int(limit)
+    if not isinstance(limit, int):
+        raise ValueError(f"limit must be an integer, got: {limit!r}")
+    if limit < 1:
+        return 1
+    if limit > FIND_SYMBOL_DB_LIMIT:
+        return FIND_SYMBOL_DB_LIMIT
+    return limit
+
+
 @app.tool(
     name="find_symbol",
     description=(
@@ -767,14 +788,20 @@ async def find_symbol(
     """Look up Function/Class nodes by their simple name.
 
     Symbol nodes store only the SIMPLE name (``foo``), never the qualname
-    (``Bar.foo``), so a dotted input is reduced to its last segment. When
-    ``file`` is given we do not silently widen the search: every candidate is
-    tagged ``file_match`` and the in-file ones sort first, so a wrong/empty
-    file filter degrades visibly (the agent still sees the global matches but
-    knows none were in the requested file) rather than routing a relationship
-    query to an arbitrary same-named symbol.
+    (``Bar.foo``), so a dotted input is reduced to its last segment. Every
+    candidate carries a ``file_match`` flag (always ``False`` when no ``file``
+    filter is requested); when ``file`` is given we do not silently widen the
+    search — the in-file ones sort first, so a wrong/empty file filter degrades
+    visibly (the agent still sees the global matches but knows none were in the
+    requested file) rather than routing a relationship query to an arbitrary
+    same-named symbol.
     """
-    simple = str(name).strip().split(".")[-1]
+    simple = str(name).strip().split(".")[-1].strip()
+    if not simple:
+        raise ValueError(
+            f"name must be a non-empty symbol name, got: {name!r}"
+        )
+    eff_limit = _clamp_find_symbol_limit(limit)
     g = _project_arg(project, branch)
     try:
         res = await g._query(
@@ -796,19 +823,28 @@ async def find_symbol(
     for r in rows:
         r["symbol_id"] = r.pop("id")
 
-    if file is not None:
-        needle = str(file).strip().lstrip("/")
+    needle = str(file).strip().lstrip("/") if file is not None else ""
 
-        def _matches(r: dict[str, Any]) -> bool:
-            fp = r.get("file") or ""
-            return bool(needle) and (fp == needle or fp.endswith(needle) or needle in fp)
+    def _matches(r: dict[str, Any]) -> bool:
+        fp = r.get("file") or ""
+        return bool(needle) and (fp == needle or fp.endswith(needle) or needle in fp)
 
-        for r in rows:
-            r["file_match"] = _matches(r)
-        # In-file matches first; preserve relative order within each group.
-        rows.sort(key=lambda r: not r["file_match"])
-
-    return rows[:limit]
+    # ``file_match`` is part of the documented response shape, so set it on
+    # every row (False when no ``file`` filter is requested) rather than only
+    # when ``file`` is given. Sort deterministically — in-file matches first,
+    # then by file/line/name/id — so ordering never depends on FalkorDB row
+    # order between runs.
+    for r in rows:
+        r["file_match"] = _matches(r)
+    rows.sort(key=lambda r: (
+        not r["file_match"],
+        r.get("file") or "",
+        r["line"] if r.get("line") is not None else math.inf,
+        r.get("name") or "",
+        r["symbol_id"],
+    ))
+
+    return rows[:eff_limit]
 
 
 @app.tool(
diff --git a/tests/mcp/test_find_symbol.py b/tests/mcp/test_find_symbol.py
@@ -0,0 +1,221 @@
+"""T9 — find_symbol MCP tool tests.
+
+Covers name resolution (simple + dotted qualname reduction), the ``file``
+disambiguation flag + ordering, response-shape stability, deterministic order,
+and ``limit`` / empty-name validation.
+"""
+
+from __future__ import annotations
+
+import uuid
+
+import pytest
+
+
+pytestmark = pytest.mark.anyio
+
+
+@pytest.fixture
+def anyio_backend() -> str:
+    return "asyncio"
+
+
+# ---------------------------------------------------------------------------
+# Validation — no FalkorDB required (these raise before touching the graph)
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize("bad", ["", "   ", "Foo.", "Bar.   "])
+async def test_find_symbol_rejects_empty_name(bad):
+    from api.mcp.tools.structural import find_symbol
+
+    with pytest.raises(ValueError, match="non-empty symbol name"):
+        await find_symbol(name=bad, project="any")
+
+
+@pytest.mark.parametrize("bad", ["not-a-number", 1.5, None, True])
+async def test_find_symbol_rejects_garbage_limit(bad):
+    from api.mcp.tools.structural import find_symbol
+
+    with pytest.raises(ValueError, match="limit"):
+        await find_symbol(name="entrypoint", project="any", limit=bad)
+
+
+# ---------------------------------------------------------------------------
+# Integration — sample_project fixture
+# ---------------------------------------------------------------------------
+
+
+async def test_find_symbol_resolves_simple_name(indexed_fixture):
+    from api.mcp.tools.structural import find_symbol
+
+    rows = await find_symbol(
+        name="entrypoint",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+    )
+    assert rows, "entrypoint must resolve"
+    r = rows[0]
+    assert isinstance(r["symbol_id"], int)
+    assert r["name"] == "entrypoint"
+    assert r["label"] == "Function"
+    assert r["file"].endswith("entrypoint.py")
+    assert isinstance(r["line"], int)
+    # file_match is part of the documented shape and present even without a
+    # ``file`` filter (False when none requested).
+    assert r["file_match"] is False
+
+
+async def test_find_symbol_reduces_dotted_qualname(indexed_fixture):
+    """``Class.method`` must resolve identically to the bare ``method``."""
+    from api.mcp.tools.structural import find_symbol
+
+    dotted = await find_symbol(
+        name="BaseRepo.repo",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+    )
+    simple = await find_symbol(
+        name="repo",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+    )
+    assert {r["symbol_id"] for r in dotted} == {r["symbol_id"] for r in simple}
+    assert all(r["name"] == "repo" for r in dotted)
+    # Fixture defines repo() on BaseRepo, UserRepo and OrderRepo.
+    assert len(simple) == 3
+
+
+async def test_find_symbol_file_match_shape_stable(indexed_fixture):
+    """Every row carries a boolean ``file_match`` regardless of the args."""
+    from api.mcp.tools.structural import find_symbol
+
+    rows = await find_symbol(
+        name="repo",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+    )
+    assert rows
+    for r in rows:
+        assert isinstance(r["file_match"], bool)
+        assert set(r) >= {"symbol_id", "name", "label", "file", "line", "file_match"}
+
+
+async def test_find_symbol_file_filter_flags_matches(indexed_fixture):
+    """A matching ``file`` flags every in-file candidate; a non-matching one
+    still returns the global matches but flags none."""
+    from api.mcp.tools.structural import find_symbol
+
+    in_file = await find_symbol(
+        name="repo",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+        file="repo.py",
+    )
+    assert in_file and all(r["file_match"] for r in in_file)
+
+    no_file = await find_symbol(
+        name="repo",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+        file="does_not_exist.py",
+    )
+    # Search is not silently widened away — global matches still surface…
+    assert {r["symbol_id"] for r in no_file} == {r["symbol_id"] for r in in_file}
+    # …but the agent can see none were in the requested file.
+    assert all(r["file_match"] is False for r in no_file)
+
+
+async def test_find_symbol_deterministic_order(indexed_fixture):
+    """Repeated calls return rows in the same order (no FalkorDB row-order
+    dependence)."""
+    from api.mcp.tools.structural import find_symbol
+
+    a = await find_symbol(
+        name="repo",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+    )
+    b = await find_symbol(
+        name="repo",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+    )
+    assert [r["symbol_id"] for r in a] == [r["symbol_id"] for r in b]
+
+
+async def test_find_symbol_honors_limit(indexed_fixture):
+    from api.mcp.tools.structural import find_symbol
+
+    rows = await find_symbol(
+        name="repo",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+        limit="1",  # stringified ints are accepted and coerced
+    )
+    assert len(rows) == 1
+
+
+async def test_find_symbol_negative_limit_clamped_not_tail_slice(indexed_fixture):
+    """A negative ``limit`` must clamp to 1, NOT flip ``rows[:limit]`` into a
+    surprising tail slice."""
+    from api.mcp.tools.structural import find_symbol
+
+    full = await find_symbol(
+        name="repo",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+    )
+    clamped = await find_symbol(
+        name="repo",
+        project=indexed_fixture.project,
+        branch=indexed_fixture.branch,
+        limit=-2,
+    )
+    assert len(clamped) == 1
+    assert clamped[0]["symbol_id"] == full[0]["symbol_id"]
+
+
+async def test_find_symbol_registered():
+    from api.mcp.server import app
+
+    names = {t.name for t in await app.list_tools()}
+    assert "find_symbol" in names
+
+
+# ---------------------------------------------------------------------------
+# Ordering across files — purpose-built graph (two same-named symbols)
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+async def two_file_graph():
+    """Two ``foo`` Functions in different files so ``file`` disambiguation can
+    actually reorder the result. Unique branch keeps it isolated; not torn down
+    (matches the ``indexed_fixture`` / ``cycle_graph`` pattern)."""
+    from api.graph import Graph
+
+    project = "find_symbol_order_test"
+    branch = f"order-{uuid.uuid4().hex[:8]}"
+    g = Graph(project, branch=branch)
+    g.g.query(
+        """
+        CREATE
+          (:Function:Searchable {name: 'foo', path: '/tmp/aaa.py', src_start: 1}),
+          (:Function:Searchable {name: 'foo', path: '/tmp/bbb.py', src_start: 1})
+        """
+    )
+    yield project, branch
+
+
+async def test_find_symbol_orders_in_file_first(two_file_graph):
+    from api.mcp.tools.structural import find_symbol
+
+    project, branch = two_file_graph
+
+    rows = await find_symbol(name="foo", project=project, branch=branch, file="bbb.py")
+    assert len(rows) == 2
+    # The requested file sorts first and is the only flagged match.
+    assert rows[0]["file"].endswith("bbb.py")
+    assert rows[0]["file_match"] is True
+    assert rows[1]["file_match"] is False
