diff --git a/docs/specs/2026-06-13-suggest-packages-design.md b/docs/specs/2026-06-13-suggest-packages-design.md index daf9d6e1..0d458fcc 100644 --- a/docs/specs/2026-06-13-suggest-packages-design.md +++ b/docs/specs/2026-06-13-suggest-packages-design.md @@ -223,8 +223,13 @@ bridges table (source, target, weight). JSON output via the same enum pattern as `layer` is `"imports"` or `"imports+calls"`. `verdict` is one of `split` / `consolidate` / `aligned` / `leave` / `no_signal`; `direction` is `under_split` / `over_split` / `matched`. -File names in `communities`/`bridges` are the last FQN segment (the module name) for -readability. +File names in `communities`/`bridges` are the file's **path under the analysed +package** — `analysis.analyzer`, not `analyzer` — or its full FQN where a +relative name would be ambiguous. Superseded the original "last FQN segment" +rule in #446/#447: a bare segment cannot tell `p/sub/` from `p/sub/sub.py`, and +an integrator sizing fields or matching on leaf names will mis-handle a dotted +member. Every name maps back to a node id, by joining the prefix when relative +and as-is when absolute. ## Error / edge handling diff --git a/src/cgis/query/analysis/suggest_service.py b/src/cgis/query/analysis/suggest_service.py index d6c7a1f1..b548909b 100644 --- a/src/cgis/query/analysis/suggest_service.py +++ b/src/cgis/query/analysis/suggest_service.py @@ -26,7 +26,12 @@ @dataclass(frozen=True) class Community: - """One detected community: an id and its member files (last FQN segment).""" + """One detected community: an id and its member files. + + A member is named by its path under the analysed package — `analysis.analyzer` + — or by its full FQN where that would be ambiguous. See `_member_names`; the + values are not bare module names, and were not since #446. + """ id: int files: list[str] @@ -34,7 +39,11 @@ class Community: @dataclass(frozen=True) class Bridge: - """A cross-community edge — the cost of splitting (file names, last segment).""" + """A cross-community edge — the cost of splitting. + + Endpoints are named exactly as community members are, so the two lists can be + read against each other. + """ source: str target: str @@ -63,9 +72,37 @@ class SuggestReport: note: str | None = None -def _leaf(fqn: str) -> str: - """Return the last FQN segment (the module name) for readable output.""" - return fqn.rsplit(".", 1)[-1] +def _member_names(file_ids: tuple[str, ...], prefix: str) -> dict[str, str]: + """Map every file under `prefix` to a display name, unique across the report. + + Uniqueness is a property of the whole set, not of each name, so it is decided + once here rather than argued per row. Two earlier attempts each fixed one + shape and left another (#446, #447 review): + + * the last FQN segment collided for `p/sub/` against `p/sub/sub.py`; + * the path under the prefix collided for the package's own node against a + module named after it — `p` and `p.p` both render `p`. + + So: members are named by their path under the prefix, the package's own node + by its full FQN, and if those still clash — only possible when a module is + named exactly after its package — the whole report falls back to full FQNs. + Degrading the entire table keeps one rule visible in the output instead of + one row spelled differently from its neighbours for reasons the reader + cannot see. + + Every name maps back to a node id: relative ones by joining the prefix, and + absolute ones as they stand. That matters because this tool is MCP-facing — + an agent reads a community, picks a member and asks about it — and it is why + `__init__` was wrong: no such file exists in a TypeScript package, where the + extractor folds `/index` just as Python folds `/__init__`, and + `prefix + ".__init__"` names nothing in either. + """ + absolute = {fid: fid for fid in file_ids} + if not prefix: + return absolute + + relative = {fid: fid if fid == prefix else fid[len(prefix) + 1 :] for fid in file_ids} + return relative if len(set(relative.values())) == len(relative) else absolute def _dir_group(fqn: str, prefix: str) -> str: @@ -188,9 +225,10 @@ def suggest_packages( ) verdict = "leave" + names = _member_names(graph.files, package) bridges = sorted( ( - Bridge(source=_leaf(a), target=_leaf(b), weight=w) + Bridge(source=names[a], target=names[b], weight=w) for a in graph.adj for b, w in graph.adj[a].items() if a < b and comm_of[a] != comm_of[b] @@ -207,7 +245,7 @@ def suggest_packages( direction=direction, verdict=verdict, communities=[ - Community(id=i, files=[_leaf(f) for f in c]) for i, c in enumerate(communities) + Community(id=i, files=[names[f] for f in c]) for i, c in enumerate(communities) ], bridges=bridges, thresholds=thresholds, diff --git a/tests/unit/test_suggest_service.py b/tests/unit/test_suggest_service.py index 650b2880..91e18eb0 100644 --- a/tests/unit/test_suggest_service.py +++ b/tests/unit/test_suggest_service.py @@ -179,3 +179,106 @@ def test_two_ingest_roots_yield_same_verdict(tmp_path: Path) -> None: assert ra.verdict == rb.verdict == "split" assert ra.modularity_q == pytest.approx(rb.modularity_q) assert ra.file_count == rb.file_count == 6 + + +def _nested_name_collision() -> tuple[list[Node], list[Edge]]: + """`p.sub` (a sub-package) beside `p.sub.sub` (a module inside it) — #446. + + An ordinary Python layout: `p/sub/` holding `p/sub/sub.py`. Both files end in + the same segment, which is what the display collapsed them to. + """ + names = ("sub", "sub.sub", "sub.other", "a", "b", "c") + files = [make_file_node(f"p.{n}") for n in names] + edges = [ + make_import_edge(f"p.{s}", f"p.{t}") + for grp in (("sub", "sub.sub", "sub.other"), ("a", "b", "c")) + for s in grp + for t in grp + if s != t + ] + # One edge across the two groups, so a bridge exists to name — and it starts + # at the ambiguous file on purpose. + edges.append(make_import_edge("p.sub.sub", "p.a")) + return files, edges + + +def test_two_files_never_render_under_the_same_name(tmp_path: Path) -> None: + """A member's rendered name identifies it, or the advice cannot be acted on (#446). + + `p.sub` and `p.sub.sub` both displayed as `sub`, in different communities, so + a reader told to split the package could not tell which one belonged where. + """ + db = _store_with(tmp_path, *_nested_name_collision()) + report = suggest_packages(db, prefix="p", with_calls=False) + + rendered = [f for community in report.communities for f in community.files] + assert len(rendered) == len(set(rendered)), rendered + assert "sub" in rendered + assert "sub.sub" in rendered + + +def test_bridge_endpoints_are_named_like_members(tmp_path: Path) -> None: + """A bridge names the same files the communities do, so it needs the same names. + + Rendered separately, the two lists could disagree — a bridge saying `sub` + while the communities say `sub` and `sub.sub` leaves the reader guessing + which one the bridge crosses (#446). + """ + db = _store_with(tmp_path, *_nested_name_collision()) + report = suggest_packages(db, prefix="p", with_calls=False) + + assert report.bridges, "fixture must produce at least one bridge to test" + + # The fixture's crossing edge starts at `p.sub.sub`. Under the old rendering + # that endpoint printed as `sub` — the name the communities give to a + # *different* file — so the bridge pointed at the wrong one. + endpoints = {b.source for b in report.bridges} | {b.target for b in report.bridges} + assert "sub.sub" in endpoints, endpoints + + members = {f for community in report.communities for f in community.files} + assert endpoints <= members, endpoints - members + + +def test_the_package_root_does_not_collide_with_a_same_named_module(tmp_path: Path) -> None: + """Analysing `p` itself, `p` and `p.p` must not both render as `p` (#447 review). + + The first fix named members relative to the prefix, which leaves the package's + own node — where `fqn == prefix` — falling through to the last-segment + fallback. Real case: `cgis suggest-packages cgis.query.drift` listed `drift` + twice, for `drift/__init__.py` and `drift/drift.py`. + + A module named exactly after its own package is the one shape where relative + naming cannot separate them, so the whole report falls back to full FQNs — + one visible rule, rather than one row spelled unlike its neighbours. + """ + ids = ("p", "p.p", "p.other", "p.third") + files = [make_file_node(n) for n in ids] + edges = [make_import_edge(s, t) for s in ids for t in ids if s != t] + db = _store_with(tmp_path, files, edges) + + report = suggest_packages(db, prefix="p", with_calls=False) + + rendered = [f for community in report.communities for f in community.files] + assert len(rendered) == len(set(rendered)), rendered + assert set(rendered) == set(ids), rendered + + +def test_every_member_name_resolves_back_to_a_node_id(tmp_path: Path) -> None: + """A member must name a file the caller can then ask about (#447 review). + + This tool is MCP-facing: an agent reads a community, picks a member and calls + `cgis_context` on it. `__init__` — the first attempt at naming the package's + own node — resolved to nothing, and named a file that does not exist at all + in a TypeScript package, where the extractor folds `/index` exactly as the + Python one folds `/__init__`. + """ + ids = ("q", "q.a", "q.sub", "q.sub.b") + files = [make_file_node(n) for n in ids] + edges = [make_import_edge(s, t) for s in ids for t in ids if s != t] + db = _store_with(tmp_path, files, edges) + + report = suggest_packages(db, prefix="q", with_calls=False) + + rendered = [f for community in report.communities for f in community.files] + resolved = {name if name in ids else f"q.{name}" for name in rendered} + assert resolved == set(ids), resolved.symmetric_difference(ids)