Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions docs/specs/2026-06-13-suggest-packages-design.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
52 changes: 45 additions & 7 deletions src/cgis/query/analysis/suggest_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,24 @@

@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]


@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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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]
Expand All @@ -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,
Expand Down
103 changes: 103 additions & 0 deletions tests/unit/test_suggest_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Loading