From 95004bc984858a01cf14a420df1242da68556250 Mon Sep 17 00:00:00 2001 From: zaebee Date: Fri, 23 Oct 2026 07:21:09 +0000 Subject: [PATCH 1/4] feat: module graph of one package, as Mermaid or JSON (cgis_module_graph, cgis module-graph) "How is this package built inside?" had no tool: cgis_get_structure gives containment, trace/impact the graph around one symbol, suggest_packages communities and bridges. cgis_module_graph and `cgis module-graph` answer it from graph.db (#741): - Nodes are the package's direct children, each sub-package one node (`expand` for every module below it). - Edges are imports -->, extends ==> and calls -.->, a symbol edge counting for the files holding its ends. Imports read IMPORTS_SYMBOL too, since `from pkg import mod` points IMPORTS at the package. Calls are off by default; type-only imports and test modules are left out. - Each kind is transitively reduced. An edge stays when it is part of a cycle or when nothing else reaches its target, so cycles never vanish from the picture. - Dependencies on code outside the package are a table per module, not edges; a package outside is named whole (lib.deep, not lib). - Mermaid ids carry an m_ prefix, so `end` or `graph` is safe. The json form is {prefix, nodes, subpackages, edges, dropped, outside}. The prefix is resolved by the surfaces through effective_prefixes (#584), and the module keeps no import inside cgis.query: with them, query's self-drift rose from 0.228 to 0.241 against a 0.24 tolerance. Without them it is 0.229. The tolerance is unchanged. Closes #741 Co-Authored-By: Claude Opus 5.5 --- src/cgis/api/mcp_server.py | 60 ++++- src/cgis/api/server_instructions.py | 1 + src/cgis/cli.py | 59 +++++ src/cgis/query/analysis/module_graph.py | 271 +++++++++++++++++++++ tests/unit/test_mcp_server_instructions.py | 3 +- tests/unit/test_module_graph.py | 244 +++++++++++++++++++ 6 files changed, 636 insertions(+), 2 deletions(-) create mode 100644 src/cgis/query/analysis/module_graph.py create mode 100644 tests/unit/test_module_graph.py diff --git a/src/cgis/api/mcp_server.py b/src/cgis/api/mcp_server.py index 9d71e7b6..901e4806 100644 --- a/src/cgis/api/mcp_server.py +++ b/src/cgis/api/mcp_server.py @@ -9,7 +9,7 @@ import os import sys from pathlib import Path -from typing import Annotated, Any, NamedTuple +from typing import Annotated, Any, Literal, NamedTuple import structlog from pydantic import Field @@ -24,6 +24,7 @@ from cgis.core.models import RAW_CALL_PREFIX, Edge, EdgeType, Node, NodeType from cgis.languages import extractors, impact_caveat from cgis.pipeline import IngestionPipeline, extraction_fingerprint, ingest_data_keys +from cgis.query.analysis.module_graph import graph_to_dict, module_graph, render_mermaid from cgis.query.analysis.overview import DEFAULT_DEPTH, DEFAULT_LIMIT, build_overview from cgis.query.analysis.suggest_service import report_to_dict, suggest_packages from cgis.query.context.audit import audit_reachability @@ -919,6 +920,63 @@ def cgis_suggest_packages( return json.dumps(_with_freshness(db_path, report_to_dict(report)), indent=2) +@mcp.tool() +@refreshes_graph +def cgis_module_graph( + prefix: Annotated[ + str, + Field( + description="FQN prefix of the package as the graph names it, e.g. cgis.query " + "(cgis_find_symbol shows it), matched on whole dot-segments. A directory above " + "the ingest root, such as src., is dropped." + ), + ], + db_path: DbPath = _DEFAULT_DB, + layers: Annotated[ + list[Literal["imports", "extends", "calls"]], + Field(description="Edge kinds to draw. Calls make the picture much denser."), + ] = ["imports", "extends"], # noqa: B006 - a JSON-schema default, never mutated + reduce: Annotated[ + bool, + Field(description="Drop each edge implied by a longer path, per kind; cycles stay."), + ] = True, + exclude_tests: Annotated[bool, Field(description="Leave test modules out.")] = True, + expand: Annotated[ + bool, + Field(description="Every module below the package instead of one node per sub-package."), + ] = False, + output_format: Annotated[ + Literal["mermaid", "json"], + Field(description="mermaid: a diagram plus a table of outside dependencies; json: data."), + ] = "mermaid", +) -> str: + """How one package is built inside: its modules and the dependencies between them. + + Nodes are the package's direct children, each sub-package one node (``expand`` + lists every module instead). Edges are imports ``-->``, inheritance ``==>`` + and calls ``-.->``, each kind transitively reduced so the direct layering + shows; an edge that closes a cycle is never dropped. Dependencies on code + outside the package are summarised per module under the diagram, not drawn. + Test modules and type-only imports are left out. The json form is + ``{prefix, nodes, subpackages, edges:[{source,target,kind}], dropped, outside}``. + Run ``cgis_ingest`` first. + """ + if not Path(db_path).exists(): + return f"❌ Database not found at: {db_path}. Run cgis_ingest first." + try: + package = effective_prefixes(db_path, [prefix.strip()])[0] + graph = module_graph( + db_path, package, layers, reduce=reduce, exclude_tests=exclude_tests, expand=expand + ) + except Exception as exc: + return f"❌ Error during module-graph: {exc}" + if not graph.nodes: + return f"No modules under '{prefix}'. Check the prefix with cgis_find_symbol." + if output_format == "json": + return json.dumps(_with_freshness(db_path, graph_to_dict(graph)), indent=2) + return render_mermaid(graph) + + @mcp.tool() @refreshes_graph def cgis_validate( diff --git a/src/cgis/api/server_instructions.py b/src/cgis/api/server_instructions.py index 37d44065..544563b0 100644 --- a/src/cgis/api/server_instructions.py +++ b/src/cgis/api/server_instructions.py @@ -65,6 +65,7 @@ def _caveats() -> str: - What calls X / is changing or renaming X safe: cgis_analyze_impact. - What does X end up calling: cgis_trace_flow. - How a module or class is laid out: cgis_get_structure. +- How one package is built inside, module by module: cgis_module_graph. - One symbol with its source and neighbours, before editing it: cgis_context. - Where is the symbol named X: cgis_find_symbol. - Coupling, drift, reachability of a guard: cgis_metrics, cgis_drift, \ diff --git a/src/cgis/cli.py b/src/cgis/cli.py index 49f40c1a..151392a5 100644 --- a/src/cgis/cli.py +++ b/src/cgis/cli.py @@ -52,6 +52,12 @@ from cgis.query.analysis.analyzer import AnalyzerEngine from cgis.query.analysis.anomaly import AnomalyType, ArchitecturalAnomaly from cgis.query.analysis.health import HealthScorer +from cgis.query.analysis.module_graph import ( + DEFAULT_LAYERS, + graph_to_dict, + module_graph, + render_mermaid, +) from cgis.query.analysis.overview import DEFAULT_DEPTH, DEFAULT_LIMIT, build_overview from cgis.query.analysis.suggest_service import SuggestReport, report_to_dict, suggest_packages from cgis.query.context.audit import NoAuditSourcesError, ReachabilityAudit, audit_reachability @@ -2000,6 +2006,59 @@ def suggest_packages_cmd( _render_suggest(report) +@app.command(name="module-graph") +def module_graph_cmd( + prefix: str = typer.Argument( + ..., + help="FQN prefix of the package as the graph names it (e.g. cgis.query).", + ), + db: str = typer.Option(_DEFAULT_DB, "--db", "-d", help=_DEFAULT_DB_HELP), + layer: list[str] = typer.Option( + list(DEFAULT_LAYERS), + "--layer", + "-l", + help="Edge kind to draw, repeatable: imports, extends, calls.", + ), + no_reduce: bool = typer.Option( + False, "--no-reduce", help="Keep edges implied by a longer path." + ), + with_tests: bool = typer.Option(False, "--with-tests", help="Include test modules."), + expand: bool = typer.Option( + False, "--expand", help="Every module below the package, not one node per sub-package." + ), + output_format: str = typer.Option("mermaid", "--format", "-f", help="mermaid or json"), +) -> None: + """Draw how one package is built inside: its modules and their dependencies. + + Imports -->, inheritance ==>, calls -.->; each kind transitively reduced, cycles + kept. Outside dependencies are listed per module under the diagram. Run + `ingest` first. + """ + if output_format not in ("mermaid", "json"): + message = "use mermaid or json" + raise typer.BadParameter(message, param_hint="--format") + if not Path(db).is_file(): + console.print( + f"[bold red]❌ Database not found:[/bold red] {escape(db)}. Run `ingest` first." + ) + raise typer.Exit(code=1) + _warn_if_not_fresh(db) + try: + package = effective_prefixes(db, [prefix.strip()])[0] + graph = module_graph( + db, package, layer, reduce=not no_reduce, exclude_tests=not with_tests, expand=expand + ) + except ValueError as exc: + raise typer.BadParameter(str(exc), param_hint="--layer") from exc + if not graph.nodes: + typer.echo(f"No modules under '{prefix}'.", err=True) + raise typer.Exit(code=1) + if output_format == "json": + typer.echo(_json.dumps(graph_to_dict(graph), indent=2)) + else: + typer.echo(render_mermaid(graph)) + + def _render_fractal(reports: list[FractalReport]) -> None: """Render one table per layer: the ladder, then the fit and verdict.""" for report in reports: diff --git a/src/cgis/query/analysis/module_graph.py b/src/cgis/query/analysis/module_graph.py new file mode 100644 index 00000000..a1437f8c --- /dev/null +++ b/src/cgis/query/analysis/module_graph.py @@ -0,0 +1,271 @@ +"""How one package is built inside: its modules and the dependencies between them (#741). + +Nodes are the package's direct children (each sub-package one node) or, with +``expand``, every file below it. Edges are module-to-module dependencies by +kind: ``imports``, ``extends`` and ``calls``. A symbol-level edge counts for the +file that holds each end. Test files are left out by default, type-only imports +always (as the cycle tools do, #499). Each kind is transitively reduced on its +own, so the picture shows the direct layering rather than every implied edge; +dependencies on code outside the package are summarised per module, not drawn. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from pathlib import Path +from typing import TYPE_CHECKING + +from cgis.core.models import EdgeType, NodeType +from cgis.storage.sqlite_store import SQLiteStore + +if TYPE_CHECKING: + from collections.abc import Iterable, Sequence + + from cgis.core.models import Edge, Node + +#: Edge kinds by name, in the order a reader takes them in. `from pkg import mod` +#: records IMPORTS to `pkg` and IMPORTS_SYMBOL to `pkg.mod`; the second is the +#: module dependency, so imports read both. +KINDS: dict[str, frozenset[EdgeType]] = { + "imports": frozenset({EdgeType.IMPORTS, EdgeType.IMPORTS_SYMBOL}), + "extends": frozenset({EdgeType.EXTENDS}), + "calls": frozenset({EdgeType.CALLS}), +} +#: What a picture shows unless asked otherwise: calls make it much denser. +DEFAULT_LAYERS: tuple[str, ...] = ("imports", "extends") + +_FILE_TYPES = frozenset({NodeType.FILE, NodeType.MODULE}) +_ARROWS = {"imports": "-->", "extends": "==>", "calls": "-.->"} +_NON_ID = re.compile(r"[^0-9A-Za-z]+") + + +@dataclass(frozen=True) +class ModuleEdge: + """One dependency of ``source`` on ``target``, both nodes of the module graph.""" + + source: str + target: str + kind: str + + +@dataclass(frozen=True) +class ModuleGraph: + """A package's module graph. + + ``dropped`` counts, per kind, the edges the transitive reduction removed; + ``outside`` maps a module to the packages outside ``prefix`` it depends on. + """ + + prefix: str + nodes: list[str] + subpackages: list[str] + edges: list[ModuleEdge] + dropped: dict[str, int] + outside: dict[str, list[str]] + + +def _under(fqn: str, prefix: str) -> bool: + return fqn == prefix or fqn.startswith(prefix + ".") + + +def transitive_reduction(edges: Iterable[ModuleEdge]) -> list[ModuleEdge]: + """Drop each edge implied by a longer path, keeping every edge of a cycle. + + ``u -> v`` stays when ``u`` is reachable from ``v`` (the edge is part of a + cycle, and dropping it could make the cycle vanish from the picture) or when + ``v`` is unreachable from ``u`` once the edge itself is removed. Input order + is preserved. + """ + edge_list = list(edges) + succ: dict[str, set[str]] = {} + for e in edge_list: + succ.setdefault(e.source, set()).add(e.target) + + def reaches(start: str, goal: str, without: tuple[str, str] | None = None) -> bool: + stack, seen = [start], {start} + while stack: + node = stack.pop() + for nxt in succ.get(node, ()): + if (node, nxt) == without or nxt in seen: + continue + if nxt == goal: + return True + seen.add(nxt) + stack.append(nxt) + return False + + return [ + e + for e in edge_list + if reaches(e.target, e.source) or not reaches(e.source, e.target, (e.source, e.target)) + ] + + +def _file_index(nodes: Sequence[Node]) -> tuple[dict[str, str], set[str], set[str]]: + """Per node id, the id of the file that holds it; the test files; the package files.""" + by_path = {n.file_path: n.id for n in nodes if n.type in _FILE_TYPES and n.file_path} + holder = {n.id: by_path[n.file_path] for n in nodes if n.file_path in by_path} + tests = {n.id for n in nodes if n.type in _FILE_TYPES and n.is_test} + ids = set(by_path.values()) + # A file is a package when another file's id continues its own. + packages = {f.rsplit(".", 1)[0] for f in ids if "." in f} & ids + return holder, tests, packages + + +def _group(file_id: str, prefix: str, expand: bool) -> str: + """The node a file belongs to: itself, or the package's direct child holding it.""" + if expand or not file_id.startswith(prefix + "."): + return file_id + return f"{prefix}.{file_id[len(prefix) + 1 :].split('.', 1)[0]}" + + +def _outside_package(file_id: str, packages: set[str]) -> str: + """The package a file outside the analysed one belongs to: itself, if it is one.""" + if file_id in packages or "." not in file_id: + return file_id + return file_id.rsplit(".", 1)[0] + + +def build_module_graph( + nodes: Sequence[Node], + edges: Sequence[Edge], + prefix: str, + layers: Sequence[str] = DEFAULT_LAYERS, + *, + reduce: bool = True, + exclude_tests: bool = True, + expand: bool = False, +) -> ModuleGraph: + """The module graph of ``prefix`` from an already-loaded node and edge list.""" + unknown = [layer for layer in layers if layer not in KINDS] + if unknown: + msg = f"unknown layer(s) {unknown}; choose from {list(KINDS)}" + raise ValueError(msg) + holder, tests, packages = _file_index(nodes) + inside = sorted( + f + for f in set(holder.values()) + if _under(f, prefix) and f != prefix and not (exclude_tests and f in tests) + ) + members = sorted({_group(f, prefix, expand) for f in inside}) + subpackages = ( + [] if expand else [m for m in members if any(f.startswith(m + ".") for f in inside)] + ) + member_set = set(members) + wanted = {edge_type: layer for layer in layers for edge_type in KINDS[layer]} + inside_set = set(inside) + found: dict[tuple[str, str, str], None] = {} + outside: dict[str, set[str]] = {} + for e in edges: + kind = wanted.get(e.type) + if kind is None or e.type_only: + continue + src_file, dst_file = holder.get(e.source), holder.get(e.target) + if src_file is None or dst_file is None or src_file not in inside_set: + continue + src = _group(src_file, prefix, expand) + if _under(dst_file, prefix): + if exclude_tests and dst_file in tests: + continue + dst = _group(dst_file, prefix, expand) + if dst in member_set and dst != src: + found[(src, dst, kind)] = None + else: + outside.setdefault(src, set()).add(_outside_package(dst_file, packages)) + all_edges = [ModuleEdge(s, t, k) for s, t, k in found] + kept: list[ModuleEdge] = [] + dropped: dict[str, int] = {} + for layer in layers: + layer_edges = [e for e in all_edges if e.kind == layer] + reduced = transitive_reduction(layer_edges) if reduce else layer_edges + dropped[layer] = len(layer_edges) - len(reduced) + kept.extend(reduced) + return ModuleGraph( + prefix=prefix, + nodes=members, + subpackages=subpackages, + edges=sorted(kept, key=lambda e: (e.kind, e.source, e.target)), + dropped=dropped, + outside={m: sorted(pkgs) for m, pkgs in sorted(outside.items())}, + ) + + +def module_graph( + db_path: str, + prefix: str, + layers: Sequence[str] = DEFAULT_LAYERS, + *, + reduce: bool = True, + exclude_tests: bool = True, + expand: bool = False, +) -> ModuleGraph: + """The module graph of the package at ``prefix`` in the graph at ``db_path``. + + ``prefix`` is matched on whole dot-segments, as given: the surfaces resolve a + prefix that omits the ingest root first (``effective_prefixes``, #584). + + Raises: + FileNotFoundError: if ``db_path`` is not an existing file (run ingest first). + """ + if not Path(db_path).is_file(): + msg = f"Graph database not found: {db_path}" + raise FileNotFoundError(msg) + with SQLiteStore(db_path) as store: + return build_module_graph( + store.get_all_nodes(), + store.get_all_edges(), + prefix.strip(), + layers, + reduce=reduce, + exclude_tests=exclude_tests, + expand=expand, + ) + + +def _short(name: str, prefix: str) -> str: + return name[len(prefix) + 1 :] if name.startswith(prefix + ".") else name + + +def _node_id(name: str, prefix: str) -> str: + return "m_" + (_NON_ID.sub("_", _short(name, prefix)).strip("_") or "root") + + +def render_mermaid(graph: ModuleGraph) -> str: + """The graph as a Mermaid flowchart, with outbound dependencies as a table under it. + + Imports are ``-->``, inheritance ``==>`` and calls ``-.->``. Node ids carry an + ``m_`` prefix, so a module named ``end`` or ``graph`` is not a keyword. + """ + lines = ["```mermaid", "flowchart TD"] + for name in graph.nodes: + label = _short(name, graph.prefix) + if name in graph.subpackages: + lines.append(f' {_node_id(name, graph.prefix)}[["{label}/"]]') + else: + lines.append(f' {_node_id(name, graph.prefix)}["{label}"]') + for e in graph.edges: + src, dst = _node_id(e.source, graph.prefix), _node_id(e.target, graph.prefix) + lines.append(f" {src} {_ARROWS[e.kind]} {dst}") + lines.append("```") + kinds = ", ".join(f"{k} {_ARROWS[k]}" for k in graph.dropped) + removed = ", ".join(f"{k} {n}" for k, n in graph.dropped.items() if n) + lines.append("") + lines.append(f"Edges: {kinds}." + (f" Implied edges dropped: {removed}." if removed else "")) + if graph.outside: + lines += ["", f"| module | reaches outside `{graph.prefix}` |", "|---|---|"] + for module, packages in graph.outside.items(): + lines.append(f"| {_short(module, graph.prefix)} | {', '.join(packages)} |") + return "\n".join(lines) + + +def graph_to_dict(graph: ModuleGraph) -> dict[str, object]: + """The graph as plain data: ``{prefix, nodes, subpackages, edges, dropped, outside}``.""" + return { + "prefix": graph.prefix, + "nodes": graph.nodes, + "subpackages": graph.subpackages, + "edges": [{"source": e.source, "target": e.target, "kind": e.kind} for e in graph.edges], + "dropped": graph.dropped, + "outside": graph.outside, + } diff --git a/tests/unit/test_mcp_server_instructions.py b/tests/unit/test_mcp_server_instructions.py index 6e6cd9d9..164f6a5e 100644 --- a/tests/unit/test_mcp_server_instructions.py +++ b/tests/unit/test_mcp_server_instructions.py @@ -80,8 +80,9 @@ def test_the_instructions_name_only_tools_that_exist() -> None: def test_every_tool_is_listed_without_an_allowlist() -> None: listed = _listed(mcp) - assert len(listed) == 16 + assert len(listed) == 17 assert "cgis_diff_check" in listed + assert "cgis_module_graph" in listed @pytest.mark.parametrize("raw", [None, "", " , "]) diff --git a/tests/unit/test_module_graph.py b/tests/unit/test_module_graph.py new file mode 100644 index 00000000..dc240229 --- /dev/null +++ b/tests/unit/test_module_graph.py @@ -0,0 +1,244 @@ +"""Module graph of one package (#741): modules as nodes, imports/extends/calls as edges.""" + +import json +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from cgis.api.mcp_server import cgis_module_graph +from cgis.cli import app +from cgis.languages import extractors +from cgis.pipeline import IngestionPipeline +from cgis.query.analysis.module_graph import ( + ModuleEdge, + ModuleGraph, + module_graph, + render_mermaid, + transitive_reduction, +) +from cgis.storage.sqlite_store import SQLiteStore + +PACKAGE = { + "pkg/__init__.py": "", + "pkg/effects.py": "class Effects:\n pass\n", + "pkg/core.py": ( + "from pkg.effects import Effects\n\n\n" + "def helper():\n return 1\n\n\n" + "class Core(Effects):\n def run(self):\n return helper()\n" + ), + # creation -> effects is implied by creation -> core -> effects + "pkg/creation.py": ( + "from pkg.core import Core\nfrom pkg.effects import Effects\n\n\n" + "class Creation(Core):\n def make(self):\n return Effects()\n" + ), + # service -> core is implied by service -> creation -> core + "pkg/service.py": ( + "from pkg.creation import Creation\nfrom pkg.core import Core\n" + "from other.out import ext\n\n\n" + "class Service(Creation):\n def go(self):\n return ext(), Core()\n" + ), + "pkg/cyc1.py": "from pkg import cyc2\n\n\ndef one():\n return cyc2.two()\n", + "pkg/cyc2.py": "from pkg import cyc1\n\n\ndef two():\n return cyc1.one()\n", + "pkg/sub/__init__.py": "", + "pkg/sub/a.py": "from pkg.core import Core\n\n\nclass A(Core):\n pass\n", + "pkg/sub/b.py": "from pkg.sub.a import A\n\n\ndef b():\n return A()\n", + "pkg/tests/__init__.py": "", + "pkg/tests/test_core.py": ( + "from pkg.core import Core\n\n\ndef test_core():\n assert Core()\n" + ), + "other/__init__.py": "", + "other/out.py": "def ext():\n return 2\n", + "lib/__init__.py": "", + "lib/deep/__init__.py": "def util():\n return 3\n", + "lib/deep/mod.py": "X = 1\n", + "pkg/uses_lib.py": "from lib.deep import util\n\n\ndef f():\n return util()\n", +} + + +@pytest.fixture(scope="module") +def db(tmp_path_factory: pytest.TempPathFactory) -> str: + """A graph of PACKAGE, ingested the way the CLI does.""" + root = tmp_path_factory.mktemp("repo") + for rel, text in PACKAGE.items(): + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + db_path = str(root.parent / "g.db") + store = SQLiteStore(db_path) + store.connect() + with store: + IngestionPipeline(extractors([])).run(str(root), store=store, rebuild=True) + return db_path + + +def _pairs(graph: ModuleGraph, kind: str) -> set[tuple[str, str]]: + return {(e.source, e.target) for e in graph.edges if e.kind == kind} + + +def test_nodes_are_the_packages_children_with_subpackages_collapsed(db: str) -> None: + """Each module is a node, `sub/` is one node, and the test package is left out.""" + graph = module_graph(db, "pkg") + assert set(graph.nodes) == { + "pkg.core", + "pkg.creation", + "pkg.cyc1", + "pkg.cyc2", + "pkg.effects", + "pkg.service", + "pkg.sub", + "pkg.uses_lib", + } + assert graph.subpackages == ["pkg.sub"] + + +def test_imports_are_reduced_to_the_direct_layering(db: str) -> None: + """An import implied by a longer path is dropped; the layering stays.""" + graph = module_graph(db, "pkg", layers=("imports",)) + assert _pairs(graph, "imports") == { + ("pkg.core", "pkg.effects"), + ("pkg.creation", "pkg.core"), + ("pkg.service", "pkg.creation"), + ("pkg.sub", "pkg.core"), + ("pkg.cyc1", "pkg.cyc2"), + ("pkg.cyc2", "pkg.cyc1"), + } + assert graph.dropped == {"imports": 2} + + +def test_without_reduction_every_import_is_kept(db: str) -> None: + """`reduce=False` keeps the implied edges too.""" + graph = module_graph(db, "pkg", layers=("imports",), reduce=False) + assert {("pkg.creation", "pkg.effects"), ("pkg.service", "pkg.core")} <= _pairs( + graph, "imports" + ) + assert graph.dropped == {"imports": 0} + + +def test_inheritance_is_its_own_layer(db: str) -> None: + """Extends edges show Effects <- Core <- Creation <- Service, and sub's A <- Core.""" + graph = module_graph(db, "pkg", layers=("extends",)) + assert _pairs(graph, "extends") == { + ("pkg.core", "pkg.effects"), + ("pkg.creation", "pkg.core"), + ("pkg.service", "pkg.creation"), + ("pkg.sub", "pkg.core"), + } + + +def test_calls_are_available_and_off_by_default(db: str) -> None: + """The default layers are imports and extends; calls must be asked for.""" + assert {e.kind for e in module_graph(db, "pkg").edges} == {"imports", "extends"} + calls = _pairs(module_graph(db, "pkg", layers=("calls",)), "calls") + assert ("pkg.cyc1", "pkg.cyc2") in calls + + +def test_test_modules_are_left_out_unless_asked(db: str) -> None: + """Production only by default; `exclude_tests=False` brings the test package back.""" + assert "pkg.tests" not in module_graph(db, "pkg").nodes + with_tests = module_graph(db, "pkg", exclude_tests=False) + assert ("pkg.tests", "pkg.core") in _pairs(with_tests, "imports") + + +def test_expand_shows_the_subpackages_modules(db: str) -> None: + """`expand` lists the files below the package instead of one node per sub-package.""" + graph = module_graph(db, "pkg", layers=("imports",), expand=True) + assert {"pkg.sub.a", "pkg.sub.b"} <= set(graph.nodes) + assert ("pkg.sub.b", "pkg.sub.a") in _pairs(graph, "imports") + assert graph.subpackages == [] + + +def test_outbound_dependencies_are_summarised_by_package(db: str) -> None: + """A dependency outside the package is listed per module, not drawn.""" + graph = module_graph(db, "pkg") + assert graph.outside == {"pkg.service": ["other"], "pkg.uses_lib": ["lib.deep"]} + + +def test_an_unknown_prefix_gives_an_empty_graph(db: str) -> None: + """No module under the prefix: an empty graph, not an error.""" + graph = module_graph(db, "nope") + assert graph.nodes == [] + assert graph.edges == [] + + +def test_a_missing_database_is_an_error(tmp_path: Path) -> None: + """Run ingest first; say so.""" + missing = str(tmp_path / "missing.db") + with pytest.raises(FileNotFoundError): + module_graph(missing, "pkg") + + +def test_reduction_never_drops_an_edge_of_a_cycle() -> None: + """An edge that closes a cycle stays, or the cycle would vanish from the picture.""" + edges = [ + ModuleEdge("a", "b", "imports"), + ModuleEdge("b", "c", "imports"), + ModuleEdge("c", "a", "imports"), + ModuleEdge("a", "c", "imports"), + ModuleEdge("c", "d", "imports"), + ModuleEdge("d", "e", "imports"), + ModuleEdge("a", "e", "imports"), + ] + kept = {(e.source, e.target) for e in transitive_reduction(edges)} + # inside the cycle {a, b, c} nothing is dropped, not even the shortcut a -> c; + # a -> e is implied by the cycle's way out, c -> d -> e + assert kept == {("a", "b"), ("b", "c"), ("c", "a"), ("a", "c"), ("c", "d"), ("d", "e")} + + +def test_mermaid_styles_kinds_and_guards_keywords() -> None: + """Imports -->, extends ==>, calls -.->; ids are prefixed, so `end` is safe.""" + graph = ModuleGraph( + prefix="p", + nodes=["p.end", "p.core", "p.sub"], + subpackages=["p.sub"], + edges=[ + ModuleEdge("p.end", "p.core", "imports"), + ModuleEdge("p.end", "p.core", "extends"), + ModuleEdge("p.sub", "p.core", "calls"), + ], + dropped={"imports": 0}, + outside={"p.end": ["q", "r"]}, + ) + text = render_mermaid(graph) + assert "m_end --> m_core" in text + assert "m_end ==> m_core" in text + assert "m_sub -.-> m_core" in text + assert 'm_end["end"]' in text + assert 'm_sub[["sub/"]]' in text + assert "| end | q, r |" in text + + +# --- surfaces: the CLI command and the MCP tool use the same service --- + + +def test_cli_prints_a_mermaid_diagram(db: str) -> None: + """`cgis module-graph pkg` draws the reduced picture.""" + result = CliRunner().invoke(app, ["module-graph", "pkg", "--db", db]) + assert result.exit_code == 0, result.output + assert "flowchart TD" in result.stdout + assert "m_creation --> m_core" in result.stdout + assert "m_creation --> m_effects" not in result.stdout + + +def test_cli_json_with_calls_and_an_unknown_layer(db: str) -> None: + """--format json is data; an unknown --layer is a usage error, not a traceback.""" + runner = CliRunner() + ok = runner.invoke(app, ["module-graph", "pkg", "--db", db, "-l", "calls", "-f", "json"]) + assert ok.exit_code == 0, ok.output + payload = json.loads(ok.stdout) + assert {e["kind"] for e in payload["edges"]} == {"calls"} + bad = runner.invoke(app, ["module-graph", "pkg", "--db", db, "-l", "vibes"]) + assert bad.exit_code == 2 + + +def test_cli_unknown_prefix_exits_1(db: str) -> None: + """Nothing under the prefix is a failure the caller can see.""" + assert CliRunner().invoke(app, ["module-graph", "nope", "--db", db]).exit_code == 1 + + +def test_mcp_tool_answers_in_both_formats(db: str) -> None: + """The MCP tool returns the diagram by default and JSON on request.""" + assert "flowchart TD" in cgis_module_graph(prefix="pkg", db_path=db) + payload = json.loads(cgis_module_graph(prefix="pkg", db_path=db, output_format="json")) + assert ("pkg.core", "pkg.effects") in {(e["source"], e["target"]) for e in payload["edges"]} + assert "No modules under" in cgis_module_graph(prefix="nope", db_path=db) From 6da7d75446921a96039904ca6d48aa4fb482bc17 Mon Sep 17 00:00:00 2001 From: zaebee Date: Fri, 23 Oct 2026 07:29:47 +0000 Subject: [PATCH 2/4] fix(module-graph): reduce on the condensation, so a cycle's only way in stays Review of #743: - Blocker: the edge-by-edge rule let parallel edges into one cycle justify each other, so x -> a1, x -> a2 with a1 <-> a2 lost both, and two cycles joined twice looked unrelated. The reduction now works on the condensation: every edge inside a component stays, and the edges joining two components go together, only when a third component carries the path. - With --expand, `from pkg.sub import mod` no longer draws an edge to the empty pkg/sub/__init__ as well as to pkg.sub.mod. - The MCP diagram carries the freshness note the other diagram tools give. - A layer named twice is one layer; no layer is an error. Test files are left out of the outside table too. The Go behaviour of --expand (files, not packages) is stated in the module docstring. - build_module_graph is split into helpers (C901). - .github/test-count-baseline raised to 4433, as the floor asked. Each fix has a test that failed first. query's self-drift is 0.231 (tolerance 0.24, unchanged). Co-Authored-By: Claude Opus 5.5 --- .github/test-count-baseline | 2 +- src/cgis/api/mcp_server.py | 2 +- src/cgis/cli.py | 2 +- src/cgis/query/analysis/module_graph.py | 186 +++++++++++++++++------- tests/unit/test_module_graph.py | 77 ++++++++++ 5 files changed, 216 insertions(+), 53 deletions(-) diff --git a/.github/test-count-baseline b/.github/test-count-baseline index b39654e8..be76654b 100644 --- a/.github/test-count-baseline +++ b/.github/test-count-baseline @@ -1 +1 @@ -4264 +4433 diff --git a/src/cgis/api/mcp_server.py b/src/cgis/api/mcp_server.py index 901e4806..a61142a5 100644 --- a/src/cgis/api/mcp_server.py +++ b/src/cgis/api/mcp_server.py @@ -974,7 +974,7 @@ def cgis_module_graph( return f"No modules under '{prefix}'. Check the prefix with cgis_find_symbol." if output_format == "json": return json.dumps(_with_freshness(db_path, graph_to_dict(graph)), indent=2) - return render_mermaid(graph) + return _freshness_note(db_path) + render_mermaid(graph) @mcp.tool() diff --git a/src/cgis/cli.py b/src/cgis/cli.py index 151392a5..25a7ab7a 100644 --- a/src/cgis/cli.py +++ b/src/cgis/cli.py @@ -2048,7 +2048,7 @@ def module_graph_cmd( graph = module_graph( db, package, layer, reduce=not no_reduce, exclude_tests=not with_tests, expand=expand ) - except ValueError as exc: + except ValueError as exc: # an unknown or missing --layer, the only ValueError raised raise typer.BadParameter(str(exc), param_hint="--layer") from exc if not graph.nodes: typer.echo(f"No modules under '{prefix}'.", err=True) diff --git a/src/cgis/query/analysis/module_graph.py b/src/cgis/query/analysis/module_graph.py index a1437f8c..1cb37cc8 100644 --- a/src/cgis/query/analysis/module_graph.py +++ b/src/cgis/query/analysis/module_graph.py @@ -4,7 +4,10 @@ ``expand``, every file below it. Edges are module-to-module dependencies by kind: ``imports``, ``extends`` and ``calls``. A symbol-level edge counts for the file that holds each end. Test files are left out by default, type-only imports -always (as the cycle tools do, #499). Each kind is transitively reduced on its +always (as the cycle tools do, #499). In Go a FILE node is one file of a +package and an import names one of the package's files, so ``expand`` there +shows files, not packages; the collapsed view and the outside table do not +depend on which file. Each kind is transitively reduced on its own, so the picture shows the direct layering rather than every implied edge; dependencies on code outside the package are summarised per module, not drawn. """ @@ -69,36 +72,57 @@ def _under(fqn: str, prefix: str) -> bool: return fqn == prefix or fqn.startswith(prefix + ".") +def _reach(succ: dict[str, set[str]], start: str) -> set[str]: + """Every node reachable from ``start`` by one step or more.""" + stack, seen = list(succ.get(start, ())), set(succ.get(start, ())) + while stack: + for nxt in succ.get(stack.pop(), ()): + if nxt not in seen: + seen.add(nxt) + stack.append(nxt) + return seen + + def transitive_reduction(edges: Iterable[ModuleEdge]) -> list[ModuleEdge]: """Drop each edge implied by a longer path, keeping every edge of a cycle. - ``u -> v`` stays when ``u`` is reachable from ``v`` (the edge is part of a - cycle, and dropping it could make the cycle vanish from the picture) or when - ``v`` is unreachable from ``u`` once the edge itself is removed. Input order - is preserved. + Reduced on the condensation, each strongly connected component one node. + Every edge inside a component stays: dropping one could make a cycle + vanish from the picture. The edges joining two components, however many + module pairs carry them, go together or stay together: they go only when + the target component is reachable from the source component through a + third. Input order is preserved. """ edge_list = list(edges) succ: dict[str, set[str]] = {} for e in edge_list: succ.setdefault(e.source, set()).add(e.target) + succ.setdefault(e.target, set()) + reach = {n: _reach(succ, n) for n in succ} + comp = {n: frozenset({n} | {m for m in reach[n] if n in reach[m]}) for n in succ} + cdag: dict[frozenset[str], set[frozenset[str]]] = {} + for e in edge_list: + if comp[e.source] != comp[e.target]: + cdag.setdefault(comp[e.source], set()).add(comp[e.target]) - def reaches(start: str, goal: str, without: tuple[str, str] | None = None) -> bool: - stack, seen = [start], {start} + def implied(a: frozenset[str], b: frozenset[str]) -> bool: + """Is component ``b`` reachable from ``a`` through some third component.""" + stack = [c for c in cdag.get(a, ()) if c != b] + seen = set(stack) while stack: - node = stack.pop() - for nxt in succ.get(node, ()): - if (node, nxt) == without or nxt in seen: - continue - if nxt == goal: + current = stack.pop() + for nxt in cdag.get(current, ()): + if nxt == b: return True - seen.add(nxt) - stack.append(nxt) + if nxt not in seen: + seen.add(nxt) + stack.append(nxt) return False return [ e for e in edge_list - if reaches(e.target, e.source) or not reaches(e.source, e.target, (e.source, e.target)) + if comp[e.source] == comp[e.target] or not implied(comp[e.source], comp[e.target]) ] @@ -127,6 +151,76 @@ def _outside_package(file_id: str, packages: set[str]) -> str: return file_id.rsplit(".", 1)[0] +def _checked_layers(layers: Sequence[str]) -> list[str]: + """The layers asked for, each once, in order; an empty or unknown one is an error.""" + unique = list(dict.fromkeys(layers)) + if not unique: + msg = f"no layer given; choose from {list(KINDS)}" + raise ValueError(msg) + unknown = [layer for layer in unique if layer not in KINDS] + if unknown: + msg = f"unknown layer(s) {unknown}; choose from {list(KINDS)}" + raise ValueError(msg) + return unique + + +def _imported_through(edges: Sequence[Edge], holder: dict[str, str]) -> dict[str, set[str]]: + """Per file, the files whose names it imports (IMPORTS_SYMBOL), for the expand view. + + `from pkg.sub import mod` records IMPORTS to pkg.sub as well as IMPORTS_SYMBOL + to pkg.sub.mod. Drawn per module, the first would put an edge on every + package __init__ its submodules are imported through. + """ + through: dict[str, set[str]] = {} + for e in edges: + if e.type == EdgeType.IMPORTS_SYMBOL: + src_file, dst_file = holder.get(e.source), holder.get(e.target) + if src_file is not None and dst_file is not None: + through.setdefault(src_file, set()).add(dst_file) + return through + + +@dataclass(frozen=True) +class _Scope: + """What decides whether an edge belongs to the picture, and where it lands.""" + + prefix: str + expand: bool + holder: dict[str, str] + skipped: set[str] + packages: set[str] + inside: set[str] + members: set[str] + through: dict[str, set[str]] + + +def _ends(e: Edge, scope: _Scope) -> tuple[str, str] | None: + """The source member and the target file of an edge the picture uses, or None.""" + src_file, dst_file = scope.holder.get(e.source), scope.holder.get(e.target) + if src_file not in scope.inside or dst_file is None or dst_file in scope.skipped: + return None + if e.type == EdgeType.IMPORTS and any( + f.startswith(dst_file + ".") for f in scope.through.get(src_file, ()) + ): + return None + return _group(src_file, scope.prefix, scope.expand), dst_file + + +def _reduced( + found: Iterable[ModuleEdge], layers: list[str], reduce: bool +) -> tuple[list[ModuleEdge], dict[str, int]]: + """Each layer reduced on its own, and how many edges each lost.""" + all_edges = list(found) + kept: list[ModuleEdge] = [] + dropped: dict[str, int] = {} + for layer in layers: + layer_edges = [e for e in all_edges if e.kind == layer] + reduced = transitive_reduction(layer_edges) if reduce else layer_edges + dropped[layer] = len(layer_edges) - len(reduced) + kept.extend(reduced) + return sorted(kept, key=lambda e: (e.kind, e.source, e.target)), dropped + + def build_module_graph( nodes: Sequence[Node], edges: Sequence[Edge], @@ -138,54 +232,46 @@ def build_module_graph( expand: bool = False, ) -> ModuleGraph: """The module graph of ``prefix`` from an already-loaded node and edge list.""" - unknown = [layer for layer in layers if layer not in KINDS] - if unknown: - msg = f"unknown layer(s) {unknown}; choose from {list(KINDS)}" - raise ValueError(msg) + chosen = _checked_layers(layers) holder, tests, packages = _file_index(nodes) + skipped = tests if exclude_tests else set() inside = sorted( - f - for f in set(holder.values()) - if _under(f, prefix) and f != prefix and not (exclude_tests and f in tests) + f for f in set(holder.values()) if _under(f, prefix) and f != prefix and f not in skipped ) members = sorted({_group(f, prefix, expand) for f in inside}) - subpackages = ( - [] if expand else [m for m in members if any(f.startswith(m + ".") for f in inside)] + scope = _Scope( + prefix=prefix, + expand=expand, + holder=holder, + skipped=skipped, + packages=packages, + inside=set(inside), + members=set(members), + through=_imported_through(edges, holder) if expand else {}, ) - member_set = set(members) - wanted = {edge_type: layer for layer in layers for edge_type in KINDS[layer]} - inside_set = set(inside) - found: dict[tuple[str, str, str], None] = {} + wanted = {edge_type: layer for layer in chosen for edge_type in KINDS[layer]} + found: dict[ModuleEdge, None] = {} outside: dict[str, set[str]] = {} for e in edges: kind = wanted.get(e.type) - if kind is None or e.type_only: - continue - src_file, dst_file = holder.get(e.source), holder.get(e.target) - if src_file is None or dst_file is None or src_file not in inside_set: + ends = None if kind is None or e.type_only else _ends(e, scope) + if kind is None or ends is None: continue - src = _group(src_file, prefix, expand) - if _under(dst_file, prefix): - if exclude_tests and dst_file in tests: - continue - dst = _group(dst_file, prefix, expand) - if dst in member_set and dst != src: - found[(src, dst, kind)] = None - else: + src, dst_file = ends + if not _under(dst_file, prefix): outside.setdefault(src, set()).add(_outside_package(dst_file, packages)) - all_edges = [ModuleEdge(s, t, k) for s, t, k in found] - kept: list[ModuleEdge] = [] - dropped: dict[str, int] = {} - for layer in layers: - layer_edges = [e for e in all_edges if e.kind == layer] - reduced = transitive_reduction(layer_edges) if reduce else layer_edges - dropped[layer] = len(layer_edges) - len(reduced) - kept.extend(reduced) + continue + dst = _group(dst_file, prefix, expand) + if dst in scope.members and dst != src: + found[ModuleEdge(src, dst, kind)] = None + kept, dropped = _reduced(found, chosen, reduce) return ModuleGraph( prefix=prefix, nodes=members, - subpackages=subpackages, - edges=sorted(kept, key=lambda e: (e.kind, e.source, e.target)), + subpackages=[] + if expand + else [m for m in members if any(f.startswith(m + ".") for f in inside)], + edges=kept, dropped=dropped, outside={m: sorted(pkgs) for m, pkgs in sorted(outside.items())}, ) diff --git a/tests/unit/test_module_graph.py b/tests/unit/test_module_graph.py index dc240229..c3de10eb 100644 --- a/tests/unit/test_module_graph.py +++ b/tests/unit/test_module_graph.py @@ -6,6 +6,7 @@ import pytest from typer.testing import CliRunner +from cgis.api import mcp_server from cgis.api.mcp_server import cgis_module_graph from cgis.cli import app from cgis.languages import extractors @@ -242,3 +243,79 @@ def test_mcp_tool_answers_in_both_formats(db: str) -> None: payload = json.loads(cgis_module_graph(prefix="pkg", db_path=db, output_format="json")) assert ("pkg.core", "pkg.effects") in {(e["source"], e["target"]) for e in payload["edges"]} assert "No modules under" in cgis_module_graph(prefix="nope", db_path=db) + + +def test_reduction_keeps_the_only_way_into_a_cycle() -> None: + """Two parallel edges into one cycle are not each other's justification.""" + edges = [ + ModuleEdge("x", "a1", "imports"), + ModuleEdge("x", "a2", "imports"), + ModuleEdge("a1", "a2", "imports"), + ModuleEdge("a2", "a1", "imports"), + ] + kept = {(e.source, e.target) for e in transitive_reduction(edges)} + assert kept & {("x", "a1"), ("x", "a2")} + + +def test_reduction_keeps_the_link_between_two_cycles() -> None: + """Two cycles joined by two edges stay joined.""" + edges = [ + ModuleEdge("a1", "a2", "imports"), + ModuleEdge("a2", "a1", "imports"), + ModuleEdge("b1", "b2", "imports"), + ModuleEdge("b2", "b1", "imports"), + ModuleEdge("a1", "b1", "imports"), + ModuleEdge("a2", "b2", "imports"), + ] + kept = {(e.source, e.target) for e in transitive_reduction(edges)} + assert kept & {("a1", "b1"), ("a2", "b2")} + + +def test_reduction_drops_a_cycle_shortcut_implied_through_another_component() -> None: + """x -> c is implied by x -> {a1, a2} -> c, however the cycle is entered.""" + edges = [ + ModuleEdge("x", "a1", "imports"), + ModuleEdge("a1", "a2", "imports"), + ModuleEdge("a2", "a1", "imports"), + ModuleEdge("a2", "c", "imports"), + ModuleEdge("x", "c", "imports"), + ] + kept = {(e.source, e.target) for e in transitive_reduction(edges)} + assert ("x", "c") not in kept + assert {("x", "a1"), ("a2", "c")} <= kept + + +def test_repeated_or_empty_layers(db: str) -> None: + """A layer named twice is one layer; no layer at all is an error.""" + twice = module_graph(db, "pkg", layers=("imports", "imports")) + once = module_graph(db, "pkg", layers=("imports",)) + assert twice.edges == once.edges + with pytest.raises(ValueError, match="layer"): + module_graph(db, "pkg", layers=()) + + +def test_expand_draws_no_edge_to_an_init_only_imported_through(tmp_path: Path) -> None: + """`from pkg.sub import a` depends on pkg.sub.a, not on the empty pkg/sub/__init__.""" + root = tmp_path / "repo" + for rel, text in { + "pkg/__init__.py": "", + "pkg/b.py": "from pkg.sub import a\n\n\ndef f():\n return a.X\n", + "pkg/sub/__init__.py": "", + "pkg/sub/a.py": "X = 1\n", + }.items(): + (root / rel).parent.mkdir(parents=True, exist_ok=True) + (root / rel).write_text(text) + db_path = str(tmp_path / "g.db") + store = SQLiteStore(db_path) + store.connect() + with store: + IngestionPipeline(extractors([])).run(str(root), store=store, rebuild=True) + pairs = _pairs(module_graph(db_path, "pkg", layers=("imports",), expand=True), "imports") + assert ("pkg.b", "pkg.sub.a") in pairs + assert ("pkg.b", "pkg.sub") not in pairs + + +def test_mcp_mermaid_carries_the_freshness_note(db: str, monkeypatch: pytest.MonkeyPatch) -> None: + """A diagram from a stale graph says so above it, as the other diagram tools do.""" + monkeypatch.setattr(mcp_server, "_freshness_note", lambda _db: "STALE NOTE\n") + assert cgis_module_graph(prefix="pkg", db_path=db).startswith("STALE NOTE\n```mermaid") From 8982011edff9991fee133c4a40425507405de6d0 Mon Sep 17 00:00:00 2001 From: zaebee Date: Fri, 23 Oct 2026 07:35:46 +0000 Subject: [PATCH 3/4] fix(module-graph): immutable layer defaults, a blank prefix refused, one pass over nodes From Gemini's review of #743: the MCP and CLI layer defaults are tuples (the MCP schema is unchanged: an array defaulting to imports, extends); a blank prefix is refused by both surfaces with a usage message, with a test; the file index is built in one pass over the nodes. Co-Authored-By: Claude Opus 5.5 --- src/cgis/api/mcp_server.py | 6 ++++-- src/cgis/cli.py | 5 ++++- src/cgis/query/analysis/module_graph.py | 10 ++++++++-- tests/unit/test_module_graph.py | 8 ++++++++ 4 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/cgis/api/mcp_server.py b/src/cgis/api/mcp_server.py index a61142a5..d10f20b0 100644 --- a/src/cgis/api/mcp_server.py +++ b/src/cgis/api/mcp_server.py @@ -933,9 +933,9 @@ def cgis_module_graph( ], db_path: DbPath = _DEFAULT_DB, layers: Annotated[ - list[Literal["imports", "extends", "calls"]], + tuple[Literal["imports", "extends", "calls"], ...], Field(description="Edge kinds to draw. Calls make the picture much denser."), - ] = ["imports", "extends"], # noqa: B006 - a JSON-schema default, never mutated + ] = ("imports", "extends"), reduce: Annotated[ bool, Field(description="Drop each edge implied by a longer path, per kind; cycles stay."), @@ -961,6 +961,8 @@ def cgis_module_graph( ``{prefix, nodes, subpackages, edges:[{source,target,kind}], dropped, outside}``. Run ``cgis_ingest`` first. """ + if not prefix.strip(): + return "❌ Give the package's prefix, e.g. cgis.query." if not Path(db_path).exists(): return f"❌ Database not found at: {db_path}. Run cgis_ingest first." try: diff --git a/src/cgis/cli.py b/src/cgis/cli.py index 25a7ab7a..0e8d3320 100644 --- a/src/cgis/cli.py +++ b/src/cgis/cli.py @@ -2014,7 +2014,7 @@ def module_graph_cmd( ), db: str = typer.Option(_DEFAULT_DB, "--db", "-d", help=_DEFAULT_DB_HELP), layer: list[str] = typer.Option( - list(DEFAULT_LAYERS), + DEFAULT_LAYERS, "--layer", "-l", help="Edge kind to draw, repeatable: imports, extends, calls.", @@ -2037,6 +2037,9 @@ def module_graph_cmd( if output_format not in ("mermaid", "json"): message = "use mermaid or json" raise typer.BadParameter(message, param_hint="--format") + if not prefix.strip(): + console.print("[bold red]❌ Give the package's prefix, e.g. cgis.query.[/bold red]") + raise typer.Exit(code=1) if not Path(db).is_file(): console.print( f"[bold red]❌ Database not found:[/bold red] {escape(db)}. Run `ingest` first." diff --git a/src/cgis/query/analysis/module_graph.py b/src/cgis/query/analysis/module_graph.py index 1cb37cc8..4214860e 100644 --- a/src/cgis/query/analysis/module_graph.py +++ b/src/cgis/query/analysis/module_graph.py @@ -128,9 +128,15 @@ def implied(a: frozenset[str], b: frozenset[str]) -> bool: def _file_index(nodes: Sequence[Node]) -> tuple[dict[str, str], set[str], set[str]]: """Per node id, the id of the file that holds it; the test files; the package files.""" - by_path = {n.file_path: n.id for n in nodes if n.type in _FILE_TYPES and n.file_path} + by_path: dict[str, str] = {} + tests: set[str] = set() + for n in nodes: + if n.type in _FILE_TYPES: + if n.file_path: + by_path[n.file_path] = n.id + if n.is_test: + tests.add(n.id) holder = {n.id: by_path[n.file_path] for n in nodes if n.file_path in by_path} - tests = {n.id for n in nodes if n.type in _FILE_TYPES and n.is_test} ids = set(by_path.values()) # A file is a package when another file's id continues its own. packages = {f.rsplit(".", 1)[0] for f in ids if "." in f} & ids diff --git a/tests/unit/test_module_graph.py b/tests/unit/test_module_graph.py index c3de10eb..859f2ede 100644 --- a/tests/unit/test_module_graph.py +++ b/tests/unit/test_module_graph.py @@ -319,3 +319,11 @@ def test_mcp_mermaid_carries_the_freshness_note(db: str, monkeypatch: pytest.Mon """A diagram from a stale graph says so above it, as the other diagram tools do.""" monkeypatch.setattr(mcp_server, "_freshness_note", lambda _db: "STALE NOTE\n") assert cgis_module_graph(prefix="pkg", db_path=db).startswith("STALE NOTE\n```mermaid") + + +def test_a_blank_prefix_is_refused_by_both_surfaces(db: str) -> None: + """A blank prefix is a usage error, not an empty picture.""" + assert cgis_module_graph(prefix=" ", db_path=db).startswith("❌") + result = CliRunner().invoke(app, ["module-graph", " ", "--db", db]) + assert result.exit_code == 1 + assert "prefix" in result.output.lower() From da0efd37efc90b17587ec98fca3e4ef66221b394 Mon Sep 17 00:00:00 2001 From: zaebee Date: Fri, 23 Oct 2026 07:49:38 +0000 Subject: [PATCH 4/4] fix(module-graph): unique Mermaid ids, and the CLI checks layers before the graph From guardian's review of #743 (round 2): - Folding a name to [A-Za-z0-9_] is not injective: pkg.a.b and pkg.a_b both gave m_a_b, and Mermaid merged them. Ids are now assigned once per render, a collision getting _2, _3... in sorted order. - The CLI validates --layer (checked_layers, now public) before it opens the graph, so its ValueError handler no longer wraps unrelated code. Co-Authored-By: Claude Opus 5.5 --- src/cgis/cli.py | 16 ++++++----- src/cgis/query/analysis/module_graph.py | 36 +++++++++++++++++-------- tests/unit/test_module_graph.py | 25 +++++++++++++++++ 3 files changed, 59 insertions(+), 18 deletions(-) diff --git a/src/cgis/cli.py b/src/cgis/cli.py index 0e8d3320..2057ea1c 100644 --- a/src/cgis/cli.py +++ b/src/cgis/cli.py @@ -54,6 +54,7 @@ from cgis.query.analysis.health import HealthScorer from cgis.query.analysis.module_graph import ( DEFAULT_LAYERS, + checked_layers, graph_to_dict, module_graph, render_mermaid, @@ -2040,19 +2041,20 @@ def module_graph_cmd( if not prefix.strip(): console.print("[bold red]❌ Give the package's prefix, e.g. cgis.query.[/bold red]") raise typer.Exit(code=1) + try: + layers = checked_layers(layer) + except ValueError as exc: + raise typer.BadParameter(str(exc), param_hint="--layer") from exc if not Path(db).is_file(): console.print( f"[bold red]❌ Database not found:[/bold red] {escape(db)}. Run `ingest` first." ) raise typer.Exit(code=1) _warn_if_not_fresh(db) - try: - package = effective_prefixes(db, [prefix.strip()])[0] - graph = module_graph( - db, package, layer, reduce=not no_reduce, exclude_tests=not with_tests, expand=expand - ) - except ValueError as exc: # an unknown or missing --layer, the only ValueError raised - raise typer.BadParameter(str(exc), param_hint="--layer") from exc + package = effective_prefixes(db, [prefix.strip()])[0] + graph = module_graph( + db, package, layers, reduce=not no_reduce, exclude_tests=not with_tests, expand=expand + ) if not graph.nodes: typer.echo(f"No modules under '{prefix}'.", err=True) raise typer.Exit(code=1) diff --git a/src/cgis/query/analysis/module_graph.py b/src/cgis/query/analysis/module_graph.py index 4214860e..1f817116 100644 --- a/src/cgis/query/analysis/module_graph.py +++ b/src/cgis/query/analysis/module_graph.py @@ -157,7 +157,7 @@ def _outside_package(file_id: str, packages: set[str]) -> str: return file_id.rsplit(".", 1)[0] -def _checked_layers(layers: Sequence[str]) -> list[str]: +def checked_layers(layers: Sequence[str]) -> list[str]: """The layers asked for, each once, in order; an empty or unknown one is an error.""" unique = list(dict.fromkeys(layers)) if not unique: @@ -238,7 +238,7 @@ def build_module_graph( expand: bool = False, ) -> ModuleGraph: """The module graph of ``prefix`` from an already-loaded node and edge list.""" - chosen = _checked_layers(layers) + chosen = checked_layers(layers) holder, tests, packages = _file_index(nodes) skipped = tests if exclude_tests else set() inside = sorted( @@ -319,8 +319,22 @@ def _short(name: str, prefix: str) -> str: return name[len(prefix) + 1 :] if name.startswith(prefix + ".") else name -def _node_id(name: str, prefix: str) -> str: - return "m_" + (_NON_ID.sub("_", _short(name, prefix)).strip("_") or "root") +def _node_ids(names: Iterable[str], prefix: str) -> dict[str, str]: + """A Mermaid id per name, ``m_`` + the name folded to ``[A-Za-z0-9_]``, made unique. + + Folding is not injective (``a.b`` and ``a_b`` both give ``a_b``); a later + name that collides gets ``_2``, ``_3``…, in sorted order, so ids are stable. + """ + ids: dict[str, str] = {} + used: set[str] = set() + for name in sorted(set(names)): + base = "m_" + (_NON_ID.sub("_", _short(name, prefix)).strip("_") or "root") + slug, n = base, 2 + while slug in used: + slug, n = f"{base}_{n}", n + 1 + used.add(slug) + ids[name] = slug + return ids def render_mermaid(graph: ModuleGraph) -> str: @@ -330,15 +344,15 @@ def render_mermaid(graph: ModuleGraph) -> str: ``m_`` prefix, so a module named ``end`` or ``graph`` is not a keyword. """ lines = ["```mermaid", "flowchart TD"] + ids = _node_ids( + [*graph.nodes, *(e.source for e in graph.edges), *(e.target for e in graph.edges)], + graph.prefix, + ) for name in graph.nodes: label = _short(name, graph.prefix) - if name in graph.subpackages: - lines.append(f' {_node_id(name, graph.prefix)}[["{label}/"]]') - else: - lines.append(f' {_node_id(name, graph.prefix)}["{label}"]') - for e in graph.edges: - src, dst = _node_id(e.source, graph.prefix), _node_id(e.target, graph.prefix) - lines.append(f" {src} {_ARROWS[e.kind]} {dst}") + shape = f'[["{label}/"]]' if name in graph.subpackages else f'["{label}"]' + lines.append(f" {ids[name]}{shape}") + lines.extend(f" {ids[e.source]} {_ARROWS[e.kind]} {ids[e.target]}" for e in graph.edges) lines.append("```") kinds = ", ".join(f"{k} {_ARROWS[k]}" for k in graph.dropped) removed = ", ".join(f"{k} {n}" for k, n in graph.dropped.items() if n) diff --git a/tests/unit/test_module_graph.py b/tests/unit/test_module_graph.py index 859f2ede..ff85e263 100644 --- a/tests/unit/test_module_graph.py +++ b/tests/unit/test_module_graph.py @@ -327,3 +327,28 @@ def test_a_blank_prefix_is_refused_by_both_surfaces(db: str) -> None: result = CliRunner().invoke(app, ["module-graph", " ", "--db", db]) assert result.exit_code == 1 assert "prefix" in result.output.lower() + + +def test_mermaid_ids_stay_distinct_when_names_flatten_alike() -> None: + """pkg.a.b and pkg.a_b both read as a_b once punctuation is folded; they must not merge.""" + graph = ModuleGraph( + prefix="pkg", + nodes=["pkg.a.b", "pkg.a_b"], + subpackages=[], + edges=[ModuleEdge("pkg.a.b", "pkg.a_b", "imports")], + dropped={"imports": 0}, + outside={}, + ) + lines = render_mermaid(graph).splitlines() + ids = [line.split("[")[0].strip() for line in lines if '["' in line] + assert len(set(ids)) == 2 + edge = next(line for line in lines if "-->" in line).split(" --> ") + assert {edge[0].strip(), edge[1].strip()} == set(ids) + + +def test_cli_reports_a_bad_layer_before_touching_the_graph(tmp_path: Path) -> None: + """The layer check runs first: a bad --layer is a usage error even with no database.""" + result = CliRunner().invoke( + app, ["module-graph", "pkg", "--db", str(tmp_path / "none.db"), "-l", "vibes"] + ) + assert result.exit_code == 2