From 4f06c0579c9dd665e5aafc713ec1803bbc7b719a Mon Sep 17 00:00:00 2001 From: Andrew Young Date: Sat, 22 Aug 2026 23:12:59 -0500 Subject: [PATCH 1/2] fix: all three critical findings and three of nine high From the 2026-08-22 four-lane review. Each fix was confirmed to fail against the unfixed code before being kept, not merely to pass against the fixed one. CRITICAL -- the diagram subsystem was unreachable by any user. `catalog diagram` imported `diagram_repo` from drydock.diagram.mermaid, a function that had never existed in any commit, behind an `except ImportError` that reported "diagram module not available" -- blaming a module that was present and importable the whole time. All four generators were unit-tested, correct, and reachable by nothing. `diagram_repo` is now written, and `drydock diagram ` exists as a top-level command: docs/concepts/diagrams.md had documented it in detail across 290 lines, and drawing a picture of a directory should not require ingesting it into a catalog first. That also makes `component_diagram` reachable for the first time. All four kinds were validated through mermaid-cli, not a regex. `--kind` is now validated against one list shared by the help text, the error message and the dispatch, and the markdown branch goes through `_emit` so `-o` is no longer accepted and dropped. CRITICAL -- the suite's only cross-cutting invariant was switched off. `test_all_import_refs_no_spaces` asserts a property across every parser and every file, and its docstring says it "would have caught Bug B". It could not: the assertion sat inside `except Exception: pass`, and AssertionError is an Exception, so the test ate its own failure. The handler now catches only what the parser raises, and a counter makes the test fail rather than pass vacuously if the fixture ever stops producing imports. Reintroducing the literal Bug B shape now fails the suite; before this it passed. CRITICAL -- docs/concepts/diagrams.md documented a command that did not exist. Every command on the page now runs. The `seams` kind was also misspelled (the real kind is `seam`) and `--seam` takes a contract file, not a type name. HIGH -- identical input produced different architecture. `_depths()` iterated a set, and because the cycle break is memoized, neighbour order decided which node in a cycle got depth 0. Layer counts flipped between consecutive runs on any project with a cycle; Drydock is acyclic, which is why dogfooding never showed it. HIGH -- bridge files changed between runs. `boundaries.py` iterated `all_files` raw and sorted on connection count alone, so ties kept walk order and the `[:30]` truncation dropped a different set each time. The ordering is now a module-level `rank_bridges` with a total key. HIGH -- the git analyzer found zero stable files in a stable repo, from the analyzer whose entire purpose is finding stable extraction candidates. `change_ratio = changes / max_changes` was purely relative: with a 90-day maximum of 3 changes, a file touched once scored 0.33, cleared the `> 0.3` bar and was reported as "high changes, concentrated risk" -- 87 files on Drydock. Churn is now absolute, against a busy line drawn from the 75th percentile with a floor, and a `moderate` category closes the dead zone the old bands left between 0.1 and 0.3, so the counts finally sum to the number of files analysed. Drydock now reports 186 stable of 198, summing exactly. Two of the new regression tests were vacuous on first writing and were rewritten until they actually failed against the unfixed code: the determinism tests had to move into subprocesses with varying PYTHONHASHSEED, because set order is stable within a single process, and the depths fixture had to become dense enough that neighbour order could vary at all. 549 tests pass on 3.10-3.13; container smoke 14/14 plus the new command; no new lint findings. Six high findings remain, listed in SESSIONS/2026-08-22.md. Co-Authored-By: Claude Opus 5 (1M context) --- docs/concepts/diagrams.md | 5 +- drydock/analyzers/boundaries.py | 21 ++- drydock/analyzers/git_history.py | 57 ++++++-- drydock/arch/graph.py | 8 +- drydock/catalog/cli.py | 26 ++-- drydock/cli.py | 6 + drydock/diagram/cli.py | 148 +++++++++++++++++++ drydock/diagram/mermaid.py | 78 ++++++++++ tests/test_critical_regressions.py | 222 +++++++++++++++++++++++++++++ tests/test_langs.py | 30 +++- 10 files changed, 567 insertions(+), 34 deletions(-) create mode 100644 drydock/diagram/cli.py create mode 100644 tests/test_critical_regressions.py diff --git a/docs/concepts/diagrams.md b/docs/concepts/diagrams.md index bbd190c..a112ddc 100644 --- a/docs/concepts/diagrams.md +++ b/docs/concepts/diagrams.md @@ -68,7 +68,7 @@ graph LR ### Seam Diagrams — "Which modules implement this contract?" ``` -drydock diagram /project/path --kind seams --seam "IProcessor" +drydock diagram /project/path --kind seam --seam src/processing/base.py ``` Shows a single contract and all its implementations, plus their dependencies. Arrows point **at the contract**, not the reverse — implementers depend on the seam, not the other way around. @@ -180,7 +180,8 @@ You still get the essential structure; less-connected modules aren't shown but t drydock diagram /project --kind layers --markdown # Pipe to mermaid-cli for PNG -drydock diagram /project --kind layers --markdown | mmdc -i /dev/stdin -o diagram.png +drydock diagram /project --kind layers --markdown -o layers.md +mmdc -i layers.md -o layers.png # Or paste into GitHub Markdown / Notion / Mermaid Live Editor ``` diff --git a/drydock/analyzers/boundaries.py b/drydock/analyzers/boundaries.py index a9ef6ef..a7524a7 100644 --- a/drydock/analyzers/boundaries.py +++ b/drydock/analyzers/boundaries.py @@ -32,6 +32,18 @@ class _Cluster: external_dependents: frozenset[FileId] +def rank_bridges(bridges: list[dict[str, Any]]) -> list[dict[str, Any]]: + """Order bridge files most-connected first, with a total order. + + The key must be total. Sorting on `total_connections` alone left equally + connected files in whatever order the walk produced -- and since the caller + keeps only the top thirty, that silently returned a different set of files + from one run to the next. Adding the path as a tie-break makes the result a + function of the input alone. + """ + return sorted(bridges, key=lambda b: (-b["total_connections"], b["file"])) + + class BoundariesAnalyzer: """Detects natural component boundaries in a codebase. @@ -512,7 +524,11 @@ def _find_bridges( bridges: list[dict[str, Any]] = [] - for fid in graph["all_files"]: + # `all_files` is a set. Iterating it raw made the insertion order of + # `bridges` vary between runs, and the tie-break below is by connection + # count alone -- so `bridges[:30]` silently kept a different thirty + # files each time. Line 303 already sorts for exactly this reason. + for fid in sorted(graph["all_files"]): # Find connected files (neighbors) neighbors: set[FileId] = set() neighbors.update(forward.get(fid, [])) @@ -536,8 +552,7 @@ def _find_bridges( "total_connections": len(neighbors), }) - # Sort by total connections - bridges.sort(key=lambda b: b["total_connections"], reverse=True) + bridges = rank_bridges(bridges) return { "total": len(bridges), diff --git a/drydock/analyzers/git_history.py b/drydock/analyzers/git_history.py index 93f2c8a..a71dc7e 100644 --- a/drydock/analyzers/git_history.py +++ b/drydock/analyzers/git_history.py @@ -479,6 +479,22 @@ def _get_file_authors(self, ctx: AnalysisContext, days: int) -> dict[str, set[st return {f: authors for f, authors in sorted(file_authors.items())} + STABLE_MAX_CHANGES = 2 + """At most this many changes in the window means stable, whatever else the + repository is doing. Absolute, because "this file barely moved" is an + absolute claim.""" + + MIN_BUSY_CHANGES = 5 + """Floor for the busy line, so a quiet repository cannot manufacture hot + files out of one or two commits.""" + + @staticmethod + def _busy_threshold(counts: list[int]) -> int: + """Where "busy" starts: the 75th percentile, never below the floor.""" + ordered = sorted(counts) + p75 = ordered[min(len(ordered) - 1, (len(ordered) * 3) // 4)] + return max(GitHistoryAnalyzer.MIN_BUSY_CHANGES, p75) + def _categorize_files( self, file_changes: dict[str, int], @@ -490,40 +506,57 @@ def _categorize_files( - hot: high change frequency, many authors - stable: low change frequency (good extraction candidates) - volatile: high changes, few authors (concentrated risk) + - moderate: neither quiet nor busy - single_owner: maintained by exactly one author + + Churn is judged in absolute terms, against a busy line drawn from the + distribution. It used to be judged as `changes / max_changes`, purely + relative, which inverted the whole analysis on any quiet repository: + with a 90-day maximum of 3 changes, a file touched *once* scored 0.33, + cleared the `> 0.3` bar and was reported as "high changes, concentrated + risk". On Drydock that labelled 87 files volatile and found zero stable + ones -- from the analyzer whose entire purpose is finding stable + extraction candidates. + + The old bands also left a dead zone: a ratio between 0.1 and 0.3 fell + into no category, so the counts never summed to the number of files + analysed. `moderate` closes it, and the four churn categories are now + exhaustive and mutually exclusive. """ if not file_changes: return { "hot": [], "stable": [], "volatile": [], + "moderate": [], "single_owner": [], } - max_changes = max(file_changes.values()) + busy = self._busy_threshold(list(file_changes.values())) categories: dict[str, list[tuple]] = { "hot": [], "stable": [], "volatile": [], + "moderate": [], "single_owner": [], } for filepath, changes in file_changes.items(): # Count distinct authors; unknown = 0 author_count = len(file_authors.get(filepath, set())) - change_ratio = changes / max_changes - - # Categorization logic - if change_ratio > 0.3 and author_count > 2: - # High changes, many authors - categories["hot"].append((filepath, changes, author_count)) - elif change_ratio < 0.1: - # Low changes = stable for extraction + + if changes <= self.STABLE_MAX_CHANGES: + # Barely touched in the window: safe to lift out. categories["stable"].append((filepath, changes, author_count)) - elif change_ratio > 0.3 and author_count <= 2: - # High changes, few authors = risky - categories["volatile"].append((filepath, changes, author_count)) + elif changes >= busy: + if author_count > 2: + categories["hot"].append((filepath, changes, author_count)) + else: + # Busy and concentrated in few hands. + categories["volatile"].append((filepath, changes, author_count)) + else: + categories["moderate"].append((filepath, changes, author_count)) # Single-owner tracking (orthogonal to above) if author_count == 1: diff --git a/drydock/arch/graph.py b/drydock/arch/graph.py index a55ffca..a016006 100644 --- a/drydock/arch/graph.py +++ b/drydock/arch/graph.py @@ -305,7 +305,13 @@ def resolve(node: FileId, on_stack: frozenset[FileId]) -> int: return depth[node] if node in on_stack: return 0 # cycle: stop rather than recurse forever - below = [resolve(m, on_stack | {node}) for m in adj.get(node, ())] + # Sorted, not raw set order. Neighbour order decides which node in a + # cycle gets depth 0 (the `on_stack` break above), and `resolve` + # memoizes that choice -- so with set iteration the layer count of any + # project containing a cycle changed between runs as string hashing + # changed. Drydock itself is acyclic, which is why testing the tool on + # its own source never showed it. + below = [resolve(m, on_stack | {node}) for m in sorted(adj.get(node, ()))] value = 1 + max(below) if below else 0 depth[node] = value return value diff --git a/drydock/catalog/cli.py b/drydock/catalog/cli.py index 23f5f63..73bb3ed 100644 --- a/drydock/catalog/cli.py +++ b/drydock/catalog/cli.py @@ -14,6 +14,7 @@ from .store import Catalog, default_root from ..core.output import Reporter, dumps, write_output +from ..diagram.mermaid import DIAGRAM_KINDS __all__ = ["main"] @@ -157,7 +158,8 @@ def _build_parser() -> argparse.ArgumentParser: ) diag_parser.add_argument( "--kind", metavar="KIND", default="layers", - help="Diagram kind (layers, packages, seam)", + choices=DIAGRAM_KINDS, + help=f"Diagram kind ({', '.join(DIAGRAM_KINDS)})", ) diag_parser.add_argument( "--seam", metavar="CONTRACT", default="", @@ -575,13 +577,13 @@ def _cmd_diagram(args) -> int: print(f"error: repository not found: {args.repo}", file=sys.stderr) return EXIT_USAGE - try: - from ..diagram.mermaid import diagram_repo - except ImportError: - print("error: diagram module not available", file=sys.stderr) - return EXIT_ERROR + # `drydock.diagram` ships with the package -- it is not optional, and + # the ImportError guard that used to sit here reported "diagram module + # not available" for a module that was present and importable, hiding + # the real cause (the function it imported did not exist) for the whole + # life of this subcommand. + from ..diagram.mermaid import diagram_repo - # Generate diagram mermaid_text = diagram_repo( repo.id, kind=args.kind, @@ -589,14 +591,18 @@ def _cmd_diagram(args) -> int: catalog=catalog, ) - if not args.markdown: + # Both branches go through `_emit`, so `-o` works in either mode. The + # markdown branch used to `print` directly, which silently dropped the + # output file -- the same bug the other catalog subcommands were fixed + # for, still here because this command never ran. + if args.markdown: + _emit(args, mermaid_text) + else: _emit(args, dumps({ "repo_id": repo.id, "diagram_kind": args.kind, "mermaid": mermaid_text, })) - else: - print(mermaid_text) return EXIT_OK except Exception as exc: diff --git a/drydock/cli.py b/drydock/cli.py index 78e644c..b413fd0 100644 --- a/drydock/cli.py +++ b/drydock/cli.py @@ -46,6 +46,7 @@ def main(argv: Sequence[str] | None = None) -> int: print(" drydock extract [selection] [options]") print(" drydock extract --help") print(" drydock catalog [options]") + print(" drydock diagram [--kind KIND]") print(" drydock catalog --help") print(" drydock list | languages | mcp\n") _print_analyzers() @@ -82,6 +83,11 @@ def main(argv: Sequence[str] | None = None) -> int: return catalog_main(rest) + if command == "diagram": + from .diagram.cli import main as diagram_main + + return diagram_main(rest) + analyzers = available() analyzer = analyzers.get(command) if analyzer is None: diff --git a/drydock/diagram/cli.py b/drydock/diagram/cli.py new file mode 100644 index 0000000..9dbd5f0 --- /dev/null +++ b/drydock/diagram/cli.py @@ -0,0 +1,148 @@ +"""``drydock diagram`` -- render a project as Mermaid, straight from a path. + +The generators in `drydock.diagram.mermaid` were written, unit-tested and +correct, but nothing reached them: the only route was `drydock catalog +diagram`, which imported a function that had never been written. So the whole +subsystem was unreachable, while `docs/concepts/diagrams.md` documented this +command in detail across 290 lines. + +This is that documented command. Drawing a picture of a directory should not +require ingesting it into a catalog first, so this takes a path; `catalog +diagram` stays for repositories already ingested. +""" + +from __future__ import annotations + +import argparse +import sys +from pathlib import Path +from typing import Sequence + +from ..arch.graph import build_graph +from ..core.cli import ( + EXIT_ERROR, + EXIT_OK, + EXIT_USAGE, + add_common_arguments, + policy_from_args, +) +from ..core.output import Reporter, dumps, write_output +from ..core.registry import AnalysisContext +from .mermaid import ( + DIAGRAM_KINDS, + component_diagram, + layer_diagram, + package_diagram, + seam_diagram, +) + +__all__ = ["main", "build_parser"] + +# `seam` needs a contract to focus on and `components` needs a file set, so the +# path-based command offers one more kind than the catalog one. +KINDS = (*DIAGRAM_KINDS, "components") + + +def build_parser(prog: str = "drydock diagram") -> argparse.ArgumentParser: + parser = argparse.ArgumentParser( + prog=prog, + description="Render a project's architecture as a Mermaid diagram.", + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + parser.add_argument("project_path", help="Path to the project root") + parser.add_argument( + "--kind", + default="layers", + choices=KINDS, + help=f"Diagram to draw ({', '.join(KINDS)}); default: layers", + ) + parser.add_argument( + "--seam", + metavar="CONTRACT", + default="", + help="For --kind seam, the contract file to centre on. " + "Omit to use the highest-cohesion seam; use `drydock extract " + "--list-seams` to see the candidates", + ) + parser.add_argument( + "--max-nodes", + type=int, + default=0, + metavar="N", + help="Cap the nodes drawn (0 = each diagram's own default). " + "Large graphs are truncated with a labelled marker rather than " + "silently trimmed", + ) + add_common_arguments(parser) + return parser + + +def _render(args, ctx: AnalysisContext) -> str: + graph = build_graph(ctx) + cap = {"max_nodes": args.max_nodes} if args.max_nodes > 0 else {} + + if args.kind == "layers": + return layer_diagram(graph, **cap) + if args.kind == "packages": + return package_diagram(graph, **cap) + if args.kind == "components": + return component_diagram( + graph, {f.id for f in ctx.walk.files}, title=Path(ctx.root).name + ) + + from ..arch.seams import find_seams + + seams = find_seams(ctx, graph) + if not seams: + raise ValueError( + "no contract-shaped seams found; a seam needs one module declaring " + "a contract and at least two implementing it" + ) + if args.seam: + matches = [s for s in seams if s.contract == args.seam] + if not matches: + raise ValueError( + f"no seam with contract {args.seam!r}; candidates: " + + ", ".join(s.contract for s in seams[:5]) + ) + chosen = matches[0] + else: + chosen = seams[0] + return seam_diagram(ctx, graph, chosen) + + +def main(argv: Sequence[str] | None = None) -> int: + args = build_parser().parse_args(argv) + + root = Path(args.project_path) + if not root.is_dir(): + print(f"error: not a directory: {root}", file=sys.stderr) + return EXIT_USAGE + + reporter = Reporter(quiet=args.quiet) + try: + ctx = AnalysisContext(root, policy=policy_from_args(args), reporter=reporter) + mermaid = _render(args, ctx) + except ValueError as exc: + print(f"error: {exc}", file=sys.stderr) + return EXIT_USAGE + except Exception as exc: # noqa: BLE001 - one message, never a traceback + print(f"error: diagram failed: {exc}", file=sys.stderr) + return EXIT_ERROR + + # Markdown gets the fenced block a Mermaid renderer expects; JSON carries + # the same text as data. Both go through `write_output`, so `-o` works in + # either mode. + if args.markdown: + payload = f"```mermaid\n{mermaid}\n```" + else: + payload = dumps( + { + "project": Path(ctx.root).name, + "kind": args.kind, + "mermaid": mermaid, + }, + compact=args.compact, + ) + write_output(payload, args.output, reporter) + return EXIT_OK diff --git a/drydock/diagram/mermaid.py b/drydock/diagram/mermaid.py index 459656e..6aac519 100644 --- a/drydock/diagram/mermaid.py +++ b/drydock/diagram/mermaid.py @@ -436,3 +436,81 @@ def _empty_diagram(reason: str) -> str: f" empty[\"{_escape_label(reason)}\"]", ] return "\n".join(lines) + + +# -------------------------------------------------------------------------- +# Catalog entry point +# -------------------------------------------------------------------------- + +DIAGRAM_KINDS = ("layers", "packages", "seam") +"""The kinds `diagram_repo` accepts. `catalog diagram --kind` validates against +this, so the help text, the error message and the dispatch cannot drift apart.""" + + +def diagram_repo( + repo_id: str, + *, + kind: str = "layers", + seam: str | None = None, + catalog: Any, +) -> str: + """Render a diagram for a repository held in the catalog. + + The single entry point `drydock catalog diagram` calls. It existed as an + import in the catalog CLI from the day that subcommand shipped, but was + never written, so the command failed for every input with "diagram module + not available" -- a message that blamed a module which was present and + importable the whole time. The generators below were unit-tested and + correct; nothing reached them. + + Diagrams need a dependency graph, and the catalog stores facts rather than + graphs, so the repository is re-analyzed from the `source` path recorded at + ingest. That path can be gone by the time anyone asks for a diagram, which + is an ordinary outcome here, not an exception: it comes back as a diagram + saying so, because the caller renders whatever this returns. + """ + from pathlib import Path + + from ..arch.graph import build_graph + from ..core.registry import AnalysisContext + + repos = [r for r in catalog.repos() if r.id == repo_id] + if not repos: + return _empty_diagram(f"No repository {repo_id} in the catalog") + repo = repos[0] + + if kind not in DIAGRAM_KINDS: + raise ValueError( + f"unknown diagram kind {kind!r}; choose one of {', '.join(DIAGRAM_KINDS)}" + ) + + root = Path(repo.source) + if not root.is_dir(): + return _empty_diagram( + f"Source path for {repo.name} is no longer readable: {repo.source}" + ) + + ctx = AnalysisContext(root) + graph = build_graph(ctx) + + if kind == "layers": + return layer_diagram(graph) + if kind == "packages": + return package_diagram(graph) + + from ..arch.seams import find_seams + + seams = find_seams(ctx, graph) + if not seams: + return _empty_diagram(f"No contract-shaped seams found in {repo.name}") + if seam: + matches = [s for s in seams if s.contract == seam] + if not matches: + raise ValueError( + f"no seam with contract {seam!r}; " + f"candidates: {', '.join(s.contract for s in seams[:5])}" + ) + chosen = matches[0] + else: + chosen = seams[0] + return seam_diagram(ctx, graph, chosen) diff --git a/tests/test_critical_regressions.py b/tests/test_critical_regressions.py new file mode 100644 index 0000000..59a6d8d --- /dev/null +++ b/tests/test_critical_regressions.py @@ -0,0 +1,222 @@ +"""Regressions for the critical and high findings of the 2026-08-22 review. + +Every test here failed against the code as it stood before its fix. They are +grouped by the finding they defend rather than by module, because the point of +each one is "this specific bug must not come back". +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys + +import pytest + +from drydock.analyzers.git_history import GitHistoryAnalyzer +from drydock.analyzers.boundaries import rank_bridges +from drydock.arch.graph import build_graph +from drydock.core.registry import AnalysisContext +from drydock.diagram import mermaid as mermaid_mod + + +# --------------------------------------------------------------------------- +# Critical: the diagram subsystem was unreachable +# --------------------------------------------------------------------------- + + +def test_diagram_repo_exists_and_is_importable() -> None: + """`catalog diagram` imported a function that had never been written. + + The call site sat behind `except ImportError`, which reported "diagram + module not available" -- blaming a module that was present the whole time. + """ + assert hasattr(mermaid_mod, "diagram_repo") + assert callable(mermaid_mod.diagram_repo) + + +def test_every_declared_diagram_kind_dispatches() -> None: + """The kind list and the dispatch must not drift apart.""" + from drydock.diagram.cli import KINDS + + assert set(mermaid_mod.DIAGRAM_KINDS) <= set(KINDS) + for kind in mermaid_mod.DIAGRAM_KINDS: + assert kind in ("layers", "packages", "seam") + + +@pytest.mark.parametrize("kind", ["layers", "packages", "components"]) +def test_diagram_command_emits_a_mermaid_graph(kind, simple_python_project, capsys) -> None: + """The whole point: a user can get a diagram out of the tool.""" + from drydock.diagram.cli import main as diagram_main + + assert diagram_main([str(simple_python_project), "--kind", kind, "-q"]) == 0 + payload = json.loads(capsys.readouterr().out) + assert payload["kind"] == kind + assert payload["mermaid"].startswith("graph "), payload["mermaid"][:80] + + +def test_diagram_writes_to_output_file_in_markdown_mode(simple_python_project, tmp_path) -> None: + """`-o` used to be accepted and ignored whenever `--markdown` was passed.""" + from drydock.diagram.cli import main as diagram_main + + dest = tmp_path / "diagram.md" + assert diagram_main([str(simple_python_project), "-m", "-o", str(dest), "-q"]) == 0 + assert dest.exists(), "-o was accepted and then dropped" + assert "```mermaid" in dest.read_text() + + +def test_unknown_diagram_kind_is_rejected(simple_python_project) -> None: + from drydock.diagram.cli import build_parser + + with pytest.raises(SystemExit): + build_parser().parse_args([str(simple_python_project), "--kind", "nonsense"]) + + +# --------------------------------------------------------------------------- +# High: identical input must produce identical output +# --------------------------------------------------------------------------- + + +def _run_in_subprocess(snippet: str, root, seed: str) -> str: + """Run `snippet` in a fresh interpreter under a specific hash seed. + + Set iteration order is stable *within* one process for the same contents, + so an in-process loop cannot observe hash-order nondeterminism at all -- + it passes just as happily against the unfixed code. Only separate + interpreters with different PYTHONHASHSEED values actually exercise it. + """ + env = {**os.environ, "PYTHONHASHSEED": seed, "DRYDOCK_ROOT": str(root)} + proc = subprocess.run( + [sys.executable, "-c", snippet], + capture_output=True, text=True, env=env, timeout=180, + ) + assert proc.returncode == 0, proc.stderr[-800:] + return proc.stdout.strip() + + +_DEPTHS_SNIPPET = """ +import os, json +from drydock.arch.graph import build_graph +from drydock.core.registry import AnalysisContext +g = build_graph(AnalysisContext(os.environ["DRYDOCK_ROOT"])) +print(json.dumps(sorted((m, g.metrics[m].depth) for m in g.modules))) +""" + + +def test_depths_are_stable_across_hash_seeds(tmp_path) -> None: + """Set iteration in `_depths` made layer counts flip between runs. + + The memoized cycle break means neighbour order decides which node gets + depth 0, so a project with a cycle reported a different layer count each + run. Drydock itself is acyclic, which is why dogfooding never showed it. + """ + # Dense enough to be sensitive: every module has several neighbours that + # are themselves in cycles, so which one `resolve` descends into first + # actually decides the memoized depths. A ring where each node has one + # neighbour has no order to vary, and such a fixture passes against the + # unfixed code -- a vacuous test dressed as a regression guard. + names = [f"mod{i:02d}" for i in range(12)] + for i, name in enumerate(names): + targets = [names[(i + step) % len(names)] for step in (1, 3, 5)] + imports = "".join(f"from {t} import SYM as _{t}\n" for t in targets) + (tmp_path / f"{name}.py").write_text(f"{imports}\nSYM = {i}\n") + + results = {_run_in_subprocess(_DEPTHS_SNIPPET, tmp_path, seed) + for seed in ("0", "1", "17", "1000")} + + assert len(results) == 1, f"module depths varied with the hash seed: {results}" + + +def test_bridge_ranking_is_a_total_order() -> None: + """Ties used to inherit set-iteration order, so `[:30]` kept a different + thirty files each run. + + Tested at the ordering itself rather than end-to-end: the whole-analyzer + version of this passed against the unfixed code on every synthetic project + I could build, because the nondeterminism only surfaced on a repository + large enough for the walk order to vary. A test that cannot fail is worse + than no test, so this pins the property that actually matters -- the sort + key is total, so the output is a function of the input alone. + """ + import random + + rows = [{"file": f"pkg/f{i:02d}.py", "total_connections": i % 4} for i in range(40)] + expected = [r["file"] for r in rank_bridges(rows)] + + for _ in range(8): + random.shuffle(rows) + assert [r["file"] for r in rank_bridges(rows)] == expected, ( + "bridge ranking depends on input order, so ties are unstable" + ) + + +def test_bridge_ranking_puts_the_most_connected_first() -> None: + """The tie-break must not disturb the primary ordering.""" + rows = [ + {"file": "b.py", "total_connections": 1}, + {"file": "a.py", "total_connections": 9}, + ] + assert [r["file"] for r in rank_bridges(rows)] == ["a.py", "b.py"] + + +# --------------------------------------------------------------------------- +# High: the git analyzer inverted its own purpose +# --------------------------------------------------------------------------- + + +def test_a_quiet_file_is_stable_not_volatile() -> None: + """The shipped bug, at the unit that caused it. + + `changes / max_changes` meant that when the busiest file in the window had + 3 changes, a file touched *once* scored 0.33, cleared the `> 0.3` bar and + was reported as "high changes, concentrated risk". + """ + analyzer = GitHistoryAnalyzer() + file_changes = {"quiet.py": 1, "also_quiet.py": 1, "busiest.py": 3} + file_authors = {k: {"solo"} for k in file_changes} + + cats = analyzer._categorize_files(file_changes, file_authors) + stable = {f for f, _, _ in cats["stable"]} + + assert "quiet.py" in stable, "a file changed once is not volatile" + assert not cats["volatile"], f"nothing here is volatile: {cats['volatile']}" + + +def test_churn_categories_are_exhaustive() -> None: + """The old bands left a dead zone between ratios 0.1 and 0.3.""" + analyzer = GitHistoryAnalyzer() + file_changes = {f"f{i}.py": i for i in range(1, 30)} + file_authors = {k: {"a", "b", "c"} for k in file_changes} + + cats = analyzer._categorize_files(file_changes, file_authors) + counted = sum(len(cats[k]) for k in ("hot", "stable", "volatile", "moderate")) + + assert counted == len(file_changes), ( + f"{len(file_changes) - counted} files fell into no category" + ) + + +def test_busy_line_never_drops_below_the_floor() -> None: + """A quiet repo must not manufacture hot files out of one commit each.""" + assert GitHistoryAnalyzer._busy_threshold([1, 1, 1, 1]) >= 5 + assert GitHistoryAnalyzer._busy_threshold([40, 50, 60, 80]) > 5 + + +# --------------------------------------------------------------------------- +# Critical: the suite's cross-cutting invariant must be able to fail +# --------------------------------------------------------------------------- + + +def test_import_ref_invariant_is_not_swallowed() -> None: + """`except Exception: pass` around an assert disables the assert. + + Guards the shape rather than the instance: an AssertionError raised inside + the invariant test's body must reach pytest. + """ + source = (__import__("pathlib").Path(__file__).parent / "test_langs.py").read_text() + body = source[source.index("def test_all_import_refs_no_spaces"):] + body = body[: body.index("class TestLanguageRegistry")] + + assert "except Exception:\n # Some parsers may fail" not in body + assert "assert checked" in body, "the invariant lost its vacuity guard" diff --git a/tests/test_langs.py b/tests/test_langs.py index 0d611d3..4386f52 100644 --- a/tests/test_langs.py +++ b/tests/test_langs.py @@ -345,6 +345,8 @@ def test_all_import_refs_no_spaces(self, realistic_rust_project: Path) -> None: # Walk all source files and parse them # A loop over an empty collection asserts nothing. assert ctx.walk.files, "nothing to check: ctx.walk.files was empty" + checked = 0 + unparseable: list[str] = [] for sf in ctx.walk.files: if sf.lang is None: continue @@ -357,14 +359,30 @@ def test_all_import_refs_no_spaces(self, realistic_rust_project: Path) -> None: if source is None: continue + # A parser may legitimately choke on a fragment, so parse errors + # are tolerated -- but the assertion must not be. This block used + # to be wrapped in `except Exception: pass`, and AssertionError is + # an Exception, so the suite's only cross-cutting invariant ate its + # own failure and could never fail. Catch what the parser raises, + # never what the assertion raises. try: imports = parser.imports(source, sf.id) - for imp in imports: - assert " " not in imp.raw, \ - f"Bug B regression: ImportRef.raw contains space in {sf.id}: {imp.raw}" - except Exception: - # Some parsers may fail on partial/test code; that's OK - pass + except Exception as exc: # noqa: BLE001 - parser robustness, not ours + unparseable.append(f"{sf.id}: {type(exc).__name__}: {exc}") + continue + + for imp in imports: + assert " " not in imp.raw, ( + f"Bug B regression: ImportRef.raw contains a space in " + f"{sf.id}: {imp.raw!r}" + ) + checked += 1 + + # Without this the invariant is vacuous whenever the fixture stops + # producing imports -- which is exactly how it would rot back into the + # no-op it used to be. + assert checked, f"nothing to check: no imports parsed (errors: {unparseable})" + assert not unparseable, f"parsers failed on fixture files: {unparseable}" class TestLanguageRegistry: From 0bb32546cce4514de9402de992a02b3940fb5e6d Mon Sep 17 00:00:00 2001 From: Andrew Young Date: Sat, 22 Aug 2026 23:13:57 -0500 Subject: [PATCH 2/2] docs: session recap for 2026-08-22 The four-lane review, what was fixed, and the six high findings left open with enough detail to resume cold. Co-Authored-By: Claude Opus 5 (1M context) --- SESSIONS/2026-08-22.md | 152 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 SESSIONS/2026-08-22.md diff --git a/SESSIONS/2026-08-22.md b/SESSIONS/2026-08-22.md new file mode 100644 index 0000000..75d39aa --- /dev/null +++ b/SESSIONS/2026-08-22.md @@ -0,0 +1,152 @@ +# Session — 2026-08-22 + +Container hardening, a four-lane review of the whole system, and the first +tranche of fixes from it. + +**Branch state at end of session:** `fix/critical-and-high` (commit `4f06c05`), +branched from `main` at `a4d7dae`. Not yet pushed or opened as a PR. +549 tests passing on 3.10–3.13. Working tree clean. + +--- + +## 1. Shipped to `main` + +**PR #5 — `fix: survive environments without TOML or a writable HOME`** (`a4d7dae`) + +Two crashes that were green in CI and green locally, both found by running the +new-user command sequence inside bare containers. + +- `drydock codemap` died on a clean python:3.10-slim with + `ModuleNotFoundError: No module named 'tomli'`. `tomllib` is 3.11+, this + package supports 3.10 dependency-free, and the guarded fallback import was + duplicated at four call sites — one of which wrapped it in + `except (OSError, ValueError)`, which does not catch `ModuleNotFoundError`. + CI never saw it because CI installs `.[dev]`, which pulls in `tomli`. + Now behind one choke point, `drydock/core/toml.py`. +- `drydock catalog ingest` exited 0 after recording nothing when `$HOME` was + not writable — a scripted run went green over an empty catalog. + +Also added `tests/container/` — a checked-in smoke harness that runs the +new-user command sequence inside a bare image. **It has now caught two classes +of bug that CI structurally cannot see.** Note the read-only-`$HOME` check needs +a *non-root* container: root ignores directory mode bits, so as root the test +passes without testing anything. + +--- + +## 2. The review + +Four parallel reviews: architecture and extensibility, adversarial correctness, +new-user experience, test-suite quality. Full report published as an artifact: + + +**Verdict: the engine is sound, the boundary is not.** No review found an +architectural problem. Scale is a non-issue (175,401 files walked in 12s at +234 MB). Extraction genuinely works — the langs seam extracted to a fresh +directory, imported all 10 modules with the original nowhere on `sys.path`, and +parsed Python and Rust. Plugin seams work with real out-of-tree packages. The +hostile-input surface came back completely clean. + +Almost every defect sits at the layer a stranger touches first, and there is one +root cause: **535 tests, and essentially none of them invoked the user-facing +surface.** Measured mutation score across the twelve most load-bearing modules +was **38%** — 417 of 1,092 mutants killed, 483 survivors on lines the suite +actually executes. + +Two process notes worth keeping: + +- Subagent reports were not reliable. One "finding" was my own probe passing the + wrong parameter name. Every finding in the report was re-run before inclusion. +- My own first attempts were not reliable either. One mutation silently failed + to apply (bad anchor), so the suite run proved nothing until redone. Two + regression tests I wrote were vacuous and passed against the unfixed code. + +--- + +## 3. Fixed this session (`4f06c05`) + +All three **critical** and three of nine **high**. Each was confirmed to *fail* +against the unfixed code, not merely to pass against the fixed one. + +| Finding | Fix | +|---|---| +| `catalog diagram` never worked in any commit — imported `diagram_repo`, which had never been written, behind an `except ImportError` printing a false diagnosis | Wrote `diagram_repo`; added top-level `drydock diagram ` (the command the docs already described); `component_diagram` now reachable for the first time; all four kinds validated through mermaid-cli | +| The suite's only cross-cutting invariant was disabled by `except Exception: pass` around its assert | Handler narrowed to parser errors; counter added so it fails rather than passes vacuously. Reintroducing Bug B now fails the suite | +| `docs/concepts/diagrams.md` — 290 lines for a nonexistent command | Every command on the page now runs; `seams` → `seam` corrected | +| `architecture` layer count flipped between identical runs | `_depths()` iterates sorted; the memoized cycle break no longer depends on set order | +| `boundaries` bridge membership varied by hash seed | Sorted iteration; ordering extracted to `rank_bridges` with a total key | +| `git` found **zero** stable files in a stable repo | Absolute churn thresholds against a percentile-derived busy line; new `moderate` category closes the dead zone. Drydock now reports 186 stable of 198, summing exactly | + +--- + +## 4. Remaining — six high findings, in priority order + +These are the next session's work. All are verified and reproducible. + +1. **`dependencies` tells you to delete working packages.** + `drydock/analyzers/dependencies.py:243–245` takes a raw set difference + between PyPI *distribution* names and Python *import* names, with no mapping + and no `-`/`_` normalization. Reports `pillow`, `python-dateutil`, `pyyaml`, + `scikit-learn` as unused when all four are imported. Needs a + distribution→import name map before any "unused" claim is printed. + +2. **Drydock declares idiomatic Rust architecturally broken.** + `pub mod child;` in a parent plus `use super::Trait;` in the child is read as + a load-time cycle, producing **Acyclic Dependencies — violated** on any + conventional Rust module tree. `pub mod X;` is a declaration that a module + exists, not a dependency on it, and must not create an edge. + +3. **The bounds guard is enforced on one of two surfaces.** + `Option.minimum`'s docstring says it is enforced "once, at the CLI boundary, + for every analyzer", but `drydock/mcp.py:48,53` never consults it. Over MCP, + `max_files=-5` analyses zero files and reports success; `top=-1` returns + 101 of 102. Move the guard to where options are resolved, then assert both + surfaces reject the same values. + +4. **`--only` zeroes the verdict census.** + `drydock/analyzers/architecture.py:92–94` computes `verdicts` from the + already-filtered findings, so a *display* filter changes what reads as a + project-wide census. A CI gate on `verdicts.violated == 0` passes on a + project with two real violations. + +5. **`git` accepts every scope flag and applies none.** + `--skip-dir`, `--max-files`, `--no-tests` are all ignored — and `meta` is + then rewritten to describe the scope that was never applied + (`--max-files 3` stamps `files_walked: 3, truncated: true` onto a document + containing all 198 files). Either honour them or reject them. + +6. **Two documented fixes have no test defending them.** + `core/paths.py:432` (`and`→`or` reintroduces the Rust `super::`/`crate::` + bug) and `diagram/mermaid.py:151` (`or`→`and` reintroduces the phantom-node + bug) both survive the suite. The code records each fix in a comment; nothing + defends it. + +### Also open, below high + +Rust structs always report `[0m]` (impl blocks unbound); Rust `used_crates` +scraped from comments and string literals; `context` unbounded by default +(194k tokens on a 100-file repo, 4.4M on 2,800 files — `--budget` works, its +default of 0 does not); seam extraction invisible over MCP; four catalog +subcommands MCP-unreachable; `--max-depth`/`--depth` dead flags; `line_count` +off-by-one on files ending in a newline; symlinks produce duplicate `FileId`s; +private methods reported as public API; `security` missing from `pyproject.toml` +entry points; README's codemap example fabricated; seven doc pages still +teaching the 0.1 invocation. + +--- + +## 5. The one structural fix that would prevent most of this + +**A smoke test that runs every command in the README and every command in +`--help`, asserting exit 0 and non-empty output.** It would have caught the dead +diagram command, both dead flags, and the git scope flags before any shipped. + +`tests/container/smoke.sh` is the skeleton — it already walks the command list; +it needs to assert *content*, not just exit status, and to be driven from the +README rather than a hand-maintained list so the two cannot drift. + +The recurring failure mode in this codebase, now seven instances deep, is +**logic applied at call sites rather than at a choke point, where the path that +skips it is invisible.** The fix is always to move it somewhere it cannot be +bypassed — and, as the diagram bug showed, to make sure a test crosses the +boundary rather than calling the function directly.