From b021c1335f79f138ca114f760713b6ca34137a18 Mon Sep 17 00:00:00 2001 From: zaebee Date: Wed, 9 Sep 2026 19:51:42 +0000 Subject: [PATCH 1/4] fix(suggest-packages): name a member by its path under the package (#446) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Members rendered as their last FQN segment, so `p/sub/` and `p/sub/sub.py` both printed as `sub` — in two different communities. A reader told to split the package could not tell which one belonged where, and the same collision made a bridge endpoint point at the wrong file. `pkg/sub/sub.py` is an ordinary layout; cgis's own `query/drift/drift.py` is one. Members are now named by their path under the analysed prefix, which is unique because full FQNs are, and no longer than the ambiguity requires. The graph itself was never wrong — it is keyed on full FQNs, 27 files and 27 distinct on `cgis.query`, so Q = 0.492 was computed over a proper partition. The issue as first filed claimed otherwise; that claim came from reading the rendered table instead of `build_file_graph`, and is corrected on the issue. A side effect worth noting: the readable output makes the *other* half of #446 visible rather than hidden. `analysis` now plainly sits alone in one community while `analysis.analyzer`, `analysis.anomaly` and `analysis.health` sit in another — a sub-package as a peer of its own contents. Whether membership should be the package's direct children is a modelling decision to take with numbers, and stays open. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018Z1HtHgNsrKkQoc7wN6xeG --- src/cgis/query/analysis/suggest_service.py | 22 ++++++-- tests/unit/test_suggest_service.py | 58 ++++++++++++++++++++++ 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/src/cgis/query/analysis/suggest_service.py b/src/cgis/query/analysis/suggest_service.py index d6c7a1f1..6ef43867 100644 --- a/src/cgis/query/analysis/suggest_service.py +++ b/src/cgis/query/analysis/suggest_service.py @@ -63,9 +63,20 @@ 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_name(fqn: str, prefix: str) -> str: + """Name a file by its path under the analysed package, for readable output. + + The last segment alone is not enough to identify a file: `p/sub/` and + `p/sub/sub.py` both end in `sub`, so a report telling the reader to split `p` + listed the same name in two communities and left them guessing which one it + meant (#446). `pkg/sub/sub.py` is an ordinary Python layout. + + The path under the prefix is unique by construction, because full FQNs are — + `sub` and `sub.sub` here — and stays as short as the ambiguity allows. + """ + if not prefix or not fqn.startswith(f"{prefix}."): + return fqn.rsplit(".", 1)[-1] + return fqn[len(prefix) + 1 :] def _dir_group(fqn: str, prefix: str) -> str: @@ -190,7 +201,7 @@ def suggest_packages( bridges = sorted( ( - Bridge(source=_leaf(a), target=_leaf(b), weight=w) + Bridge(source=_member_name(a, package), target=_member_name(b, package), 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 +218,8 @@ 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=[_member_name(f, package) 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..ce2be2ea 100644 --- a/tests/unit/test_suggest_service.py +++ b/tests/unit/test_suggest_service.py @@ -179,3 +179,61 @@ 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 From 9fc8b34b96d4055c7c6020999143ae8bf4c07b81 Mon Sep 17 00:00:00 2001 From: zaebee Date: Wed, 9 Sep 2026 19:57:41 +0000 Subject: [PATCH 2/4] fix(suggest-packages): the package's own node renders as __init__ (#447 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Naming members relative to the prefix left the analysed package itself — where `fqn == prefix` — falling through to the last-segment fallback, which reintroduced the collision one level down. Real case, measured: cgis suggest-packages cgis.query.drift members: ['_scc', 'drift', 'drift', 'drift_service', ...] duplicated: ['drift'] for `drift/__init__.py` and `drift/drift.py`. It renders as `__init__` now, the file it stands for, which cannot clash: a nested `__init__.py` has its id folded into its own package's name, so only the root ever produces it. Same shape as the bug this PR fixes, one level down — which is a fair comment on the first fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018Z1HtHgNsrKkQoc7wN6xeG --- src/cgis/query/analysis/suggest_service.py | 9 +++++++ tests/unit/test_suggest_service.py | 28 ++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/cgis/query/analysis/suggest_service.py b/src/cgis/query/analysis/suggest_service.py index 6ef43867..c4503cf9 100644 --- a/src/cgis/query/analysis/suggest_service.py +++ b/src/cgis/query/analysis/suggest_service.py @@ -73,7 +73,16 @@ def _member_name(fqn: str, prefix: str) -> str: The path under the prefix is unique by construction, because full FQNs are — `sub` and `sub.sub` here — and stays as short as the ambiguity allows. + + The package's own node is the one file with nothing under the prefix left to + name, and naming it by the prefix's last segment reintroduced the collision + one level down: `suggest-packages cgis.query.drift` listed `drift` for both + `drift/__init__.py` and `drift/drift.py` (#447 review). It renders as + `__init__`, the file it stands for, which cannot clash — a nested + `__init__.py` has its id folded into its own package's name. """ + if fqn == prefix: + return "__init__" if not prefix or not fqn.startswith(f"{prefix}."): return fqn.rsplit(".", 1)[-1] return fqn[len(prefix) + 1 :] diff --git a/tests/unit/test_suggest_service.py b/tests/unit/test_suggest_service.py index ce2be2ea..3dc21cb0 100644 --- a/tests/unit/test_suggest_service.py +++ b/tests/unit/test_suggest_service.py @@ -237,3 +237,31 @@ def test_bridge_endpoints_are_named_like_members(tmp_path: Path) -> None: 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`. + + The root renders as `__init__`, which is the file it stands for and cannot + clash: a nested `__init__.py` has its id folded into its package's name. + """ + files = [make_file_node(n) for n in ("p", "p.p", "p.other", "p.third")] + edges = [ + make_import_edge(s, t) + for s in ("p", "p.p", "p.other", "p.third") + for t in ("p", "p.p", "p.other", "p.third") + 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 "__init__" in rendered + assert "p" in rendered From 4b097094a41032bbb9bf67374a35979bef1cd87d Mon Sep 17 00:00:00 2001 From: zaebee Date: Wed, 9 Sep 2026 20:08:34 +0000 Subject: [PATCH 3/4] fix(suggest-packages): decide member naming over the whole set, not per name (#447 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, and fixing the first showed the previous two attempts had each closed one shape and left another. **`__init__` was the wrong name, twice over.** It is a Python filename, while this tool reads FILE nodes in any language and the TypeScript extractor folds `/index` exactly as the Python one folds `/__init__` — a TS package would have listed a file that exists nowhere. And it did not resolve: every other member satisfies `prefix + "." + name == node id`, so an agent can read a community, pick a member and ask about it, but `prefix + ".__init__"` names nothing. This tool is MCP-facing; one unresolvable member per report is a real cost. Rendering the package's own node as its FQN instead looked right and was not: with prefix `p`, the root renders `p` and a module `p.p` renders `p` too. Same collision, third shape. So uniqueness is now decided once over the whole file set rather than argued per name: relative paths under the prefix, the package's own node by its 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 instead of one row spelled unlike its neighbours for a reason the reader cannot see. Every name still maps back to a node id. **The shipped contract said something else.** `Community` and `Bridge` docstrings, and `docs/specs/2026-06-13-suggest-packages-design.md:226`, all still described members as "the last FQN segment". Those three are what an integrator sizes fields and matches names against, and they had been false since the first commit on this branch. Corrected. Dogfooded at HEAD: `cgis.query` 27/27 distinct, `cgis.query.drift` 9/9, `cgis.guardian` 26/26, no duplicates anywhere. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018Z1HtHgNsrKkQoc7wN6xeG --- .../2026-06-13-suggest-packages-design.md | 9 ++- src/cgis/query/analysis/suggest_service.py | 65 ++++++++++++------- tests/unit/test_suggest_service.py | 39 +++++++---- 3 files changed, 76 insertions(+), 37 deletions(-) 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 c4503cf9..4d37117d 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,29 +72,37 @@ class SuggestReport: note: str | None = None -def _member_name(fqn: str, prefix: str) -> str: - """Name a file by its path under the analysed package, for readable output. +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 segment alone is not enough to identify a file: `p/sub/` and - `p/sub/sub.py` both end in `sub`, so a report telling the reader to split `p` - listed the same name in two communities and left them guessing which one it - meant (#446). `pkg/sub/sub.py` is an ordinary Python layout. + * 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`. - The path under the prefix is unique by construction, because full FQNs are — - `sub` and `sub.sub` here — and stays as short as the ambiguity allows. + 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. - The package's own node is the one file with nothing under the prefix left to - name, and naming it by the prefix's last segment reintroduced the collision - one level down: `suggest-packages cgis.query.drift` listed `drift` for both - `drift/__init__.py` and `drift/drift.py` (#447 review). It renders as - `__init__`, the file it stands for, which cannot clash — a nested - `__init__.py` has its id folded into its own package's name. + 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. """ - if fqn == prefix: - return "__init__" - if not prefix or not fqn.startswith(f"{prefix}."): - return fqn.rsplit(".", 1)[-1] - return fqn[len(prefix) + 1 :] + relative = { + fid: fid if fid == prefix else fid[len(prefix) + 1 :] if prefix else fid for fid in file_ids + } + if len(set(relative.values())) == len(relative): + return relative + return {fid: fid for fid in file_ids} def _dir_group(fqn: str, prefix: str) -> str: @@ -208,9 +225,10 @@ def suggest_packages( ) verdict = "leave" + names = _member_names(graph.files, package) bridges = sorted( ( - Bridge(source=_member_name(a, package), target=_member_name(b, package), 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] @@ -227,8 +245,7 @@ def suggest_packages( direction=direction, verdict=verdict, communities=[ - Community(id=i, files=[_member_name(f, package) 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 3dc21cb0..91e18eb0 100644 --- a/tests/unit/test_suggest_service.py +++ b/tests/unit/test_suggest_service.py @@ -247,21 +247,38 @@ def test_the_package_root_does_not_collide_with_a_same_named_module(tmp_path: Pa fallback. Real case: `cgis suggest-packages cgis.query.drift` listed `drift` twice, for `drift/__init__.py` and `drift/drift.py`. - The root renders as `__init__`, which is the file it stands for and cannot - clash: a nested `__init__.py` has its id folded into its package's name. + 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. """ - files = [make_file_node(n) for n in ("p", "p.p", "p.other", "p.third")] - edges = [ - make_import_edge(s, t) - for s in ("p", "p.p", "p.other", "p.third") - for t in ("p", "p.p", "p.other", "p.third") - if s != t - ] + 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 "__init__" in rendered - assert "p" in 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) From e56e29b139c76ec3d34b0aac66e0eafa03d402ce Mon Sep 17 00:00:00 2001 From: zaebee Date: Wed, 9 Sep 2026 20:17:19 +0000 Subject: [PATCH 4/4] refactor(suggest-packages): lift the loop-invariant prefix check out (#447 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nested ternary re-tested `prefix` for every element, though it is constant for the call. Handled once up front, which also lets the collision fallback and the empty-prefix case share one `absolute` map instead of building it twice. Behaviour identical, checked rather than assumed: 2373 passed, and the dogfood numbers are unchanged — cgis.query 27/27 distinct, cgis.query.drift 9/9, cgis.guardian 26/26. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018Z1HtHgNsrKkQoc7wN6xeG --- src/cgis/query/analysis/suggest_service.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/cgis/query/analysis/suggest_service.py b/src/cgis/query/analysis/suggest_service.py index 4d37117d..b548909b 100644 --- a/src/cgis/query/analysis/suggest_service.py +++ b/src/cgis/query/analysis/suggest_service.py @@ -97,12 +97,12 @@ def _member_names(file_ids: tuple[str, ...], prefix: str) -> dict[str, str]: extractor folds `/index` just as Python folds `/__init__`, and `prefix + ".__init__"` names nothing in either. """ - relative = { - fid: fid if fid == prefix else fid[len(prefix) + 1 :] if prefix else fid for fid in file_ids - } - if len(set(relative.values())) == len(relative): - return relative - return {fid: fid for fid in file_ids} + 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: