fix: all three critical findings and three of nine high

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:
+<https://claude.ai/code/artifact/8b1df379-403b-4c2c-a866-29c541c25a45>
+
+**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 <path>` (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.

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
 ```

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),

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:

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

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,28 +577,32 @@ 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,
             seam=args.seam or None,
             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: