diff --git a/src/java_codebase_rag/absence/absence_vocab.py b/src/java_codebase_rag/absence/absence_vocab.py index 6d041e0..a6deb39 100644 --- a/src/java_codebase_rag/absence/absence_vocab.py +++ b/src/java_codebase_rag/absence/absence_vocab.py @@ -375,8 +375,13 @@ def get_vocabulary_index(graph: Any, cfg: Any) -> VocabularyIndex: # (JSONDecodeError/KeyError) — all subsumed by Exception; rebuild. log.debug(f"Vocab index missing/stale/corrupt ({e}), rebuilding from graph") - # Build from graph - index = VocabularyIndex.build(graph, q=cfg.absence_ngram_q) + # Build from graph. Coerce q to int (mirror absence_diagnosis.py's + # int(getattr(cfg, ...)) pattern): a non-int cfg.absence_ngram_q (e.g. a + # MagicMock cfg from a leaked test mock, or a YAML string) would otherwise + # crash _qgrams at `len(text) < q` ('int < MagicMock'). Default 3 = the + # config default for absence_ngram_q. + q = int(getattr(cfg, "absence_ngram_q", 3) or 3) + index = VocabularyIndex.build(graph, q=q) # Save to sidecar (best-effort) try: diff --git a/src/java_codebase_rag/jrag_envelope.py b/src/java_codebase_rag/jrag_envelope.py index cc7249c..cfdd50d 100644 --- a/src/java_codebase_rag/jrag_envelope.py +++ b/src/java_codebase_rag/jrag_envelope.py @@ -16,6 +16,7 @@ import json import re from dataclasses import dataclass, field +from pathlib import Path from typing import Any, Literal from java_codebase_rag.absence.absence_types import AbsenceDiagnosis @@ -274,10 +275,22 @@ def simple_name(node_dict: dict[str, Any]) -> str: ``NodeRef`` carries no ``name`` field; the rendering layer derives a short label from the FQN on demand. Empty/missing FQN returns "". + + File/package Symbol nodes are the exception: their ``fqn`` is a filesystem + path (e.g. ``chat-assign/.../AssignConfiguration.java``), so the dot-split + would yield the file *extension* (``java``) instead of a name. When the fqn + is a path (contains ``/`` — the indexer POSIX-normalizes via ``.as_posix()`` + so backslashes don't occur) we return the basename. Route fqns + (``"GET /api/x"``) also contain ``/`` but carry a space, so they're excluded + here and stay on the dot-split path. """ fqn = str(node_dict.get("fqn") or "") if not fqn: return "" + if "/" in fqn and " " not in fqn: + base = Path(fqn).name + if base: + return base return fqn.rsplit(".", 1)[-1] diff --git a/src/java_codebase_rag/mcp/mcp_v2.py b/src/java_codebase_rag/mcp/mcp_v2.py index 3cccedf..f2e1544 100644 --- a/src/java_codebase_rag/mcp/mcp_v2.py +++ b/src/java_codebase_rag/mcp/mcp_v2.py @@ -1120,6 +1120,20 @@ def find_v2( fetch_cap = int(limit) + int(offset) + 1 if kind == "symbol": where, params = _symbol_where_from_filter(nf) + # Exclude structural Symbol nodes. Files and packages are :Symbol- + # labeled (kind='file'/'package') but aren't code declarations — + # without this, `fqn_contains` matches their filesystem-path fqn + # (e.g. 'Assign' in '.../DevAssignmentController.java') and surfaces + # them as hits. Mirrors search_lexical.py. Safe to apply + # unconditionally: DeclarationSymbolKind (the only values + # symbol_kind/symbol_kinds can take) excludes 'file'/'package', so no + # filter ever requests them. + struct_pred = "(s.kind <> 'file' AND s.kind <> 'package')" + where = ( + f"WHERE {struct_pred}" + if not where + else where.replace("WHERE ", f"WHERE {struct_pred} AND ", 1) + ) params["lim"] = fetch_cap rows = g._rows( # noqa: SLF001 f"MATCH (s:Symbol) {where} RETURN s.id AS id, s.fqn AS fqn, s.microservice AS microservice, " diff --git a/tests/absence/test_absence_mcp_integration.py b/tests/absence/test_absence_mcp_integration.py index a357829..5a3ef18 100644 --- a/tests/absence/test_absence_mcp_integration.py +++ b/tests/absence/test_absence_mcp_integration.py @@ -216,3 +216,40 @@ def test_resolve_non_empty_result_has_no_absence(ladybug_graph) -> None: assert out.success is True assert out.status in ("one", "many") assert out.absence is None + + +# --- Absence-config singleton isolation regression --- +# +# server.main() caches the operator cfg in a module-global singleton via +# set_absence_config() (mcp/server.py). Tests that drive server.main() with a +# MOCKED resolve_operator_config (tests/package/test_java_codebase_rag_cli.py) +# cache a MagicMock there, and monkeypatch does NOT revert it (set via a function +# call, not an attribute patch). On the graph-only macOS Intel CI leg this leaks +# into later absence/resolve tests in the same xdist worker and surfaces as +# `int < MagicMock` (best_sim < cfg.absence_absent_floor). The autouse +# `_reset_absence_config_singleton` fixture in conftest.py is the fix; the pair +# below pins the cross-test isolation contract. Runs in definition order. + + +def test_absence_config_singleton_leak_regression_poison() -> None: + """Part 1: simulate server.main() caching a MagicMock cfg in the singleton.""" + from unittest.mock import MagicMock + + from java_codebase_rag.analysis import resolve_service + from java_codebase_rag.mcp import mcp_v2 + + mcp_v2._absence_config = MagicMock() + resolve_service._absence_config = MagicMock() + assert isinstance(mcp_v2._absence_config, MagicMock) + + +def test_absence_config_singleton_leak_regression_is_clean() -> None: + """Part 2: the autouse isolation fixture must have reset the poisoned singleton + from part 1, so absence/resolve tools build a real config (no `int < MagicMock`). + Fails if the `_reset_absence_config_singleton` fixture is removed/broken. + """ + from java_codebase_rag.analysis import resolve_service + from java_codebase_rag.mcp import mcp_v2 + + assert mcp_v2._absence_config is None + assert resolve_service._absence_config is None diff --git a/tests/conftest.py b/tests/conftest.py index 240858a..0a1959e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -97,6 +97,33 @@ def _enforce_dev_deps() -> None: ) +@pytest.fixture(autouse=True) +def _reset_absence_config_singleton(): + """Isolate the module-global absence-diagnosis config between tests. + + ``server.main()`` calls ``mcp_v2.set_absence_config(cfg)`` and + ``resolve_service.set_absence_config(cfg)`` (mcp/server.py), caching ``cfg`` in + a module-global singleton. Tests that drive ``server.main()`` with a *mocked* + ``resolve_operator_config`` (tests/package/test_java_codebase_rag_cli.py) cache + a ``MagicMock`` there. ``monkeypatch`` does NOT revert it — the value is set via + a function call, not an attribute patch — so it leaks across tests in the same + xdist worker (``--dist loadfile``). Later absence/resolve tests then read the + mock; ``get_vocabulary_index`` passes ``cfg.absence_ngram_q`` (a MagicMock) + uncoerced into ``_qgrams`` → ``len(text) < q`` → ``int < MagicMock`` + (absence_vocab.py), surfacing as flaky ``success=False`` failures on the + graph-only macOS Intel CI leg. Resetting here guarantees each test sees a clean + config. (The production side also coerces ``q`` to int as defense-in-depth.) + """ + from java_codebase_rag.analysis import resolve_service + from java_codebase_rag.mcp import mcp_v2 + + mcp_v2._absence_config = None + resolve_service._absence_config = None + yield + mcp_v2._absence_config = None + resolve_service._absence_config = None + + @pytest.fixture(scope="session") def corpus_root() -> Path: assert CORPUS_ROOT.is_dir(), f"corpus missing: {CORPUS_ROOT}" diff --git a/tests/mcp/test_mcp_v2.py b/tests/mcp/test_mcp_v2.py index a6f614a..fe99412 100644 --- a/tests/mcp/test_mcp_v2.py +++ b/tests/mcp/test_mcp_v2.py @@ -171,8 +171,32 @@ def test_find_symbol_empty_filter_returns_results(ladybug_graph) -> None: out = find_v2("symbol", {}, graph=ladybug_graph) assert out.success is True assert out.results - # Regression guard: Symbol rows can include non-declaration kinds (e.g., package/file). + # Every returned row is a code declaration with a non-empty kind ... assert all(isinstance(r.symbol_kind, str) and r.symbol_kind for r in out.results) + # ... and structural file/package Symbol nodes are excluded (they are not + # code declarations). See test_find_symbol_excludes_file_and_package_nodes. + kinds = {r.symbol_kind for r in out.results} + assert "file" not in kinds + assert "package" not in kinds + + +def test_find_symbol_excludes_file_and_package_nodes(ladybug_graph) -> None: + """File/package Symbol nodes are structural, not code declarations, so find_v2 + must never surface them. Their ``fqn`` is a filesystem path, so a substring + filter otherwise matches the *filename* (e.g. ``Assign`` in + ``.../DevAssignmentController.java``) and leaks a file row whose rendered + label collapses to the ``java`` extension. Reproduces the chat-assign case. + """ + out = find_v2("symbol", {"fqn_contains": "Assign"}, graph=ladybug_graph) + assert out.success is True + assert out.results, "fixture (bank-chat-system) has Assign-named declarations" + kinds = {r.symbol_kind for r in out.results} + assert "file" not in kinds + # 'package' can't match here (CONTAINS is case-sensitive and package fqns are + # lowercase) — the package exclusion is proven by test_find_symbol_empty_filter_returns_results. + assert "package" not in kinds + # Real declarations still surface (the chat-assign module has Assign* types). + assert kinds & {"class", "interface", "enum", "record", "annotation", "method", "constructor"} def test_find_symbol_by_symbol_kind_method(ladybug_graph) -> None: diff --git a/tests/package/test_jrag_render.py b/tests/package/test_jrag_render.py index ea9e706..67454ce 100644 --- a/tests/package/test_jrag_render.py +++ b/tests/package/test_jrag_render.py @@ -396,6 +396,39 @@ def test_simple_name_derived_from_fqn() -> None: assert simple_name({}) == "" +def test_simple_name_file_path_fqn_returns_basename_not_extension() -> None: + """File/package Symbol nodes carry a filesystem path as ``fqn``; the dot-split + would otherwise yield the file *extension* (``java``) instead of a name. + A path-shaped fqn returns its basename; a route fqn (``"GET /api/x"``) still + has a space so it stays on the dot-split path; a package fqn (dots only) is + unchanged. + """ + assert simple_name({"fqn": "chat-assign/src/main/java/com/bank/chat/assign/config/AssignConfiguration.java"}) == "AssignConfiguration.java" + assert simple_name({"fqn": "src/main/java/Foo.java"}) == "Foo.java" + # Package node fqn: dots only, no separator -> unchanged dot-split. + assert simple_name({"fqn": "com.bank.chat.assign"}) == "assign" + # Route fqn contains '/' but also a space -> NOT treated as a path. + assert simple_name({"fqn": "GET /api/assign"}) == "GET /api/assign" + + +def test_render_listing_file_node_shows_basename_not_extension() -> None: + """User-visible symptom guard: a file Symbol node (fqn = filesystem path) must + render its basename, never collapse to the ``java`` extension. Covers the + render path (display_name -> simple_name) end-to-end, complementing the + simple_name unit test. Defense-in-depth for Layer 2: Layer 1 keeps file nodes + out of `find`, but query-mode exact match or traversal can still surface one. + """ + from java_codebase_rag.jrag_render import display_name + + node = {"fqn": "chat-assign/src/main/java/com/bank/chat/assign/config/AssignConfiguration.java"} + # The label is the basename, not the 'java' extension. + assert display_name(node) == "AssignConfiguration.java" + # And it reaches the rendered listing the user actually sees. + env = Envelope(status="ok", nodes={"sym:1": node}) + out = render(env, fmt="text", noun="matches") + assert "AssignConfiguration.java" in out + + # ----- Bonus: tiered_name tiers -----