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. 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: